On Wed, 15 Jan 2020 at 13:50, Laszlo Ersek <ler...@redhat.com> wrote:
>
> Hi Ard,
>
Hi Laszlo,
Thanks for taking a look.
> On 01/15/20 11:22, Ard Biesheuvel wrote:
> > Currently, ArmPkg's implementation of PlatformBootManagerBeforeConsole()
> > iterates over all PCI I/O handles in the database and connects the ones
> > that may produce an instance of the Graphics Output Protocol (GOP) once
> > connected. After that, it enumerates all the GOP instances and adds them
> > to the stdout/stderr console variables so that they will be used for
> > console output.
> >
> > At this point in the boot, the BDS has not dispatched drivers loaded via
> > Driver#### options yet, making Driver#### options unsuitable for loading
> > drivers that are needed to produce GOP consoles. This is unfortunate, since
> > it prevents us from using commodity GFX hardware that ships without AARCH64
> > option ROMs on AARCH64 hardware and load the driver from the ESP.
> >
> > So let's fix this, by deferring the discovery of PCI backed GOP instances
> > until PlatformBootManagerAfterConsole(). Note that non-PCI GOP instances
> > are still connected in the original spot, so platform framebuffers will
> > still work as before. Also note that the entire dance of connecting PCI
> > root bridges and I/O handles, matching PCI class codes and updating console
> > variables is all encapsulated in EfiBootManagerConnectVideoController(),
> > so let's just call that from PlatformBootManagerAfterConsole().
>
> I too have learned about the EfiBootManagerConnectVideoController() ACPI
> just recently, when Ray mentioned it.
>
> (1) When we call EfiBootManagerConnectVideoController() with a NULL
> parameter, it further calls BmGetVideoController(), which does the
> "dance" you mention. This wasn't obvious to me from the commit message,
> so I'd suggest mentioning BmGetVideoController() there, by name.
>
OK
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > ---
> >
> > Tested on AMD Overdrive with an AMD Radeon GPU plugged in and the
> > AARCH64 AmdGop.efi driver loaded via Driver0000.
> >
> > ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 81
> > ++------------------
> > 1 file changed, 7 insertions(+), 74 deletions(-)
> >
> > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > index e6e788e0f107..7c63a7b98847 100644
> > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > @@ -219,66 +219,6 @@ FilterAndProcess (
> > }
> >
> >
> > -/**
> > - This FILTER_FUNCTION checks if a handle corresponds to a PCI display
> > device.
> > -**/
> > -STATIC
> > -BOOLEAN
> > -EFIAPI
> > -IsPciDisplay (
> > - IN EFI_HANDLE Handle,
> > - IN CONST CHAR16 *ReportText
> > - )
> > -{
> > - EFI_STATUS Status;
> > - EFI_PCI_IO_PROTOCOL *PciIo;
> > - PCI_TYPE00 Pci;
> > -
> > - Status = gBS->HandleProtocol (Handle, &gEfiPciIoProtocolGuid,
> > - (VOID**)&PciIo);
> > - if (EFI_ERROR (Status)) {
> > - //
> > - // This is not an error worth reporting.
> > - //
> > - return FALSE;
> > - }
> > -
> > - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0 /* Offset */,
> > - sizeof Pci / sizeof (UINT32), &Pci);
> > - if (EFI_ERROR (Status)) {
> > - DEBUG ((EFI_D_ERROR, "%a: %s: %r\n", __FUNCTION__, ReportText,
> > Status));
> > - return FALSE;
> > - }
> > -
> > - return IS_PCI_DISPLAY (&Pci);
> > -}
> > -
> > -
> > -/**
> > - This CALLBACK_FUNCTION attempts to connect a handle non-recursively,
> > asking
> > - the matching driver to produce all first-level child handles.
> > -**/
> > -STATIC
> > -VOID
> > -EFIAPI
> > -Connect (
> > - IN EFI_HANDLE Handle,
> > - IN CONST CHAR16 *ReportText
> > - )
> > -{
> > - EFI_STATUS Status;
> > -
> > - Status = gBS->ConnectController (
> > - Handle, // ControllerHandle
> > - NULL, // DriverImageHandle
> > - NULL, // RemainingDevicePath -- produce all children
> > - FALSE // Recursive
> > - );
> > - DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE, "%a: %s: %r\n",
> > - __FUNCTION__, ReportText, Status));
> > -}
> > -
> > -
> > /**
> > This CALLBACK_FUNCTION retrieves the EFI_DEVICE_PATH_PROTOCOL from the
> > handle, and adds it to ConOut and ErrOut.
> > @@ -554,20 +494,6 @@ PlatformBootManagerBeforeConsole (
> > //
> > EfiBootManagerDispatchDeferredImages ();
> >
> > - //
> > - // Locate the PCI root bridges and make the PCI bus driver connect each,
> > - // non-recursively. This will produce a number of child handles with
> > PciIo on
> > - // them.
> > - //
> > - FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect);
> > -
> > - //
> > - // Find all display class PCI devices (using the handles from the
> > previous
> > - // step), and connect them non-recursively. This should produce a number
> > of
> > - // child handles with GOPs on them.
> > - //
> > - FilterAndProcess (&gEfiPciIoProtocolGuid, IsPciDisplay, Connect);
> > -
> > //
> > // Now add the device path of all handles with GOP on them to ConOut and
> > // ErrOut.
>
> (2) Second, BmGetVideoController() differs from the logic that's being
> removed here. I see two differences:
>
> (2a) the IsPciDisplay() helper function uses IS_PCI_DISPLAY(), while
> BmGetVideoController() uses IS_PCI_VGA().
>
> Interestingly, there is a comment:
>
> // TODO: use IS_PCI_DISPLAY??
>
> If I understand correctly, IS_PCI_VGA() matches a subset of
> IS_PCI_DISPLAY(), namely the such devices that offer legacy BIOS VGA
> interfaces (IO ports and whatnot). In support of my understanding,
> please see:
>
> - 4fdb585c69d6 ("OvmfPkg/PlatformBootManagerLib: relax device class
> requirement for ConOut", 2016-09-01)
>
> - commit 70dbd16361ee ("OvmfPkg/QemuVideoDxe: Add SubClass field to
> QEMU_VIDEO_CARD", 2018-05-17)
>
> Thus, IS_PCI_VGA() appears overly restrictive *especially* on AARCH64,
> where you are more likely to encounter "VGA legacy"-free PCI graphics
> cards than on x86.
>
> Now, this doesn't necessarily mean "ArmPkg/PlatformBmLib" needs to use
> its own IS_PCI_DISPLAY logic -- we could also turn the comment in
> BmGetVideoController() into actual code, as an alternative.
>
I think that would be a worthwhile separate change, although for bare
metal platforms today, it doesn't really make a difference, given
that, AFAICT, non-VGA PCI display controllers are rare, if they exist
at all.
> (2b) The other difference is that BmGetVideoController() only grabs the
> first video controller it finds, and runs with it. Whereas, logic being
> removed here would connect (and add, to ConOut/StdErr) *all* PCI video
> controllers. I think that's nicer in a multi-controller setup.
>
True.
> > @@ -674,6 +600,13 @@ PlatformBootManagerAfterConsole (
> > UINTN PosX;
> > UINTN PosY;
> >
> > + //
> > + // Defer this call to PlatformBootManagerAfterConsole () so that devices
> > + // managed by drivers that were loaded via Driver#### options are covered
> > + // as well.
> > + //
> > + EfiBootManagerConnectVideoController (NULL);
> > +
> > FirmwareVerLength = StrLen (PcdGetPtr (PcdFirmwareVersionString));
> >
> > //
> >
>
> (3) So how about the following approach:
>
> (3a) factor the following sequence:
>
> FilterAndProcess (&gEfiPciIoProtocolGuid, IsPciDisplay, Connect);
> FilterAndProcess (&gEfiGraphicsOutputProtocolGuid, NULL, AddOutput);
>
> out to a helper function,
>
> (3b) call the extracted sequence in *both* its current place, *and* at
> the top of PlatformBootManagerAfterConsole() (instead of
> EfiBootManagerConnectVideoController())?
>
> ?
>
> I expect this should give you *some* consoles in BeforeConsole() (on
> such PCI and non-PCI graphics controllers whose drivers are in the
> platform firmware), just to be safe; and *all* the rest would be picked
> up in AfterConsole().
>
I wonder whether there is a point to doing it twice, regardless of
whether we are talking about PCI or non-PCI. I experimented with just
moving those calls to AfterConsole(), and I get basically the same
behavior as with this patch.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53276): https://edk2.groups.io/g/devel/message/53276
Mute This Topic: https://groups.io/mt/69714171/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
