Re: [edk2] [PATCH v2 00/19] OvmfPkg: improve video mode selection

Subject: Re: [edk2] [PATCH v2 00/19] OvmfPkg: improve video mode selection

From: Jordan Justen <jljusten@gmail.com>

To: Laszlo Ersek <lersek@redhat.com>

Date: 2014-02-26 22:25:20

On Wed, Feb 26, 2014 at 1:59 PM, Laszlo Ersek  wrote:
> On 02/26/14 20:16, Jordan Justen wrote:
>> Commonly a platform will have a PlatformDxe driver. It has kind of
>> surprised me how long OVMF has lived without one. One of the main
>> things that lived in the PlatformDxe driver is the setup form data.
>>
>> So, rather than PlatformConfigDxe, could you add PlatformDxe?
>
> As long as this idea covers only the renaming / relocating of the
> driver, sure. But, in that case, would you mind if I squashed the
> relevant patches? Their cumulative diffstat is now
>
>  OvmfPkg/OvmfPkgIa32.dsc                       |   2 +
>  OvmfPkg/OvmfPkgIa32.fdf                       |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   2 +
>  OvmfPkg/OvmfPkgIa32X64.fdf                    |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   2 +
>  OvmfPkg/OvmfPkgX64.fdf                        |   1 +
>  OvmfPkg/PlatformConfigDxe/PlatformConfig.c    | 730 ++++++++++++++++++
>  OvmfPkg/PlatformConfigDxe/PlatformConfig.h    |  42 +
>  OvmfPkg/PlatformConfigDxe/PlatformConfig.inf  |  59 ++
>  OvmfPkg/PlatformConfigDxe/PlatformConfig.uni  | Bin 0 -> 3322 bytes
>  .../PlatformConfigDxe/PlatformConfigForms.vfr |  74 ++
>  11 files changed, 914 insertions(+)
>
> Basically it stays completely in PlatformConfigDxe.
>
> I'm asking because threading a rename through such a rebase is a big
> pain. Too bad I'd lose the nice (I hope!) structure of these patches,
> ie. the progress and the thought process. Perhaps if I re-enable rename
> detection temporarily in my repo, it could work.

I think that would work. (Reenabling rename detection.)

I don't have that rename detection disabled, and a quick git rebase
test seemed to show it should work.

>> PlatformDxe can also look at the setup variable and set the resolution
>> PCDs. (Rather than PlatformBds.)
>>
>> At that point, I think we could (at least for now) fold
>> PlatformConfigLib into PlatformDxe.
>
> I think this would not work. Consider the following relative dependencies:
>
> (1) the resolution preference must be read from the NvVar, and the PCDs
> must be set for GraphicsConsoleDxe, before the consoles are connected.
> Patch v2 04/19 does this at the last minute, just before
> PlatformBdsConnectConsole().
>
>   PCDs --> PlatformBdsConnectConsole()
>
> (2) The platfom config driver installs its HII stuff in its entry point
> (it must, it is a device-independent driver). The contents of the
> drop-down list is part of the HII stuff. In order to populate the
> drop-down list with the selectable resolutions, it needs the GOP, so
> there's a Depex on the GOP.

PlatformDxe should not have a depex on GOP, but it could have a GOP callback.

For a more complicated version, I think the setup data could be
published very early, and the form could be updated once GOP is
available.

I think the form could even have a callback into PlatformDxe when the
form is displayed allowing resolution population at that time.

We should also make sure that -vga none does not cause major breakage.

-Jordan

> The GOP interface is installed in the video
> driver's binding start function.
>
>   video binding start --> platform config entry point
>
> (3) The platform driver can't do anything, including set any PCDs (your
> proposal), before its entry point is invoked.
>
>   platform config entry point --> PCDs
>
> (4) The video driver can be loaded whenever, but its binding start
> function will be called no sooner than BDS tries to connect the VGA card.
>
>   PlatformBdsConnectConsole() --> video binding start
>
> If you add these up, they build a circular total dependency:
>
>     PCDs
> --> PlatformBdsConnectConsole()
> --> video binding start
> --> platform config entry point
> --> PCDs
>
> In practice the cycle would be entered at PlatformBdsConnectConsole(),
> and the PCDs would be loaded/set from the NvVar preference too late, at
> each boot.
>
> Thanks,
> Laszlo

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel