From: "Brian J. Johnson" <bjohnson@sgi.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
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 13:06:17 -0500 [thread overview]
Message-ID: <a346f608-938b-0547-11d4-c2cbf5ce6ced@sgi.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F56483C027@ORSMSX113.amr.corp.intel.com>
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
next prev parent reply other threads:[~2016-10-27 18:06 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 [this message]
2016-10-27 18:14 ` Kinney, Michael D
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=a346f608-938b-0547-11d4-c2cbf5ce6ced@sgi.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