[edk2-devel] [PATCH v1 3/3] MdeModulePkg/ConSplitter: Change StdErr color to EFI_LIGHTGRAY

Tue Dec 8 00:59:23 UTC 2020

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 at arm.com>
> Sent: Tuesday, December 8, 2020 2:36 AM
> To: Ard Biesheuvel <Ard.Biesheuvel at arm.com>; devel at edk2.groups.io; Gao,
> Zhichao <zhichao.gao at intel.com>; lersek at redhat.com; gaoliming
> <gaoliming at byosoft.com.cn>
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>; Ni,
> Ray <ray.ni at intel.com>; 'Andy Lutomirski' <luto at kernel.org>; Samer El-Haj-
> Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; nd <nd at 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 at arm.com>
> > Sent: Friday, December 4, 2020 9:14 AM
> > To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>;
> > devel at edk2.groups.io; zhichao.gao at intel.com; lersek at redhat.com;
> > gaoliming <gaoliming at byosoft.com.cn>
> > Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
> > <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; 'Andy Lutomirski'
> > <luto at 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 at edk2.groups.io <devel at edk2.groups.io> *On Behalf Of
> > *Gao,
> > > Zhichao via groups.io
> > > *Sent:* Friday, December 4, 2020 12:24 AM
> > > *To:* devel at edk2.groups.io; Samer El-Haj-Mahmoud
> > > <Samer.El-Haj-Mahmoud at arm.com>; lersek at redhat.com; gaoliming
> > > <gaoliming at byosoft.com.cn>
> > > *Cc:* Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
> > > <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Ard Biesheuvel
> > > <Ard.Biesheuvel at arm.com>; 'Andy Lutomirski' <luto at 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 at edk2.groups.io <mailto:devel at edk2.groups.io>
> > > <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> *On Behalf Of
> > > *Samer El-Haj-Mahmoud
> > > *Sent:* Friday, December 4, 2020 9:21 AM
> > > *To:* devel at edk2.groups.io <mailto:devel at edk2.groups.io>; Gao,
> > Zhichao
> > > <zhichao.gao at intel.com <mailto:zhichao.gao at intel.com>>;
> > > lersek at redhat.com <mailto:lersek at redhat.com>; gaoliming
> > > <gaoliming at byosoft.com.cn <mailto:gaoliming at byosoft.com.cn>>
> > > *Cc:* Wang, Jian J <jian.j.wang at intel.com
> > > <mailto:jian.j.wang at intel.com>>; Wu, Hao A <hao.a.wu at intel.com
> > > <mailto:hao.a.wu at intel.com>>; Ni, Ray <ray.ni at intel.com
> > > <mailto:ray.ni at intel.com>>; Ard Biesheuvel <Ard.Biesheuvel at arm.com
> > > <mailto:Ard.Biesheuvel at arm.com>>; 'Andy Lutomirski' <luto at kernel.org
> > > <mailto:luto at 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 at byosoft.com.cn
> > > <mailto:gaoliming at byosoft.com.cn>>
> > > *Sent:* Thursday, December 3, 2020, 8:11 PM
> > > *To:* devel at edk2.groups.io <mailto:devel at edk2.groups.io>; Samer
> > > El-Haj-Mahmoud; zhichao.gao at intel.com
> > <mailto:zhichao.gao at intel.com>;
> > > lersek at redhat.com <mailto:lersek at 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 at groups.io
> > > <mailto:bounce+27952+68301+4905953+8761045 at groups.io>
> > >> <bounce+27952+68301+4905953+8761045 at groups.io
> > > <mailto:bounce+27952+68301+4905953+8761045 at groups.io>> 代表Samer
> > >> El-Haj-Mahmoud
> > >> 发送时间: 2020年12月4日8:05
> > >> 收件人: devel at edk2.groups.io <mailto:devel at edk2.groups.io>;
> > > zhichao.gao at intel.com <mailto:zhichao.gao at intel.com>;
> > > lersek at redhat.com <mailto:lersek at redhat.com>
> > >> 抄送: Wang, Jian J <jian.j.wang at intel.com
> > > <mailto:jian.j.wang at intel.com>>; Wu, Hao A
> > >> <hao.a.wu at intel.com <mailto:hao.a.wu at intel.com>>; Ni, Ray
> > > <ray.ni at intel.com <mailto:ray.ni at intel.com>>; Ard Biesheuvel
> > >> <Ard.Biesheuvel at arm.com <mailto:Ard.Biesheuvel at arm.com>>; Andy
> > >> Lutomirski
> > > <luto at kernel.org <mailto:luto at kernel.org>>; Samer
> > >> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com
> > >> <mailto:Samer.El-Haj-Mahmoud at 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 at edk2.groups.io
> > >> > <mailto:devel at edk2.groups.io><devel at edk2.groups.io
> > > <mailto:devel at 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 at arm.com
> > >> > <mailto:Samer.El-Haj-Mahmoud at arm.com>>;
> > >> > devel at edk2.groups.io <mailto:devel at edk2.groups.io>;
> > >> > lersek at redhat.com
> > > <mailto:lersek at redhat.com>
> > >> > Cc: Wang, Jian J <jian.j.wang at intel.com
> > >> > <mailto:jian.j.wang at intel.com>>; Wu, Hao A <hao.a.wu at intel.com
> > >> > <mailto:hao.a.wu at intel.com>>; Ni, Ray
> > > <ray.ni at intel.com <mailto:ray.ni at intel.com>>; Ard Biesheuvel
> > >> > <Ard.Biesheuvel at arm.com <mailto:Ard.Biesheuvel at arm.com>>; Andy
> > >> > Lutomirski
> > > <luto at kernel.org <mailto:luto at 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 at arm.com
> > >> > > <mailto:Samer.El-Haj-Mahmoud at arm.com>>
> > >> > > Sent: Tuesday, December 1, 2020 11:17 PM
> > >> > > To: Gao, Zhichao <zhichao.gao at intel.com
> > >> > > <mailto:zhichao.gao at intel.com>>;
> > > devel at edk2.groups.io <mailto:devel at edk2.groups.io>;
> > >> > > lersek at redhat.com <mailto:lersek at redhat.com>
> > >> > > Cc: Wang, Jian J <jian.j.wang at intel.com
> > >> > > <mailto:jian.j.wang at intel.com>>; Wu, Hao A <hao.a.wu at intel.com
> > >> > > <mailto:hao.a.wu at intel.com>>; Ni, Ray
> > > <ray.ni at intel.com <mailto:ray.ni at intel.com>>; Ard Biesheuvel
> > >> > > <Ard.Biesheuvel at arm.com <mailto:Ard.Biesheuvel at arm.com>>;
> > Andy
> > >> > > Lutomirski
> > > <luto at kernel.org <mailto:luto at kernel.org>>; Samer
> > >> > > El-Haj-Mahmoud <Samer.El-Haj- Mahmoud at arm.com
> > >> > > <mailto:Mahmoud at 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 at intel.com
> > >> > > > <mailto:zhichao.gao at intel.com>>
> > >> > > > Sent: Monday, November 30, 2020 8:00 PM
> > >> > > > To: devel at edk2.groups.io <mailto:devel at edk2.groups.io>;
> > >> > > > lersek at redhat.com
> > > <mailto:lersek at redhat.com>; Samer El-Haj-Mahmoud
> > >> > > > <Samer.El-Haj-Mahmoud at arm.com
> > >> > > > <mailto:Samer.El-Haj-Mahmoud at arm.com>>
> > >> > > > Cc: Wang, Jian J <jian.j.wang at intel.com
> > >> > > > <mailto:jian.j.wang at intel.com>>; Wu, Hao A
> > >> > > > <hao.a.wu at intel.com <mailto:hao.a.wu at intel.com>>; Ni, Ray
> > > <ray.ni at intel.com <mailto:ray.ni at intel.com>>; Ard Biesheuvel
> > >> > > > <Ard.Biesheuvel at arm.com <mailto:Ard.Biesheuvel at arm.com>>;
> > Andy
> > >> > > > Lutomirski
> > > <luto at kernel.org <mailto:luto at 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 at edk2.groups.io
> > >> > > > > <mailto:devel at edk2.groups.io><devel at edk2.groups.io
> > > <mailto:devel at edk2.groups.io>> On Behalf Of
> > >> > > > > Laszlo Ersek
> > >> > > > > Sent: Wednesday, November 25, 2020 7:30 AM
> > >> > > > > To: devel at edk2.groups.io <mailto:devel at edk2.groups.io>;
> > > samer.el-haj-mahmoud at arm.com <mailto:samer.el-haj-
> > mahmoud at arm.com>
> > >> > > > > Cc: Wang, Jian J <jian.j.wang at intel.com
> > >> > > > > <mailto:jian.j.wang at intel.com>>; Wu, Hao A
> > >> > > > > <hao.a.wu at intel.com <mailto:hao.a.wu at intel.com>>; Gao,
> > >> > > > > Zhichao
> > > <zhichao.gao at intel.com <mailto:zhichao.gao at intel.com>>; Ni,
> > >> > > > > Ray <ray.ni at intel.com <mailto:ray.ni at intel.com>>; Ard
> > >> > > > > Biesheuvel
> > > <Ard.Biesheuvel at arm.com <mailto:Ard.Biesheuvel at arm.com>>;
> > >> > > > > Andy Lutomirski <luto at kernel.org <mailto:luto at 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 at intel.com
> > >> > > > > > <mailto:jian.j.wang at intel.com>>
> > >> > > > > > Cc: Hao A Wu <hao.a.wu at intel.com
> > >> > > > > > <mailto:hao.a.wu at intel.com>>
> > >> > > > > > Cc: Zhichao Gao <zhichao.gao at intel.com
> > >> > > > > > <mailto:zhichao.gao at intel.com>>
> > >> > > > > > Cc: Ray Ni <ray.ni at intel.com <mailto:ray.ni at intel.com>>
> > >> > > > > > Cc: Ard Biesheuvel <Ard.Biesheuvel at arm.com
> > >> > > > > > <mailto:Ard.Biesheuvel at arm.com>>
> > >> > > > > > Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-
> > >> > > > Mahmoud at arm.com <mailto:Mahmoud at 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
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68408): https://edk2.groups.io/g/devel/message/68408
Mute This Topic: https://groups.io/mt/78700256/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-