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

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

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

Thanks,
Zhichao

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