[PATCH V2 0/2] MdeModulePkg: Make the screen seamless

[PATCH V2 0/2] MdeModulePkg: Make the screen seamless

[Gao, Zhichao]

For now most platforms support display function at PEI phase.
But the conspliter and graphics console driver would clear the
screen at BDS connect console phase. Maybe some platforms would
show logo in the next or maybe not. For consumers, it looks like
the screen flashed.
So change the behavior of graphics console devices while connect
console devices to maintain seamless screen from PEI.

Test has done on MinPlatform Kabylake-RVP3 which support PEI
display.

V2:
Make the SetMode not clear the screen only at the first boot during
the first conncettion of graphics device.

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>

Aaron Antone (2):
  MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode
  MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

 .../Console/ConSplitterDxe/ConSplitter.c      | 34 +++++---
 .../Console/ConSplitterDxe/ConSplitter.h      |  4 +-
 .../GraphicsConsoleDxe/GraphicsConsole.c      | 82 +++++++++++++------
 .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf |  3 +
 4 files changed, 88 insertions(+), 35 deletions(-)

-- 
2.21.0.windows.1

[PATCH V2 1/2] MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode

[Gao, Zhichao]

From: Aaron Antone <aanton@microsoft.com>

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

For Console Out device, it would always set all present devices'
text out mode again through ConSplitterTextOutSetMode while adding
devices. That may cause the screen cleared for serval times.
So add a BOOLEAN to judge if it is adding device then we will not
set the same text mode again for same console out device.

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>
---
 .../Console/ConSplitterDxe/ConSplitter.c      | 34 +++++++++++++------
 .../Console/ConSplitterDxe/ConSplitter.h      |  4 ++-
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
index 6fc0e4796f..fc9b9e08e5 100644
--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
@@ -16,7 +16,7 @@
   never removed. Such design ensures sytem function well during none console
   device situation.
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -180,7 +180,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED TEXT_OUT_SPLITTER_PRIVATE_DATA mConOut = {
   0,
   (TEXT_OUT_SPLITTER_QUERY_DATA *) NULL,
   0,
-  (INT32 *) NULL
+  (INT32 *) NULL,
+  FALSE
 };
 
 //
@@ -235,7 +236,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED TEXT_OUT_SPLITTER_PRIVATE_DATA mStdErr = {
   0,
   (TEXT_OUT_SPLITTER_QUERY_DATA *) NULL,
   0,
-  (INT32 *) NULL
+  (INT32 *) NULL,
+  FALSE
 };
 
 //
