From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web10.6562.1579093385759349085 for ; Wed, 15 Jan 2020 05:03:06 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=gKiVhvRc; spf=pass (domain: linaro.org, ip: 209.85.221.67, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f67.google.com with SMTP id j42so15618871wrj.12 for ; Wed, 15 Jan 2020 05:03:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=G9Ph/qiROk0UIccSD2mtHqeTgJ0FvbwzUnYXVsiofEc=; b=gKiVhvRcWPJdLrZSf/OcYlL/XBpf0tBz+B6NDyQ2ok4e+M3GUBxr7mPTNGifSdY7H+ jKly3VxRCnNQOiheOh+Z3esQS7Q0hTsrsmI5ucE+2hPnxRKE0TdD0s6knximWIbSrlL8 0ucClEdhLca57GW/c7f1pI9pbFlTx5zoLoAxUAPkxokv1+TBP28XJGRTKwCtTArpXzs8 Vb8sRvB/b71OMI4Tb0A/xhHnvSBQXTEbLOBEPZRawja9tmZMk8A3G3JWv2LcjmW2W9+X NerkoWVyiIxBSdMJPCCKfCw2MH/8ExyvZPXzfdjGbUDBiYOaXiSQANtl+b93TjNrqX+T di4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=G9Ph/qiROk0UIccSD2mtHqeTgJ0FvbwzUnYXVsiofEc=; b=uLckiZLYjQfwLYVzJGOC5wbcp8Ep76oc7WXf+CXQ3RoFLQnMYnnXTFpzrrhRhLrtzH yuptBGc6QN6UkJf6PZsnZxCFxyIWr3JaBYQ8px9+69skWSWRx8bappLkFx4uAzqNeMkY ZmT10dNzcNCpst43Ix1Zrq3XQbQfl0hNY+1M0WfqVEboyoNlsP9/NcD8fEEEokmKxWr/ 9/IV8pmzezsaXQ0Hs/YVj2VMwGRpl92bvVWIfwS2VlmwmtYJ64wPQi8X+OzAig7l9nBN a8LTWAuaSx5gPSlAiaJsOKZMvGXMJfMBGK/DB2XuhyepTbVNBN+VRDIbPUKuldhbfjqn WUIQ== X-Gm-Message-State: APjAAAWNr5spHbyBpMyTsq0JovfQIejKfTH3jiv6FziiDbGkV8zocHoq QdVypGypFnDha9MUvxtDLOqLWEQsGubRR/VHjs1Z6A== X-Google-Smtp-Source: APXvYqxBaJljqRk4aq0i1QtXBtft8T2wFtWMLV8dl0Vbm74owv7U3L3ugAg8nj+sZoJywqgQ3HnMwYArD8TalIDaXLw= X-Received: by 2002:a5d:43c7:: with SMTP id v7mr29460367wrr.32.1579093384105; Wed, 15 Jan 2020 05:03:04 -0800 (PST) MIME-Version: 1.0 References: <20200115102217.3027-1-ard.biesheuvel@linaro.org> <23a9be06-791f-517a-ccd4-fd36c93af8b0@redhat.com> In-Reply-To: <23a9be06-791f-517a-ccd4-fd36c93af8b0@redhat.com> From: "Ard Biesheuvel" Date: Wed, 15 Jan 2020 14:02:52 +0100 Message-ID: Subject: Re: [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch To: Laszlo Ersek Cc: edk2-devel-groups-io , Leif Lindholm , "Ni, Ray" , Andrew Fish Content-Type: text/plain; charset="UTF-8" On Wed, 15 Jan 2020 at 13:50, Laszlo Ersek 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 > > --- > > > > 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.