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

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

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>

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      | 45 +++++++++----------
 3 files changed, 48 insertions(+), 35 deletions(-)

-- 
2.21.0.windows.1

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

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>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../GraphicsConsoleDxe/GraphicsConsole.c      | 45 +++++++++----------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
index 26ea19f300..0a35b59d64 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
 
 **/
@@ -567,16 +567,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 +1357,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 set the mode, do not clear the display.
+      // Some platform would show logo at PEIM and this would clear
+      // the whole screen. So for first time set mode, do not clear
+      // the screen.
+      //
+      if (This->Mode->Mode != -1) {
+        Status = GraphicsOutput->Blt (
+                            GraphicsOutput,
+                            &mGraphicsEfiColors[0],
+                            EfiBltVideoFill,
+                            0,
+                            0,
+                            0,
+                            0,
+                            ModeData->GopWidth,
+                            ModeData->GopHeight,
+                            0
+                            );
+      }
     }
   } else if (FeaturePcdGet (PcdUgaConsumeSupport)) {
     //
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 0/2] MdeModulePkg: Make the screen seamless

[Laszlo Ersek]

On 04/12/19 05:14, Gao, Zhichao wrote:
> 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.
> 
> 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>
> 
> 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      | 45 +++++++++----------
>  3 files changed, 48 insertions(+), 35 deletions(-)
> 

EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() is specified to clear the screen
to black. Is this series compatible with that?

Thanks,
Laszlo

Re: [edk2-devel] [PATCH 0/2] MdeModulePkg: Make the screen seamless

[Gao, Zhichao]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, April 12, 2019 4:06 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: 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 0/2] MdeModulePkg: Make the screen
> seamless
> 
> On 04/12/19 05:14, Gao, Zhichao wrote:
> > 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.
> >
> > 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>
> >
> > 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      | 45 +++++++++----------
> >  3 files changed, 48 insertions(+), 35 deletions(-)
> >
> 
> EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() is specified to clear the
> screen to black. Is this series compatible with that?

No. We only consider the console section.
There are two pcds to control the graphics output mode PcdVideoHorizontalResolution and PcdVideoVerticalResolution. Usually we set them as zero to make the mode to be the max mode the graphics supported and the graphics output protocol would initialize the mode to be the max mode in general. If so the SetMode would not  be runt. But that is done in the graphics output driver and the driver is usually a binary file. So we can't desire that the graphics driver  would set the max mode, that is the graphics output driver's vendor decided.
In the other condition, these two pcds would set a value and then graphics output driver would focus to set the mode and clear the screen. That is controlled by the consumer. By default the two pcds is initialized as 800 and 600. Because this resolution may be the most normal resolution and the screen would always be cleared.
 
In my opinion, the behavior of graphics output section in this driver is fine and should not be changed. And also, it is hard for us to control it because the driver is usually not open source.
The upon results are based on kabylake Rvp3 platform. Maybe I missed something. Any incorrect, please feel free to point out.

Thanks,
Zhichao

> 
> Thanks,
> Laszlo

Re: [edk2-devel] [PATCH 0/2] MdeModulePkg: Make the screen seamless

[Laszlo Ersek]

Hi Zhichao,

On 04/13/19 09:52, Gao, Zhichao wrote:
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, April 12, 2019 4:06 PM
>> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
>> Cc: 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 0/2] MdeModulePkg: Make the screen
>> seamless
>>
>> On 04/12/19 05:14, Gao, Zhichao wrote:
>>> 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.
>>>
>>> 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>
>>>
>>> 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      | 45 +++++++++----------
>>>  3 files changed, 48 insertions(+), 35 deletions(-)
>>>
>>
>> EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() is specified to clear the
>> screen to black. Is this series compatible with that?
>
> No. We only consider the console section.

You answered my question

  "is this compatible with...?"

with "no", so I first thought you meant, "no, this series is
incompatible with the EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode()
specification".

But reading the rest of your reply, I think your answer is,
"EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() is not affected by this series".

Can you confirm that?

Because, I think I'd agree with that. This series modifies two drivers,
and from those, only ConSplitterDxe *produces* a GOP. Therefore my
original question is relevant only for patch#1 (which is the one
modifying ConSplitterDxe).

And in patch #1, indeed it looks like we don't touch the GOP.

(I guess this is also what you mean by "console section" -- that is, the
text console functionality.)

The rest of the discussion refers to patch #2 / GraphicsConsoleDxe:

> There are two pcds to control the graphics output mode
> PcdVideoHorizontalResolution and PcdVideoVerticalResolution. Usually
> we set them as zero to make the mode to be the max mode the graphics
> supported and the graphics output protocol would initialize the mode
> to be the max mode in general. If so the SetMode would not  be runt.
> But that is done in the graphics output driver and the driver is
> usually a binary file. So we can't desire that the graphics driver
> would set the max mode, that is the graphics output driver's vendor
> decided.
> In the other condition, these two pcds would set a value and then
> graphics output driver would focus to set the mode and clear the
> screen. That is controlled by the consumer. By default the two pcds is
> initialized as 800 and 600. Because this resolution may be the most
> normal resolution and the screen would always be cleared.
>
> In my opinion, the behavior of graphics output section in this driver
> is fine and should not be changed. And also, it is hard for us to
> control it because the driver is usually not open source.
> The upon results are based on kabylake Rvp3 platform. Maybe I missed
> something. Any incorrect, please feel free to point out.

The above is all fine, thank you for the answer.

However, now having looked at patch#2 in a bit more detail, I have to
ask my question again, although a bit differently:

In patch #2, we modify the GraphicsConsoleConOutSetMode() function,
to omit clearing the display under some conditions.

