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

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

On 04/24/19 12:24, Laszlo Ersek wrote:

> ... 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>.

Two more thoughts:

* regarding (4a), it's not necessary to modify (or introduce) a platform
DXE driver for installing the protocol, with <value-2>. I think it can
be done in PlatformBootManagerBeforeConsole() as well.


* the new protocol could be replced with a new event group:

- GraphicsConsoleDxe would get a new global variable (a BOOLEAN), with
the default value meaning "clear screen on SetMode".

- GraphicsConsoleDxe would also install an event group notification
function, for *inverting* the boolean variable (and doing nothing else).

- In the affected platforms, BeforeConsole would signal the new event
group once (to disable screen clearing temporarily). Then AfterConsole
would signal the new event group once more, to restore the default
(standards conformant) behavior.

- Unaffected platforms would never signal the new event group; they'd
ignore it.

Personally, I like the protocol approach a lot more, because at any
point, you can query the state of the "system" (= the protocol DB) with
external utilities. The new event group allows for much less
introspection, not to mention that the meaning of signaling the event
group depends on the current state. (That could be avoided by
introducing two event groups, one for setting the variable and another
for clearing it, but that's getting baroque in my opinion.)

I'd go with the new protocol -- you don't need GraphicsConsoleDxe to
react to PlatformBootManagerLib with a callback.

Thanks
Laszlo

> 
> 
> 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
>>>>
>>>
>>>
>>> 
>>
>