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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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/PlatformBootManagerLib.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.