Re: [edk2-devel] [PATCH 00/18] Remove All UGA Support

["Guomin Jiang" <guomin.jiang@intel.com>]

Ard and Laszlo,

It's my problem.

I will improve the patch and make it  more professional.

I am busy recently and will do it after July.

Thanks

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Friday, May 8, 2020 7:00 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Jiang, Guomin
> <guomin.jiang@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/18] Remove All UGA Support
> 
> On 5/8/20 12:09 PM, Laszlo Ersek wrote:
> > Hello Guomin,
> >
> > On 05/08/20 10:38, Guomin Jiang wrote:
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2368
> >>
> >> UGA is replaced by GOP and remove all related code.
> >
> > I'm responding under the cover letter.
> >
> >
> > (1) You should have CC'd the cover letter to each person that is CC'd
> > on at least one patch in the series. Please do that in the future.
> >
> > I'm CC'ing Ard now, at least.
> >
> 
> Thanks Laszlo
> 
> >
> > (2) The series currently has identical subjects for some of the patches.
> > For example:
> >
> > [PATCH 08/18] OvmfPkg: Remove All UGA Support [PATCH 15/18] OvmfPkg:
> > Remove All UGA Support
> >
> > [PATCH 10/18] ArmVirtPkg: Remove All UGA Support [PATCH 17/18]
> > ArmVirtPkg: Remove All UGA Support
> >
> > This is extremely bad practice. Please don't do this.
> >
> >
> > (3) The commit messages do a bad job explaining why the PCDs are being
> > removed.
> >
> > If I understand correctly, the core modules are updated in this series
> > as follows:
> >
> > - PcdConOutUgaSupport is assumed constant FALSE
> > - PcdConOutGopSupport is assumed constant TRUE
> > - conditions are simplified and dead code is eliminated
> >
> >
> > The proper approach is therefore the following:
> >
> > - In patch#1, change the DEC default value of PcdConOutUgaSupport from
> > TRUE to FALSE.
> >
> > - In patches #2 .. #n, remove the PCD settins from all the DSC files.
> > Just one patch per package is sufficient. (No need for a separate
> > patch per PCD per Package.)
> >
> > The commit messages on these patches should explain that the PCDs are
> > going away, and by deleting the DSC settings, the platforms in
> > question inherit the DEC defaults. And, at this point the DEC defaults are:
> >
> > PcdConOutUgaSupport = FALSE
> > PcdConOutGopSupport = TRUE
> >
> > which is going to be the only configuration supported by edk2 in the future.
> >
> > For example, patch#2 could be for ArmVirtPkg, patch#3 could be for
> OvmfPkg.
> >
> > - In the rest of the patches, simplify code / eliminate dead code by
> > substituting FALSE for PcdConOutUgaSupport, and TRUE for
> > PcdConOutGopSupport. If you want, you can keep the code changes for
> > each PCD separate, in every module. (This kind of separation may be a
> > good idea.)
> >
> > - In the last patch in the series, remove both PCDs from the DEC file.
> >
> >
> > Before posting v2, please verify that the series (including the
> > platforms in edk2) build fine at every stage (at every patch in the series).
> >
> 
> Also, looking at edk2-platforms
> 
> $ git grep -l PcdUga
> Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> Platform/Comcast/RDKQemu/RDKQemu.dsc
> Platform/Intel/SimicsOpenBoardPkg/BoardX58Ich10/OpenBoardPkgPcd.dsc
> Platform/Intel/SimicsOpenBoardPkg/Library/DxeLogoLib/DxeLogoLib.inf
> Platform/Intel/SimicsOpenBoardPkg/Library/DxeLogoLib/Logo.c
> Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManage
> rLib.inf
> Platform/RaspberryPi/RPi3/RPi3.dsc
> Platform/RaspberryPi/RPi4/RPi4.dsc
> Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> Silicon/NXP/NxpQoriqLs.dsc.inc
> 
> Those platforms will all be broken as soon as you drop the PCD definitions.