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

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

From: Laszlo Ersek <lersek@redhat.com>

To: Jordan Justen <jljusten@gmail.com>

Date: 2014-02-27 22:17:07

On 02/26/14 23:25, Jordan Justen wrote:
> 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.

OK, I'll look into it.

>> (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.

Do you have gBS->RegisterProtocolNotify() in mind? Ie. the platform
driver would load and start early, but only register a callback for the
time when the GOP is installed?

I did consider that cursorily. I haven't done such before, and I'm
worried about how the callback would be "scheduled".

For example, *if* the callback is entered as soon as the video driver
installs the GOP interface, there could be problems. (Technically this
would mean that the callback is called on the stack of
InstallProtocolInterface(), ie. the callback would run before
InstallProtocolInterface() returns).

Namely, the video driver's internals may not be fully consistent before
it returns from its binding start function. However, in the callback we
want to retrieve the mode list, so we call back into the GOP driver (ie.
we reenter it).

This could crash the video driver in general.

Or, it could force the video driver to be reentrant, or to use locking
(with TPLs). As in, the binding start function would acquire the lock,
install the GOP interface, get all the internals in a row, then release
the lock. The callback would then run inside the "release the lock"
function.

Is this your suggestion? Because then I'll have to test the above
function calling orders in QemuVideoDxe, with DEBUG()s.

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

If -vga none works right now, I agree that the feature shouldn't break
it. If -vga none doesn't work right now, then I don't think we should
fix it in this feature.

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