On Nov 6, 2013, at 1:47 PM, Jordan Justen <[email protected]> wrote:
> On Wed, Nov 6, 2013 at 10:02 AM, Chris Ruffin <[email protected]> wrote:
>> Hi Laszlo, thanks for your feedback. After careful consideration I
>> must agree with you - normally a bus driver would create the child
>> handle and open the parent's protocols by the child controller with
>> the BY_CHILD_CONTROLLER attribute. Then a device driver would open
>> the protocol on the child controller with the BY_DRIVER attribute. In
>> this case we're acting as a hybrid driver which should do both. I
>> think :)
>
> I agree with your assessment here. I think this is right thing to do
> for the simple case in QemuVideoDxe.
>
> But, I wish this aspect of GOP was better spec'd by UEFI. Or ...
> possibly via some EDK II sample code. (Either GOP side sample code, or
> BDS code that made it clear what is expected of GOP drivers.)
>
> I guess the reason this has gotten complicated is that many GOP
> drivers can support multiple outputs for a single card. Therefore the
> GOP driver wants to both open the PCI device, and produce children
> devices which can then be started and stopped individually.
>
> So, what happens when the children are individually started & stopped?
This is no different than any bus driver in EFI. If all your children stop, the
bus driver can stop.
> Should the driver open the PCI device only when 1 or more children
> devices are started?
Generally the GOP driver will use the remaining device path as a hint of which
output to light up. If no remaining device path is passed in the GOP driver
picks a default output.
> Also if the PCI device is stopped, then I guess
> the children devices should be removed.
>
Stopped by their bus driver (GOP driver) yes.
> Luckily, for QemuVideoDxe there is only 1 child device, so hopefully
> we don't have to get too concerned about the more complicated
> questions...
>
> -Jordan
>
>> On Mon, Nov 4, 2013 at 2:05 PM, Laszlo Ersek <[email protected]> wrote:
>>> comments related to the UEFI driver model at the bottom
>>>
>>> On 11/04/13 07:21, Chris Ruffin wrote:
>>>>> From c799ca97b4dfb8e8c39ee93e72be92a8d1b693ff Mon Sep 17 00:00:00 2001
>>>> From: Chris Ruffin <[email protected]>
>>>> Date: Sun, 3 Nov 2013 21:40:34 -0500
>>>> Subject: [PATCH] OvmfPkg/QemuVideoDxe: use child handle to open parent
>>>> protocols
>>>>
>>>> Reorder when the child handle is created so that a parent / child
>>>> relationship is established when PciIo protocol is opened.
>>>> This resolves some issues with code which depends on the parent/child
>>>> relationship to locate the GOP protocol.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Chris Ruffin <[email protected])
>>>> ---
>>>> OvmfPkg/QemuVideoDxe/Driver.c | 112
>>>> +++++++++++++++++++++---------------------
>>>> 1 file changed, 56 insertions(+), 56 deletions(-)
>>>>
>>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
>>>> index 1dd8899..d8b9498 100644
>>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>>>> @@ -2,7 +2,7 @@
>>>> This driver is a sample implementation of the Graphics Output Protocol
>>>> for
>>>> the QEMU (Cirrus Logic 5446) video controller.
>>>>
>>>> - Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
>>>> + Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>
>>>>
>>>> This program and the accompanying materials
>>>> are licensed and made available under the terms and conditions of
>>>> the BSD License
>>>> @@ -227,6 +227,59 @@ QemuVideoControllerDriverStart (
>>>> Private->Handle = NULL;
>>>>
>>>> //
>>>> + // Get ParentDevicePath
>>>> + //
>>>> + Status = gBS->HandleProtocol (
>>>> + Controller,
>>>> + &gEfiDevicePathProtocolGuid,
>>>> + (VOID **) &ParentDevicePath
>>>> + );
>>>> + if (EFI_ERROR (Status)) {
>>>> + goto Error;
>>>> + }
>>>> +
>>>> + //
>>>> + // Set Gop Device Path
>>>> + //
>>>> + if (RemainingDevicePath == NULL) {
>>>> + ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
>>>> + AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
>>>> + AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
>>>> + AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
>>>> ACPI_ADR_DISPLAY_TYPE_VGA, 0, 0);
>>>> + SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof
>>>> (ACPI_ADR_DEVICE_PATH));
>>>> +
>>>> + Private->GopDevicePath = AppendDevicePathNode (
>>>> + ParentDevicePath,
>>>> + (EFI_DEVICE_PATH_PROTOCOL *)
>>>> &AcpiDeviceNode
>>>> + );
>>>> + } else if (!IsDevicePathEnd (RemainingDevicePath)) {
>>>> + //
>>>> + // If RemainingDevicePath isn't the End of Device Path Node,
>>>> + // only scan the specified device by RemainingDevicePath
>>>> + //
>>>> + Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath,
>>>> RemainingDevicePath);
>>>> + } else {
>>>> + //
>>>> + // If RemainingDevicePath is the End of Device Path Node,
>>>> + // don't create child device and return EFI_SUCCESS
>>>> + //
>>>> + Private->GopDevicePath = NULL;
>>>> + }
>>>> +
>>>> + if (Private->GopDevicePath != NULL) {
>>>> + //
>>>> + // Creat child handle and device path protocol firstly
>>>> + //
>>>> + Private->Handle = NULL;
>>>> + Status = gBS->InstallMultipleProtocolInterfaces (
>>>> + &Private->Handle,
>>>> + &gEfiDevicePathProtocolGuid,
>>>> + Private->GopDevicePath,
>>>> + NULL
>>>> + );
>>>> + }
>>>> +
>>>> + //
>>>> // Open PCI I/O Protocol
>>>> //
>>>> Status = gBS->OpenProtocol (
>>>> @@ -234,8 +287,8 @@ QemuVideoControllerDriverStart (
>>>> &gEfiPciIoProtocolGuid,
>>>> (VOID **) &Private->PciIo,
>>>> This->DriverBindingHandle,
>>>> - Controller,
>>>> - EFI_OPEN_PROTOCOL_BY_DRIVER
>>>> + Private->Handle,
>>>> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
>>>> );
>>>> if (EFI_ERROR (Status)) {
>>>> goto Error;
>>>> @@ -322,59 +375,6 @@ QemuVideoControllerDriverStart (
>>>> }
>>>>
>>>> //
>>>> - // Get ParentDevicePath
>>>> - //
>>>> - Status = gBS->HandleProtocol (
>>>> - Controller,
>>>> - &gEfiDevicePathProtocolGuid,
>>>> - (VOID **) &ParentDevicePath
>>>> - );
>>>> - if (EFI_ERROR (Status)) {
>>>> - goto Error;
>>>> - }
>>>> -
>>>> - //
>>>> - // Set Gop Device Path
>>>> - //
>>>> - if (RemainingDevicePath == NULL) {
>>>> - ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
>>>> - AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
>>>> - AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
>>>> - AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
>>>> ACPI_ADR_DISPLAY_TYPE_VGA, 0, 0);
>>>> - SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof
>>>> (ACPI_ADR_DEVICE_PATH));
>>>> -
>>>> - Private->GopDevicePath = AppendDevicePathNode (
>>>> - ParentDevicePath,
>>>> - (EFI_DEVICE_PATH_PROTOCOL *)
>>>> &AcpiDeviceNode
>>>> - );
>>>> - } else if (!IsDevicePathEnd (RemainingDevicePath)) {
>>>> - //
>>>> - // If RemainingDevicePath isn't the End of Device Path Node,
>>>> - // only scan the specified device by RemainingDevicePath
>>>> - //
>>>> - Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath,
>>>> RemainingDevicePath);
>>>> - } else {
>>>> - //
>>>> - // If RemainingDevicePath is the End of Device Path Node,
>>>> - // don't create child device and return EFI_SUCCESS
>>>> - //
>>>> - Private->GopDevicePath = NULL;
>>>> - }
>>>> -
>>>> - if (Private->GopDevicePath != NULL) {
>>>> - //
>>>> - // Creat child handle and device path protocol firstly
>>>> - //
>>>> - Private->Handle = NULL;
>>>> - Status = gBS->InstallMultipleProtocolInterfaces (
>>>> - &Private->Handle,
>>>> - &gEfiDevicePathProtocolGuid,
>>>> - Private->GopDevicePath,
>>>> - NULL
>>>> - );
>>>> - }
>>>> -
>>>> - //
>>>> // Construct video mode buffer
>>>> //
>>>> switch (Private->Variant) {
>>>
>>> OK, so the current OVMF GOP driver seems to follow the general GOP
>>> driver requirements set forth in:
>>>
>>> - 23.2 Graphics Output Protocol Implementation (DWG)
>>>
>>> - 11.10 Rules for PCI/AGP Devices (spec) -- these are BTW
>>> mind-bogglingly complex and I only skimmed them
>>>
>>> In QemuVideoControllerDriverStart() we
>>> (1) open (ie. persistently reference) the parent controller (which
>>> is the graphics card, ie. a PCI device/function pair), with
>>> - PciIo,
>>> - Attributes == EFI_OPEN_PROTOCOL_BY_DRIVER,
>>> - ControllerHandle == the parent controller (the graphics card),
>>>
>>> (2) create a child handle,
>>> (3) install a new Device Path Protocol instance on it,
>>> (4) install a Graphics Output Protocol instance on it.
>>>
>>> As far as I understand, the patch effects the following (for me the
>>> patch doesn't apply from the email so I can't really look at the
>>> resultant code):
>>>
>>> (1') create a child handle,
>>> (2') install a new Device Path Protocol instance on it,
>>>
>>> (3') open (ie. persistently reference) the parent controller (the
>>> graphics card) with:
>>> - PciIo,
>>> - Attributes == EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER,
>>> - ControllerHandle == the child controller from step (1')
>>>
>>> (4') install a Graphics Output Protocol instance on the child handle.
>>>
>>> I think the change goes in the right direction, but I think it's
>>> incomplete. I'm basing this on:
>>>
>>> (a) the EFI_DRIVER_BINDING_PROTOCOL.Start() specification in 10.1, under
>>>
>>> Pseudo Code
>>>
>>> Bus Driver that is able to create all or one of its child
>>> handles on each call to Start()
>>>
>>> step 1
>>> step 4, RemainingDevicePath != NULL, paragraph f
>>> step 4, RemainingDevicePath == NULL, end of paragraph c
>>>
>>> (b) earlier emails from Michael and Andrew (which I can't of course
>>> locate now!)
>>>
>>> In essence, step (3') should not *replace* original step (1), step (3')
>>> should happen *in addition*. The patch as-is will remove the BY_DRIVER
>>> reference to the parent controller's PciIo, and I think that's not good.
>>>
>>> I think the patch should keep the original sequence (1) to (4), and add
>>> the following step:
>>>
>>> (5) same as (3')
>>>
>>>
>>> I believe this is what I did in the virtio-net driver
>>> (VirtioNetDriverBindingStart() in
>>> "OvmfPkg/VirtioNetDxe/DriverBinding.c"), based on the emails I seem to
>>> recall from Michael and Andrew. That use case seems quite symmetric,
>>> with the MAC child node doubling for the GOP child node.
>>>
>>> Anyway I'm not certain, it's just a thought.
>>>
>>>
>>> Regarding your other patch ("[edk2] [PATCH] OvmfPkg/QemuVideoDxe: close
>>> the appropriate protocol"), I agree that the current CloseProtocol() on
>>> the error path in QemuVideoControllerDriverStart() is incorrect.
>>>
>>> There should be two CloseProtocol() calls in total, mirroring the
>>> BY_DRIVER reference in (1), and the (proposed) BY_CHILD_CONTROLLER
>>> reference in (5).
>>>
>>> Thanks,
>>> Laszlo
>>
>> ------------------------------------------------------------------------------
>> November Webinars for C, C++, Fortran Developers
>> Accelerate application performance with scalable programming models. Explore
>> techniques for threading, error checking, porting, and tuning. Get the most
>> from the latest Intel processors and coprocessors. See abstracts and register
>> http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
>> _______________________________________________
>> edk2-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ------------------------------------------------------------------------------
> November Webinars for C, C++, Fortran Developers
> Accelerate application performance with scalable programming models. Explore
> techniques for threading, error checking, porting, and tuning. Get the most
> from the latest Intel processors and coprocessors. See abstracts and register
> http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
November Webinars for C, C++, Fortran Developers
Accelerate application performance with scalable programming models. Explore
techniques for threading, error checking, porting, and tuning. Get the most
from the latest Intel processors and coprocessors. See abstracts and register
http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
