public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Kinney, Michael D" <michael.d.kinney@intel.com>
To: "Brian J. Johnson" <bjohnson@sgi.com>,
	"edk2-devel@lists.01.org" <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: [PATCH 2/3] MdeModulePkg/TerminalDxe: Optimize TtyTerm cursor motion
Date: Thu, 27 Oct 2016 18:14:28 +0000	[thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F56483C576@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <a346f608-938b-0547-11d4-c2cbf5ce6ced@sgi.com>

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


  reply	other threads:[~2016-10-27 18:14 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-07 14:53 [PATCH 0/3] MdeModulePkg/TerminalDxe: TtyTerm improvements Brian J. Johnson
2016-10-07 14:53 ` [PATCH 1/3] MdeModulePkg/TerminalDxe: Improve TtyTerm cursor position tracking Brian J. Johnson
2016-10-07 14:53 ` [PATCH 2/3] MdeModulePkg/TerminalDxe: Optimize TtyTerm cursor motion Brian J. Johnson
2016-10-27  3:09   ` Kinney, Michael D
2016-10-27  5:06     ` Tian, Feng
2016-10-27 18:06     ` Brian J. Johnson
2016-10-27 18:14       ` Kinney, Michael D [this message]
2016-10-27 18:32         ` Kinney, Michael D
2016-10-07 14:54 ` [PATCH 3/3] MdeModulePkg/TerminalDxe: Handle more keys with TtyTerm Brian J. Johnson
2016-10-14 20:53   ` Roy Franz (HPE)
2016-10-07 15:56 ` [PATCH 0/3] MdeModulePkg/TerminalDxe: TtyTerm improvements Laszlo Ersek
2016-10-07 15:59   ` Leif Lindholm
2016-10-12  8:17     ` Ryan Harkin
2016-10-14 19:39       ` Brian J. Johnson
2016-10-14 20:37         ` Laszlo Ersek
2016-10-18 15:34           ` Brian J. Johnson
2016-10-26 15:00             ` Brian J. Johnson
2016-10-27  1:01               ` Tian, Feng

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=E92EE9817A31E24EB0585FDF735412F56483C576@ORSMSX113.amr.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