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. (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). Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58871): https://edk2.groups.io/g/devel/message/58871 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] -=-=-=-=-=-=-=-=-=-=-=-
