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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Committed at

https://github.com/tianocore/edk2/commit/d1b757e2cd034e32676c5cc2d542f785e74f8c5d


Mike


> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, October 27, 2016 11:14 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
> 
> Brian,
> 
> Thanks.  I will do the commit with your rb.
> 
> Mike
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Brian J.
> Johnson
> > Sent: Thursday, October 27, 2016 11:06 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> > 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
> >
> > 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 <bjohnson@sgi.com>
> >
> > >
> > >> -----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
> >
> >
> > --
> >
> >                                                  Brian
> >
> > --------------------------------------------------------------------
> >
> >     "I have discovered that all human evil comes from this, man's being
> >      unable to sit still in a room."
> >                                             -- Blaise Pascal
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel