Re: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

["Laszlo Ersek" <lersek@redhat.com>]

On 04/24/19 04:37, Gao, Zhichao wrote:
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Tuesday, April 23, 2019 9:51 PM
>> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
>> Cc: Aaron Antone <aanton@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
>> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>
>> Subject: Re: [edk2-devel] [PATCH V2 2/2]
>> MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen
>>
>> On 04/23/19 09:04, Zhichao Gao wrote:
>>> From: Aaron Antone <aanton@microsoft.com>
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412
>>>
>>> For now, most platform support to display during PEIM. It means the
>>> logo can show at PEI phase. But the screen would be cleared at BDS
>>> connect console phase. That may make the screen flush and turn into black
>> screen.
>>> So do not clear the screen while set the text mode for graphics
>>> console device for the first time boot.
>>> As the shell reconnect command would make the same reslut with the
>>> first boot, use the gEfiEventReadyToBootGuid to distinguish them.
>>>
>>> Also replace the debug code in GraphicsConsoleControllerDriverStart.
>>> The origin one would set a basic mode and then print the text info to
>>> graphic console device. Then the conspliter would set a best mode for
>>> graphics console device. If the best mode is different with the basic
>>> one, the screen would be cleared. So use the debug output instead.
>>>
>>> This patch only affect the behavior of SetMode at the first boot
>>> during the graphics console devices first connect operations. That
>>> means at DXE phase before ReadyToBoot, the Graphics Simple Text Out
>>> SetMode would not clear the screen during the first connecttion of the
>> graphics devices.
>>
>> The UEFI spec requirement doesn't apply after ReadyToBoot only. What
>> about SysPrep applications, for example:
>>
>> """
>> When launched, the platform is required to provide the application loaded by
>> SysPrep####, with the same services such as console and network as are
>> normally provided at launch to applications referenced by a Boot####
>> variable. [...] After all SysPrep#### variables have been launched and exited,
>> the platform shall notify EFI_EVENT_GROUP_READY_TO_BOOT event group
>> and begin to evaluate Boot#### variables [...] """
>>
>> Thus a SysPrep application is permitted to expect, and to use, the console,
>> but it is launched before ReadyToBoot; and so this patch could break the
>> console's std conformance for SysPrep apps.
> 
> Thanks for your reminder. The 'SysPrep####' and 'PlatformRecovery####' are both optional for the consumers. The condition of 'SysPrep####' is like what you mentioned above. For 'PlatformRecovery####', if the platform set the 'OsIndications' EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit, then the system would directly try to execute the 'PlatformRecovery####'. And the PlatformRecovery application would have the same issue with SysPrep application.
> To cover all the conditions we discussed, the phase immediately after the PlatformBootManagerAfterConsole() is the best choice. I also thought of the EndOfDxe event. But it is controlled by the PlatformBdsLib and the usual signal function is at the end of PlatformBootManagerBeforeConsole(). That would miss the PCI graphics devices connected in the after console.
> For now seems there is no suitable event to distinguish the behaviors in different phases. Adding an event in the Bds is also unsuitable.

I think we should find a general way to add edk2 extensions (status
codes and events) to BdsDxe / UefiBootManagerLib, without having to go
through standardization every time.

Can we introduce a new PlatformBootManagerLib hook for the present use
case? See for example

  https://bugzilla.tianocore.org/show_bug.cgi?id=982

which was fixed in commit range

  cef7ecf6cdb4..1010873becc5

... Actually, you mention "the phase immediately after the
PlatformBootManagerAfterConsole() is the best choice". So how about the
following approach:

(1) Introduce a new protocol (an edk2 extension). The UEFI protocol
database is supposed to contain *at most* one instance of this protocol,
at any time.

(2) The protocol should consist of a BOOLEAN field only. (You can add a
"revision" field at the beginning too, if you like.)

(3) In GraphicsConsoleDxe, which is a UEFI driver, you try to locate the
protocol at the right spot -- when SetMode is about to clear the screen.
If the protocol instance is missing, there is no change in behavior (the
screen is cleared). If the protocol instance is available, but the
BOOLEAN field is <value-1>, there is no change in behavior (the screen
is cleared). If the protocol instance is available, and the BOOLEAN
field is <value-2>, then the screen is *not* cleared.

