From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.8314.1588935625506338452 for ; Fri, 08 May 2020 04:00:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 43FB430E; Fri, 8 May 2020 04:00:24 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 687C63F71F; Fri, 8 May 2020 04:00:23 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 00/18] Remove All UGA Support To: Laszlo Ersek , devel@edk2.groups.io, guomin.jiang@intel.com References: <20200508083824.1785-1-guomin.jiang@intel.com> From: "Ard Biesheuvel" Message-ID: <0cfa0d84-8637-73ec-5d36-50182169cd67@arm.com> Date: Fri, 8 May 2020 13:00:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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.