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

["Laszlo Ersek" <lersek@redhat.com>]

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