From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 366FC81C03 for ; Thu, 12 Jan 2017 02:47:25 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EED04C050919; Thu, 12 Jan 2017 10:41:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-42.phx2.redhat.com [10.3.116.42]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0CAfjo2006231; Thu, 12 Jan 2017 05:41:45 -0500 To: Ruiyu Ni , edk2-devel@ml01.01.org References: <20170110083904.34104-1-ruiyu.ni@intel.com> <20170110083904.34104-5-ruiyu.ni@intel.com> Cc: Feng Tian , Star Zeng From: Laszlo Ersek Message-ID: <32be930e-69f2-0a7e-7641-b57a464e3092@redhat.com> Date: Thu, 12 Jan 2017 11:41:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170110083904.34104-5-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 12 Jan 2017 10:41:47 +0000 (UTC) Subject: Re: [PATCH 4/8] MdeModulePkg/TerminalDxe: Refine InitializeTerminalConsoleTextMode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Jan 2017 10:47:25 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > Cc: Star Zeng > Cc: Feng Tian > --- > .../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 > // >