Re: [PATCH V4 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Friday, May 24, 2019 10:40 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming; Sean Brogan;
> Michael Turner; Bret Barkelew; Laszlo Ersek
> Subject: [PATCH V4 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the
> output mode
> 
> 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.
> 
> Impact: as the text mode may not be an invalid value, the SetMode
> may have no chance to be called during reconnect the graphics device.
> That means the screen may not be cleaned after finishing reconnect
> operation. There is one common condition: shell command "recoonect -r".
> 
> 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      | 42 ++++++++++++++-----
>  .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf |  2 +
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
> .c
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> e.c
> index 26ea19f300..c042451a9b 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
> 
>  **/
> @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart (
>    EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE    *Mode;
>    UINTN                                SizeOfInfo;
>    EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
> +  INT32                                PreferMode;
> +  INT32                                Index;
> +  UINTN                                Column;
> +  UINTN                                Row;
> +  UINTN                                DefaultColumn;
> +  UINTN                                DefaultRow;
> 
>    ModeNumber = 0;
> 
> @@ -567,16 +573,32 @@ 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;
> +  //
> +  // Initialize the Mode of graphics console devices
> +  //
> +  PreferMode = -1;
> +  DefaultColumn = PcdGet32 (PcdConOutColumn);
> +  DefaultRow = PcdGet32 (PcdConOutRow);
> +  Column = 0;
> +  Row = 0;
> +  for (Index = 0; Index < (INT32)MaxMode; Index++) {
> +    if (DefaultColumn != 0 && DefaultRow != 0) {
> +      if ((Private->ModeData[Index].Columns == DefaultColumn) &&
> +          (Private->ModeData[Index].Rows == DefaultRow)) {
> +        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;
> +  DEBUG ((DEBUG_INFO, "Graphics Console Started, Mode: %d\n",
> PreferMode));

The change looks good to me,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> 
>    //
>    // Install protocol interfaces for the Graphics Console device.
> diff --git
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
> Dxe.inf
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> eDxe.inf
> index f7caa65aa9..bcfd306eee 100644
> ---
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
> Dxe.inf
> +++
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> eDxe.inf
> @@ -65,6 +65,8 @@
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution   ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow                 ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn              ##
> SOMETIMES_CONSUMES
> 
>  [UserExtensions.TianoCore."ExtraFiles"]
>    GraphicsConsoleDxeExtra.uni
> --
> 2.21.0.windows.1