Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, May 7, 2019 1:42 AM
> To: devel@edk2.groups.io; Gao, Zhichao
> Cc: Wang, Jian J ; Wu, Hao A ;
> Ni, Ray ; Zeng, Star ; Gao, Liming
> ; Sean Brogan ;
> Michael Turner ; Bret Barkelew
>
> Subject: Re: [edk2-devel] [PATCH V3 2/2]
> MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode
>
> On 05/05/19 04:17, Gao, Zhichao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412
> >
> > Original logic:
> > Connect the graphics device -> connect it as graphics consoles and
> > initialize its parameters(Mode = -1, invalid) -> connect it as console
> > spliter and add the device to the list(use SetMode to set mode to the
> > user defined mode or the best mode the devices supported if the mode
> > is invalid. *clear the screen at this phase*)
> >
> > Changed logic:
> > Connect the graphics device -> connect it as graphics consoles and
> > initialize its parameters(initialize the mode to the user defined mode
> > or the best mode. *directly set the mode value without using SetMode,
> > that would not clear the screen) -> connect it as console spliter and
> > add the device to the list(use SetMode to set mode to the user defined
> > mode or the best mode the devices supported if the mode is invalid.
> > *now the mode is already set, so it would not clear the screen*).
> >
> > Also remove the section of SetMode for debug version.
> >
> > But there would be no clear screen operation while reconnect the
> > graphics device. Instead, clear the screen when stop the graphics
> > console device.
> >
> > Cc: Jian J Wang
> > Cc: Hao Wu
> > Cc: Ray Ni
> > Cc: Star Zeng
> > Cc: Liming Gao
> > Cc: Sean Brogan
> > Cc: Michael Turner
> > Cc: Bret Barkelew
> > Cc: Laszlo Ersek
> > Signed-off-by: Zhichao Gao
> > ---
> > .../GraphicsConsoleDxe/GraphicsConsole.c | 44 ++-
> > .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 2 +
> > 2 files changed, 36 insertions(+), 10 deletions(-)
>
> This driver is too complex for me to decide whether this patch is correct.
>
> The spec writes:
>
> > If the output device is not in a valid text mode at the time of the
> > EFI_BOOT_SERVICES.HandleProtocol() call, the device is to indicate
> > that its CurrentMode is -1.
>
> (1) Because the patch sets a value different from -1 to CurrentMode, can we
> guarantee that the console will be usable immediately?
I think we can guarantee that the CurrentMode value is valid except the value
is 0 or -1. Because the initialize mode section is same with the one in
ConSplitterDxe.
>
>
> More comments below:
>
> On 05/05/19 04:17, Gao, Zhichao wrote:
> > diff --git
> >
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
> .c
> >
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> e.c
> > index 26ea19f300..42386de3f5 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.
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart (
> >EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE*Mode;
> >UINTNSizeOfInfo;
> >EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
> > + UINTNPreferMode;
> > + UINTNIndex;
> > + UINTNColumn;
> > + UINTNRow;
> > + UINTNDefaultColumn;
> > + UINTNDefaultRow;
> >
> >ModeNumber = 0;
> >
> > @@ -567,16 +573,32 @@ GraphicsConsoleControllerDriverStart (
> >//
> >Private->SimpleTextOutputMode.MaxMode = (INT32) MaxMode;
> >
> > - DEBUG_CODE_BEGIN ();
> > -Status = GraphicsConsoleConOutSetMode (
> >SimpleTextOutput, 0);
> > -if (EFI_ERROR (Status)) {
> > - goto Error;
> > -}
> > -Status = GraphicsConsoleConOutO
Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode
On 05/05/19 04:17, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412
>
> Original logic:
> Connect the graphics device -> connect it as graphics consoles
> and initialize its parameters(Mode = -1, invalid) -> connect it
> as console spliter and add the device to the list(use SetMode to
> set mode to the user defined mode or the best mode the devices
> supported if the mode is invalid. *clear the screen at this phase*)
>
> Changed logic:
> Connect the graphics device -> connect it as graphics consoles
> and initialize its parameters(initialize the mode to the user
> defined mode or the best mode. *directly set the mode value without
> using SetMode, that would not clear the screen) -> connect it as
> console spliter and add the device to the list(use SetMode to set
> mode to the user defined mode or the best mode the devices supported
> if the mode is invalid. *now the mode is already set, so it would
> not clear the screen*).
>
> Also remove the section of SetMode for debug version.
>
> But there would be no clear screen operation while reconnect the
> graphics device. Instead, clear the screen when stop the graphics
> console device.
>
> Cc: Jian J Wang
> Cc: Hao Wu
> Cc: Ray Ni
> Cc: Star Zeng
> Cc: Liming Gao
> Cc: Sean Brogan
> Cc: Michael Turner
> Cc: Bret Barkelew
> Cc: Laszlo Ersek
> Signed-off-by: Zhichao Gao
> ---
> .../GraphicsConsoleDxe/GraphicsConsole.c | 44 ++-
> .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 2 +
> 2 files changed, 36 insertions(+), 10 deletions(-)
This driver is too complex for me to decide whether this patch is
correct.
The spec writes:
> If the output device is not in a valid text mode at the time of the
> EFI_BOOT_SERVICES.HandleProtocol() call, the device is to indicate
> that its CurrentMode is -1.
(1) Because the patch sets a value different from -1 to CurrentMode, can
we guarantee that the console will be usable immediately?
More comments below:
On 05/05/19 04:17, Gao, Zhichao wrote:
> diff --git
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> index 26ea19f300..42386de3f5 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.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart (
>EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE*Mode;
>UINTNSizeOfInfo;
>EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
> + UINTNPreferMode;
> + UINTNIndex;
> + UINTNColumn;
> + UINTNRow;
> + UINTNDefaultColumn;
> + UINTNDefaultRow;
>
>ModeNumber = 0;
>
> @@ -567,16 +573,32 @@ GraphicsConsoleControllerDriverStart (
>//
>Private->SimpleTextOutputMode.MaxMode = (INT32) MaxMode;
>
> - DEBUG_CODE_BEGIN ();
> -Status = GraphicsConsoleConOutSetMode (>SimpleTextOutput, 0);
> -if (EFI_ERROR (Status)) {
> - goto Error;
> -}
> -Status = GraphicsConsoleConOutOutputString (>SimpleTextOutput,
> (CHAR16 *)L"Graphics Console Started\n\r");
> -if (EFI_ERROR (Status)) {
> - goto Error;
> + //
> + // Initialize the Mode of graphics console devices
> + //
> + PreferMode = 0;
> + DefaultColumn = PcdGet32 (PcdConOutColumn);
> + DefaultRow = PcdGet32 (PcdConOutRow);
> + Column = 0;
> + Row = 0;
> + for (Index = 0; Index < MaxMode; Index++) {
> +if (DefaultColumn != 0 && DefaultRow != 0) {
> + if ((Private->ModeData[Index].Columns == Column) &&
> + (Private->ModeData[Index].Rows == Row)) {
> +PreferMode = Index;
> +break;
> + }
> +} else {
> + if ((Private->ModeData[Index].Columns > Column) &&
> + (Private->ModeData[Index].Rows > Row)) {
> +Column = Private->ModeData[Index].Columns;
> +Row = Private->ModeData[Index].Rows;
> +PreferMode = Index;
> + }
> }
> - DEBUG_CODE_END ();
> + }
> + Private->SimpleTextOutput.Mode->Mode = (INT32)PreferMode;
(2) The loop above can terminate without assigning Index to PreferMode.
In that case, the assignment right after the loop will set the Mode
field to 0, when overwriting the initial value (-1).
Is this intentional?
Because, if we can find no matching mode, it seems like we should stick
with (-1) -- for "current text mode is not valid". In that case, the
following passage from the spec would apply:
> On