The GraphicsConsoleConOutSetMode() function implements
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.SetMode().

According to the UEFI spec,

    The SetMode() function sets the output device(s) to the requested
    mode. On success the device is in the geometry for the requested
    mode, and the device has been cleared to the current background
    color with the cursor at (0,0).

So, if patch #2 implements a EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.SetMode()
member function that does not clear the screen on every call, does it
conform to the spec?

Now, if the "do not clear the screen" exception is not observable /
triggerable by any UEFI driver or UEFI application that is written
against the UEFI spec -- in other words, if the exception only applies
to platform / DXE modules --, then I guess the exception could be fine.

But I'd like to hear others' comments too, and if this behavior is
conformant, then this nuance should be mentioned in the commit message
and/or in a comment, in patch #2.

... Finally, what about controller disconnect/reconnect in the UEFI
shell (or in other UEFI applications)? I assume that the
(This->Mode->Mode == -1) exception would apply again, even though at
that time we wouldn't be starting up just after the PEI phase.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH 0/2] MdeModulePkg: Make the screen seamless

[Gao, Zhichao]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, April 15, 2019 11:56 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: 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 0/2] MdeModulePkg: Make the screen
> seamless
> 
> Hi Zhichao,
> 
> On 04/13/19 09:52, Gao, Zhichao wrote:
> >
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Friday, April 12, 2019 4:06 PM
> >> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> >> Cc: 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 0/2] MdeModulePkg: Make the screen
> >> seamless
> >>
> >> On 04/12/19 05:14, Gao, Zhichao wrote:
> >>> 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.
> >>>
> >>> 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>
> >>>
> >>> 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      | 45 +++++++++----------
> >>>  3 files changed, 48 insertions(+), 35 deletions(-)
> >>>
> >>
> >> EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() is specified to clear the
> >> screen to black. Is this series compatible with that?
> >
> > No. We only consider the console section.
> 
> You answered my question
> 
>   "is this compatible with...?"
> 
> with "no", so I first thought you meant, "no, this series is incompatible with
> the EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode()
> specification".
> 
> But reading the rest of your reply, I think your answer is,
> "EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() is not affected by this
> series".
> 
> Can you confirm that?

Yes. You are right. No effect with the EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode().

> 
> Because, I think I'd agree with that. This series modifies two drivers, and
> from those, only ConSplitterDxe *produces* a GOP. Therefore my original
> question is relevant only for patch#1 (which is the one modifying
> ConSplitterDxe).
> 
> And in patch #1, indeed it looks like we don't touch the GOP.
> 
> (I guess this is also what you mean by "console section" -- that is, the text
> console functionality.)
> 
> The rest of the discussion refers to patch #2 / GraphicsConsoleDxe:
> 
> > There are two pcds to control the graphics output mode
> > PcdVideoHorizontalResolution and PcdVideoVerticalResolution. Usually
> > we set them as zero to make the mode to be the max mode the graphics
> > supported and the graphics output protocol would initialize the mode
> > to be the max mode in general. If so the SetMode would not  be runt.
> > But that is done in the graphics output driver and the driver is
> > usually a binary file. So we can't desire that the graphics driver
> > would set the max mode, that is the graphics output driver's vendor
> > decided.
> > In the other condition, these two pcds would set a value and then
> > graphics output driver would focus to set the mode and clear the
> > screen. That is controlled by the consumer. By default the two pcds is
> > initialized as 800 and 600. Because this resolution may be the most
> > normal resolution and the screen would always be cleared.
> >
> > In my opinion, the behavior of graphics output section in this driver
> > is fine and should not be changed. And also, it is hard for us to
> > control it because the driver is usually not open source.
> > The upon results are based on kabylake Rvp3 platform. Maybe I missed
> > something. Any incorrect, please feel free to point out.
> 
> The above is all fine, thank you for the answer.
> 
> However, now having looked at patch#2 in a bit more detail, I have to ask my
> question again, although a bit differently:
> 
> In patch #2, we modify the GraphicsConsoleConOutSetMode() function, to
> omit clearing the display under some conditions.
> 
> The GraphicsConsoleConOutSetMode() function implements
> EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.SetMode().
> 
> According to the UEFI spec,
> 
>     The SetMode() function sets the output device(s) to the requested
>     mode. On success the device is in the geometry for the requested
>     mode, and the device has been cleared to the current background
>     color with the cursor at (0,0).
> 
> So, if patch #2 implements a
> EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.SetMode()
> member function that does not clear the screen on every call, does it
> conform to the spec?
> 
> Now, if the "do not clear the screen" exception is not observable /
> triggerable by any UEFI driver or UEFI application that is written against the
> UEFI spec -- in other words, if the exception only applies to platform / DXE
> modules --, then I guess the exception could be fine.

This patch only wants to modify the connection behavior of the BDS phase without affecting the results of other phases or other drivers calling this interface. In the beginning, I think if I only change the behavior of the first time the interface running, maybe I would not violate the spec. I also want more comments about whether  it violate the spec.

> 
> But I'd like to hear others' comments too, and if this behavior is conformant,
> then this nuance should be mentioned in the commit message and/or in a
> comment, in patch #2.
> 
> ... Finally, what about controller disconnect/reconnect in the UEFI shell (or in
> other UEFI applications)? I assume that the (This->Mode->Mode == -1)
> exception would apply again, even though at that time we wouldn't be
> starting up just after the PEI phase.

Yes, you are right. We treat - 1 as an uninitialized state, and this state is set only at the driver first running. As you mentioned above, disconnect and reconnect in a shell environment can also lead to this situation, so this patch should be rejected. I should think another way to figure out this issue.

> 
> Thanks,
> Laszlo