@@ -3132,8 +3134,9 @@ ConSplitterTextOutAddDevice (
   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
   EFI_STATUS                           DeviceStatus;
 
-  Status                = EFI_SUCCESS;
-  CurrentNumOfConsoles  = Private->CurrentNumberOfConsoles;
+  Status                      = EFI_SUCCESS;
+  CurrentNumOfConsoles        = Private->CurrentNumberOfConsoles;
+  Private->AddingConOutDevice = TRUE;
 
   //
   // If the Text Out List is full, enlarge it by calling ConSplitterGrowBuffer().
@@ -3290,6 +3293,8 @@ ConSplitterTextOutAddDevice (
   //
   ConsplitterSetConsoleOutMode (Private);
 
+  Private->AddingConOutDevice = FALSE;
+
   return Status;
 }
 
@@ -4849,12 +4854,19 @@ ConSplitterTextOutSetMode (
   //
   TextOutModeMap = Private->TextOutModeMap + Private->TextOutListCount * ModeNumber;
   for (Index = 0, ReturnStatus = EFI_SUCCESS; Index < Private->CurrentNumberOfConsoles; Index++) {
-    Status = Private->TextOutList[Index].TextOut->SetMode (
-                                                    Private->TextOutList[Index].TextOut,
-                                                    TextOutModeMap[Index]
-                                                    );
-    if (EFI_ERROR (Status)) {
-      ReturnStatus = Status;
+    //
+    // While adding a console out device do not set same mode again for the same device.
+    //
+    if (Private->AddingConOutDevice != TRUE ||
+      TextOutModeMap[Index] != Private->TextOutList[Index].TextOut->Mode->Mode) {
+
+      Status = Private->TextOutList[Index].TextOut->SetMode (
+                                                      Private->TextOutList[Index].TextOut,
+                                                      TextOutModeMap[Index]
+                                                      );
+      if (EFI_ERROR (Status)) {
+        ReturnStatus = Status;
+      }
     }
   }
 
diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h
index e9b68e58c6..419635c3f5 100644
--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h
+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h
@@ -1,7 +1,7 @@
 /** @file
   Private data structures for the Console Splitter driver
 
-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
 
 **/
@@ -218,6 +218,8 @@ typedef struct {
   UINTN                                 TextOutQueryDataCount;
   INT32                                 *TextOutModeMap;
 
+  BOOLEAN                               AddingConOutDevice;
+
 } TEXT_OUT_SPLITTER_PRIVATE_DATA;
 
 #define TEXT_OUT_SPLITTER_PRIVATE_DATA_FROM_THIS(a) \
-- 
2.21.0.windows.1

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

[Gao, Zhichao]

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.

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
-- 
2.21.0.windows.1

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

[Laszlo Ersek]

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
> 

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

[Gao, Zhichao]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Gao, Zhichao]

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

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

[Sean]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

Is there a branch where this code change can be reviewed?  

The intent of bug 1412 was not to break spec alignment on the uefi defined protocol and SetMode() function.  We had a proposed change that can be seen here. https://github.com/Microsoft/mu_basecore/pull/13/files 
The idea was for new devices published, only clear the screen if the mode needs to change.  Our reading of the UEFI spec didn't define how new devices needed to be added and didn't require the screen be cleared.

[-- Attachment #2: Type: text/html, Size: 670 bytes --]

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

[Gao, Zhichao]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

The change you provided is covered in patch #1.
But that can only save one problem:
not to repeat clearing the screen which is already set a console mode while adding console devices.
This is not match the info in 1412<https://bugzilla.tianocore.org/show_bug.cgi?id=1412>: “This is not a desired behavior when booting a consumer device as it clears the boot logo.”

For the first time, the screens are always cleared because its initial mode is ‘-1’ which is regarding as an invalid mode.

Here is the code covered your change:
https://github.com/ZhichaoGao/edk2/commit/72d5198cb131c456eb250b88a8bd7e6cf2d18b3b
And the blow is the change which can solve the clear screen issue, and this is incorrect based on the discussion in the community:
https://github.com/ZhichaoGao/edk2/commit/8ed0248c74564e2d8be0babca1ef0b49e4147478

If I have any mistakes, please correct them.

Thanks,
Zhichao

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Sean via Groups.Io
Sent: Friday, April 26, 2019 9:12 AM
To: Gao; Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

Is there a branch where this code change can be reviewed?

The intent of bug 1412 was not to break spec alignment on the uefi defined protocol and SetMode() function.  We had a proposed change that can be seen here. https://github.com/Microsoft/mu_basecore/pull/13/files
The idea was for new devices published, only clear the screen if the mode needs to change.  Our reading of the UEFI spec didn't define how new devices needed to be added and didn't require the screen be cleared.


[-- Attachment #2: Type: text/html, Size: 6918 bytes --]

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

[Laszlo Ersek]

On 04/25/19 17:21, Gao, Zhichao wrote:
> 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.

As long as 3rd party UEFI drivers and UEFI apps cannot tell the
difference, I think it should be fine.

Note that I'm not arguing for the above changes -- it would be simplest
for us to call the use case (the goal of no flicker) *itself*
incompatible with the spec. I just tried to invent some ways with the
least divergence from the spec (and with the least impact on platforms
that aren't interested), if the use case is considered really important.

Thanks
Laszlo

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

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

[Sean]

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

There is no reason the devices have to start in mode -1.  A device when started should report an accurate list of modes and if it is already in one of those modes it should report the correct mode.  This way higher level software can choose not to call setmode because it identifies that the device is already set.

This avoids the spec compliance issue in that the SetMode function is still always spec compliant.

Did I miss something for your second patch?

Thanks
Sean



From: Gao, Zhichao <zhichao.gao@intel.com>
Sent: Thursday, April 25, 2019 7:39 PM
To: devel@edk2.groups.io; Sean Brogan <sean.brogan@microsoft.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

The change you provided is covered in patch #1.
But that can only save one problem:
not to repeat clearing the screen which is already set a console mode while adding console devices.
This is not match the info in 1412<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1412&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=SF4ieluLBvC0PcWzY2vvwiTSsDK%2FlGUaSNwd69Aa9Z0%3D&reserved=0>: “This is not a desired behavior when booting a consumer device as it clears the boot logo.”

For the first time, the screens are always cleared because its initial mode is ‘-1’ which is regarding as an invalid mode.

Here is the code covered your change:
https://github.com/ZhichaoGao/edk2/commit/72d5198cb131c456eb250b88a8bd7e6cf2d18b3b<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FZhichaoGao%2Fedk2%2Fcommit%2F72d5198cb131c456eb250b88a8bd7e6cf2d18b3b&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=7evSbqLIJ5qRAwu3fQRYTtsM0dwVHtM%2BvYbiIFITmYo%3D&reserved=0>
And the blow is the change which can solve the clear screen issue, and this is incorrect based on the discussion in the community:
https://github.com/ZhichaoGao/edk2/commit/8ed0248c74564e2d8be0babca1ef0b49e4147478<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FZhichaoGao%2Fedk2%2Fcommit%2F8ed0248c74564e2d8be0babca1ef0b49e4147478&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=5eL1ceaPzm5%2FsoEWx4nvVBTpgySEROC2vnj1P0zSlWE%3D&reserved=0>

If I have any mistakes, please correct them.

Thanks,
Zhichao

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Sean via Groups.Io
Sent: Friday, April 26, 2019 9:12 AM
To: Gao; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

Is there a branch where this code change can be reviewed?

The intent of bug 1412 was not to break spec alignment on the uefi defined protocol and SetMode() function.  We had a proposed change that can be seen here. https://github.com/Microsoft/mu_basecore/pull/13/files<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fmu_basecore%2Fpull%2F13%2Ffiles&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=vWV9wNrRBglVT1T0l1oGv%2BdVLJ0mpbBf9Nlo%2FX6pnWg%3D&reserved=0>
The idea was for new devices published, only clear the screen if the mode needs to change.  Our reading of the UEFI spec didn't define how new devices needed to be added and didn't require the screen be cleared.


[-- Attachment #2: Type: text/html, Size: 10634 bytes --]

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

[Gao, Zhichao]

[-- Attachment #1: Type: text/plain, Size: 5380 bytes --]

I agree with your point completely. And I regard that as an optimization of the ConSpliterDxe.

But what I want talk is that this optimization do not fix the issue in 1412<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1412&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=SF4ieluLBvC0PcWzY2vvwiTSsDK%2FlGUaSNwd69Aa9Z0%3D&reserved=0>.
The 1412<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1412&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=SF4ieluLBvC0PcWzY2vvwiTSsDK%2FlGUaSNwd69Aa9Z0%3D&reserved=0> want to fix the issue that the system would clear the logo showed in PEI phase. Did I make a misunderstanding?

The mode ‘-1’ is the initial mode of the graphics console devices. So the SetMode would be called at least once in BDS phase and it would clear the screen. That means the screen would always be cleared during boot. That is why I add the second patch.

Thanks,
Zhichao

From: Sean Brogan [mailto:sean.brogan@microsoft.com]
Sent: Saturday, April 27, 2019 3:25 AM
To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

There is no reason the devices have to start in mode -1.  A device when started should report an accurate list of modes and if it is already in one of those modes it should report the correct mode.  This way higher level software can choose not to call setmode because it identifies that the device is already set.

This avoids the spec compliance issue in that the SetMode function is still always spec compliant.

Did I miss something for your second patch?

Thanks
Sean



From: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
Sent: Thursday, April 25, 2019 7:39 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Sean Brogan <sean.brogan@microsoft.com<mailto:sean.brogan@microsoft.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Subject: RE: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

The change you provided is covered in patch #1.
But that can only save one problem:
not to repeat clearing the screen which is already set a console mode while adding console devices.
This is not match the info in 1412<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1412&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=SF4ieluLBvC0PcWzY2vvwiTSsDK%2FlGUaSNwd69Aa9Z0%3D&reserved=0>: “This is not a desired behavior when booting a consumer device as it clears the boot logo.”

For the first time, the screens are always cleared because its initial mode is ‘-1’ which is regarding as an invalid mode.

Here is the code covered your change:
https://github.com/ZhichaoGao/edk2/commit/72d5198cb131c456eb250b88a8bd7e6cf2d18b3b<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FZhichaoGao%2Fedk2%2Fcommit%2F72d5198cb131c456eb250b88a8bd7e6cf2d18b3b&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=7evSbqLIJ5qRAwu3fQRYTtsM0dwVHtM%2BvYbiIFITmYo%3D&reserved=0>
And the blow is the change which can solve the clear screen issue, and this is incorrect based on the discussion in the community:
https://github.com/ZhichaoGao/edk2/commit/8ed0248c74564e2d8be0babca1ef0b49e4147478<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FZhichaoGao%2Fedk2%2Fcommit%2F8ed0248c74564e2d8be0babca1ef0b49e4147478&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=5eL1ceaPzm5%2FsoEWx4nvVBTpgySEROC2vnj1P0zSlWE%3D&reserved=0>

If I have any mistakes, please correct them.

Thanks,
Zhichao

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Sean via Groups.Io
Sent: Friday, April 26, 2019 9:12 AM
To: Gao; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

Is there a branch where this code change can be reviewed?

The intent of bug 1412 was not to break spec alignment on the uefi defined protocol and SetMode() function.  We had a proposed change that can be seen here. https://github.com/Microsoft/mu_basecore/pull/13/files<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fmu_basecore%2Fpull%2F13%2Ffiles&data=01%7C01%7Csean.brogan%40microsoft.com%7Cbc7432dac0774fb5d44808d6c9f05101%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=vWV9wNrRBglVT1T0l1oGv%2BdVLJ0mpbBf9Nlo%2FX6pnWg%3D&reserved=0>
The idea was for new devices published, only clear the screen if the mode needs to change.  Our reading of the UEFI spec didn't define how new devices needed to be added and didn't require the screen be cleared.


[-- Attachment #2: Type: text/html, Size: 16251 bytes --]

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

[Sean]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

Zhichao,

That sounds like a bug in you graphics console devices.  If they are already initialized (by pei or other platform code) they should not start in mode -1.  
For bug 1412, can we make sure all Tianocore edk2 drivers support "flicker free" and don't unncecessarly re-initialize.  Do you see other issues in edk2 code?  

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 402 bytes --]

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

[Gao, Zhichao]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Maybe you are right. I would try to figure out the issue in graphics console devices.
Making edk2 do not unnecessarily re-initialize is a good idea. But the ‘flicker free’ is depend on the platform code designer. By default the flicker would always happen because of some pcds (gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution, gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution). The two pcds have default value, we suggest the platform to set them to zero in the platform dsc file and that means the platform would initialize the screen resolution to the maximum. If not and the screen’s resolution is different with the default value, then the screen would flicker and set the screen to the default resolution.
For now, I didn’t find other issues in edk2 code.

Thanks,
Zhichao

From: sean.brogan via groups.io [mailto:sean.brogan=microsoft.com@groups.io]
Sent: Tuesday, April 30, 2019 9:08 AM
To: Gao; Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen

Zhichao,

That sounds like a bug in you graphics console devices.  If they are already initialized (by pei or other platform code) they should not start in mode -1.
For bug 1412, can we make sure all Tianocore edk2 drivers support "flicker free" and don't unncecessarly re-initialize.  Do you see other issues in edk2 code?

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 4738 bytes --]