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

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

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