Re: [PATCH 2/3] MdeModulePkg/TerminalDxe: Optimize TtyTerm cursor motion

["Tian, Feng" <feng.tian@intel.com>]

Mike,

You are right, the VS2015x64 IA32 build would fail.

With your fix, the code could pass build.

If there is no objection, I will help to push this fix tomorrow after adding EDKII commit log with your Sign-off and my Review-by.

Thanks
Feng

-----Original Message-----
From: Kinney, Michael D 
Sent: Thursday, October 27, 2016 11:10 AM
To: Brian J. Johnson <bjohnson@sgi.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH 2/3] MdeModulePkg/TerminalDxe: Optimize TtyTerm cursor motion

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

> -----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 <bjohnson@sgi.com>; Tian, Feng 
> <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
> 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 <bjohnson@sgi.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  .../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.<BR>
> +Copyright (C) 2016 Silicon Graphics, Inc. All rights reserved.<BR>
>  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