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

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

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.

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/GraphicsConsole.c
> index 26ea19f300..39a999838c 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.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/GraphicsConsoleDxe.inf b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> index f7caa65aa9..59751893f6 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> @@ -49,6 +49,9 @@
>    HiiLib
>    PcdLib
>  
> +[Guids]
> +  gEfiEventReadyToBootGuid                      ## CONSUMES
> +
>  [Protocols]
>    gEfiDevicePathProtocolGuid                    ## TO_START
>    gEfiSimpleTextOutProtocolGuid                 ## BY_START
>