public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ruiyu" <ruiyu.ni@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
	"edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>
Cc: "Tian, Feng" <feng.tian@intel.com>, "Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH 4/8] MdeModulePkg/TerminalDxe: Refine InitializeTerminalConsoleTextMode
Date: Thu, 12 Jan 2017 15:39:43 +0000	[thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5B87CAE9@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <32be930e-69f2-0a7e-7641-b57a464e3092@redhat.com>

Laszlo,
Thanks for your reply.
I will remove the comments in C function.

Regards,
Ray

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, January 12, 2017 6:42 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@ml01.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 4/8] MdeModulePkg/TerminalDxe: Refine InitializeTerminalConsoleTextMode

Ray,

sorry for the late followup, I just noticed a (trivial) omission in the
patch:

On 01/10/17 09:39, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Cc: Feng Tian <feng.tian@intel.com<mailto:feng.tian@intel.com>>
> ---
>  .../Universal/Console/TerminalDxe/Terminal.c       | 108 +++++----------------
>  1 file changed, 26 insertions(+), 82 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> index f1ce12d..4b0c00d 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> @@ -113,6 +113,8 @@ TERMINAL_DEV  mTerminalDevTemplate = {
>  };
>
>  TERMINAL_CONSOLE_MODE_DATA mTerminalConsoleModeData[] = {
> +  {80,  25},
> +  {80,  50},
>    {100, 31},
>    //
>    // New modes can be added here.
> @@ -450,97 +452,39 @@ TerminalFreeNotifyList (
>    It returns information for available text modes that the terminal can support.
>
>    @param[out] TextModeCount      The total number of text modes that terminal console supports.
> -  @param[out] TextModeData       The buffer to the text modes column and row information.
> -                                 Caller is responsible to free it when it's non-NULL.
>
> -  @retval EFI_SUCCESS            The supporting mode information is returned.
> -  @retval EFI_INVALID_PARAMETER  The parameters are invalid.
> +  @return   The buffer to the text modes column and row information.
> +            Caller is responsible to free it when it's non-NULL.
>
>  **/
> -EFI_STATUS
> +TERMINAL_CONSOLE_MODE_DATA *
>  InitializeTerminalConsoleTextMode (
> -  OUT UINTN                         *TextModeCount,
> -  OUT TERMINAL_CONSOLE_MODE_DATA    **TextModeData
> -  )
> +  OUT INT32                         *TextModeCount
> +)
>  {
> -  UINTN                       Index;
> -  UINTN                       Count;
> -  TERMINAL_CONSOLE_MODE_DATA  *ModeBuffer;
> -  TERMINAL_CONSOLE_MODE_DATA  *NewModeBuffer;
> -  UINTN                       ValidCount;
> -  UINTN                       ValidIndex;
> -
> -  if ((TextModeCount == NULL) || (TextModeData == NULL)) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  Count = sizeof (mTerminalConsoleModeData) / sizeof (TERMINAL_CONSOLE_MODE_DATA);
> -
> -  //
> -  // Get defined mode buffer pointer.
> -  //
> -  ModeBuffer = mTerminalConsoleModeData;
> -
> +  TERMINAL_CONSOLE_MODE_DATA  *TextModeData;
> +
> +  ASSERT (TextModeCount != NULL);
> +
>    //
>    // Here we make sure that the final mode exposed does not include the duplicated modes,
>    // and does not include the invalid modes which exceed the max column and row.
>    // Reserve 2 modes for 80x25, 80x50 of terminal console.
>    //

This comment too should have been removed, given that no reservation
occurs any longer; the 80x25 and 80x50 modes have been moved to
"mTerminalConsoleModeData", and are copied like all the other modes in
that array, with AllocateCopyPool().

Thanks
Laszlo


> -  NewModeBuffer = AllocateZeroPool (sizeof (TERMINAL_CONSOLE_MODE_DATA) * (Count + 2));
> -  ASSERT (NewModeBuffer != NULL);
> -
> -  //
> -  // Mode 0 and mode 1 is for 80x25, 80x50 according to UEFI spec.
> -  //
> -  ValidCount = 0;
> -
> -  NewModeBuffer[ValidCount].Columns = 80;
> -  NewModeBuffer[ValidCount].Rows    = 25;
> -  ValidCount++;
> -
> -  NewModeBuffer[ValidCount].Columns = 80;
> -  NewModeBuffer[ValidCount].Rows    = 50;
> -  ValidCount++;
> -
> -  //
> -  // Start from mode 2 to put the valid mode other than 80x25 and 80x50 in the output mode buffer.
> -  //
> -  for (Index = 0; Index < Count; Index++) {
> -    if ((ModeBuffer[Index].Columns == 0) || (ModeBuffer[Index].Rows == 0)) {
> -      //
> -      // Skip the pre-defined mode which is invalid.
> -      //
> -      continue;
> -    }
> -    for (ValidIndex = 0; ValidIndex < ValidCount; ValidIndex++) {
> -      if ((ModeBuffer[Index].Columns == NewModeBuffer[ValidIndex].Columns) &&
> -          (ModeBuffer[Index].Rows == NewModeBuffer[ValidIndex].Rows)) {
> -        //
> -        // Skip the duplicated mode.
> -        //
> -        break;
> -      }
> -    }
> -    if (ValidIndex == ValidCount) {
> -      NewModeBuffer[ValidCount].Columns = ModeBuffer[Index].Columns;
> -      NewModeBuffer[ValidCount].Rows    = ModeBuffer[Index].Rows;
> -      ValidCount++;
> -    }
> +  TextModeData = AllocateCopyPool (sizeof (mTerminalConsoleModeData), mTerminalConsoleModeData);
> +  if (TextModeData == NULL) {
> +    return NULL;
>    }
> -
> +  *TextModeCount = ARRAY_SIZE (mTerminalConsoleModeData);
> +
>    DEBUG_CODE (
> -    for (Index = 0; Index < ValidCount; Index++) {
> -      DEBUG ((EFI_D_INFO, "Terminal - Mode %d, Column = %d, Row = %d\n",
> -                           Index, NewModeBuffer[Index].Columns, NewModeBuffer[Index].Rows));
> +    INT32 Index;
> +    for (Index = 0; Index < *TextModeCount; Index++) {
> +      DEBUG ((DEBUG_INFO, "Terminal - Mode %d, Column = %d, Row = %d\n",
> +              Index, TextModeData[Index].Columns, TextModeData[Index].Rows));
>      }
>    );
> -
> -  //
> -  // Return valid mode count and mode information buffer.
> -  //
> -  *TextModeCount = ValidCount;
> -  *TextModeData  = NewModeBuffer;
> -  return EFI_SUCCESS;
> +  return TextModeData;
>  }
>
>  /**
> @@ -632,7 +576,6 @@ TerminalDriverBindingStart (
>    BOOLEAN                             SimTxtInInstalled;
>    BOOLEAN                             SimTxtOutInstalled;
>    BOOLEAN                             FirstEnter;
> -  UINTN                               ModeCount;
>
>    TerminalDevice     = NULL;
>    ConInSelected      = FALSE;
> @@ -896,12 +839,13 @@ TerminalDriverBindingStart (
>                           );
>      SimpleTextOutput->Mode = &TerminalDevice->SimpleTextOutputMode;
>
> -    Status = InitializeTerminalConsoleTextMode (&ModeCount, &TerminalDevice->TerminalConsoleModeData);
> -    if (EFI_ERROR (Status)) {
> +    TerminalDevice->TerminalConsoleModeData = InitializeTerminalConsoleTextMode (
> +                                                &SimpleTextOutput->Mode->MaxMode
> +                                                );
> +    if (TerminalDevice->TerminalConsoleModeData == NULL) {
>        goto ReportError;
>      }
> -    TerminalDevice->SimpleTextOutputMode.MaxMode = (INT32) ModeCount;
> -
> +
>      //
>      // For terminal devices, cursor is always visible
>      //
>


  reply	other threads:[~2017-01-12 15:39 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-10  8:38 [PATCH 0/8] Fix TerminalDxe driver model bug Ruiyu Ni
2017-01-10  8:38 ` [PATCH 1/8] MdeModulePkg/TerminalDxe: Replace macro with enum for terminal types Ruiyu Ni
2017-01-11  7:32   ` Tian, Feng
2017-01-10  8:38 ` [PATCH 2/8] MdeModulePkg/TerminalDxe: Add TerminalTypeFromGuid internal function Ruiyu Ni
2017-01-10  8:38 ` [PATCH 3/8] MdeModulePkg/TerminalDxe: Separate controller name init logic Ruiyu Ni
2017-01-10  8:39 ` [PATCH 4/8] MdeModulePkg/TerminalDxe: Refine InitializeTerminalConsoleTextMode Ruiyu Ni
2017-01-12 10:41   ` Laszlo Ersek
2017-01-12 15:39     ` Ni, Ruiyu [this message]
2017-01-10  8:39 ` [PATCH 5/8] MdeModulePkg/TerminalDxe: Refine SetTerminalDevicePath Ruiyu Ni
2017-01-10  8:39 ` [PATCH 6/8] MdeModulePkg/TerminalDxe: Separate state machine start/stop logic Ruiyu Ni
2017-01-10  8:39 ` [PATCH 7/8] MdeModulePkg/TerminalDxe: Remove unnecessary NULL pointer check Ruiyu Ni
2017-01-10  8:39 ` [PATCH 8/8] MdeModulePkg/TerminalDxe: Fix driver model bug Ruiyu Ni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=734D49CCEBEEF84792F5B80ED585239D5B87CAE9@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox