Re: [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Wed, 15 Jan 2020 at 13:50, Laszlo Ersek <lersek@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.biesheuvel@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.