From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay3.sgi.com [192.48.152.1]) by ml01.01.org (Postfix) with ESMTP id 513A11A1E28 for ; Thu, 27 Oct 2016 11:06:20 -0700 (PDT) Received: from xmail.sgi.com (pv-excas3-dc21.corp.sgi.com [137.38.106.11]) by relay3.corp.sgi.com (Postfix) with ESMTP id 5D998AC001; Thu, 27 Oct 2016 11:06:18 -0700 (PDT) Received: from [128.162.232.243] (128.162.232.243) by xmail.sgi.com (137.38.106.6) with Microsoft SMTP Server (TLS) id 14.3.301.0; Thu, 27 Oct 2016 13:06:18 -0500 To: "Kinney, Michael D" , "edk2-devel@lists.01.org" References: CC: "Tian, Feng" , "Zeng, Star" From: "Brian J. Johnson" Message-ID: Date: Thu, 27 Oct 2016 13:06:17 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [128.162.232.243] Subject: Re: [PATCH 2/3] MdeModulePkg/TerminalDxe: Optimize TtyTerm cursor motion 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, 27 Oct 2016 18:06:20 -0000 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit On 10/26/2016 10:09 PM, Kinney, Michael D wrote: > Tian Feng, > > Unfortunately, this patch that was pushed to edk2/master today > breaks on IA32 VS2015x86 builds with a signed/unsigned mismatch > on 3 lines. I think the right fix might be: > > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c > index e9b5ed0..9625f4d 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c > @@ -790,13 +790,13 @@ TerminalConOutSetCursorPosition ( > // it isn't necessary. > // > if (TerminalDevice->TerminalType == TTYTERMTYPE && > - Mode->CursorRow == Row) { > - if (Mode->CursorColumn > Column) { > + (UINTN)Mode->CursorRow == Row) { > + if ((UINTN)Mode->CursorColumn > Column) { > mCursorBackwardString[FW_BACK_OFFSET + 0] = (CHAR16) ('0' + ((Mode->CursorColumn - Column) / 10)); > mCursorBackwardString[FW_BACK_OFFSET + 1] = (CHAR16) ('0' + ((Mode->CursorColumn - Column) % 10)); > String = mCursorBackwardString; > } > - else if (Column > Mode->CursorColumn) { > + else if (Column > (UINTN)Mode->CursorColumn) { > mCursorForwardString[FW_BACK_OFFSET + 0] = (CHAR16) ('0' + ((Column - Mode->CursorColumn) / 10)); > mCursorForwardString[FW_BACK_OFFSET + 1] = (CHAR16) ('0' + ((Column - Mode->CursorColumn) % 10)); > String = mCursorForwardString; > > Please see if you can reproduce this issue. > > Thanks, > > Mike The changes look fine to me. Unfortunately, I don't have that compiler to test with. If it's needed, Reviewed-by: Brian Johnson > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Brian J. Johnson >> Sent: Friday, October 7, 2016 7:54 AM >> To: edk2-devel@lists.01.org >> Cc: Brian J. Johnson ; Tian, Feng ; Zeng, Star >> >> Subject: [edk2] [PATCH 2/3] MdeModulePkg/TerminalDxe: Optimize TtyTerm cursor motion >> >> For TtyTerm terminals, output a shorter escape sequence when possible >> to move the cursor within the current line, and don't print any escape >> sequence if the cursor is already at the correct position. This >> removes extra cursor motion activity at the EFI shell prompt, >> improving performance. It also makes it possible in many cases to >> successfully use a terminal window which is taller than the driver's >> mode setting (eg. 80x25.) >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Brian Johnson >> Cc: Feng Tian >> Cc: Star Zeng >> --- >> .../Universal/Console/TerminalDxe/Terminal.h | 2 ++ >> .../Universal/Console/TerminalDxe/TerminalConOut.c | 36 +++++++++++++++++++--- >> 2 files changed, 33 insertions(+), 5 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >> index 269d2ae..3ee3969 100644 >> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h >> @@ -2,6 +2,7 @@ >> Header file for Terminal driver. >> >> Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
>> +Copyright (C) 2016 Silicon Graphics, Inc. All rights reserved.
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> which accompanies this distribution. The full text of the license may be found at >> @@ -157,6 +158,7 @@ typedef union { >> #define BACKGROUND_CONTROL_OFFSET 11 >> #define ROW_OFFSET 2 >> #define COLUMN_OFFSET 5 >> +#define FW_BACK_OFFSET 2 >> >> typedef struct { >> UINT16 Unicode; >> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c >> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c >> index b11e83f..e9b5ed0 100644 >> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c >> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c >> @@ -83,6 +83,8 @@ CHAR16 mSetModeString[] = { ESC, '[', '=', '3', 'h', 0 }; >> CHAR16 mSetAttributeString[] = { ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', >> ESC, '[', '4', '0', 'm', 0 }; >> CHAR16 mClearScreenString[] = { ESC, '[', '2', 'J', 0 }; >> CHAR16 mSetCursorPositionString[] = { ESC, '[', '0', '0', ';', '0', '0', 'H', 0 }; >> +CHAR16 mCursorForwardString[] = { ESC, '[', '0', '0', 'C', 0 }; >> +CHAR16 mCursorBackwardString[] = { ESC, '[', '0', '0', 'D', 0 }; >> >> // >> // Body of the ConOut functions >> @@ -755,6 +757,7 @@ TerminalConOutSetCursorPosition ( >> UINTN MaxRow; >> EFI_STATUS Status; >> TERMINAL_DEV *TerminalDevice; >> + CHAR16 *String; >> >> TerminalDevice = TERMINAL_CON_OUT_DEV_FROM_THIS (This); >> >> @@ -782,13 +785,36 @@ TerminalConOutSetCursorPosition ( >> // >> // control sequence to move the cursor >> // >> - mSetCursorPositionString[ROW_OFFSET + 0] = (CHAR16) ('0' + ((Row + 1) / 10)); >> - mSetCursorPositionString[ROW_OFFSET + 1] = (CHAR16) ('0' + ((Row + 1) % 10)); >> - mSetCursorPositionString[COLUMN_OFFSET + 0] = (CHAR16) ('0' + ((Column + 1) / 10)); >> - mSetCursorPositionString[COLUMN_OFFSET + 1] = (CHAR16) ('0' + ((Column + 1) % 10)); >> + // Optimize cursor motion control sequences for TtyTerm. Move >> + // within the current line if possible, and don't output anyting if >> + // it isn't necessary. >> + // >> + if (TerminalDevice->TerminalType == TTYTERMTYPE && >> + Mode->CursorRow == Row) { >> + if (Mode->CursorColumn > Column) { >> + mCursorBackwardString[FW_BACK_OFFSET + 0] = (CHAR16) ('0' + ((Mode->CursorColumn >> - Column) / 10)); >> + mCursorBackwardString[FW_BACK_OFFSET + 1] = (CHAR16) ('0' + ((Mode->CursorColumn >> - Column) % 10)); >> + String = mCursorBackwardString; >> + } >> + else if (Column > Mode->CursorColumn) { >> + mCursorForwardString[FW_BACK_OFFSET + 0] = (CHAR16) ('0' + ((Column - Mode- >>> CursorColumn) / 10)); >> + mCursorForwardString[FW_BACK_OFFSET + 1] = (CHAR16) ('0' + ((Column - Mode- >>> CursorColumn) % 10)); >> + String = mCursorForwardString; >> + } >> + else { >> + String = L""; // No cursor motion necessary >> + } >> + } >> + else { >> + mSetCursorPositionString[ROW_OFFSET + 0] = (CHAR16) ('0' + ((Row + 1) / 10)); >> + mSetCursorPositionString[ROW_OFFSET + 1] = (CHAR16) ('0' + ((Row + 1) % 10)); >> + mSetCursorPositionString[COLUMN_OFFSET + 0] = (CHAR16) ('0' + ((Column + 1) / >> 10)); >> + mSetCursorPositionString[COLUMN_OFFSET + 1] = (CHAR16) ('0' + ((Column + 1) % >> 10)); >> + String = mSetCursorPositionString; >> + } >> >> TerminalDevice->OutputEscChar = TRUE; >> - Status = This->OutputString (This, mSetCursorPositionString); >> + Status = This->OutputString (This, String); >> TerminalDevice->OutputEscChar = FALSE; >> >> if (EFI_ERROR (Status)) { >> -- >> 2.7.4 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel -- Brian -------------------------------------------------------------------- "I have discovered that all human evil comes from this, man's being unable to sit still in a room." -- Blaise Pascal