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.biesheu...@arm.com> > Sent: Friday, May 8, 2020 7:00 PM > To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Jiang, Guomin > <guomin.ji...@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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58881): https://edk2.groups.io/g/devel/message/58881 Mute This Topic: https://groups.io/mt/74068776/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
