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

["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: Aaron Antone; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming;
> Sean Brogan; Michael Turner; Bret Barkelew
> Subject: [PATCH V4 1/2] MdeModulePkg/ConSplitterDxe: Optimize the
> ConSplitterTextOutSetMode
> 
> 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      | 33 ++++++++++++-------
>  .../Console/ConSplitterDxe/ConSplitter.h      |  4 ++-
>  2 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> index 6fc0e4796f..5de022b02a 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,18 @@ 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) ||

Just a minor comment.

BOOLEAN type does not need to be explicitly compared with 'TRUE' or 'FALSE':
  if ((!Private->AddingConOutDevice) ||

With the above handled,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

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