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

["Gao, Zhichao" <zhichao.gao@intel.com>]

Hi Laszlo,

Thanks for your thoughts and the specific description.

Seems you offer three methods to enable and then disable the screen clear.
1. Protocol with data only
2. Boolean global variable trigger by event
3. Pcd Boolean value(for the pcd protocol depex, I think it is fine to use it, there some other uefi driver such as PciBusDxe using PcdLib would automatic include the depex)
If we want to change the control value, we should do the change operation at BeforeConsole and AfterConsole.
But I want to know if it is compatible with the spec while we provide a approach to change the behavior in the spec. Even though most consumers would not do that.

Thanks,
Zhichao

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, April 24, 2019 6:48 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/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
> >>>>
> >>>
> >>>
> >>> 
> >>
> >