(4) Platforms that need the anti-flicker tweak are expected to do the
following two steps:

(4a) add a platform DXE driver (or modify one) to install the protocol,
with the BOOLEAN field set to <value-2>.

(4b) append new logic to PlatformBootManagerAfterConsole(), for locating
the protocol, and setting the BOOLEAN field in it to <value-1>.


A variant could be to introduce a tri-state DynamicEx PCD -- <value-0>,
the DEC default, means no change in behavior (= the platform doesn't
need the anti-flicker tweak); while <value-1> and <value-2> work like
above. The question is whether we are OK adding an EFI_PCD_PROTOCOL
dependency to GraphicsConsoleDxe, which is a UEFI driver
(EFI_PCD_PROTOCOL comes from the PI spec, not from the UEFI spec).
Inventing a new edk2 extension protocol (with a fresh protocol GUID)
would avoid that problem -- it could be implemented purely in terms of
the UEFI spec.

Thanks
Laszlo

>> I guess you have already investigated adding a boolean field to
>> GRAPHICS_CONSOLE_DEV, and found it unsuitable for those platforms that
>> need this anti-flicker tweak. So I'm not going to suggest such a boolean field
>> now.
>>
>> Instead, I propose a PCD (feature PCD or dynamic boolean PCD). If you add a
>> PCD, I won't care about the particulars of this patch, as long as platforms
>> continue observing the std conformant behavior, under the default value of
>> the PCD (i.e., from "MdeModulePkg.dec").
>>
>> Thanks,
>> Laszlo
>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>>> Cc: Michael Turner <Michael.Turner@microsoft.com>
>>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
>>> ---
>>>  .../GraphicsConsoleDxe/GraphicsConsole.c      | 82 +++++++++++++------
>>>  .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf |  3 +
>>>  2 files changed, 62 insertions(+), 23 deletions(-)
>>>
>>> diff --git
>>>
>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
>> .c
>>>
>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
>> e.c
>>> index 26ea19f300..39a999838c 100644
>>> ---
>>>
>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
>> .c
>>> +++
>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
>>> +++ e.c
>>> @@ -1,7 +1,7 @@
>>>  /** @file
>>>    This is the main routine for initializing the Graphics Console support
>> routines.
>>>
>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights
>>> reserved.<BR>
>>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights
>>> +reserved.<BR>
>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>>  **/
>>> @@ -96,6 +96,12 @@ EFI_DRIVER_BINDING_PROTOCOL
>> gGraphicsConsoleDriverBinding = {
>>>    NULL
>>>  };
>>>
>>> +//
>>> +// Event and variable to indicate the boot phase.
>>> +//
>>> +EFI_EVENT   mGraphicsConsoleReadyToBootEvent;
>>> +BOOLEAN     mGraphicsConsoleReadyToBoot       = FALSE;
>>> +
>>>  /**
>>>    Test to see if Graphics Console could be supported on the Controller.
>>>
>>> @@ -567,16 +573,7 @@ GraphicsConsoleControllerDriverStart (
>>>    //
>>>    Private->SimpleTextOutputMode.MaxMode = (INT32) MaxMode;
>>>
>>> -  DEBUG_CODE_BEGIN ();
>>> -    Status = GraphicsConsoleConOutSetMode (&Private-
>>> SimpleTextOutput, 0);
>>> -    if (EFI_ERROR (Status)) {
>>> -      goto Error;
>>> -    }
>>> -    Status = GraphicsConsoleConOutOutputString (&Private-
>>> SimpleTextOutput, (CHAR16 *)L"Graphics Console Started\n\r");
>>> -    if (EFI_ERROR (Status)) {
>>> -      goto Error;
>>> -    }
>>> -  DEBUG_CODE_END ();
>>> +  DEBUG ((DEBUG_INFO, "Graphics Console Started!\n\r"));
>>>
>>>    //
>>>    // Install protocol interfaces for the Graphics Console device.
>>> @@ -1366,18 +1363,26 @@ GraphicsConsoleConOutSetMode (
>>>        //
>>>        // The current graphics mode is correct, so simply clear the entire
>> display
>>>        //
>>> -      Status = GraphicsOutput->Blt (
>>> -                          GraphicsOutput,
>>> -                          &mGraphicsEfiColors[0],
>>> -                          EfiBltVideoFill,
>>> -                          0,
>>> -                          0,
>>> -                          0,
>>> -                          0,
>>> -                          ModeData->GopWidth,
>>> -                          ModeData->GopHeight,
>>> -                          0
>>> -                          );
>>> +      //
>>> +      // For the first time boot system, do not clear the display.
>>> +      // Some platforms would show logo at PEIM and this would clear
>>> +      // the whole screen. So for first boot set mode, do not clear
>>> +      // the screen.
>>> +      //
>>> +      if (This->Mode->Mode != -1 || mGraphicsConsoleReadyToBoot) {
>>> +        Status = GraphicsOutput->Blt (
>>> +                            GraphicsOutput,
>>> +                            &mGraphicsEfiColors[0],
>>> +                            EfiBltVideoFill,
>>> +                            0,
>>> +                            0,
>>> +                            0,
>>> +                            0,
>>> +                            ModeData->GopWidth,
>>> +                            ModeData->GopHeight,
>>> +                            0
>>> +                            );
>>> +      }
>>>      }
>>>    } else if (FeaturePcdGet (PcdUgaConsumeSupport)) {
>>>      //
>>> @@ -2065,6 +2070,24 @@ RegisterFontPackage (
>>>    FreePool (Package);
>>>  }
>>>
>>> +/**
>>> +  Indicate the Boot phase is at ReadyToBoot.
>>> +
>>> +  @param[in]  Event   The Event that is being processed.
>>> +  @param[in]  Context The Event Context.
>>> +
>>> +**/
>>> +VOID
>>> +GraphicsConsoleReadyToBootEvent (
>>> +  IN EFI_EVENT        Event,
>>> +  IN VOID             *Context
>>> +  )
>>> +{
>>> +  mGraphicsConsoleReadyToBoot = TRUE;
>>> +
>>> +  gBS->CloseEvent (mGraphicsConsoleReadyToBootEvent);
>>> +}
>>> +
>>>  /**
>>>    The user Entry Point for module GraphicsConsole. The user code starts
>> with this function.
>>>
>>> @@ -2095,6 +2118,19 @@ InitializeGraphicsConsole (
>>>      &mHiiRegistration
>>>      );
>>>
>>> +  //
>>> +  // Create a event function of ReadyToBoot to avoid clearing screen
>>> + before boot  //  Status = gBS->CreateEventEx (
>>> +                  EVT_NOTIFY_SIGNAL,
>>> +                  TPL_NOTIFY,
>>> +                  GraphicsConsoleReadyToBootEvent,
>>> +                  NULL,
>>> +                  &gEfiEventReadyToBootGuid,
>>> +                  &mGraphicsConsoleReadyToBootEvent
>>> +                  );
>>> +  ASSERT_EFI_ERROR (Status);
>>> +
>>>    //
>>>    // Install driver model protocol(s).
>>>    //
>>> diff --git
>>>
>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
>> Dxe
>>> .inf
>>>
>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
>> eDxe
>>> .inf
>>> index f7caa65aa9..59751893f6 100644
>>> ---
>>>
>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
>> Dxe
>>> .inf
>>> +++
>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
>>> +++ eDxe.inf
>>> @@ -49,6 +49,9 @@
>>>    HiiLib
>>>    PcdLib
>>>
>>> +[Guids]
>>> +  gEfiEventReadyToBootGuid                      ## CONSUMES
>>> +
>>>  [Protocols]
>>>    gEfiDevicePathProtocolGuid                    ## TO_START
>>>    gEfiSimpleTextOutProtocolGuid                 ## BY_START
>>>
>>
>>
>> 
>