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

["Samer El-Haj-Mahmoud" <samer.el-haj-mahmoud@arm.com>]

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
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > >
> >> > >