* [PATCH v1 0/3] MdeModulePkg/GraphicsConsole : Console cleanup
@ 2020-11-24 19:15 Samer El-Haj-Mahmoud
2020-11-24 19:15 ` [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE Samer El-Haj-Mahmoud
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-11-24 19:15 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Zhichao Gao, Ray Ni, Ard Biesheuvel,
Pete Batard
This series fixes a few minor issues in the GraphicsConsole
and ConSplitter
1. Remove extra cursor on graphics console during boot logo
2. Fix misc spelling mistakes
3. Change StdErr color from MAGENTA to LIGHTGRAY
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Samer El-Haj-Mahmoud (3):
MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE
MdeModulePkg/Graphics: Fix spelling mistakes
MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h | 8 +--
MdePkg/Include/Protocol/SimpleTextOut.h | 6 +-
MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 68 ++++++++++----------
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c | 14 ++--
4 files changed, 48 insertions(+), 48 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE
2020-11-24 19:15 [PATCH v1 0/3] MdeModulePkg/GraphicsConsole : Console cleanup Samer El-Haj-Mahmoud
@ 2020-11-24 19:15 ` Samer El-Haj-Mahmoud
2020-12-01 0:45 ` Gao, Zhichao
2020-11-24 19:15 ` [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes Samer El-Haj-Mahmoud
2020-11-24 19:15 ` [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY Samer El-Haj-Mahmoud
2 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-11-24 19:15 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Zhichao Gao, Ray Ni, Ard Biesheuvel,
Pete Batard
REF: https://github.com/pftf/RPi4/issues/115
GraphicsConsoleDxe defaults the ConOut Mode.CursorVisible to TRUE.
However, the driver never draws the cursor during init. This results
in the first call to disable the cursor (using ConOut->EnableCursor(FALSE))
to actually draw the cursor on the screen, as the logic in FlushCursor depends
on the Mode.CursorVisible state to determine if it should draw or erase
the cursor.
Fix by changing the default CursorVisible in this driver to FALSE.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
---
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
index c042451a9b52..6b8d11d587d1 100644
--- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
+++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
@@ -33,7 +33,7 @@ GRAPHICS_CONSOLE_DEV mGraphicsConsoleDevTemplate = {
EFI_TEXT_ATTR(EFI_LIGHTGRAY, EFI_BLACK),
0,
0,
- TRUE
+ FALSE
},
(GRAPHICS_CONSOLE_MODE_DATA *) NULL,
(EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) NULL
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes
2020-11-24 19:15 [PATCH v1 0/3] MdeModulePkg/GraphicsConsole : Console cleanup Samer El-Haj-Mahmoud
2020-11-24 19:15 ` [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE Samer El-Haj-Mahmoud
@ 2020-11-24 19:15 ` Samer El-Haj-Mahmoud
2020-12-01 0:45 ` Gao, Zhichao
2020-11-24 19:15 ` [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY Samer El-Haj-Mahmoud
2 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-11-24 19:15 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Zhichao Gao, Ray Ni, Ard Biesheuvel,
Pete Batard
Fix various spelling mistakes in GraphicsConsoleDxe, ConsPlitter,
and SimpleTextOut header
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
---
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h | 8 +--
MdePkg/Include/Protocol/SimpleTextOut.h | 6 +-
MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 66 ++++++++++----------
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c | 12 ++--
4 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h
index 28d47ac7cb1e..11d254b70f32 100644
--- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h
+++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h
@@ -205,7 +205,7 @@ GraphicsConsoleComponentNameGetControllerName (
Reset the text output device hardware and optionally run diagnostics.
Implements SIMPLE_TEXT_OUTPUT.Reset().
- If ExtendeVerification is TRUE, then perform dependent Graphics Console
+ If ExtendedVerification is TRUE, then perform dependent Graphics Console
device reset, and set display mode to mode 0.
If ExtendedVerification is FALSE, only set display mode to mode 0.
@@ -286,7 +286,7 @@ GraphicsConsoleConOutTestString (
supports
Implements SIMPLE_TEXT_OUTPUT.QueryMode().
- It returnes information for an available text mode that the Graphics Console supports.
+ It returns information for an available text mode that the Graphics Console supports.
In this driver,we only support text mode 80x25, which is defined as mode 0.
@param This Protocol instance pointer.
@@ -422,7 +422,7 @@ GraphicsConsoleConOutEnableCursor (
/**
Test to see if Graphics Console could be supported on the Controller.
- Graphics Console could be supported if Graphics Output Protocol or UGA Draw
+ Graphics Console could be supported if Graphics Output Protocol or UGADraw
Protocol exists on the Controller. (UGA Draw Protocol could be skipped
if PcdUgaConsumeSupport is set to FALSE.)
@@ -510,7 +510,7 @@ EfiLocateHiiProtocol (
/**
- Gets Graphics Console devcie's foreground color and background color.
+ Gets Graphics Console device's foreground color and background color.
@param This Protocol instance pointer.
@param Foreground Returned text foreground color.
diff --git a/MdePkg/Include/Protocol/SimpleTextOut.h b/MdePkg/Include/Protocol/SimpleTextOut.h
index a849c08d66df..100d69a23a9b 100644
--- a/MdePkg/Include/Protocol/SimpleTextOut.h
+++ b/MdePkg/Include/Protocol/SimpleTextOut.h
@@ -32,7 +32,7 @@ typedef struct _EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL;
typedef EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL SIMPLE_TEXT_OUTPUT_INTERFACE;
//
-// Define's for required EFI Unicode Box Draw characters
+// Defines for required EFI Unicode Box Draw characters
//
#define BOXDRAW_HORIZONTAL 0x2500
#define BOXDRAW_VERTICAL 0x2502
@@ -151,7 +151,7 @@ typedef EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL SIMPLE_TEXT_OUTPUT_INTERFACE;
#define EFI_WIDE_ATTRIBUTE 0x80
/**
- Reset the text output device hardware and optionaly run diagnostics
+ Reset the text output device hardware and optionally run diagnostics
@param This The protocol instance pointer.
@param ExtendedVerification Driver may perform more exhaustive verification
@@ -373,7 +373,7 @@ typedef struct {
///
INT32 CursorRow;
///
- /// The cursor is currently visbile or not.
+ /// The cursor is currently visible or not.
///
BOOLEAN CursorVisible;
} EFI_SIMPLE_TEXT_OUTPUT_MODE;
diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
index 9c38271b65f9..b090de288517 100644
--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
@@ -1,5 +1,5 @@
/** @file
- Console Splitter Driver. Any Handle that attatched console I/O protocols
+ Console Splitter Driver. Any Handle that attached console I/O protocols
(Console In device, Console Out device, Console Error device, Simple Pointer
protocol, Absolute Pointer protocol) can be bound by this driver.
@@ -13,7 +13,7 @@
Each virtual handle, that supports the Console I/O protocol, will be produced
in the driver entry point. The virtual handle are added on driver entry and
- never removed. Such design ensures sytem function well during none console
+ never removed. Such design ensures system function well during none console
device situation.
Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
@@ -381,7 +381,7 @@ ToggleStateSyncInitialization (
}
/**
- Reinitialization for toggle state sync.
+ Re-initialization for toggle state sync.
@param Private Text In Splitter pointer.
@@ -594,7 +594,7 @@ ConSplitterDriverEntry(
structure.
@retval EFI_OUT_OF_RESOURCES Out of resources.
- @retval EFI_SUCCESS Text Input Devcie's private data has been constructed.
+ @retval EFI_SUCCESS Text Input Device's private data has been constructed.
@retval other Failed to construct private data.
**/
@@ -761,7 +761,7 @@ ConSplitterTextOutConstructor (
}
//
- // Initilize console output splitter's private data.
+ // Initialize console output splitter's private data.
//
ConOutPrivate->TextOut.Mode = &ConOutPrivate->TextOutMode;
@@ -860,7 +860,7 @@ ConSplitterTextOutConstructor (
@param Guid The specified protocol.
@retval EFI_SUCCESS The specified protocol is supported on this device.
- @retval EFI_UNSUPPORTED The specified protocol attempts to be installed on virtul handle.
+ @retval EFI_UNSUPPORTED The specified protocol attempts to be installed on virtual handle.
@retval other Failed to open specified protocol on this device.
**/
@@ -1108,7 +1108,7 @@ ConSplitterStart (
}
//
- // Open InterfaceGuid on the virtul handle.
+ // Open InterfaceGuid on the virtual handle.
//
Status = gBS->OpenProtocol (
ControllerHandle,
@@ -1174,7 +1174,7 @@ ConSplitterConInDriverBindingStart (
//
// Start ConSplitter on ControllerHandle, and create the virtual
- // agrogated console device on first call Start for a SimpleTextIn handle.
+ // aggregated console device on first call Start for a SimpleTextIn handle.
//
Status = ConSplitterStart (
This,
@@ -1241,7 +1241,7 @@ ConSplitterSimplePointerDriverBindingStart (
//
// Start ConSplitter on ControllerHandle, and create the virtual
- // agrogated console device on first call Start for a SimplePointer handle.
+ // aggregated console device on first call Start for a SimplePointer handle.
//
Status = ConSplitterStart (
This,
@@ -1287,7 +1287,7 @@ ConSplitterAbsolutePointerDriverBindingStart (
//
// Start ConSplitter on ControllerHandle, and create the virtual
- // agrogated console device on first call Start for a AbsolutePointer handle.
+ // aggregated console device on first call Start for a AbsolutePointer handle.
//
Status = ConSplitterStart (
This,
@@ -1338,7 +1338,7 @@ ConSplitterConOutDriverBindingStart (
//
// Start ConSplitter on ControllerHandle, and create the virtual
- // agrogated console device on first call Start for a ConsoleOut handle.
+ // aggregated console device on first call Start for a ConsoleOut handle.
//
Status = ConSplitterStart (
This,
@@ -1451,7 +1451,7 @@ ConSplitterStdErrDriverBindingStart (
//
// Start ConSplitter on ControllerHandle, and create the virtual
- // agrogated console device on first call Start for a StandardError handle.
+ // aggregated console device on first call Start for a StandardError handle.
//
Status = ConSplitterStart (
This,
@@ -1522,7 +1522,7 @@ ConSplitterStop (
return Status;
}
//
- // close the protocol refered.
+ // close the protocol referred.
//
gBS->CloseProtocol (
ControllerHandle,
@@ -1543,7 +1543,7 @@ ConSplitterStop (
/**
- Stop Console In ConSplitter on ControllerHandle by closing Console In Devcice GUID.
+ Stop Console In ConSplitter on ControllerHandle by closing Console In Device GUID.
@param This Driver Binding protocol instance pointer.
@param ControllerHandle Handle of device to stop driver on
@@ -1718,7 +1718,7 @@ ConSplitterAbsolutePointerDriverBindingStop (
/**
- Stop Console Out ConSplitter on device handle by closing Console Out Devcice GUID.
+ Stop Console Out ConSplitter on device handle by closing Console Out Devcie GUID.
@param This Driver Binding protocol instance pointer.
@param ControllerHandle Handle of device to stop driver on
@@ -2725,7 +2725,7 @@ ConSplitterGetIntersectionBetweenConOutAndStrErr (
/**
- Add Grahpics Output modes into Consplitter Text Out list.
+ Add Graphics Output modes into Consplitter Text Out list.
@param Private Text Out Splitter pointer.
@param GraphicsOutput Graphics Output protocol pointer.
@@ -3392,7 +3392,7 @@ ConSplitterTextOutDeleteDevice (
return EFI_SUCCESS;
}
//
- // Max Mode is realy an intersection of the QueryMode command to all
+ // Max Mode is really an intersection of the QueryMode command to all
// devices. So we must copy the QueryMode of the first device to
// QueryData.
//
@@ -3430,7 +3430,7 @@ ConSplitterTextOutDeleteDevice (
/**
- Reset the input device and optionaly run diagnostics
+ Reset the input device and optionally run diagnostics
@param This Protocol instance pointer.
@param ExtendedVerification Driver may perform diagnostics on reset.
@@ -3514,7 +3514,7 @@ ConSplitterTextInExDequeueKey (
/**
Reads the next keystroke from the input device. The WaitForKey Event can
- be used to test for existance of a keystroke via WaitForEvent () call.
+ be used to test for existence of a keystroke via WaitForEvent () call.
@param Private Protocol instance pointer.
@param Key Driver may perform diagnostics on reset.
@@ -3587,7 +3587,7 @@ ConSplitterTextInPrivateReadKeyStroke (
/**
Reads the next keystroke from the input device. The WaitForKey Event can
- be used to test for existance of a keystroke via WaitForEvent () call.
+ be used to test for existence of a keystroke via WaitForEvent () call.
@param This Protocol instance pointer.
@param Key Driver may perform diagnostics on reset.
@@ -3631,7 +3631,7 @@ ConSplitterTextInReadKeyStroke (
spliter event. This will cause the calling code to call
ConSplitterTextInReadKeyStroke ().
- @param Event The Event assoicated with callback.
+ @param Event The Event associated with callback.
@param Context Context registered when Event was created.
**/
@@ -3681,7 +3681,7 @@ ConSplitterTextInWaitForKey (
pressed.
@retval TRUE Key be pressed matches a registered key.
- @retval FLASE Match failed.
+ @retval FALSE Match failed.
**/
BOOLEAN
@@ -3715,7 +3715,7 @@ IsKeyRegistered (
/**
- Reset the input device and optionaly run diagnostics
+ Reset the input device and optionally run diagnostics
@param This Protocol instance pointer.
@param ExtendedVerification Driver may perform diagnostics on reset.
@@ -3769,7 +3769,7 @@ ConSplitterTextInResetEx (
/**
Reads the next keystroke from the input device. The WaitForKey Event can
- be used to test for existance of a keystroke via WaitForEvent () call.
+ be used to test for existence of a keystroke via WaitForEvent () call.
@param This Protocol instance pointer.
@param KeyData A pointer to a buffer that is filled in with the
@@ -3978,7 +3978,7 @@ ConSplitterTextInSetState (
@retval EFI_SUCCESS The notification function was registered
successfully.
- @retval EFI_OUT_OF_RESOURCES Unable to allocate resources for necesssary data
+ @retval EFI_OUT_OF_RESOURCES Unable to allocate resources for necessary data
structures.
@retval EFI_INVALID_PARAMETER KeyData or KeyNotificationFunction or NotifyHandle is NULL.
@@ -4126,7 +4126,7 @@ ConSplitterTextInUnregisterKeyNotify (
/**
- Reset the input device and optionaly run diagnostics
+ Reset the input device and optionally run diagnostics
@param This Protocol instance pointer.
@param ExtendedVerification Driver may perform diagnostics on reset.
@@ -4174,7 +4174,7 @@ ConSplitterSimplePointerReset (
/**
Reads the next keystroke from the input device. The WaitForKey Event can
- be used to test for existance of a keystroke via WaitForEvent () call.
+ be used to test for existence of a keystroke via WaitForEvent () call.
@param Private Protocol instance pointer.
@param State The state information of simple pointer device.
@@ -4279,12 +4279,12 @@ ConSplitterSimplePointerGetState (
/**
- This event agregates all the events of the ConIn devices in the spliter.
+ This event aggregates all the events of the ConIn devices in the spliter.
If any events of physical ConIn devices are signaled, signal the ConIn
spliter event. This will cause the calling code to call
ConSplitterTextInReadKeyStroke ().
- @param Event The Event assoicated with callback.
+ @param Event The Event associated with callback.
@param Context Context registered when Event was created.
**/
@@ -4493,12 +4493,12 @@ ConSplitterAbsolutePointerGetState (
/**
- This event agregates all the events of the pointer devices in the splitter.
+ This event aggregates all the events of the pointer devices in the splitter.
If any events of physical pointer devices are signaled, signal the pointer
splitter event. This will cause the calling code to call
ConSplitterAbsolutePointerGetState ().
- @param Event The Event assoicated with callback.
+ @param Event The Event associated with callback.
@param Context Context registered when Event was created.
**/
@@ -4537,10 +4537,10 @@ ConSplitterAbsolutePointerWaitForInput (
/**
- Reset the text output device hardware and optionaly run diagnostics
+ Reset the text output device hardware and optionally run diagnostics
@param This Protocol instance pointer.
- @param ExtendedVerification Driver may perform more exhaustive verfication
+ @param ExtendedVerification Driver may perform more exhaustive verification
operation of the device during reset.
@retval EFI_SUCCESS The text output device was reset.
diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
index 6b8d11d587d1..d6d8db51d5e9 100644
--- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
+++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
@@ -214,7 +214,7 @@ Error:
@param[in] HorizontalResolution The size of video screen in pixels in the X dimension.
@param[in] VerticalResolution The size of video screen in pixels in the Y dimension.
- @param[in] GopModeNumber The graphics mode number which graphis console is based on.
+ @param[in] GopModeNumber The graphics mode number which graphics console is based on.
@param[out] TextModeCount The total number of text modes that graphics console supports.
@param[out] TextModeData The buffer to the text modes column and row information.
Caller is responsible to free it when it's non-NULL.
@@ -491,7 +491,7 @@ GraphicsConsoleControllerDriverStart (
Mode = Private->GraphicsOutput->Mode;
if (EFI_ERROR (Status) && Mode->MaxMode != 0) {
//
- // Set default mode failed or device don't support default mode, then get the current mode information
+ // If set default mode failed or device doesn't support default mode, then get the current mode information
//
HorizontalResolution = Mode->Info->HorizontalResolution;
VerticalResolution = Mode->Info->VerticalResolution;
@@ -501,7 +501,7 @@ GraphicsConsoleControllerDriverStart (
}
if (ModeNumber != Private->GraphicsOutput->Mode->Mode) {
//
- // Current graphics mode is not set or is not set to the mode which we has found,
+ // Current graphics mode is not set or is not set to the mode which we have found,
// set the new graphic mode.
//
Status = Private->GraphicsOutput->SetMode (Private->GraphicsOutput, ModeNumber);
@@ -740,7 +740,7 @@ GraphicsConsoleControllerDriverStop (
Check if the current specific mode supported the user defined resolution
for the Graphics Console device based on Graphics Output Protocol.
- If yes, set the graphic devcice's current mode to this specific mode.
+ If yes, set the graphic device's current mode to this specific mode.
@param GraphicsOutput Graphics Output Protocol instance pointer.
@param HorizontalResolution User defined horizontal resolution
@@ -843,7 +843,7 @@ EfiLocateHiiProtocol (
Reset the text output device hardware and optionally run diagnostics.
Implements SIMPLE_TEXT_OUTPUT.Reset().
- If ExtendeVerification is TRUE, then perform dependent Graphics Console
+ If ExtendedVerification is TRUE, then perform dependent Graphics Console
device reset, and set display mode to mode 0.
If ExtendedVerification is FALSE, only set display mode to mode 0.
@@ -1713,7 +1713,7 @@ GraphicsConsoleConOutEnableCursor (
}
/**
- Gets Graphics Console devcie's foreground color and background color.
+ Gets Graphics Console device's foreground color and background color.
@param This Protocol instance pointer.
@param Foreground Returned text foreground color.
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-11-24 19:15 [PATCH v1 0/3] MdeModulePkg/GraphicsConsole : Console cleanup Samer El-Haj-Mahmoud
2020-11-24 19:15 ` [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE Samer El-Haj-Mahmoud
2020-11-24 19:15 ` [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes Samer El-Haj-Mahmoud
@ 2020-11-24 19:15 ` Samer El-Haj-Mahmoud
2020-11-24 23:30 ` [edk2-devel] " Laszlo Ersek
2 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-11-24 19:15 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Zhichao Gao, Ray Ni, Ard Biesheuvel
ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
and EFI_MAGENTA for StdErr.
This does not work all the time, and StdErr ends up showing parts in
MAGENTA and other parts in LIGHTGRAY. Changing StdErr to LIGHTGRAY looks
better and is more consistent.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
---
MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
index b090de288517..e8cd4ce120a0 100644
--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
@@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
// their MaxMode and QueryData should be the intersection of both.
//
Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut, NULL, NULL);
- ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
+ ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
return Status;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-11-24 19:15 ` [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY Samer El-Haj-Mahmoud
@ 2020-11-24 23:30 ` Laszlo Ersek
2020-12-01 0:59 ` Gao, Zhichao
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-11-24 23:30 UTC (permalink / raw)
To: devel, samer.el-haj-mahmoud
Cc: Jian J Wang, Hao A Wu, Zhichao Gao, Ray Ni, Ard Biesheuvel,
Andy Lutomirski
On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> and EFI_MAGENTA for StdErr.
>
> This does not work all the time, and StdErr ends up showing parts in
> MAGENTA and other parts in LIGHTGRAY. Changing StdErr to LIGHTGRAY looks
> better and is more consistent.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> ---
> MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> index b090de288517..e8cd4ce120a0 100644
> --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> // their MaxMode and QueryData should be the intersection of both.
> //
> Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut, NULL, NULL);
> - ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> + ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
>
> return Status;
> }
>
I am very curious as to how this patch is going to fare, as Andy
Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla ticket
4+ years ago:
https://bugzilla.redhat.com/show_bug.cgi?id=1355913
As you can see in that BZ, I found the same code location, I just didn't
feel up to starting another crusade on edk2-devel -- about colors
even!... So I'll be watching this one now. :)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE
2020-11-24 19:15 ` [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE Samer El-Haj-Mahmoud
@ 2020-12-01 0:45 ` Gao, Zhichao
0 siblings, 0 replies; 21+ messages in thread
From: Gao, Zhichao @ 2020-12-01 0:45 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel, Pete Batard
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
> -----Original Message-----
> From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Sent: Wednesday, November 25, 2020 3:16 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard
> Biesheuvel <Ard.Biesheuvel@arm.com>; Pete Batard <pete@akeo.ie>
> Subject: [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default
> CursorVisible to FALSE
>
> REF: https://github.com/pftf/RPi4/issues/115
>
> GraphicsConsoleDxe defaults the ConOut Mode.CursorVisible to TRUE.
> However, the driver never draws the cursor during init. This results in the first
> call to disable the cursor (using ConOut->EnableCursor(FALSE)) to actually draw
> the cursor on the screen, as the logic in FlushCursor depends on the
> Mode.CursorVisible state to determine if it should draw or erase the cursor.
>
> Fix by changing the default CursorVisible in this driver to FALSE.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> Cc: Pete Batard <pete@akeo.ie>
> Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> ---
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c | 2
> +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> index c042451a9b52..6b8d11d587d1 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.
> +++ c
> @@ -33,7 +33,7 @@ GRAPHICS_CONSOLE_DEV
> mGraphicsConsoleDevTemplate = {
> EFI_TEXT_ATTR(EFI_LIGHTGRAY, EFI_BLACK), 0, 0,- TRUE+ FALSE },
> (GRAPHICS_CONSOLE_MODE_DATA *) NULL,
> (EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) NULL--
> 2.25.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes
2020-11-24 19:15 ` [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes Samer El-Haj-Mahmoud
@ 2020-12-01 0:45 ` Gao, Zhichao
0 siblings, 0 replies; 21+ messages in thread
From: Gao, Zhichao @ 2020-12-01 0:45 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel, Pete Batard
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
> -----Original Message-----
> From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Sent: Wednesday, November 25, 2020 3:16 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard
> Biesheuvel <Ard.Biesheuvel@arm.com>; Pete Batard <pete@akeo.ie>
> Subject: [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes
>
> Fix various spelling mistakes in GraphicsConsoleDxe, ConsPlitter, and
> SimpleTextOut header
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> Cc: Pete Batard <pete@akeo.ie>
> Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> ---
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h | 8
> +--
> MdePkg/Include/Protocol/SimpleTextOut.h | 6 +-
> MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 66
> ++++++++++----------
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c | 12
> ++--
> 4 files changed, 46 insertions(+), 46 deletions(-)
>
> diff --git
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h
> index 28d47ac7cb1e..11d254b70f32 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.h
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.
> +++ h
> @@ -205,7 +205,7 @@ GraphicsConsoleComponentNameGetControllerName (
> Reset the text output device hardware and optionally run diagnostics.
> Implements SIMPLE_TEXT_OUTPUT.Reset().- If ExtendeVerification is TRUE,
> then perform dependent Graphics Console+ If ExtendedVerification is TRUE,
> then perform dependent Graphics Console device reset, and set display mode to
> mode 0. If ExtendedVerification is FALSE, only set display mode to mode 0. @@ -
> 286,7 +286,7 @@ GraphicsConsoleConOutTestString (
> supports Implements SIMPLE_TEXT_OUTPUT.QueryMode().- It returnes
> information for an available text mode that the Graphics Console supports.+ It
> returns information for an available text mode that the Graphics Console
> supports. In this driver,we only support text mode 80x25, which is defined as
> mode 0. @param This Protocol instance pointer.@@ -422,7 +422,7
> @@ GraphicsConsoleConOutEnableCursor (
> /** Test to see if Graphics Console could be supported on the Controller. -
> Graphics Console could be supported if Graphics Output Protocol or UGA Draw+
> Graphics Console could be supported if Graphics Output Protocol or UGADraw
> Protocol exists on the Controller. (UGA Draw Protocol could be skipped if
> PcdUgaConsumeSupport is set to FALSE.) @@ -510,7 +510,7 @@
> EfiLocateHiiProtocol (
> /**- Gets Graphics Console devcie's foreground color and background color.+
> Gets Graphics Console device's foreground color and background color.
> @param This Protocol instance pointer. @param Foreground
> Returned text foreground color.diff --git
> a/MdePkg/Include/Protocol/SimpleTextOut.h
> b/MdePkg/Include/Protocol/SimpleTextOut.h
> index a849c08d66df..100d69a23a9b 100644
> --- a/MdePkg/Include/Protocol/SimpleTextOut.h
> +++ b/MdePkg/Include/Protocol/SimpleTextOut.h
> @@ -32,7 +32,7 @@ typedef struct _EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL
> EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL;
> typedef EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL
> SIMPLE_TEXT_OUTPUT_INTERFACE; //-// Define's for required EFI Unicode Box
> Draw characters+// Defines for required EFI Unicode Box Draw characters //
> #define BOXDRAW_HORIZONTAL 0x2500 #define BOXDRAW_VERTICAL
> 0x2502@@ -151,7 +151,7 @@ typedef EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL
> SIMPLE_TEXT_OUTPUT_INTERFACE;
> #define EFI_WIDE_ATTRIBUTE 0x80 /**- Reset the text output device
> hardware and optionaly run diagnostics+ Reset the text output device hardware
> and optionally run diagnostics @param This The protocol instance
> pointer. @param ExtendedVerification Driver may perform more exhaustive
> verification@@ -373,7 +373,7 @@ typedef struct {
> /// INT32 CursorRow; ///- /// The cursor is currently visbile or not.+ /// The
> cursor is currently visible or not. /// BOOLEAN CursorVisible; }
> EFI_SIMPLE_TEXT_OUTPUT_MODE;diff --git
> a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> index 9c38271b65f9..b090de288517 100644
> --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> @@ -1,5 +1,5 @@
> /** @file- Console Splitter Driver. Any Handle that attatched console I/O
> protocols+ Console Splitter Driver. Any Handle that attached console I/O
> protocols (Console In device, Console Out device, Console Error device, Simple
> Pointer protocol, Absolute Pointer protocol) can be bound by this driver. @@ -
> 13,7 +13,7 @@
> Each virtual handle, that supports the Console I/O protocol, will be produced
> in the driver entry point. The virtual handle are added on driver entry and- never
> removed. Such design ensures sytem function well during none console+ never
> removed. Such design ensures system function well during none console device
> situation. Copyright (c) 2006 - 2019, Intel Corporation. All rights
> reserved.<BR>@@ -381,7 +381,7 @@ ToggleStateSyncInitialization (
> } /**- Reinitialization for toggle state sync.+ Re-initialization for toggle state
> sync. @param Private Text In Splitter pointer. @@ -594,7 +594,7
> @@ ConSplitterDriverEntry(
> structure. @retval EFI_OUT_OF_RESOURCES Out of
> resources.- @retval EFI_SUCCESS Text Input Devcie's private data has
> been constructed.+ @retval EFI_SUCCESS Text Input Device's private
> data has been constructed. @retval other Failed to construct private
> data. **/@@ -761,7 +761,7 @@ ConSplitterTextOutConstructor (
> } //- // Initilize console output splitter's private data.+ // Initialize console
> output splitter's private data. // ConOutPrivate->TextOut.Mode =
> &ConOutPrivate->TextOutMode; @@ -860,7 +860,7 @@
> ConSplitterTextOutConstructor (
> @param Guid The specified protocol. @retval EFI_SUCCESS The
> specified protocol is supported on this device.- @retval EFI_UNSUPPORTED
> The specified protocol attempts to be installed on virtul handle.+ @retval
> EFI_UNSUPPORTED The specified protocol attempts to be installed on virtual
> handle. @retval other Failed to open specified protocol on this device.
> **/@@ -1108,7 +1108,7 @@ ConSplitterStart (
> } //- // Open InterfaceGuid on the virtul handle.+ // Open InterfaceGuid on
> the virtual handle. // Status = gBS->OpenProtocol
> ( ControllerHandle,@@ -1174,7 +1174,7 @@
> ConSplitterConInDriverBindingStart (
> // // Start ConSplitter on ControllerHandle, and create the virtual- //
> agrogated console device on first call Start for a SimpleTextIn handle.+ //
> aggregated console device on first call Start for a SimpleTextIn handle. //
> Status = ConSplitterStart ( This,@@ -1241,7 +1241,7 @@
> ConSplitterSimplePointerDriverBindingStart (
> // // Start ConSplitter on ControllerHandle, and create the virtual- //
> agrogated console device on first call Start for a SimplePointer handle.+ //
> aggregated console device on first call Start for a SimplePointer handle. //
> Status = ConSplitterStart ( This,@@ -1287,7 +1287,7 @@
> ConSplitterAbsolutePointerDriverBindingStart (
> // // Start ConSplitter on ControllerHandle, and create the virtual- //
> agrogated console device on first call Start for a AbsolutePointer handle.+ //
> aggregated console device on first call Start for a AbsolutePointer handle. //
> Status = ConSplitterStart ( This,@@ -1338,7 +1338,7 @@
> ConSplitterConOutDriverBindingStart (
> // // Start ConSplitter on ControllerHandle, and create the virtual- //
> agrogated console device on first call Start for a ConsoleOut handle.+ //
> aggregated console device on first call Start for a ConsoleOut handle. // Status
> = ConSplitterStart ( This,@@ -1451,7 +1451,7 @@
> ConSplitterStdErrDriverBindingStart (
> // // Start ConSplitter on ControllerHandle, and create the virtual- //
> agrogated console device on first call Start for a StandardError handle.+ //
> aggregated console device on first call Start for a StandardError handle. //
> Status = ConSplitterStart ( This,@@ -1522,7 +1522,7 @@ ConSplitterStop
> (
> return Status; } //- // close the protocol refered.+ // close the protocol
> referred. // gBS->CloseProtocol ( ControllerHandle,@@ -1543,7 +1543,7
> @@ ConSplitterStop (
> /**- Stop Console In ConSplitter on ControllerHandle by closing Console In
> Devcice GUID.+ Stop Console In ConSplitter on ControllerHandle by closing
> Console In Device GUID. @param This Driver Binding protocol instance
> pointer. @param ControllerHandle Handle of device to stop driver on@@ -
> 1718,7 +1718,7 @@ ConSplitterAbsolutePointerDriverBindingStop (
> /**- Stop Console Out ConSplitter on device handle by closing Console Out
> Devcice GUID.+ Stop Console Out ConSplitter on device handle by closing
> Console Out Devcie GUID. @param This Driver Binding protocol
> instance pointer. @param ControllerHandle Handle of device to stop driver
> on@@ -2725,7 +2725,7 @@
> ConSplitterGetIntersectionBetweenConOutAndStrErr (
> /**- Add Grahpics Output modes into Consplitter Text Out list.+ Add Graphics
> Output modes into Consplitter Text Out list. @param Private Text Out
> Splitter pointer. @param GraphicsOutput Graphics Output protocol
> pointer.@@ -3392,7 +3392,7 @@ ConSplitterTextOutDeleteDevice (
> return EFI_SUCCESS; } //- // Max Mode is realy an intersection of the
> QueryMode command to all+ // Max Mode is really an intersection of the
> QueryMode command to all // devices. So we must copy the QueryMode of the
> first device to // QueryData. //@@ -3430,7 +3430,7 @@
> ConSplitterTextOutDeleteDevice (
> /**- Reset the input device and optionaly run diagnostics+ Reset the input
> device and optionally run diagnostics @param This Protocol instance
> pointer. @param ExtendedVerification Driver may perform diagnostics on
> reset.@@ -3514,7 +3514,7 @@ ConSplitterTextInExDequeueKey (
> /** Reads the next keystroke from the input device. The WaitForKey Event can-
> be used to test for existance of a keystroke via WaitForEvent () call.+ be used to
> test for existence of a keystroke via WaitForEvent () call. @param Private
> Protocol instance pointer. @param Key Driver may perform
> diagnostics on reset.@@ -3587,7 +3587,7 @@
> ConSplitterTextInPrivateReadKeyStroke (
> /** Reads the next keystroke from the input device. The WaitForKey Event can-
> be used to test for existance of a keystroke via WaitForEvent () call.+ be used to
> test for existence of a keystroke via WaitForEvent () call. @param This
> Protocol instance pointer. @param Key Driver may perform
> diagnostics on reset.@@ -3631,7 +3631,7 @@ ConSplitterTextInReadKeyStroke
> (
> spliter event. This will cause the calling code to call
> ConSplitterTextInReadKeyStroke (). - @param Event The Event
> assoicated with callback.+ @param Event The Event associated with
> callback. @param Context Context registered when Event was
> created. **/@@ -3681,7 +3681,7 @@ ConSplitterTextInWaitForKey (
> pressed. @retval TRUE Key be pressed matches a
> registered key.- @retval FLASE Match failed.+ @retval FALSE
> Match failed. **/ BOOLEAN@@ -3715,7 +3715,7 @@ IsKeyRegistered (
> /**- Reset the input device and optionaly run diagnostics+ Reset the input
> device and optionally run diagnostics @param This Protocol instance
> pointer. @param ExtendedVerification Driver may perform diagnostics on
> reset.@@ -3769,7 +3769,7 @@ ConSplitterTextInResetEx (
> /** Reads the next keystroke from the input device. The WaitForKey Event can-
> be used to test for existance of a keystroke via WaitForEvent () call.+ be used to
> test for existence of a keystroke via WaitForEvent () call. @param This
> Protocol instance pointer. @param KeyData A pointer to a buffer that
> is filled in with the@@ -3978,7 +3978,7 @@ ConSplitterTextInSetState (
> @retval EFI_SUCCESS The notification function was registered
> successfully.- @retval EFI_OUT_OF_RESOURCES Unable to allocate resources
> for necesssary data+ @retval EFI_OUT_OF_RESOURCES Unable to allocate
> resources for necessary data structures. @retval
> EFI_INVALID_PARAMETER KeyData or KeyNotificationFunction or NotifyHandle
> is NULL. @@ -4126,7 +4126,7 @@ ConSplitterTextInUnregisterKeyNotify (
> /**- Reset the input device and optionaly run diagnostics+ Reset the input
> device and optionally run diagnostics @param This Protocol instance
> pointer. @param ExtendedVerification Driver may perform diagnostics on
> reset.@@ -4174,7 +4174,7 @@ ConSplitterSimplePointerReset (
> /** Reads the next keystroke from the input device. The WaitForKey Event can-
> be used to test for existance of a keystroke via WaitForEvent () call.+ be used to
> test for existence of a keystroke via WaitForEvent () call. @param Private
> Protocol instance pointer. @param State The state information of
> simple pointer device.@@ -4279,12 +4279,12 @@
> ConSplitterSimplePointerGetState (
> /**- This event agregates all the events of the ConIn devices in the spliter.+
> This event aggregates all the events of the ConIn devices in the spliter. If any
> events of physical ConIn devices are signaled, signal the ConIn spliter event.
> This will cause the calling code to call ConSplitterTextInReadKeyStroke (). -
> @param Event The Event assoicated with callback.+ @param Event
> The Event associated with callback. @param Context Context
> registered when Event was created. **/@@ -4493,12 +4493,12 @@
> ConSplitterAbsolutePointerGetState (
> /**- This event agregates all the events of the pointer devices in the splitter.+
> This event aggregates all the events of the pointer devices in the splitter. If any
> events of physical pointer devices are signaled, signal the pointer splitter event.
> This will cause the calling code to call ConSplitterAbsolutePointerGetState (). -
> @param Event The Event assoicated with callback.+ @param Event
> The Event associated with callback. @param Context Context
> registered when Event was created. **/@@ -4537,10 +4537,10 @@
> ConSplitterAbsolutePointerWaitForInput (
> /**- Reset the text output device hardware and optionaly run diagnostics+
> Reset the text output device hardware and optionally run diagnostics @param
> This Protocol instance pointer.- @param ExtendedVerification
> Driver may perform more exhaustive verfication+ @param ExtendedVerification
> Driver may perform more exhaustive verification operation of
> the device during reset. @retval EFI_SUCCESS The text output device
> was reset.diff --git
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> index 6b8d11d587d1..d6d8db51d5e9 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.
> +++ c
> @@ -214,7 +214,7 @@ Error:
> @param[in] HorizontalResolution The size of video screen in pixels in the X
> dimension. @param[in] VerticalResolution The size of video screen in pixels
> in the Y dimension.- @param[in] GopModeNumber The graphics mode
> number which graphis console is based on.+ @param[in] GopModeNumber
> The graphics mode number which graphics console is based on. @param[out]
> TextModeCount The total number of text modes that graphics console
> supports. @param[out] TextModeData The buffer to the text modes
> column and row information. Caller is responsible to free it
> when it's non-NULL.@@ -491,7 +491,7 @@
> GraphicsConsoleControllerDriverStart (
> Mode = Private->GraphicsOutput->Mode; if (EFI_ERROR (Status) &&
> Mode->MaxMode != 0) { //- // Set default mode failed or device don't
> support default mode, then get the current mode information+ // If set
> default mode failed or device doesn't support default mode, then get the current
> mode information // HorizontalResolution = Mode->Info-
> >HorizontalResolution; VerticalResolution = Mode->Info-
> >VerticalResolution;@@ -501,7 +501,7 @@
> GraphicsConsoleControllerDriverStart (
> } if (ModeNumber != Private->GraphicsOutput->Mode->Mode) { //- //
> Current graphics mode is not set or is not set to the mode which we has found,+
> // Current graphics mode is not set or is not set to the mode which we have
> found, // set the new graphic mode. // Status = Private-
> >GraphicsOutput->SetMode (Private->GraphicsOutput, ModeNumber);@@ -
> 740,7 +740,7 @@ GraphicsConsoleControllerDriverStop (
> Check if the current specific mode supported the user defined resolution for
> the Graphics Console device based on Graphics Output Protocol. - If yes, set the
> graphic devcice's current mode to this specific mode.+ If yes, set the graphic
> device's current mode to this specific mode. @param GraphicsOutput
> Graphics Output Protocol instance pointer. @param HorizontalResolution User
> defined horizontal resolution@@ -843,7 +843,7 @@ EfiLocateHiiProtocol (
> Reset the text output device hardware and optionally run diagnostics.
> Implements SIMPLE_TEXT_OUTPUT.Reset().- If ExtendeVerification is TRUE,
> then perform dependent Graphics Console+ If ExtendedVerification is TRUE,
> then perform dependent Graphics Console device reset, and set display mode to
> mode 0. If ExtendedVerification is FALSE, only set display mode to mode 0. @@ -
> 1713,7 +1713,7 @@ GraphicsConsoleConOutEnableCursor (
> } /**- Gets Graphics Console devcie's foreground color and background color.+
> Gets Graphics Console device's foreground color and background color.
> @param This Protocol instance pointer. @param Foreground
> Returned text foreground color.--
> 2.25.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-11-24 23:30 ` [edk2-devel] " Laszlo Ersek
@ 2020-12-01 0:59 ` Gao, Zhichao
2020-12-01 15:17 ` Samer El-Haj-Mahmoud
0 siblings, 1 reply; 21+ messages in thread
From: Gao, Zhichao @ 2020-12-01 0:59 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com,
samer.el-haj-mahmoud@arm.com
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel, Andy Lutomirski
I agree the EFI_MAGENTA is not a good choose. But this may be a different issue. Many platforms would set serial port as ConOut and ErrOut. The different colors for them can differ the origin. I don't think change them to the same color is a good idea.
Thanks,
Zhichao
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Wednesday, November 25, 2020 7:30 AM
> To: devel@edk2.groups.io; samer.el-haj-mahmoud@arm.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard
> Biesheuvel <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut and
> > EFI_MAGENTA for StdErr.
> >
> > This does not work all the time, and StdErr ends up showing parts in
> > MAGENTA and other parts in LIGHTGRAY. Changing StdErr to LIGHTGRAY
> > looks better and is more consistent.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > ---
> > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git
> > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > index b090de288517..e8cd4ce120a0 100644
> > --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > // their MaxMode and QueryData should be the intersection of both.
> > //
> > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut, NULL,
> > NULL);
> > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR
> > (EFI_MAGENTA, EFI_BLACK));
> > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR
> > + (EFI_LIGHTGRAY, EFI_BLACK));
> >
> > return Status;
> > }
> >
>
> I am very curious as to how this patch is going to fare, as Andy Lutomirski (CC'd)
> reported the same symptom in a Fedora bugzilla ticket
> 4+ years ago:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1355913
>
> As you can see in that BZ, I found the same code location, I just didn't feel up to
> starting another crusade on edk2-devel -- about colors even!... So I'll be watching
> this one now. :)
>
> Thanks
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-01 0:59 ` Gao, Zhichao
@ 2020-12-01 15:17 ` Samer El-Haj-Mahmoud
2020-12-02 11:04 ` Gao, Zhichao
0 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-01 15:17 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io, lersek@redhat.com
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel, Andy Lutomirski,
Samer El-Haj-Mahmoud
Why does StdErr have to be a different color from ConOut? If the system redirected both streams to the same console output then that is their choice. Serial DEBUG output is not a different color even if the DEBUG is redirected to the same console as ConOut and StdErr. Also, from what I have seen, StdErr does not seem to always retain this MAGENTA color later (for example, after booting a UEFI Shell?).
Do users really care (other than being annoyed by the inconsistency of "some" text showing up in purple?). Using the same color for consoles/DEBUG output by default is consistent and clean. Applications/users can always change the colors later to whatever is the preference for that particular UI/CLI.
Thanks,
--Samer
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, November 30, 2020 8:00 PM
> To: devel@edk2.groups.io; lersek@redhat.com; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@arm.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
> I agree the EFI_MAGENTA is not a good choose. But this may be a different
> issue. Many platforms would set serial port as ConOut and ErrOut. The
> different colors for them can differ the origin. I don't think change them to
> the same color is a good idea.
>
> Thanks,
> Zhichao
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > Sent: Wednesday, November 25, 2020 7:30 AM
> > To: devel@edk2.groups.io; samer.el-haj-mahmoud@arm.com
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Andy
> > Lutomirski <luto@kernel.org>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut and
> > > EFI_MAGENTA for StdErr.
> > >
> > > This does not work all the time, and StdErr ends up showing parts in
> > > MAGENTA and other parts in LIGHTGRAY. Changing StdErr to LIGHTGRAY
> > > looks better and is more consistent.
> > >
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> > > ---
> > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > index b090de288517..e8cd4ce120a0 100644
> > > --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > // their MaxMode and QueryData should be the intersection of both.
> > > //
> > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut, NULL,
> > > NULL);
> > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR
> > > (EFI_MAGENTA, EFI_BLACK));
> > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR
> > > + (EFI_LIGHTGRAY, EFI_BLACK));
> > >
> > > return Status;
> > > }
> > >
> >
> > I am very curious as to how this patch is going to fare, as Andy
> > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > ticket
> > 4+ years ago:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> >
> > As you can see in that BZ, I found the same code location, I just
> > didn't feel up to starting another crusade on edk2-devel -- about
> > colors even!... So I'll be watching this one now. :)
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-01 15:17 ` Samer El-Haj-Mahmoud
@ 2020-12-02 11:04 ` Gao, Zhichao
2020-12-04 0:04 ` Samer El-Haj-Mahmoud
0 siblings, 1 reply; 21+ messages in thread
From: Gao, Zhichao @ 2020-12-02 11:04 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, devel@edk2.groups.io, lersek@redhat.com
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel, Andy Lutomirski
> -----Original Message-----
> From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Sent: Tuesday, December 1, 2020 11:17 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io;
> lersek@redhat.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Andy
> Lutomirski <luto@kernel.org>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Why does StdErr have to be a different color from ConOut? If the system
> redirected both streams to the same console output then that is their choice.
> Serial DEBUG output is not a different color even if the DEBUG is redirected to
> the same console as ConOut and StdErr. Also, from what I have seen, StdErr does
> not seem to always retain this MAGENTA color later (for example, after booting a
> UEFI Shell?).
Can you share the use case of StdErr? Seems when using StdErr->OutputString, the output is not always MAGENTA color. If so, it is a bug of console driver.
I am thinking of one case. The platform only have the serial port without any other display device. System boots to uefi shell and run a debug build application. And the app would have both print output and debug print. If the color are same, the info of normal print and debug print would be mixed up. I am saying StdErr output not normal DebugLib.
Thanks,
Zhichao
>
> Do users really care (other than being annoyed by the inconsistency of "some"
> text showing up in purple?). Using the same color for consoles/DEBUG output by
> default is consistent and clean. Applications/users can always change the colors
> later to whatever is the preference for that particular UI/CLI.
>
> Thanks,
> --Samer
>
>
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Monday, November 30, 2020 8:00 PM
> > To: devel@edk2.groups.io; lersek@redhat.com; Samer El-Haj-Mahmoud
> > <Samer.El-Haj-Mahmoud@arm.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > different issue. Many platforms would set serial port as ConOut and
> > ErrOut. The different colors for them can differ the origin. I don't
> > think change them to the same color is a good idea.
> >
> > Thanks,
> > Zhichao
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Laszlo Ersek
> > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > To: devel@edk2.groups.io; samer.el-haj-mahmoud@arm.com
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Andy
> > > Lutomirski <luto@kernel.org>
> > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > and EFI_MAGENTA for StdErr.
> > > >
> > > > This does not work all the time, and StdErr ends up showing parts
> > > > in MAGENTA and other parts in LIGHTGRAY. Changing StdErr to
> > > > LIGHTGRAY looks better and is more consistent.
> > > >
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>
> > > > ---
> > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2
> > > > +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git
> > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > index b090de288517..e8cd4ce120a0 100644
> > > > --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > // their MaxMode and QueryData should be the intersection of both.
> > > > //
> > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut, NULL,
> > > > NULL);
> > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR
> > > > (EFI_MAGENTA, EFI_BLACK));
> > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut, EFI_TEXT_ATTR
> > > > + (EFI_LIGHTGRAY, EFI_BLACK));
> > > >
> > > > return Status;
> > > > }
> > > >
> > >
> > > I am very curious as to how this patch is going to fare, as Andy
> > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > ticket
> > > 4+ years ago:
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > >
> > > As you can see in that BZ, I found the same code location, I just
> > > didn't feel up to starting another crusade on edk2-devel -- about
> > > colors even!... So I'll be watching this one now. :)
> > >
> > > Thanks
> > > Laszlo
> > >
> > >
> > >
> > >
> > >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-02 11:04 ` Gao, Zhichao
@ 2020-12-04 0:04 ` Samer El-Haj-Mahmoud
2020-12-04 1:11 ` 回复: " gaoliming
0 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-04 0:04 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com, lersek@redhat.com
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel, Andy Lutomirski,
Samer El-Haj-Mahmoud
[-- Attachment #1: Type: text/plain, Size: 8263 bytes --]
Zhichao,
I can understand the rationale if this is truly only for StdErr (although it would have been nice to allow platforms to customize the color with a PCD). However, I see the inconsistency in debug output with platforms I tested with. For example, on the RPi, with DEBUG build, and all ConOut/StdErr and DebugLiub using the same serial console. The serial debug starts with LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is installed. At that point, the debug output switches to MAGENTA, and continues to do so until entering the UI or booting to UEFI Shell, where the color switches back to LIGHTGRAY (attached screenshot2). After that, all ConOut and Debug output is LIGHTGRAY . I do not really know of any actual StdErr output from the Shell.
So, there might be a bug somewhere that causes DEBUG output to switch to MAGENTA and back. I am not really sure. But this inconsistency is annoying. Can we simply avoid this by using a consistent color for all console output? Or at least allow platforms to decide?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao via groups.io
> Sent: Wednesday, December 2, 2020 6:05 AM
> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>;
> devel@edk2.groups.io; lersek@redhat.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
>
>
> > -----Original Message-----
> > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > Sent: Tuesday, December 1, 2020 11:17 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io;
> > lersek@redhat.com
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>; Samer
> > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com>
> > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> > Why does StdErr have to be a different color from ConOut? If the
> > system redirected both streams to the same console output then that is
> their choice.
> > Serial DEBUG output is not a different color even if the DEBUG is
> > redirected to the same console as ConOut and StdErr. Also, from what I
> > have seen, StdErr does not seem to always retain this MAGENTA color
> > later (for example, after booting a UEFI Shell?).
>
> Can you share the use case of StdErr? Seems when using StdErr-
> >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> console driver.
>
> I am thinking of one case. The platform only have the serial port without any
> other display device. System boots to uefi shell and run a debug build
> application. And the app would have both print output and debug print. If
> the color are same, the info of normal print and debug print would be mixed
> up. I am saying StdErr output not normal DebugLib.
>
> Thanks,
> Zhichao
>
> >
> > Do users really care (other than being annoyed by the inconsistency of
> "some"
> > text showing up in purple?). Using the same color for consoles/DEBUG
> > output by default is consistent and clean. Applications/users can
> > always change the colors later to whatever is the preference for that
> particular UI/CLI.
> >
> > Thanks,
> > --Samer
> >
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Monday, November 30, 2020 8:00 PM
> > > To: devel@edk2.groups.io; lersek@redhat.com; Samer El-Haj-Mahmoud
> > > <Samer.El-Haj-Mahmoud@arm.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > different issue. Many platforms would set serial port as ConOut and
> > > ErrOut. The different colors for them can differ the origin. I don't
> > > think change them to the same color is a good idea.
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Laszlo Ersek
> > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > To: devel@edk2.groups.io; samer.el-haj-mahmoud@arm.com
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > > > Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>;
> > > > Andy Lutomirski <luto@kernel.org>
> > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > and EFI_MAGENTA for StdErr.
> > > > >
> > > > > This does not work all the time, and StdErr ends up showing
> > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > to LIGHTGRAY looks better and is more consistent.
> > > > >
> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > Mahmoud@arm.com>
> > > > > ---
> > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2
> > > > > +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git
> > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > ---
> > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > +++
> b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > +++ c
> > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > // their MaxMode and QueryData should be the intersection of
> both.
> > > > > //
> > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > NULL, NULL);
> > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > >
> > > > > return Status;
> > > > > }
> > > > >
> > > >
> > > > I am very curious as to how this patch is going to fare, as Andy
> > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > ticket
> > > > 4+ years ago:
> > > >
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > >
> > > > As you can see in that BZ, I found the same code location, I just
> > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > colors even!... So I'll be watching this one now. :)
> > > >
> > > > Thanks
> > > > Laszlo
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended
> > recipient, please notify the sender immediately and do not disclose
> > the contents to any other person, use it for any purpose, or store or
> > copy the information in any medium. Thank you.
>
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
[-- Attachment #2: screenshot1.png --]
[-- Type: image/png, Size: 375616 bytes --]
[-- Attachment #3: screenshot2.png --]
[-- Type: image/png, Size: 347997 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 0:04 ` Samer El-Haj-Mahmoud
@ 2020-12-04 1:11 ` gaoliming
2020-12-04 1:20 ` Samer El-Haj-Mahmoud
0 siblings, 1 reply; 21+ messages in thread
From: gaoliming @ 2020-12-04 1:11 UTC (permalink / raw)
To: devel, samer.el-haj-mahmoud, zhichao.gao, lersek
Cc: 'Wang, Jian J', 'Wu, Hao A', 'Ni, Ray',
'Ard Biesheuvel', 'Andy Lutomirski'
Samer:
Does all debug message output by PeiDxeDebugLibReportStatusCode? There is not debug message to print as UefiDebugLibStdErr or UefiDebugLibConOut. Right?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+68301+4905953+8761045@groups.io
> <bounce+27952+68301+4905953+8761045@groups.io> 代表 Samer
> El-Haj-Mahmoud
> 发送时间: 2020年12月4日 8:05
> 收件人: devel@edk2.groups.io; zhichao.gao@intel.com; lersek@redhat.com
> 抄送: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>; Samer
> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Zhichao,
>
> I can understand the rationale if this is truly only for StdErr (although it would
> have been nice to allow platforms to customize the color with a PCD).
> However, I see the inconsistency in debug output with platforms I tested with.
> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> DebugLiub using the same serial console. The serial debug starts with
> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
> installed. At that point, the debug output switches to MAGENTA, and
> continues to do so until entering the UI or booting to UEFI Shell, where the
> color switches back to LIGHTGRAY (attached screenshot2). After that, all
> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
> StdErr output from the Shell.
>
> So, there might be a bug somewhere that causes DEBUG output to switch to
> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
> Can we simply avoid this by using a consistent color for all console output? Or
> at least allow platforms to decide?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> > Zhichao via groups.io
> > Sent: Wednesday, December 2, 2020 6:05 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>;
> > devel@edk2.groups.io; lersek@redhat.com
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > > -----Original Message-----
> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io;
> > > lersek@redhat.com
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>; Samer
> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Why does StdErr have to be a different color from ConOut? If the
> > > system redirected both streams to the same console output then that is
> > their choice.
> > > Serial DEBUG output is not a different color even if the DEBUG is
> > > redirected to the same console as ConOut and StdErr. Also, from what I
> > > have seen, StdErr does not seem to always retain this MAGENTA color
> > > later (for example, after booting a UEFI Shell?).
> >
> > Can you share the use case of StdErr? Seems when using StdErr-
> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> > console driver.
> >
> > I am thinking of one case. The platform only have the serial port without any
> > other display device. System boots to uefi shell and run a debug build
> > application. And the app would have both print output and debug print. If
> > the color are same, the info of normal print and debug print would be mixed
> > up. I am saying StdErr output not normal DebugLib.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Do users really care (other than being annoyed by the inconsistency of
> > "some"
> > > text showing up in purple?). Using the same color for consoles/DEBUG
> > > output by default is consistent and clean. Applications/users can
> > > always change the colors later to whatever is the preference for that
> > particular UI/CLI.
> > >
> > > Thanks,
> > > --Samer
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > > To: devel@edk2.groups.io; lersek@redhat.com; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > > different issue. Many platforms would set serial port as ConOut and
> > > > ErrOut. The different colors for them can differ the origin. I don't
> > > > think change them to the same color is a good idea.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Laszlo Ersek
> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > > To: devel@edk2.groups.io; samer.el-haj-mahmoud@arm.com
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > > <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > > > > Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>;
> > > > > Andy Lutomirski <luto@kernel.org>
> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > > >
> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > > and EFI_MAGENTA for StdErr.
> > > > > >
> > > > > > This does not work all the time, and StdErr ends up showing
> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > > to LIGHTGRAY looks better and is more consistent.
> > > > > >
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > > > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com>
> > > > > > ---
> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
> 2
> > > > > > +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > > ---
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > +++
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > > +++ c
> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > > // their MaxMode and QueryData should be the intersection of
> > both.
> > > > > > //
> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > > NULL, NULL);
> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > > >
> > > > > > return Status;
> > > > > > }
> > > > > >
> > > > >
> > > > > I am very curious as to how this patch is going to fare, as Andy
> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > > ticket
> > > > > 4+ years ago:
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > > >
> > > > > As you can see in that BZ, I found the same code location, I just
> > > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > > colors even!... So I'll be watching this one now. :)
> > > > >
> > > > > Thanks
> > > > > Laszlo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose
> > > the contents to any other person, use it for any purpose, or store or
> > > copy the information in any medium. Thank you.
> >
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 1:11 ` 回复: " gaoliming
@ 2020-12-04 1:20 ` Samer El-Haj-Mahmoud
2020-12-04 5:23 ` Gao, Zhichao
0 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-04 1:20 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com, lersek@redhat.com,
gaoliming
Cc: 'Wang, Jian J', 'Wu, Hao A', 'Ni, Ray',
Ard Biesheuvel, 'Andy Lutomirski'
[-- Attachment #1: Type: text/plain, Size: 10562 bytes --]
It's actually all *DebugLibSerialPort :
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.dsc
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
But I have seen this on other systems as well.
________________________________
From: gaoliming <gaoliming@byosoft.com.cn>
Sent: Thursday, December 3, 2020, 8:11 PM
To: devel@edk2.groups.io; Samer El-Haj-Mahmoud; zhichao.gao@intel.com; lersek@redhat.com
Cc: 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy Lutomirski'
Subject: 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Samer:
Does all debug message output by PeiDxeDebugLibReportStatusCode? There is not debug message to print as UefiDebugLibStdErr or UefiDebugLibConOut. Right?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+68301+4905953+8761045@groups.io
> <bounce+27952+68301+4905953+8761045@groups.io> 代表 Samer
> El-Haj-Mahmoud
> 发送时间: 2020年12月4日 8:05
> 收件人: devel@edk2.groups.io; zhichao.gao@intel.com; lersek@redhat.com
> 抄送: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>; Samer
> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Zhichao,
>
> I can understand the rationale if this is truly only for StdErr (although it would
> have been nice to allow platforms to customize the color with a PCD).
> However, I see the inconsistency in debug output with platforms I tested with.
> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> DebugLiub using the same serial console. The serial debug starts with
> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
> installed. At that point, the debug output switches to MAGENTA, and
> continues to do so until entering the UI or booting to UEFI Shell, where the
> color switches back to LIGHTGRAY (attached screenshot2). After that, all
> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
> StdErr output from the Shell.
>
> So, there might be a bug somewhere that causes DEBUG output to switch to
> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
> Can we simply avoid this by using a consistent color for all console output? Or
> at least allow platforms to decide?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> > Zhichao via groups.io
> > Sent: Wednesday, December 2, 2020 6:05 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>;
> > devel@edk2.groups.io; lersek@redhat.com
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > > -----Original Message-----
> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io;
> > > lersek@redhat.com
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>; Samer
> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Why does StdErr have to be a different color from ConOut? If the
> > > system redirected both streams to the same console output then that is
> > their choice.
> > > Serial DEBUG output is not a different color even if the DEBUG is
> > > redirected to the same console as ConOut and StdErr. Also, from what I
> > > have seen, StdErr does not seem to always retain this MAGENTA color
> > > later (for example, after booting a UEFI Shell?).
> >
> > Can you share the use case of StdErr? Seems when using StdErr-
> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> > console driver.
> >
> > I am thinking of one case. The platform only have the serial port without any
> > other display device. System boots to uefi shell and run a debug build
> > application. And the app would have both print output and debug print. If
> > the color are same, the info of normal print and debug print would be mixed
> > up. I am saying StdErr output not normal DebugLib.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Do users really care (other than being annoyed by the inconsistency of
> > "some"
> > > text showing up in purple?). Using the same color for consoles/DEBUG
> > > output by default is consistent and clean. Applications/users can
> > > always change the colors later to whatever is the preference for that
> > particular UI/CLI.
> > >
> > > Thanks,
> > > --Samer
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > > To: devel@edk2.groups.io; lersek@redhat.com; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com>; Andy Lutomirski <luto@kernel.org>
> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > > different issue. Many platforms would set serial port as ConOut and
> > > > ErrOut. The different colors for them can differ the origin. I don't
> > > > think change them to the same color is a good idea.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Laszlo Ersek
> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > > To: devel@edk2.groups.io; samer.el-haj-mahmoud@arm.com
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > > <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > > > > Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>;
> > > > > Andy Lutomirski <luto@kernel.org>
> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > > >
> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > > and EFI_MAGENTA for StdErr.
> > > > > >
> > > > > > This does not work all the time, and StdErr ends up showing
> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > > to LIGHTGRAY looks better and is more consistent.
> > > > > >
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > > > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com>
> > > > > > ---
> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
> 2
> > > > > > +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > > ---
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > +++
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > > +++ c
> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > > // their MaxMode and QueryData should be the intersection of
> > both.
> > > > > > //
> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > > NULL, NULL);
> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > > >
> > > > > > return Status;
> > > > > > }
> > > > > >
> > > > >
> > > > > I am very curious as to how this patch is going to fare, as Andy
> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > > ticket
> > > > > 4+ years ago:
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > > >
> > > > > As you can see in that BZ, I found the same code location, I just
> > > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > > colors even!... So I'll be watching this one now. :)
> > > > >
> > > > > Thanks
> > > > > Laszlo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose
> > > the contents to any other person, use it for any purpose, or store or
> > > copy the information in any medium. Thank you.
> >
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
[-- Attachment #2: Type: text/html, Size: 16618 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 1:20 ` Samer El-Haj-Mahmoud
@ 2020-12-04 5:23 ` Gao, Zhichao
2020-12-04 12:42 ` Samer El-Haj-Mahmoud
0 siblings, 1 reply; 21+ messages in thread
From: Gao, Zhichao @ 2020-12-04 5:23 UTC (permalink / raw)
To: devel@edk2.groups.io, samer.el-haj-mahmoud@arm.com,
lersek@redhat.com, gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel,
'Andy Lutomirski'
[-- Attachment #1: Type: text/plain, Size: 13379 bytes --]
Normal DebugLib usually outputs string debug message only. It would show at the serial device with it current attributes. ErrOut and ConOut would change the attribute. That is why the color changes.
Actually I don’t see many usage of the ErrOut. I would suggest to remove ErrOut if you’re not using it for debug output.
For the Pcds, I agree to let the consumer to choose their wonder default color. But it is better to get the MdeModulePkg maintainers’ point.
Thanks,
Zhichao
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Samer El-Haj-Mahmoud
Sent: Friday, December 4, 2020 9:21 AM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; lersek@redhat.com; gaoliming <gaoliming@byosoft.com.cn>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
It's actually all *DebugLibSerialPort :
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.dsc
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
But I have seen this on other systems as well.
________________________________
From: gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Sent: Thursday, December 3, 2020, 8:11 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Samer El-Haj-Mahmoud; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
Cc: 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy Lutomirski'
Subject: 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Samer:
Does all debug message output by PeiDxeDebugLibReportStatusCode? There is not debug message to print as UefiDebugLibStdErr or UefiDebugLibConOut. Right?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>
> <bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表 Samer
> El-Haj-Mahmoud
> 发送时间: 2020年12月4日 8:05
> 收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Zhichao,
>
> I can understand the rationale if this is truly only for StdErr (although it would
> have been nice to allow platforms to customize the color with a PCD).
> However, I see the inconsistency in debug output with platforms I tested with.
> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> DebugLiub using the same serial console. The serial debug starts with
> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
> installed. At that point, the debug output switches to MAGENTA, and
> continues to do so until entering the UI or booting to UEFI Shell, where the
> color switches back to LIGHTGRAY (attached screenshot2). After that, all
> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
> StdErr output from the Shell.
>
> So, there might be a bug somewhere that causes DEBUG output to switch to
> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
> Can we simply avoid this by using a consistent color for all console output? Or
> at least allow platforms to decide?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> > Zhichao via groups.io
> > Sent: Wednesday, December 2, 2020 6:05 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> > devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > > -----Original Message-----
> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> > > lersek@redhat.com<mailto:lersek@redhat.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Why does StdErr have to be a different color from ConOut? If the
> > > system redirected both streams to the same console output then that is
> > their choice.
> > > Serial DEBUG output is not a different color even if the DEBUG is
> > > redirected to the same console as ConOut and StdErr. Also, from what I
> > > have seen, StdErr does not seem to always retain this MAGENTA color
> > > later (for example, after booting a UEFI Shell?).
> >
> > Can you share the use case of StdErr? Seems when using StdErr-
> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> > console driver.
> >
> > I am thinking of one case. The platform only have the serial port without any
> > other display device. System boots to uefi shell and run a debug build
> > application. And the app would have both print output and debug print. If
> > the color are same, the info of normal print and debug print would be mixed
> > up. I am saying StdErr output not normal DebugLib.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Do users really care (other than being annoyed by the inconsistency of
> > "some"
> > > text showing up in purple?). Using the same color for consoles/DEBUG
> > > output by default is consistent and clean. Applications/users can
> > > always change the colors later to whatever is the preference for that
> > particular UI/CLI.
> > >
> > > Thanks,
> > > --Samer
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > > different issue. Many platforms would set serial port as ConOut and
> > > > ErrOut. The different colors for them can differ the origin. I don't
> > > > think change them to the same color is a good idea.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > > > Laszlo Ersek
> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; samer.el-haj-mahmoud@arm.com<mailto:samer.el-haj-mahmoud@arm.com>
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; Ni,
> > > > > Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>;
> > > > > Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > > >
> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > > and EFI_MAGENTA for StdErr.
> > > > > >
> > > > > > This does not work all the time, and StdErr ends up showing
> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > > to LIGHTGRAY looks better and is more consistent.
> > > > > >
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > > > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>
> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > > > > ---
> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
> 2
> > > > > > +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > > ---
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > +++
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > > +++ c
> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > > // their MaxMode and QueryData should be the intersection of
> > both.
> > > > > > //
> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > > NULL, NULL);
> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > > >
> > > > > > return Status;
> > > > > > }
> > > > > >
> > > > >
> > > > > I am very curious as to how this patch is going to fare, as Andy
> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > > ticket
> > > > > 4+ years ago:
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > > >
> > > > > As you can see in that BZ, I found the same code location, I just
> > > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > > colors even!... So I'll be watching this one now. :)
> > > > >
> > > > > Thanks
> > > > > Laszlo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose
> > > the contents to any other person, use it for any purpose, or store or
> > > copy the information in any medium. Thank you.
> >
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
[-- Attachment #2: Type: text/html, Size: 28179 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 5:23 ` Gao, Zhichao
@ 2020-12-04 12:42 ` Samer El-Haj-Mahmoud
2020-12-04 14:13 ` Ard Biesheuvel
2020-12-08 1:28 ` Ni, Ray
0 siblings, 2 replies; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-04 12:42 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com, lersek@redhat.com,
gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Ard Biesheuvel,
'Andy Lutomirski', Samer El-Haj-Mahmoud
[-- Attachment #1: Type: text/plain, Size: 15361 bytes --]
StdErr console is required to be implemented by the UEFI spec to fill in the pointers in ST (even with a stubbed protocol implementation).
So if we agree that the color of the StdErr (required but low to no usage) is impacting the serial debug prints (high usage for DEBUG targets), why do we still insist on keeping the MAGENTA color for StdErr? Yes adding PCDs would be nice for platforms to select their default colors (and it should be done for both ConOut and StdErr), but that can be added later if there is a real ask for it.
I still do not see why the original patch of simply changing StdErr default color from MAGENTA to LIGHTGRAY (and avoid all of this headache) is problematic?! Please consider the original patch so we can close on this topic and move on.
Thanks,
--Samer
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via groups.io
Sent: Friday, December 4, 2020 12:24 AM
To: devel@edk2.groups.io; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; lersek@redhat.com; gaoliming <gaoliming@byosoft.com.cn>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Normal DebugLib usually outputs string debug message only. It would show at the serial device with it current attributes. ErrOut and ConOut would change the attribute. That is why the color changes.
Actually I don’t see many usage of the ErrOut. I would suggest to remove ErrOut if you’re not using it for debug output.
For the Pcds, I agree to let the consumer to choose their wonder default color. But it is better to get the MdeModulePkg maintainers’ point.
Thanks,
Zhichao
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Samer El-Haj-Mahmoud
Sent: Friday, December 4, 2020 9:21 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; lersek@redhat.com<mailto:lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org<mailto:luto@kernel.org>>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
It's actually all *DebugLibSerialPort :
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.dsc
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
But I have seen this on other systems as well.
________________________________
From: gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Sent: Thursday, December 3, 2020, 8:11 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Samer El-Haj-Mahmoud; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
Cc: 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy Lutomirski'
Subject: 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Samer:
Does all debug message output by PeiDxeDebugLibReportStatusCode? There is not debug message to print as UefiDebugLibStdErr or UefiDebugLibConOut. Right?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>
> <bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表 Samer
> El-Haj-Mahmoud
> 发送时间: 2020年12月4日 8:05
> 收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Zhichao,
>
> I can understand the rationale if this is truly only for StdErr (although it would
> have been nice to allow platforms to customize the color with a PCD).
> However, I see the inconsistency in debug output with platforms I tested with.
> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> DebugLiub using the same serial console. The serial debug starts with
> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
> installed. At that point, the debug output switches to MAGENTA, and
> continues to do so until entering the UI or booting to UEFI Shell, where the
> color switches back to LIGHTGRAY (attached screenshot2). After that, all
> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
> StdErr output from the Shell.
>
> So, there might be a bug somewhere that causes DEBUG output to switch to
> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
> Can we simply avoid this by using a consistent color for all console output? Or
> at least allow platforms to decide?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> > Zhichao via groups.io
> > Sent: Wednesday, December 2, 2020 6:05 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> > devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > > -----Original Message-----
> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> > > lersek@redhat.com<mailto:lersek@redhat.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Why does StdErr have to be a different color from ConOut? If the
> > > system redirected both streams to the same console output then that is
> > their choice.
> > > Serial DEBUG output is not a different color even if the DEBUG is
> > > redirected to the same console as ConOut and StdErr. Also, from what I
> > > have seen, StdErr does not seem to always retain this MAGENTA color
> > > later (for example, after booting a UEFI Shell?).
> >
> > Can you share the use case of StdErr? Seems when using StdErr-
> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> > console driver.
> >
> > I am thinking of one case. The platform only have the serial port without any
> > other display device. System boots to uefi shell and run a debug build
> > application. And the app would have both print output and debug print. If
> > the color are same, the info of normal print and debug print would be mixed
> > up. I am saying StdErr output not normal DebugLib.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Do users really care (other than being annoyed by the inconsistency of
> > "some"
> > > text showing up in purple?). Using the same color for consoles/DEBUG
> > > output by default is consistent and clean. Applications/users can
> > > always change the colors later to whatever is the preference for that
> > particular UI/CLI.
> > >
> > > Thanks,
> > > --Samer
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > > different issue. Many platforms would set serial port as ConOut and
> > > > ErrOut. The different colors for them can differ the origin. I don't
> > > > think change them to the same color is a good idea.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > > > Laszlo Ersek
> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; samer.el-haj-mahmoud@arm.com<mailto:samer.el-haj-mahmoud@arm.com>
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; Ni,
> > > > > Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>;
> > > > > Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > > >
> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > > and EFI_MAGENTA for StdErr.
> > > > > >
> > > > > > This does not work all the time, and StdErr ends up showing
> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > > to LIGHTGRAY looks better and is more consistent.
> > > > > >
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > > > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>
> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > > > > ---
> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
> 2
> > > > > > +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > > ---
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > +++
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > > +++ c
> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > > // their MaxMode and QueryData should be the intersection of
> > both.
> > > > > > //
> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > > NULL, NULL);
> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > > >
> > > > > > return Status;
> > > > > > }
> > > > > >
> > > > >
> > > > > I am very curious as to how this patch is going to fare, as Andy
> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > > ticket
> > > > > 4+ years ago:
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > > >
> > > > > As you can see in that BZ, I found the same code location, I just
> > > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > > colors even!... So I'll be watching this one now. :)
> > > > >
> > > > > Thanks
> > > > > Laszlo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose
> > > the contents to any other person, use it for any purpose, or store or
> > > copy the information in any medium. Thank you.
> >
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
[-- Attachment #2: Type: text/html, Size: 32793 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 12:42 ` Samer El-Haj-Mahmoud
@ 2020-12-04 14:13 ` Ard Biesheuvel
2020-12-07 18:36 ` Samer El-Haj-Mahmoud
2020-12-08 1:28 ` Ni, Ray
1 sibling, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2020-12-04 14:13 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, devel@edk2.groups.io, zhichao.gao@intel.com,
lersek@redhat.com, gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, 'Andy Lutomirski'
Agreed. We can theorize endlessly about different combinations of StdOut
and StdErr, but in the meantime, the magenta serial output is very
annoying. For instance, if you boot Linux in OVMF using the serial
console, all Linux's console output is magenta (and therefore barely
legible on a black background) simply because it inherits the color
setting from the firmware.
On 12/4/20 1:42 PM, Samer El-Haj-Mahmoud wrote:
> StdErr console is required to be implemented by the UEFI spec to fill in
> the pointers in ST (even with a stubbed protocol implementation).
>
>
>
> So if we agree that the color of the StdErr (required but low to no
> usage) is impacting the serial debug prints (high usage for DEBUG
> targets), why do we still insist on keeping the MAGENTA color for
> StdErr? Yes adding PCDs would be nice for platforms to select their
> default colors (and it should be done for both ConOut and StdErr), but
> that can be added later if there is a real ask for it.
>
>
>
> I still do not see why the original patch of simply changing StdErr
> default color from MAGENTA to LIGHTGRAY (and avoid all of this headache)
> is problematic?! Please consider the original patch so we can close on
> this topic and move on.
>
>
>
> Thanks,
>
> --Samer
>
>
>
> *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Gao,
> Zhichao via groups.io
> *Sent:* Friday, December 4, 2020 12:24 AM
> *To:* devel@edk2.groups.io; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@arm.com>; lersek@redhat.com; gaoliming
> <gaoliming@byosoft.com.cn>
> *Cc:* Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
> *Subject:* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
>
>
> Normal DebugLib usually outputs string debug message only. It would show
> at the serial device with it current attributes. ErrOut and ConOut would
> change the attribute. That is why the color changes.
>
> Actually I don’t see many usage of the ErrOut. I would suggest to remove
> ErrOut if you’re not using it for debug output.
>
> For the Pcds, I agree to let the consumer to choose their wonder default
> color. But it is better to get the MdeModulePkg maintainers’ point.
>
>
>
> Thanks,
>
> Zhichao
>
>
>
> *From:*devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf Of
> *Samer El-Haj-Mahmoud
> *Sent:* Friday, December 4, 2020 9:21 AM
> *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Gao, Zhichao
> <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
> lersek@redhat.com <mailto:lersek@redhat.com>; gaoliming
> <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>
> *Cc:* Wang, Jian J <jian.j.wang@intel.com
> <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com
> <mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com
> <mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org
> <mailto:luto@kernel.org>>
> *Subject:* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
>
>
> It's actually all *DebugLibSerialPort :
>
>
>
>
>
> https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.dsc
>
>
>
>
>
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>
> DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>
>
> But I have seen this on other systems as well.
>
>
>
>
>
>
>
>
>
> ------------------------------------------------------------------------
>
> *From:* gaoliming <gaoliming@byosoft.com.cn
> <mailto:gaoliming@byosoft.com.cn>>
> *Sent:* Thursday, December 3, 2020, 8:11 PM
> *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Samer
> El-Haj-Mahmoud; zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>;
> lersek@redhat.com <mailto:lersek@redhat.com>
> *Cc:* 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy
> Lutomirski'
> *Subject:* 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
>
>
> Samer:
> Does all debug message output by PeiDxeDebugLibReportStatusCode? There
> is not debug message to print as UefiDebugLibStdErr or
> UefiDebugLibConOut. Right?
>
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: bounce+27952+68301+4905953+8761045@groups.io
> <mailto:bounce+27952+68301+4905953+8761045@groups.io>
>> <bounce+27952+68301+4905953+8761045@groups.io
> <mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表Samer
>> El-Haj-Mahmoud
>> 发送时间: 2020年12月4日8:05
>> 收件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>; lersek@redhat.com
> <mailto:lersek@redhat.com>
>> 抄送: Wang, Jian J <jian.j.wang@intel.com
> <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
>> <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski
> <luto@kernel.org <mailto:luto@kernel.org>>; Samer
>> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com <mailto:Samer.El-Haj-Mahmoud@arm.com>>
>> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
>> StdErr color to EFI_LIGHTGRAY
>>
>> Zhichao,
>>
>> I can understand the rationale if this is truly only for StdErr (although it would
>> have been nice to allow platforms to customize the color with a PCD).
>> However, I see the inconsistency in debug output with platforms I tested with.
>> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
>> DebugLiub using the same serial console. The serial debug starts with
>> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
>> installed. At that point, the debug output switches to MAGENTA, and
>> continues to do so until entering the UI or booting to UEFI Shell, where the
>> color switches back to LIGHTGRAY (attached screenshot2). After that, all
>> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
>> StdErr output from the Shell.
>>
>> So, there might be a bug somewhere that causes DEBUG output to switch to
>> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
>> Can we simply avoid this by using a consistent color for all console output? Or
>> at least allow platforms to decide?
>>
>>
>> > -----Original Message-----
>> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>> On Behalf Of Gao,
>> > Zhichao via groups.io
>> > Sent: Wednesday, December 2, 2020 6:05 AM
>> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com <mailto:Samer.El-Haj-Mahmoud@arm.com>>;
>> > devel@edk2.groups.io <mailto:devel@edk2.groups.io>; lersek@redhat.com
> <mailto:lersek@redhat.com>
>> > Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
>> > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski
> <luto@kernel.org <mailto:luto@kernel.org>>
>> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
>> > Change StdErr color to EFI_LIGHTGRAY
>> >
>> >
>> >
>> > > -----Original Message-----
>> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com <mailto:Samer.El-Haj-Mahmoud@arm.com>>
>> > > Sent: Tuesday, December 1, 2020 11:17 PM
>> > > To: Gao, Zhichao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
>> > > lersek@redhat.com <mailto:lersek@redhat.com>
>> > > Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
>> > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski
> <luto@kernel.org <mailto:luto@kernel.org>>; Samer
>> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com <mailto:Mahmoud@arm.com>>
>> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
>> > > Change StdErr color to EFI_LIGHTGRAY
>> > >
>> > > Why does StdErr have to be a different color from ConOut? If the
>> > > system redirected both streams to the same console output then that is
>> > their choice.
>> > > Serial DEBUG output is not a different color even if the DEBUG is
>> > > redirected to the same console as ConOut and StdErr. Also, from what I
>> > > have seen, StdErr does not seem to always retain this MAGENTA color
>> > > later (for example, after booting a UEFI Shell?).
>> >
>> > Can you share the use case of StdErr? Seems when using StdErr-
>> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
>> > console driver.
>> >
>> > I am thinking of one case. The platform only have the serial port without any
>> > other display device. System boots to uefi shell and run a debug build
>> > application. And the app would have both print output and debug print. If
>> > the color are same, the info of normal print and debug print would be mixed
>> > up. I am saying StdErr output not normal DebugLib.
>> >
>> > Thanks,
>> > Zhichao
>> >
>> > >
>> > > Do users really care (other than being annoyed by the inconsistency of
>> > "some"
>> > > text showing up in purple?). Using the same color for consoles/DEBUG
>> > > output by default is consistent and clean. Applications/users can
>> > > always change the colors later to whatever is the preference for that
>> > particular UI/CLI.
>> > >
>> > > Thanks,
>> > > --Samer
>> > >
>> > >
>> > > > -----Original Message-----
>> > > > From: Gao, Zhichao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>
>> > > > Sent: Monday, November 30, 2020 8:00 PM
>> > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; lersek@redhat.com
> <mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
>> > > > <Samer.El-Haj-Mahmoud@arm.com <mailto:Samer.El-Haj-Mahmoud@arm.com>>
>> > > > Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
>> > > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski
> <luto@kernel.org <mailto:luto@kernel.org>>
>> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
>> > > > Change StdErr color to EFI_LIGHTGRAY
>> > > >
>> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
>> > > > different issue. Many platforms would set serial port as ConOut and
>> > > > ErrOut. The different colors for them can differ the origin. I don't
>> > > > think change them to the same color is a good idea.
>> > > >
>> > > > Thanks,
>> > > > Zhichao
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>> On Behalf Of
>> > > > > Laszlo Ersek
>> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
>> > > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> samer.el-haj-mahmoud@arm.com <mailto:samer.el-haj-mahmoud@arm.com>
>> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> > > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Gao, Zhichao
> <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>; Ni,
>> > > > > Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
>> > > > > Andy Lutomirski <luto@kernel.org <mailto:luto@kernel.org>>
>> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
>> > > > > Change StdErr color to EFI_LIGHTGRAY
>> > > > >
>> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
>> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
>> > > > > > and EFI_MAGENTA for StdErr.
>> > > > > >
>> > > > > > This does not work all the time, and StdErr ends up showing
>> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
>> > > > > > to LIGHTGRAY looks better and is more consistent.
>> > > > > >
>> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>
>> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>
>> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>
>> > > > > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>
>> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
>> > > > Mahmoud@arm.com <mailto:Mahmoud@arm.com>>
>> > > > > > ---
>> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
>> 2
>> > > > > > +-
>> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git
>> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
>> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
>> > > > > > index b090de288517..e8cd4ce120a0 100644
>> > > > > > ---
>> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
>> > > > > > +++
>> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
>> > > > > > +++ c
>> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
>> > > > > > // their MaxMode and QueryData should be the intersection of
>> > both.
>> > > > > > //
>> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
>> > > > > > NULL, NULL);
>> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
>> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
>> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
>> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
>> > > > > >
>> > > > > > return Status;
>> > > > > > }
>> > > > > >
>> > > > >
>> > > > > I am very curious as to how this patch is going to fare, as Andy
>> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
>> > > > > ticket
>> > > > > 4+ years ago:
>> > > > >
>> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
>> > > > >
>> > > > > As you can see in that BZ, I found the same code location, I just
>> > > > > didn't feel up to starting another crusade on edk2-devel -- about
>> > > > > colors even!... So I'll be watching this one now. :)
>> > > > >
>> > > > > Thanks
>> > > > > Laszlo
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > >
>> > > IMPORTANT NOTICE: The contents of this email and any attachments are
>> > > confidential and may also be privileged. If you are not the intended
>> > > recipient, please notify the sender immediately and do not disclose
>> > > the contents to any other person, use it for any purpose, or store or
>> > > copy the information in any medium. Thank you.
>> >
>> >
>> >
>> >
>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended recipient,
>> please notify the sender immediately and do not disclose the contents to any
>> other person, use it for any purpose, or store or copy the information in any
>> medium. Thank you.
>>
>>
>>
>>
>
>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium. Thank you.
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 14:13 ` Ard Biesheuvel
@ 2020-12-07 18:36 ` Samer El-Haj-Mahmoud
2020-12-08 0:59 ` Gao, Zhichao
0 siblings, 1 reply; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-07 18:36 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io, zhichao.gao@intel.com,
lersek@redhat.com, gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, 'Andy Lutomirski',
Samer El-Haj-Mahmoud, nd
Thanks Ard..
Any further feedback or a RB for this patch, so we can close on the "console color" topic and move on?
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Friday, December 4, 2020 9:14 AM
> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>;
> devel@edk2.groups.io; zhichao.gao@intel.com; lersek@redhat.com;
> gaoliming <gaoliming@byosoft.com.cn>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; 'Andy Lutomirski'
> <luto@kernel.org>
> Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
> Agreed. We can theorize endlessly about different combinations of StdOut
> and StdErr, but in the meantime, the magenta serial output is very annoying.
> For instance, if you boot Linux in OVMF using the serial console, all Linux's
> console output is magenta (and therefore barely legible on a black
> background) simply because it inherits the color setting from the firmware.
>
>
>
> On 12/4/20 1:42 PM, Samer El-Haj-Mahmoud wrote:
> > StdErr console is required to be implemented by the UEFI spec to fill
> > in the pointers in ST (even with a stubbed protocol implementation).
> >
> >
> >
> > So if we agree that the color of the StdErr (required but low to no
> > usage) is impacting the serial debug prints (high usage for DEBUG
> > targets), why do we still insist on keeping the MAGENTA color for
> > StdErr? Yes adding PCDs would be nice for platforms to select their
> > default colors (and it should be done for both ConOut and StdErr), but
> > that can be added later if there is a real ask for it.
> >
> >
> >
> > I still do not see why the original patch of simply changing StdErr
> > default color from MAGENTA to LIGHTGRAY (and avoid all of this
> > headache) is problematic?! Please consider the original patch so we
> > can close on this topic and move on.
> >
> >
> >
> > Thanks,
> >
> > --Samer
> >
> >
> >
> > *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
> *Gao,
> > Zhichao via groups.io
> > *Sent:* Friday, December 4, 2020 12:24 AM
> > *To:* devel@edk2.groups.io; Samer El-Haj-Mahmoud
> > <Samer.El-Haj-Mahmoud@arm.com>; lersek@redhat.com; gaoliming
> > <gaoliming@byosoft.com.cn>
> > *Cc:* Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
> > *Subject:* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > Normal DebugLib usually outputs string debug message only. It would
> > show at the serial device with it current attributes. ErrOut and
> > ConOut would change the attribute. That is why the color changes.
> >
> > Actually I don’t see many usage of the ErrOut. I would suggest to
> > remove ErrOut if you’re not using it for debug output.
> >
> > For the Pcds, I agree to let the consumer to choose their wonder
> > default color. But it is better to get the MdeModulePkg maintainers’ point.
> >
> >
> >
> > Thanks,
> >
> > Zhichao
> >
> >
> >
> > *From:*devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf Of
> > *Samer El-Haj-Mahmoud
> > *Sent:* Friday, December 4, 2020 9:21 AM
> > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Gao,
> Zhichao
> > <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
> > lersek@redhat.com <mailto:lersek@redhat.com>; gaoliming
> > <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>
> > *Cc:* Wang, Jian J <jian.j.wang@intel.com
> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> > <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com
> > <mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com
> > <mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org
> > <mailto:luto@kernel.org>>
> > *Subject:* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > It's actually all *DebugLibSerialPort :
> >
> >
> >
> >
> >
> > https://github.com/tianocore/edk2-
> platforms/blob/master/Platform/Raspb
> > erryPi/RPi4/RPi4.dsc
> >
> >
> >
> >
> >
> >
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.
> > DebugLib|inf
> >
> >
> DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDeb
> ugLi
> > DebugLib|bSerialPort.inf
> >
> >
> > But I have seen this on other systems as well.
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > ----------------------------------------------------------------------
> > --
> >
> > *From:* gaoliming <gaoliming@byosoft.com.cn
> > <mailto:gaoliming@byosoft.com.cn>>
> > *Sent:* Thursday, December 3, 2020, 8:11 PM
> > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Samer
> > El-Haj-Mahmoud; zhichao.gao@intel.com
> <mailto:zhichao.gao@intel.com>;
> > lersek@redhat.com <mailto:lersek@redhat.com>
> > *Cc:* 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy
> > Lutomirski'
> > *Subject:* 回复: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > Samer:
> > Does all debug message output by PeiDxeDebugLibReportStatusCode?
> > There is not debug message to print as UefiDebugLibStdErr or
> > UefiDebugLibConOut. Right?
> >
> > Thanks
> > Liming
> >> -----邮件原件-----
> >> 发件人: bounce+27952+68301+4905953+8761045@groups.io
> > <mailto:bounce+27952+68301+4905953+8761045@groups.io>
> >> <bounce+27952+68301+4905953+8761045@groups.io
> > <mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表Samer
> >> El-Haj-Mahmoud
> >> 发送时间: 2020年12月4日8:05
> >> 收件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>;
> > lersek@redhat.com <mailto:lersek@redhat.com>
> >> 抄送: Wang, Jian J <jian.j.wang@intel.com
> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> >> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> >> <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy
> >> Lutomirski
> > <luto@kernel.org <mailto:luto@kernel.org>>; Samer
> >> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> >> <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> >> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change
> >> StdErr color to EFI_LIGHTGRAY
> >>
> >> Zhichao,
> >>
> >> I can understand the rationale if this is truly only for StdErr
> >> (although it would have been nice to allow platforms to customize the
> color with a PCD).
> >> However, I see the inconsistency in debug output with platforms I tested
> with.
> >> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> >> DebugLiub using the same serial console. The serial debug starts with
> >> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid
> >> is installed. At that point, the debug output switches to MAGENTA,
> >> and continues to do so until entering the UI or booting to UEFI
> >> Shell, where the color switches back to LIGHTGRAY (attached
> >> screenshot2). After that, all ConOut and Debug output is LIGHTGRAY .
> >> I do not really know of any actual StdErr output from the Shell.
> >>
> >> So, there might be a bug somewhere that causes DEBUG output to switch
> >> to MAGENTA and back. I am not really sure. But this inconsistency is
> annoying.
> >> Can we simply avoid this by using a consistent color for all console
> >> output? Or at least allow platforms to decide?
> >>
> >>
> >> > -----Original Message-----
> >> > From: devel@edk2.groups.io
> >> > <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> > <mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> >> > Zhichao via groups.io
> >> > Sent: Wednesday, December 2, 2020 6:05 AM
> >> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> >> > <mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> >> > devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> >> > lersek@redhat.com
> > <mailto:lersek@redhat.com>
> >> > Cc: Wang, Jian J <jian.j.wang@intel.com
> >> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> >> > <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> >> > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy
> >> > Lutomirski
> > <luto@kernel.org <mailto:luto@kernel.org>>
> >> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> >> > Change StdErr color to EFI_LIGHTGRAY
> >> >
> >> >
> >> >
> >> > > -----Original Message-----
> >> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> >> > > <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> >> > > Sent: Tuesday, December 1, 2020 11:17 PM
> >> > > To: Gao, Zhichao <zhichao.gao@intel.com
> >> > > <mailto:zhichao.gao@intel.com>>;
> > devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> >> > > lersek@redhat.com <mailto:lersek@redhat.com>
> >> > > Cc: Wang, Jian J <jian.j.wang@intel.com
> >> > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> >> > > <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> >> > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> Andy
> >> > > Lutomirski
> > <luto@kernel.org <mailto:luto@kernel.org>>; Samer
> >> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com
> >> > > <mailto:Mahmoud@arm.com>>
> >> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> >> > > Change StdErr color to EFI_LIGHTGRAY
> >> > >
> >> > > Why does StdErr have to be a different color from ConOut? If the
> >> > > system redirected both streams to the same console output then
> >> > > that is
> >> > their choice.
> >> > > Serial DEBUG output is not a different color even if the DEBUG is
> >> > > redirected to the same console as ConOut and StdErr. Also, from
> >> > > what I have seen, StdErr does not seem to always retain this
> >> > > MAGENTA color later (for example, after booting a UEFI Shell?).
> >> >
> >> > Can you share the use case of StdErr? Seems when using StdErr-
> >> > >OutputString, the output is not always MAGENTA color. If so, it is
> >> > >a bug of
> >> > console driver.
> >> >
> >> > I am thinking of one case. The platform only have the serial port
> >> > without any other display device. System boots to uefi shell and
> >> > run a debug build application. And the app would have both print
> >> > output and debug print. If the color are same, the info of normal
> >> > print and debug print would be mixed up. I am saying StdErr output not
> normal DebugLib.
> >> >
> >> > Thanks,
> >> > Zhichao
> >> >
> >> > >
> >> > > Do users really care (other than being annoyed by the
> >> > > inconsistency of
> >> > "some"
> >> > > text showing up in purple?). Using the same color for
> >> > > consoles/DEBUG output by default is consistent and clean.
> >> > > Applications/users can always change the colors later to whatever
> >> > > is the preference for that
> >> > particular UI/CLI.
> >> > >
> >> > > Thanks,
> >> > > --Samer
> >> > >
> >> > >
> >> > > > -----Original Message-----
> >> > > > From: Gao, Zhichao <zhichao.gao@intel.com
> >> > > > <mailto:zhichao.gao@intel.com>>
> >> > > > Sent: Monday, November 30, 2020 8:00 PM
> >> > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> >> > > > lersek@redhat.com
> > <mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> >> > > > <Samer.El-Haj-Mahmoud@arm.com
> >> > > > <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> >> > > > Cc: Wang, Jian J <jian.j.wang@intel.com
> >> > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> >> > > > <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> >> > > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> Andy
> >> > > > Lutomirski
> > <luto@kernel.org <mailto:luto@kernel.org>>
> >> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> >> > > > Change StdErr color to EFI_LIGHTGRAY
> >> > > >
> >> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> >> > > > different issue. Many platforms would set serial port as ConOut
> >> > > > and ErrOut. The different colors for them can differ the
> >> > > > origin. I don't think change them to the same color is a good idea.
> >> > > >
> >> > > > Thanks,
> >> > > > Zhichao
> >> > > >
> >> > > > > -----Original Message-----
> >> > > > > From: devel@edk2.groups.io
> >> > > > > <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> > <mailto:devel@edk2.groups.io>> On Behalf Of
> >> > > > > Laszlo Ersek
> >> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> >> > > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > samer.el-haj-mahmoud@arm.com <mailto:samer.el-haj-
> mahmoud@arm.com>
> >> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com
> >> > > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> >> > > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Gao,
> >> > > > > Zhichao
> > <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>; Ni,
> >> > > > > Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard
> >> > > > > Biesheuvel
> > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> >> > > > > Andy Lutomirski <luto@kernel.org <mailto:luto@kernel.org>>
> >> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> >> > > > > Change StdErr color to EFI_LIGHTGRAY
> >> > > > >
> >> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> >> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for
> >> > > > > > ConOut and EFI_MAGENTA for StdErr.
> >> > > > > >
> >> > > > > > This does not work all the time, and StdErr ends up showing
> >> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing
> >> > > > > > StdErr to LIGHTGRAY looks better and is more consistent.
> >> > > > > >
> >> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com
> >> > > > > > <mailto:jian.j.wang@intel.com>>
> >> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com
> >> > > > > > <mailto:hao.a.wu@intel.com>>
> >> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com
> >> > > > > > <mailto:zhichao.gao@intel.com>>
> >> > > > > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> >> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com
> >> > > > > > <mailto:Ard.Biesheuvel@arm.com>>
> >> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> >> > > > Mahmoud@arm.com <mailto:Mahmoud@arm.com>>
> >> > > > > > ---
> >> > > > > >
> >> > > > > >MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> >> > > > > >|
> >> 2
> >> > > > > > +-
> >> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> > > > > >
> >> > > > > > diff --git
> >> > > > > >
> a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> >> > > > > > .c
> >> > > > > >
> b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> >> > > > > > .c index b090de288517..e8cd4ce120a0 100644
> >> > > > > > ---
> >> > > > > >
> a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> >> > > > > > .c
> >> > > > > > +++
> >> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> >> > > > > > +++ c
> >> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> >> > > > > > // their MaxMode and QueryData should be the
> >> > > > > >intersection of
> >> > both.
> >> > > > > > //
> >> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> >> > > > > >NULL, NULL);
> >> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> >> > > > > >EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> >> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> >> > > > > > +EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> >> > > > > >
> >> > > > > > return Status;
> >> > > > > > }
> >> > > > > >
> >> > > > >
> >> > > > > I am very curious as to how this patch is going to fare, as
> >> > > > > Andy Lutomirski (CC'd) reported the same symptom in a Fedora
> >> > > > > bugzilla ticket
> >> > > > > 4+ years ago:
> >> > > > >
> >> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> >> > > > >
> >> > > > > As you can see in that BZ, I found the same code location, I
> >> > > > > just didn't feel up to starting another crusade on edk2-devel
> >> > > > > -- about colors even!... So I'll be watching this one now. :)
> >> > > > >
> >> > > > > Thanks
> >> > > > > Laszlo
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > >
> >> > >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-07 18:36 ` Samer El-Haj-Mahmoud
@ 2020-12-08 0:59 ` Gao, Zhichao
2020-12-08 17:07 ` Samer El-Haj-Mahmoud
0 siblings, 1 reply; 21+ messages in thread
From: Gao, Zhichao @ 2020-12-08 0:59 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, Ard Biesheuvel, devel@edk2.groups.io,
lersek@redhat.com, gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, 'Andy Lutomirski', nd
My consideration is the impact of make StdErr and StdCon same foreground and background. I don't think they should be the same. But it actually affects the DebugLib and other terminal operation's output.
I agree with the change but the commit message should be changed. ErrOut would always output with its own attribute but DebugLib wouldn't. And we should add the impact as well: it makes ConOut and ErrOut same foreground and background. If consumers want to differ them in the same output devices, they should change one of their attributes by themselves.
Thanks,
Zhichao
> -----Original Message-----
> From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Sent: Tuesday, December 8, 2020 2:36 AM
> To: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; devel@edk2.groups.io; Gao,
> Zhichao <zhichao.gao@intel.com>; lersek@redhat.com; gaoliming
> <gaoliming@byosoft.com.cn>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni,
> Ray <ray.ni@intel.com>; 'Andy Lutomirski' <luto@kernel.org>; Samer El-Haj-
> Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; nd <nd@arm.com>
> Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Thanks Ard..
>
> Any further feedback or a RB for this patch, so we can close on the "console
> color" topic and move on?
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Sent: Friday, December 4, 2020 9:14 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>;
> > devel@edk2.groups.io; zhichao.gao@intel.com; lersek@redhat.com;
> > gaoliming <gaoliming@byosoft.com.cn>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; 'Andy Lutomirski'
> > <luto@kernel.org>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> > Agreed. We can theorize endlessly about different combinations of
> > StdOut and StdErr, but in the meantime, the magenta serial output is very
> annoying.
> > For instance, if you boot Linux in OVMF using the serial console, all
> > Linux's console output is magenta (and therefore barely legible on a
> > black
> > background) simply because it inherits the color setting from the firmware.
> >
> >
> >
> > On 12/4/20 1:42 PM, Samer El-Haj-Mahmoud wrote:
> > > StdErr console is required to be implemented by the UEFI spec to
> > > fill in the pointers in ST (even with a stubbed protocol implementation).
> > >
> > >
> > >
> > > So if we agree that the color of the StdErr (required but low to no
> > > usage) is impacting the serial debug prints (high usage for DEBUG
> > > targets), why do we still insist on keeping the MAGENTA color for
> > > StdErr? Yes adding PCDs would be nice for platforms to select their
> > > default colors (and it should be done for both ConOut and StdErr),
> > > but that can be added later if there is a real ask for it.
> > >
> > >
> > >
> > > I still do not see why the original patch of simply changing StdErr
> > > default color from MAGENTA to LIGHTGRAY (and avoid all of this
> > > headache) is problematic?! Please consider the original patch so we
> > > can close on this topic and move on.
> > >
> > >
> > >
> > > Thanks,
> > >
> > > --Samer
> > >
> > >
> > >
> > > *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
> > *Gao,
> > > Zhichao via groups.io
> > > *Sent:* Friday, December 4, 2020 12:24 AM
> > > *To:* devel@edk2.groups.io; Samer El-Haj-Mahmoud
> > > <Samer.El-Haj-Mahmoud@arm.com>; lersek@redhat.com; gaoliming
> > > <gaoliming@byosoft.com.cn>
> > > *Cc:* Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
> > > *Subject:* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > >
> > >
> > > Normal DebugLib usually outputs string debug message only. It would
> > > show at the serial device with it current attributes. ErrOut and
> > > ConOut would change the attribute. That is why the color changes.
> > >
> > > Actually I don’t see many usage of the ErrOut. I would suggest to
> > > remove ErrOut if you’re not using it for debug output.
> > >
> > > For the Pcds, I agree to let the consumer to choose their wonder
> > > default color. But it is better to get the MdeModulePkg maintainers’ point.
> > >
> > >
> > >
> > > Thanks,
> > >
> > > Zhichao
> > >
> > >
> > >
> > > *From:*devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf Of
> > > *Samer El-Haj-Mahmoud
> > > *Sent:* Friday, December 4, 2020 9:21 AM
> > > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Gao,
> > Zhichao
> > > <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
> > > lersek@redhat.com <mailto:lersek@redhat.com>; gaoliming
> > > <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>
> > > *Cc:* Wang, Jian J <jian.j.wang@intel.com
> > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> > > <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com
> > > <mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com
> > > <mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org
> > > <mailto:luto@kernel.org>>
> > > *Subject:* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > >
> > >
> > > It's actually all *DebugLibSerialPort :
> > >
> > >
> > >
> > >
> > >
> > > https://github.com/tianocore/edk2-
> > platforms/blob/master/Platform/Raspb
> > > erryPi/RPi4/RPi4.dsc
> > >
> > >
> > >
> > >
> > >
> > >
> > DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.
> > > DebugLib|inf
> > >
> > >
> > DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDeb
> > ugLi
> > > DebugLib|bSerialPort.inf
> > >
> > >
> > > But I have seen this on other systems as well.
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > --------------------------------------------------------------------
> > > --
> > > --
> > >
> > > *From:* gaoliming <gaoliming@byosoft.com.cn
> > > <mailto:gaoliming@byosoft.com.cn>>
> > > *Sent:* Thursday, December 3, 2020, 8:11 PM
> > > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Samer
> > > El-Haj-Mahmoud; zhichao.gao@intel.com
> > <mailto:zhichao.gao@intel.com>;
> > > lersek@redhat.com <mailto:lersek@redhat.com>
> > > *Cc:* 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy
> > > Lutomirski'
> > > *Subject:* 回复: [edk2-devel] [PATCH v1 3/3]
> > MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > >
> > >
> > > Samer:
> > > Does all debug message output by PeiDxeDebugLibReportStatusCode?
> > > There is not debug message to print as UefiDebugLibStdErr or
> > > UefiDebugLibConOut. Right?
> > >
> > > Thanks
> > > Liming
> > >> -----邮件原件-----
> > >> 发件人: bounce+27952+68301+4905953+8761045@groups.io
> > > <mailto:bounce+27952+68301+4905953+8761045@groups.io>
> > >> <bounce+27952+68301+4905953+8761045@groups.io
> > > <mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表Samer
> > >> El-Haj-Mahmoud
> > >> 发送时间: 2020年12月4日8:05
> > >> 收件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>;
> > > lersek@redhat.com <mailto:lersek@redhat.com>
> > >> 抄送: Wang, Jian J <jian.j.wang@intel.com
> > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > >> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > >> <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy
> > >> Lutomirski
> > > <luto@kernel.org <mailto:luto@kernel.org>>; Samer
> > >> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> > >> <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > >> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change
> > >> StdErr color to EFI_LIGHTGRAY
> > >>
> > >> Zhichao,
> > >>
> > >> I can understand the rationale if this is truly only for StdErr
> > >> (although it would have been nice to allow platforms to customize
> > >> the
> > color with a PCD).
> > >> However, I see the inconsistency in debug output with platforms I
> > >> tested
> > with.
> > >> For example, on the RPi, with DEBUG build, and all ConOut/StdErr
> > >> and DebugLiub using the same serial console. The serial debug
> > >> starts with LIGHTGRAY (attached screenshot 1), until
> > >> gEfiStandardErrorDeviceGuid is installed. At that point, the debug
> > >> output switches to MAGENTA, and continues to do so until entering
> > >> the UI or booting to UEFI Shell, where the color switches back to
> > >> LIGHTGRAY (attached screenshot2). After that, all ConOut and Debug
> output is LIGHTGRAY .
> > >> I do not really know of any actual StdErr output from the Shell.
> > >>
> > >> So, there might be a bug somewhere that causes DEBUG output to
> > >> switch to MAGENTA and back. I am not really sure. But this
> > >> inconsistency is
> > annoying.
> > >> Can we simply avoid this by using a consistent color for all
> > >> console output? Or at least allow platforms to decide?
> > >>
> > >>
> > >> > -----Original Message-----
> > >> > From: devel@edk2.groups.io
> > >> > <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> > > <mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> > >> > Zhichao via groups.io
> > >> > Sent: Wednesday, December 2, 2020 6:05 AM
> > >> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> > >> > <mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> > >> > devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > >> > lersek@redhat.com
> > > <mailto:lersek@redhat.com>
> > >> > Cc: Wang, Jian J <jian.j.wang@intel.com
> > >> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> > >> > <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > >> > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>; Andy
> > >> > Lutomirski
> > > <luto@kernel.org <mailto:luto@kernel.org>>
> > >> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > >> > Change StdErr color to EFI_LIGHTGRAY
> > >> >
> > >> >
> > >> >
> > >> > > -----Original Message-----
> > >> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> > >> > > <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > >> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > >> > > To: Gao, Zhichao <zhichao.gao@intel.com
> > >> > > <mailto:zhichao.gao@intel.com>>;
> > > devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > >> > > lersek@redhat.com <mailto:lersek@redhat.com>
> > >> > > Cc: Wang, Jian J <jian.j.wang@intel.com
> > >> > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> > >> > > <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > >> > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> > Andy
> > >> > > Lutomirski
> > > <luto@kernel.org <mailto:luto@kernel.org>>; Samer
> > >> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com
> > >> > > <mailto:Mahmoud@arm.com>>
> > >> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > >> > > Change StdErr color to EFI_LIGHTGRAY
> > >> > >
> > >> > > Why does StdErr have to be a different color from ConOut? If
> > >> > > the system redirected both streams to the same console output
> > >> > > then that is
> > >> > their choice.
> > >> > > Serial DEBUG output is not a different color even if the DEBUG
> > >> > > is redirected to the same console as ConOut and StdErr. Also,
> > >> > > from what I have seen, StdErr does not seem to always retain
> > >> > > this MAGENTA color later (for example, after booting a UEFI Shell?).
> > >> >
> > >> > Can you share the use case of StdErr? Seems when using StdErr-
> > >> > >OutputString, the output is not always MAGENTA color. If so, it
> > >> > >is a bug of
> > >> > console driver.
> > >> >
> > >> > I am thinking of one case. The platform only have the serial port
> > >> > without any other display device. System boots to uefi shell and
> > >> > run a debug build application. And the app would have both print
> > >> > output and debug print. If the color are same, the info of normal
> > >> > print and debug print would be mixed up. I am saying StdErr
> > >> > output not
> > normal DebugLib.
> > >> >
> > >> > Thanks,
> > >> > Zhichao
> > >> >
> > >> > >
> > >> > > Do users really care (other than being annoyed by the
> > >> > > inconsistency of
> > >> > "some"
> > >> > > text showing up in purple?). Using the same color for
> > >> > > consoles/DEBUG output by default is consistent and clean.
> > >> > > Applications/users can always change the colors later to
> > >> > > whatever is the preference for that
> > >> > particular UI/CLI.
> > >> > >
> > >> > > Thanks,
> > >> > > --Samer
> > >> > >
> > >> > >
> > >> > > > -----Original Message-----
> > >> > > > From: Gao, Zhichao <zhichao.gao@intel.com
> > >> > > > <mailto:zhichao.gao@intel.com>>
> > >> > > > Sent: Monday, November 30, 2020 8:00 PM
> > >> > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > >> > > > lersek@redhat.com
> > > <mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> > >> > > > <Samer.El-Haj-Mahmoud@arm.com
> > >> > > > <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > >> > > > Cc: Wang, Jian J <jian.j.wang@intel.com
> > >> > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > >> > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > >> > > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> > Andy
> > >> > > > Lutomirski
> > > <luto@kernel.org <mailto:luto@kernel.org>>
> > >> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3]
> > MdeModulePkg/ConSplitter:
> > >> > > > Change StdErr color to EFI_LIGHTGRAY
> > >> > > >
> > >> > > > I agree the EFI_MAGENTA is not a good choose. But this may be
> > >> > > > a different issue. Many platforms would set serial port as
> > >> > > > ConOut and ErrOut. The different colors for them can differ
> > >> > > > the origin. I don't think change them to the same color is a good idea.
> > >> > > >
> > >> > > > Thanks,
> > >> > > > Zhichao
> > >> > > >
> > >> > > > > -----Original Message-----
> > >> > > > > From: devel@edk2.groups.io
> > >> > > > > <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> > > <mailto:devel@edk2.groups.io>> On Behalf Of
> > >> > > > > Laszlo Ersek
> > >> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > >> > > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > samer.el-haj-mahmoud@arm.com <mailto:samer.el-haj-
> > mahmoud@arm.com>
> > >> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com
> > >> > > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > >> > > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Gao,
> > >> > > > > Zhichao
> > > <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>; Ni,
> > >> > > > > Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard
> > >> > > > > Biesheuvel
> > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> > >> > > > > Andy Lutomirski <luto@kernel.org <mailto:luto@kernel.org>>
> > >> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3]
> > MdeModulePkg/ConSplitter:
> > >> > > > > Change StdErr color to EFI_LIGHTGRAY
> > >> > > > >
> > >> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > >> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for
> > >> > > > > > ConOut and EFI_MAGENTA for StdErr.
> > >> > > > > >
> > >> > > > > > This does not work all the time, and StdErr ends up
> > >> > > > > > showing parts in MAGENTA and other parts in LIGHTGRAY.
> > >> > > > > > Changing StdErr to LIGHTGRAY looks better and is more consistent.
> > >> > > > > >
> > >> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com
> > >> > > > > > <mailto:jian.j.wang@intel.com>>
> > >> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com
> > >> > > > > > <mailto:hao.a.wu@intel.com>>
> > >> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com
> > >> > > > > > <mailto:zhichao.gao@intel.com>>
> > >> > > > > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> > >> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com
> > >> > > > > > <mailto:Ard.Biesheuvel@arm.com>>
> > >> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > >> > > > Mahmoud@arm.com <mailto:Mahmoud@arm.com>>
> > >> > > > > > ---
> > >> > > > > >
> > >> > > > > >MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > >> > > > > >c
> > >> > > > > >|
> > >> 2
> > >> > > > > > +-
> > >> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >> > > > > >
> > >> > > > > > diff --git
> > >> > > > > >
> > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> > >> > > > > > .c
> > >> > > > > >
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> > >> > > > > > .c index b090de288517..e8cd4ce120a0 100644
> > >> > > > > > ---
> > >> > > > > >
> > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> > >> > > > > > .c
> > >> > > > > > +++
> > >> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > >> > > > > > +++ c
> > >> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart
> > >> > > > > >(
> > >> > > > > > // their MaxMode and QueryData should be the
> > >> > > > > >intersection of
> > >> > both.
> > >> > > > > > //
> > >> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr,
> > >> > > > > >TextOut, NULL, NULL);
> > >> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > >> > > > > >EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > >> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > >> > > > > > +EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > >> > > > > >
> > >> > > > > > return Status;
> > >> > > > > > }
> > >> > > > > >
> > >> > > > >
> > >> > > > > I am very curious as to how this patch is going to fare, as
> > >> > > > > Andy Lutomirski (CC'd) reported the same symptom in a
> > >> > > > > Fedora bugzilla ticket
> > >> > > > > 4+ years ago:
> > >> > > > >
> > >> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > >> > > > >
> > >> > > > > As you can see in that BZ, I found the same code location,
> > >> > > > > I just didn't feel up to starting another crusade on
> > >> > > > > edk2-devel
> > >> > > > > -- about colors even!... So I'll be watching this one now.
> > >> > > > > :)
> > >> > > > >
> > >> > > > > Thanks
> > >> > > > > Laszlo
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-04 12:42 ` Samer El-Haj-Mahmoud
2020-12-04 14:13 ` Ard Biesheuvel
@ 2020-12-08 1:28 ` Ni, Ray
2020-12-08 17:13 ` Samer El-Haj-Mahmoud
1 sibling, 1 reply; 21+ messages in thread
From: Ni, Ray @ 2020-12-08 1:28 UTC (permalink / raw)
To: Samer El-Haj-Mahmoud, devel@edk2.groups.io, Gao, Zhichao,
lersek@redhat.com, gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ard Biesheuvel,
'Andy Lutomirski'
[-- Attachment #1: Type: text/plain, Size: 16485 bytes --]
The issue happens when the terminal and debug log use the same device.
When they use different UARTs, such issue won’t happen.
Can such issue be resolved by restoring back to light gray after printing errors from StdErr?
Thanks,
Ray
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Sent: Friday, December 4, 2020 8:42 PM
To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; lersek@redhat.com; gaoliming <gaoliming@byosoft.com.cn>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
StdErr console is required to be implemented by the UEFI spec to fill in the pointers in ST (even with a stubbed protocol implementation).
So if we agree that the color of the StdErr (required but low to no usage) is impacting the serial debug prints (high usage for DEBUG targets), why do we still insist on keeping the MAGENTA color for StdErr? Yes adding PCDs would be nice for platforms to select their default colors (and it should be done for both ConOut and StdErr), but that can be added later if there is a real ask for it.
I still do not see why the original patch of simply changing StdErr default color from MAGENTA to LIGHTGRAY (and avoid all of this headache) is problematic?! Please consider the original patch so we can close on this topic and move on.
Thanks,
--Samer
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Gao, Zhichao via groups.io
Sent: Friday, December 4, 2020 12:24 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>; lersek@redhat.com<mailto:lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org<mailto:luto@kernel.org>>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Normal DebugLib usually outputs string debug message only. It would show at the serial device with it current attributes. ErrOut and ConOut would change the attribute. That is why the color changes.
Actually I don’t see many usage of the ErrOut. I would suggest to remove ErrOut if you’re not using it for debug output.
For the Pcds, I agree to let the consumer to choose their wonder default color. But it is better to get the MdeModulePkg maintainers’ point.
Thanks,
Zhichao
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Samer El-Haj-Mahmoud
Sent: Friday, December 4, 2020 9:21 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; lersek@redhat.com<mailto:lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org<mailto:luto@kernel.org>>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
It's actually all *DebugLibSerialPort :
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.dsc
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
But I have seen this on other systems as well.
________________________________
From: gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Sent: Thursday, December 3, 2020, 8:11 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Samer El-Haj-Mahmoud; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
Cc: 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy Lutomirski'
Subject: 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Samer:
Does all debug message output by PeiDxeDebugLibReportStatusCode? There is not debug message to print as UefiDebugLibStdErr or UefiDebugLibConOut. Right?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>
> <bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表 Samer
> El-Haj-Mahmoud
> 发送时间: 2020年12月4日 8:05
> 收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Zhichao,
>
> I can understand the rationale if this is truly only for StdErr (although it would
> have been nice to allow platforms to customize the color with a PCD).
> However, I see the inconsistency in debug output with platforms I tested with.
> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> DebugLiub using the same serial console. The serial debug starts with
> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
> installed. At that point, the debug output switches to MAGENTA, and
> continues to do so until entering the UI or booting to UEFI Shell, where the
> color switches back to LIGHTGRAY (attached screenshot2). After that, all
> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
> StdErr output from the Shell.
>
> So, there might be a bug somewhere that causes DEBUG output to switch to
> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
> Can we simply avoid this by using a consistent color for all console output? Or
> at least allow platforms to decide?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> > Zhichao via groups.io
> > Sent: Wednesday, December 2, 2020 6:05 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> > devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > > -----Original Message-----
> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> > > lersek@redhat.com<mailto:lersek@redhat.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Why does StdErr have to be a different color from ConOut? If the
> > > system redirected both streams to the same console output then that is
> > their choice.
> > > Serial DEBUG output is not a different color even if the DEBUG is
> > > redirected to the same console as ConOut and StdErr. Also, from what I
> > > have seen, StdErr does not seem to always retain this MAGENTA color
> > > later (for example, after booting a UEFI Shell?).
> >
> > Can you share the use case of StdErr? Seems when using StdErr-
> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> > console driver.
> >
> > I am thinking of one case. The platform only have the serial port without any
> > other display device. System boots to uefi shell and run a debug build
> > application. And the app would have both print output and debug print. If
> > the color are same, the info of normal print and debug print would be mixed
> > up. I am saying StdErr output not normal DebugLib.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Do users really care (other than being annoyed by the inconsistency of
> > "some"
> > > text showing up in purple?). Using the same color for consoles/DEBUG
> > > output by default is consistent and clean. Applications/users can
> > > always change the colors later to whatever is the preference for that
> > particular UI/CLI.
> > >
> > > Thanks,
> > > --Samer
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > > different issue. Many platforms would set serial port as ConOut and
> > > > ErrOut. The different colors for them can differ the origin. I don't
> > > > think change them to the same color is a good idea.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > > > Laszlo Ersek
> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; samer.el-haj-mahmoud@arm.com<mailto:samer.el-haj-mahmoud@arm.com>
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; Ni,
> > > > > Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>;
> > > > > Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > > >
> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > > and EFI_MAGENTA for StdErr.
> > > > > >
> > > > > > This does not work all the time, and StdErr ends up showing
> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > > to LIGHTGRAY looks better and is more consistent.
> > > > > >
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > > > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>
> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > > > > ---
> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
> 2
> > > > > > +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > > ---
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > +++
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > > +++ c
> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > > // their MaxMode and QueryData should be the intersection of
> > both.
> > > > > > //
> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > > NULL, NULL);
> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > > >
> > > > > > return Status;
> > > > > > }
> > > > > >
> > > > >
> > > > > I am very curious as to how this patch is going to fare, as Andy
> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > > ticket
> > > > > 4+ years ago:
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > > >
> > > > > As you can see in that BZ, I found the same code location, I just
> > > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > > colors even!... So I'll be watching this one now. :)
> > > > >
> > > > > Thanks
> > > > > Laszlo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose
> > > the contents to any other person, use it for any purpose, or store or
> > > copy the information in any medium. Thank you.
> >
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
[-- Attachment #2: Type: text/html, Size: 34868 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-08 0:59 ` Gao, Zhichao
@ 2020-12-08 17:07 ` Samer El-Haj-Mahmoud
0 siblings, 0 replies; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-08 17:07 UTC (permalink / raw)
To: devel@edk2.groups.io, zhichao.gao@intel.com, Ard Biesheuvel,
lersek@redhat.com, gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, 'Andy Lutomirski', nd,
Samer El-Haj-Mahmoud
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao,
> Zhichao via groups.io
> Sent: Monday, December 7, 2020 7:59 PM
> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Ard
> Biesheuvel <Ard.Biesheuvel@arm.com>; devel@edk2.groups.io;
> lersek@redhat.com; gaoliming <gaoliming@byosoft.com.cn>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; 'Andy Lutomirski'
> <luto@kernel.org>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> Change StdErr color to EFI_LIGHTGRAY
>
> My consideration is the impact of make StdErr and StdCon same foreground
> and background. I don't think they should be the same. But it actually affects
> the DebugLib and other terminal operation's output.
> I agree with the change but the commit message should be changed. ErrOut
> would always output with its own attribute but DebugLib wouldn't. And we
> should add the impact as well: it makes ConOut and ErrOut same foreground
> and background. If consumers want to differ them in the same output
> devices, they should change one of their attributes by themselves.
>
Thanks Zhichao. Makes sense. I will send an updated patch explaining the reason behind the change (impact on DebugLib) the impact of the change on ConOut/StdErr.
> Thanks,
> Zhichao
>
> > -----Original Message-----
> > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > Sent: Tuesday, December 8, 2020 2:36 AM
> > To: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; devel@edk2.groups.io;
> > Gao, Zhichao <zhichao.gao@intel.com>; lersek@redhat.com; gaoliming
> > <gaoliming@byosoft.com.cn>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; 'Andy Lutomirski'
> > <luto@kernel.org>; Samer El-Haj- Mahmoud
> > <Samer.El-Haj-Mahmoud@arm.com>; nd <nd@arm.com>
> > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> > Thanks Ard..
> >
> > Any further feedback or a RB for this patch, so we can close on the
> > "console color" topic and move on?
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > Sent: Friday, December 4, 2020 9:14 AM
> > > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>;
> > > devel@edk2.groups.io; zhichao.gao@intel.com; lersek@redhat.com;
> > > gaoliming <gaoliming@byosoft.com.cn>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; 'Andy Lutomirski'
> > > <luto@kernel.org>
> > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Agreed. We can theorize endlessly about different combinations of
> > > StdOut and StdErr, but in the meantime, the magenta serial output is
> > > very
> > annoying.
> > > For instance, if you boot Linux in OVMF using the serial console,
> > > all Linux's console output is magenta (and therefore barely legible
> > > on a black
> > > background) simply because it inherits the color setting from the
> firmware.
> > >
> > >
> > >
> > > On 12/4/20 1:42 PM, Samer El-Haj-Mahmoud wrote:
> > > > StdErr console is required to be implemented by the UEFI spec to
> > > > fill in the pointers in ST (even with a stubbed protocol implementation).
> > > >
> > > >
> > > >
> > > > So if we agree that the color of the StdErr (required but low to
> > > > no
> > > > usage) is impacting the serial debug prints (high usage for DEBUG
> > > > targets), why do we still insist on keeping the MAGENTA color for
> > > > StdErr? Yes adding PCDs would be nice for platforms to select
> > > > their default colors (and it should be done for both ConOut and
> > > > StdErr), but that can be added later if there is a real ask for it.
> > > >
> > > >
> > > >
> > > > I still do not see why the original patch of simply changing
> > > > StdErr default color from MAGENTA to LIGHTGRAY (and avoid all of
> > > > this
> > > > headache) is problematic?! Please consider the original patch so
> > > > we can close on this topic and move on.
> > > >
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > --Samer
> > > >
> > > >
> > > >
> > > > *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
> > > *Gao,
> > > > Zhichao via groups.io
> > > > *Sent:* Friday, December 4, 2020 12:24 AM
> > > > *To:* devel@edk2.groups.io; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com>; lersek@redhat.com; gaoliming
> > > > <gaoliming@byosoft.com.cn>
> > > > *Cc:* Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
> > > > *Subject:* Re: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > >
> > > >
> > > > Normal DebugLib usually outputs string debug message only. It
> > > > would show at the serial device with it current attributes. ErrOut
> > > > and ConOut would change the attribute. That is why the color changes.
> > > >
> > > > Actually I don’t see many usage of the ErrOut. I would suggest to
> > > > remove ErrOut if you’re not using it for debug output.
> > > >
> > > > For the Pcds, I agree to let the consumer to choose their wonder
> > > > default color. But it is better to get the MdeModulePkg maintainers’
> point.
> > > >
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Zhichao
> > > >
> > > >
> > > >
> > > > *From:*devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf
> Of
> > > > *Samer El-Haj-Mahmoud
> > > > *Sent:* Friday, December 4, 2020 9:21 AM
> > > > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Gao,
> > > Zhichao
> > > > <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
> > > > lersek@redhat.com <mailto:lersek@redhat.com>; gaoliming
> > > > <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>
> > > > *Cc:* Wang, Jian J <jian.j.wang@intel.com
> > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> > > > <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com
> > > > <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com
> > > > <mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski'
> > > > <luto@kernel.org <mailto:luto@kernel.org>>
> > > > *Subject:* Re: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > >
> > > >
> > > > It's actually all *DebugLibSerialPort :
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > https://github.com/tianocore/edk2-
> > > platforms/blob/master/Platform/Raspb
> > > > erryPi/RPi4/RPi4.dsc
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.
> > > > DebugLib|inf
> > > >
> > > >
> > >
> DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDeb
> > > ugLi
> > > > DebugLib|bSerialPort.inf
> > > >
> > > >
> > > > But I have seen this on other systems as well.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > ------------------------------------------------------------------
> > > > --
> > > > --
> > > > --
> > > >
> > > > *From:* gaoliming <gaoliming@byosoft.com.cn
> > > > <mailto:gaoliming@byosoft.com.cn>>
> > > > *Sent:* Thursday, December 3, 2020, 8:11 PM
> > > > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Samer
> > > > El-Haj-Mahmoud; zhichao.gao@intel.com
> > > <mailto:zhichao.gao@intel.com>;
> > > > lersek@redhat.com <mailto:lersek@redhat.com>
> > > > *Cc:* 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel;
> > > > 'Andy Lutomirski'
> > > > *Subject:* 回复: [edk2-devel] [PATCH v1 3/3]
> > > MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > >
> > > >
> > > > Samer:
> > > > Does all debug message output by PeiDxeDebugLibReportStatusCode?
> > > > There is not debug message to print as UefiDebugLibStdErr or
> > > > UefiDebugLibConOut. Right?
> > > >
> > > > Thanks
> > > > Liming
> > > >> -----邮件原件-----
> > > >> 发件人: bounce+27952+68301+4905953+8761045@groups.io
> > > > <mailto:bounce+27952+68301+4905953+8761045@groups.io>
> > > >> <bounce+27952+68301+4905953+8761045@groups.io
> > > > <mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表
> Samer
> > > >> El-Haj-Mahmoud
> > > >> 发送时间: 2020年12月4日8:05
> > > >> 收件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > > zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>;
> > > > lersek@redhat.com <mailto:lersek@redhat.com>
> > > >> 抄送: Wang, Jian J <jian.j.wang@intel.com
> > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > >> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > >> <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> Andy
> > > >> Lutomirski
> > > > <luto@kernel.org <mailto:luto@kernel.org>>; Samer
> > > >> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> > > >> <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > >> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change
> > > >> StdErr color to EFI_LIGHTGRAY
> > > >>
> > > >> Zhichao,
> > > >>
> > > >> I can understand the rationale if this is truly only for StdErr
> > > >> (although it would have been nice to allow platforms to customize
> > > >> the
> > > color with a PCD).
> > > >> However, I see the inconsistency in debug output with platforms I
> > > >> tested
> > > with.
> > > >> For example, on the RPi, with DEBUG build, and all ConOut/StdErr
> > > >> and DebugLiub using the same serial console. The serial debug
> > > >> starts with LIGHTGRAY (attached screenshot 1), until
> > > >> gEfiStandardErrorDeviceGuid is installed. At that point, the
> > > >> debug output switches to MAGENTA, and continues to do so until
> > > >> entering the UI or booting to UEFI Shell, where the color
> > > >> switches back to LIGHTGRAY (attached screenshot2). After that,
> > > >> all ConOut and Debug
> > output is LIGHTGRAY .
> > > >> I do not really know of any actual StdErr output from the Shell.
> > > >>
> > > >> So, there might be a bug somewhere that causes DEBUG output to
> > > >> switch to MAGENTA and back. I am not really sure. But this
> > > >> inconsistency is
> > > annoying.
> > > >> Can we simply avoid this by using a consistent color for all
> > > >> console output? Or at least allow platforms to decide?
> > > >>
> > > >>
> > > >> > -----Original Message-----
> > > >> > From: devel@edk2.groups.io
> > > >> > <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> > > > <mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> > > >> > Zhichao via groups.io
> > > >> > Sent: Wednesday, December 2, 2020 6:05 AM
> > > >> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com
> > > >> > <mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> > > >> > devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > >> > lersek@redhat.com
> > > > <mailto:lersek@redhat.com>
> > > >> > Cc: Wang, Jian J <jian.j.wang@intel.com
> > > >> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com
> > > >> > <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > >> > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> Andy
> > > >> > Lutomirski
> > > > <luto@kernel.org <mailto:luto@kernel.org>>
> > > >> > Subject: Re: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> > > >> > Change StdErr color to EFI_LIGHTGRAY
> > > >> >
> > > >> >
> > > >> >
> > > >> > > -----Original Message-----
> > > >> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com
> > > >> > > <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > >> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > >> > > To: Gao, Zhichao <zhichao.gao@intel.com
> > > >> > > <mailto:zhichao.gao@intel.com>>;
> > > > devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > >> > > lersek@redhat.com <mailto:lersek@redhat.com>
> > > >> > > Cc: Wang, Jian J <jian.j.wang@intel.com
> > > >> > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > >> > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > >> > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> > > Andy
> > > >> > > Lutomirski
> > > > <luto@kernel.org <mailto:luto@kernel.org>>; Samer
> > > >> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com
> > > >> > > <mailto:Mahmoud@arm.com>>
> > > >> > > Subject: RE: [edk2-devel] [PATCH v1 3/3]
> MdeModulePkg/ConSplitter:
> > > >> > > Change StdErr color to EFI_LIGHTGRAY
> > > >> > >
> > > >> > > Why does StdErr have to be a different color from ConOut? If
> > > >> > > the system redirected both streams to the same console output
> > > >> > > then that is
> > > >> > their choice.
> > > >> > > Serial DEBUG output is not a different color even if the
> > > >> > > DEBUG is redirected to the same console as ConOut and StdErr.
> > > >> > > Also, from what I have seen, StdErr does not seem to always
> > > >> > > retain this MAGENTA color later (for example, after booting a UEFI
> Shell?).
> > > >> >
> > > >> > Can you share the use case of StdErr? Seems when using StdErr-
> > > >> > >OutputString, the output is not always MAGENTA color. If so,
> > > >> > >it is a bug of
> > > >> > console driver.
> > > >> >
> > > >> > I am thinking of one case. The platform only have the serial
> > > >> > port without any other display device. System boots to uefi
> > > >> > shell and run a debug build application. And the app would have
> > > >> > both print output and debug print. If the color are same, the
> > > >> > info of normal print and debug print would be mixed up. I am
> > > >> > saying StdErr output not
> > > normal DebugLib.
> > > >> >
> > > >> > Thanks,
> > > >> > Zhichao
> > > >> >
> > > >> > >
> > > >> > > Do users really care (other than being annoyed by the
> > > >> > > inconsistency of
> > > >> > "some"
> > > >> > > text showing up in purple?). Using the same color for
> > > >> > > consoles/DEBUG output by default is consistent and clean.
> > > >> > > Applications/users can always change the colors later to
> > > >> > > whatever is the preference for that
> > > >> > particular UI/CLI.
> > > >> > >
> > > >> > > Thanks,
> > > >> > > --Samer
> > > >> > >
> > > >> > >
> > > >> > > > -----Original Message-----
> > > >> > > > From: Gao, Zhichao <zhichao.gao@intel.com
> > > >> > > > <mailto:zhichao.gao@intel.com>>
> > > >> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > >> > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > >> > > > lersek@redhat.com
> > > > <mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> > > >> > > > <Samer.El-Haj-Mahmoud@arm.com
> > > >> > > > <mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > >> > > > Cc: Wang, Jian J <jian.j.wang@intel.com
> > > >> > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > >> > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray
> > > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > >> > > > <Ard.Biesheuvel@arm.com
> <mailto:Ard.Biesheuvel@arm.com>>;
> > > Andy
> > > >> > > > Lutomirski
> > > > <luto@kernel.org <mailto:luto@kernel.org>>
> > > >> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3]
> > > MdeModulePkg/ConSplitter:
> > > >> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >> > > >
> > > >> > > > I agree the EFI_MAGENTA is not a good choose. But this may
> > > >> > > > be a different issue. Many platforms would set serial port
> > > >> > > > as ConOut and ErrOut. The different colors for them can
> > > >> > > > differ the origin. I don't think change them to the same color is a
> good idea.
> > > >> > > >
> > > >> > > > Thanks,
> > > >> > > > Zhichao
> > > >> > > >
> > > >> > > > > -----Original Message-----
> > > >> > > > > From: devel@edk2.groups.io
> > > >> > > > > <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> > > > <mailto:devel@edk2.groups.io>> On Behalf Of
> > > >> > > > > Laszlo Ersek
> > > >> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > >> > > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> > > > samer.el-haj-mahmoud@arm.com <mailto:samer.el-haj-
> > > mahmoud@arm.com>
> > > >> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com
> > > >> > > > > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > >> > > > > <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Gao,
> > > >> > > > > Zhichao
> > > > <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>; Ni,
> > > >> > > > > Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Ard
> > > >> > > > > Biesheuvel
> > > > <Ard.Biesheuvel@arm.com <mailto:Ard.Biesheuvel@arm.com>>;
> > > >> > > > > Andy Lutomirski <luto@kernel.org
> > > >> > > > > <mailto:luto@kernel.org>>
> > > >> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3]
> > > MdeModulePkg/ConSplitter:
> > > >> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > >> > > > >
> > > >> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > >> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color
> > > >> > > > > > for ConOut and EFI_MAGENTA for StdErr.
> > > >> > > > > >
> > > >> > > > > > This does not work all the time, and StdErr ends up
> > > >> > > > > > showing parts in MAGENTA and other parts in LIGHTGRAY.
> > > >> > > > > > Changing StdErr to LIGHTGRAY looks better and is more
> consistent.
> > > >> > > > > >
> > > >> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com
> > > >> > > > > > <mailto:jian.j.wang@intel.com>>
> > > >> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com
> > > >> > > > > > <mailto:hao.a.wu@intel.com>>
> > > >> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com
> > > >> > > > > > <mailto:zhichao.gao@intel.com>>
> > > >> > > > > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> > > >> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com
> > > >> > > > > > <mailto:Ard.Biesheuvel@arm.com>>
> > > >> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > >> > > > Mahmoud@arm.com <mailto:Mahmoud@arm.com>>
> > > >> > > > > > ---
> > > >> > > > > >
> > > >> > > > >
> >MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > >> > > > > >c
> > > >> > > > > >|
> > > >> 2
> > > >> > > > > > +-
> > > >> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >> > > > > >
> > > >> > > > > > diff --git
> > > >> > > > > >
> > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> > > >> > > > > > .c
> > > >> > > > > >
> > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> > > >> > > > > > .c index b090de288517..e8cd4ce120a0 100644
> > > >> > > > > > ---
> > > >> > > > > >
> > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter
> > > >> > > > > > .c
> > > >> > > > > > +++
> > > >> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > >> > > > > > +++ c
> > > >> > > > > > @@ -1476,7 +1476,7 @@
> > > >> > > > > >ConSplitterStdErrDriverBindingStart
> > > >> > > > > >(
> > > >> > > > > > // their MaxMode and QueryData should be the
> > > >> > > > > >intersection of
> > > >> > both.
> > > >> > > > > > //
> > > >> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr,
> > > >> > > > > >TextOut, NULL, NULL);
> > > >> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > >> > > > > >EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > >> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > >> > > > > > +EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > >> > > > > >
> > > >> > > > > > return Status;
> > > >> > > > > > }
> > > >> > > > > >
> > > >> > > > >
> > > >> > > > > I am very curious as to how this patch is going to fare,
> > > >> > > > > as Andy Lutomirski (CC'd) reported the same symptom in a
> > > >> > > > > Fedora bugzilla ticket
> > > >> > > > > 4+ years ago:
> > > >> > > > >
> > > >> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > >> > > > >
> > > >> > > > > As you can see in that BZ, I found the same code
> > > >> > > > > location, I just didn't feel up to starting another
> > > >> > > > > crusade on edk2-devel
> > > >> > > > > -- about colors even!... So I'll be watching this one now.
> > > >> > > > > :)
> > > >> > > > >
> > > >> > > > > Thanks
> > > >> > > > > Laszlo
> > > >> > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > >
>
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
2020-12-08 1:28 ` Ni, Ray
@ 2020-12-08 17:13 ` Samer El-Haj-Mahmoud
0 siblings, 0 replies; 21+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-12-08 17:13 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Gao, Zhichao, lersek@redhat.com,
gaoliming
Cc: Wang, Jian J, Wu, Hao A, Ard Biesheuvel,
'Andy Lutomirski', Samer El-Haj-Mahmoud
[-- Attachment #1: Type: text/plain, Size: 18284 bytes --]
Ray,
As explained in my reply to Zhichao, we are more concerned about the impact to serial console users (DebugLib and in some cases the kernel messages, which is common and annoying), than the impact to StdErr users (not common, in fact none that I know of other than UefiDebugLibStdErr and possibly some Shell scripts with console redirection). The simplest solution is to make StdErr use the same color as ConOut and DebugLib, and let platforms deal with changing the colors in their own platform console initialization BDS code. I will send an updated patch with the commit message explaining this.
Thanks,
--Samer
From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, December 7, 2020 8:28 PM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; lersek@redhat.com; gaoliming <gaoliming@byosoft.com.cn>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; 'Andy Lutomirski' <luto@kernel.org>
Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
The issue happens when the terminal and debug log use the same device.
When they use different UARTs, such issue won’t happen.
Can such issue be resolved by restoring back to light gray after printing errors from StdErr?
Thanks,
Ray
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
Sent: Friday, December 4, 2020 8:42 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; lersek@redhat.com<mailto:lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org<mailto:luto@kernel.org>>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
StdErr console is required to be implemented by the UEFI spec to fill in the pointers in ST (even with a stubbed protocol implementation).
So if we agree that the color of the StdErr (required but low to no usage) is impacting the serial debug prints (high usage for DEBUG targets), why do we still insist on keeping the MAGENTA color for StdErr? Yes adding PCDs would be nice for platforms to select their default colors (and it should be done for both ConOut and StdErr), but that can be added later if there is a real ask for it.
I still do not see why the original patch of simply changing StdErr default color from MAGENTA to LIGHTGRAY (and avoid all of this headache) is problematic?! Please consider the original patch so we can close on this topic and move on.
Thanks,
--Samer
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Gao, Zhichao via groups.io
Sent: Friday, December 4, 2020 12:24 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>; lersek@redhat.com<mailto:lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org<mailto:luto@kernel.org>>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Normal DebugLib usually outputs string debug message only. It would show at the serial device with it current attributes. ErrOut and ConOut would change the attribute. That is why the color changes.
Actually I don’t see many usage of the ErrOut. I would suggest to remove ErrOut if you’re not using it for debug output.
For the Pcds, I agree to let the consumer to choose their wonder default color. But it is better to get the MdeModulePkg maintainers’ point.
Thanks,
Zhichao
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Samer El-Haj-Mahmoud
Sent: Friday, December 4, 2020 9:21 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; lersek@redhat.com<mailto:lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; 'Andy Lutomirski' <luto@kernel.org<mailto:luto@kernel.org>>
Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
It's actually all *DebugLibSerialPort :
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.dsc
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
But I have seen this on other systems as well.
________________________________
From: gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Sent: Thursday, December 3, 2020, 8:11 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Samer El-Haj-Mahmoud; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
Cc: 'Wang, Jian J'; 'Wu, Hao A'; 'Ni, Ray'; Ard Biesheuvel; 'Andy Lutomirski'
Subject: 回复: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY
Samer:
Does all debug message output by PeiDxeDebugLibReportStatusCode? There is not debug message to print as UefiDebugLibStdErr or UefiDebugLibConOut. Right?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>
> <bounce+27952+68301+4905953+8761045@groups.io<mailto:bounce+27952+68301+4905953+8761045@groups.io>> 代表 Samer
> El-Haj-Mahmoud
> 发送时间: 2020年12月4日 8:05
> 收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change
> StdErr color to EFI_LIGHTGRAY
>
> Zhichao,
>
> I can understand the rationale if this is truly only for StdErr (although it would
> have been nice to allow platforms to customize the color with a PCD).
> However, I see the inconsistency in debug output with platforms I tested with.
> For example, on the RPi, with DEBUG build, and all ConOut/StdErr and
> DebugLiub using the same serial console. The serial debug starts with
> LIGHTGRAY (attached screenshot 1), until gEfiStandardErrorDeviceGuid is
> installed. At that point, the debug output switches to MAGENTA, and
> continues to do so until entering the UI or booting to UEFI Shell, where the
> color switches back to LIGHTGRAY (attached screenshot2). After that, all
> ConOut and Debug output is LIGHTGRAY . I do not really know of any actual
> StdErr output from the Shell.
>
> So, there might be a bug somewhere that causes DEBUG output to switch to
> MAGENTA and back. I am not really sure. But this inconsistency is annoying.
> Can we simply avoid this by using a consistent color for all console output? Or
> at least allow platforms to decide?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Gao,
> > Zhichao via groups.io
> > Sent: Wednesday, December 2, 2020 6:05 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>;
> > devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > Change StdErr color to EFI_LIGHTGRAY
> >
> >
> >
> > > -----Original Message-----
> > > From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> > > lersek@redhat.com<mailto:lersek@redhat.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>; Samer
> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > Change StdErr color to EFI_LIGHTGRAY
> > >
> > > Why does StdErr have to be a different color from ConOut? If the
> > > system redirected both streams to the same console output then that is
> > their choice.
> > > Serial DEBUG output is not a different color even if the DEBUG is
> > > redirected to the same console as ConOut and StdErr. Also, from what I
> > > have seen, StdErr does not seem to always retain this MAGENTA color
> > > later (for example, after booting a UEFI Shell?).
> >
> > Can you share the use case of StdErr? Seems when using StdErr-
> > >OutputString, the output is not always MAGENTA color. If so, it is a bug of
> > console driver.
> >
> > I am thinking of one case. The platform only have the serial port without any
> > other display device. System boots to uefi shell and run a debug build
> > application. And the app would have both print output and debug print. If
> > the color are same, the info of normal print and debug print would be mixed
> > up. I am saying StdErr output not normal DebugLib.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Do users really care (other than being annoyed by the inconsistency of
> > "some"
> > > text showing up in purple?). Using the same color for consoles/DEBUG
> > > output by default is consistent and clean. Applications/users can
> > > always change the colors later to whatever is the preference for that
> > particular UI/CLI.
> > >
> > > Thanks,
> > > --Samer
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > Sent: Monday, November 30, 2020 8:00 PM
> > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Samer El-Haj-Mahmoud
> > > > <Samer.El-Haj-Mahmoud@arm.com<mailto:Samer.El-Haj-Mahmoud@arm.com>>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel
> > > > <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>; Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > Subject: RE: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > Change StdErr color to EFI_LIGHTGRAY
> > > >
> > > > I agree the EFI_MAGENTA is not a good choose. But this may be a
> > > > different issue. Many platforms would set serial port as ConOut and
> > > > ErrOut. The different colors for them can differ the origin. I don't
> > > > think change them to the same color is a good idea.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > > > Laszlo Ersek
> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; samer.el-haj-mahmoud@arm.com<mailto:samer.el-haj-mahmoud@arm.com>
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> > > > > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; Ni,
> > > > > Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>;
> > > > > Andy Lutomirski <luto@kernel.org<mailto:luto@kernel.org>>
> > > > > Subject: Re: [edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter:
> > > > > Change StdErr color to EFI_LIGHTGRAY
> > > > >
> > > > > On 11/24/20 20:15, Samer El-Haj-Mahmoud wrote:
> > > > > > ConSplitter was using EFI_LIGHTGRAY foreground color for ConOut
> > > > > > and EFI_MAGENTA for StdErr.
> > > > > >
> > > > > > This does not work all the time, and StdErr ends up showing
> > > > > > parts in MAGENTA and other parts in LIGHTGRAY. Changing StdErr
> > > > > > to LIGHTGRAY looks better and is more consistent.
> > > > > >
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> > > > > > Cc: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
> > > > > > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com<mailto:Ard.Biesheuvel@arm.com>>
> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com<mailto:Mahmoud@arm.com>>
> > > > > > ---
> > > > > > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c |
> 2
> > > > > > +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > index b090de288517..e8cd4ce120a0 100644
> > > > > > ---
> > > > > > a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
> > > > > > +++
> > b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.
> > > > > > +++ c
> > > > > > @@ -1476,7 +1476,7 @@ ConSplitterStdErrDriverBindingStart (
> > > > > > // their MaxMode and QueryData should be the intersection of
> > both.
> > > > > > //
> > > > > > Status = ConSplitterTextOutAddDevice (&mStdErr, TextOut,
> > > > > > NULL, NULL);
> > > > > > - ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > EFI_TEXT_ATTR (EFI_MAGENTA, EFI_BLACK));
> > > > > > + ConSplitterTextOutSetAttribute (&mStdErr.TextOut,
> > > > > > + EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
> > > > > >
> > > > > > return Status;
> > > > > > }
> > > > > >
> > > > >
> > > > > I am very curious as to how this patch is going to fare, as Andy
> > > > > Lutomirski (CC'd) reported the same symptom in a Fedora bugzilla
> > > > > ticket
> > > > > 4+ years ago:
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1355913
> > > > >
> > > > > As you can see in that BZ, I found the same code location, I just
> > > > > didn't feel up to starting another crusade on edk2-devel -- about
> > > > > colors even!... So I'll be watching this one now. :)
> > > > >
> > > > > Thanks
> > > > > Laszlo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose
> > > the contents to any other person, use it for any purpose, or store or
> > > copy the information in any medium. Thank you.
> >
> >
> >
> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
[-- Attachment #2: Type: text/html, Size: 39301 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-12-08 17:13 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-24 19:15 [PATCH v1 0/3] MdeModulePkg/GraphicsConsole : Console cleanup Samer El-Haj-Mahmoud
2020-11-24 19:15 ` [PATCH v1 1/3] MdeModulePkg/GraphicsConsoleDxe: Change default CursorVisible to FALSE Samer El-Haj-Mahmoud
2020-12-01 0:45 ` Gao, Zhichao
2020-11-24 19:15 ` [PATCH v1 2/3] MdeModulePkg/Graphics: Fix spelling mistakes Samer El-Haj-Mahmoud
2020-12-01 0:45 ` Gao, Zhichao
2020-11-24 19:15 ` [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY Samer El-Haj-Mahmoud
2020-11-24 23:30 ` [edk2-devel] " Laszlo Ersek
2020-12-01 0:59 ` Gao, Zhichao
2020-12-01 15:17 ` Samer El-Haj-Mahmoud
2020-12-02 11:04 ` Gao, Zhichao
2020-12-04 0:04 ` Samer El-Haj-Mahmoud
2020-12-04 1:11 ` 回复: " gaoliming
2020-12-04 1:20 ` Samer El-Haj-Mahmoud
2020-12-04 5:23 ` Gao, Zhichao
2020-12-04 12:42 ` Samer El-Haj-Mahmoud
2020-12-04 14:13 ` Ard Biesheuvel
2020-12-07 18:36 ` Samer El-Haj-Mahmoud
2020-12-08 0:59 ` Gao, Zhichao
2020-12-08 17:07 ` Samer El-Haj-Mahmoud
2020-12-08 1:28 ` Ni, Ray
2020-12-08 17:13 ` Samer El-Haj-Mahmoud
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox