* [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch @ 2020-01-15 10:22 Ard Biesheuvel 2020-01-15 12:50 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2020-01-15 10:22 UTC (permalink / raw) To: devel; +Cc: lersek, leif.lindholm, ray.ni, afish, Ard Biesheuvel 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(). 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. @@ -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)); // -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch 2020-01-15 10:22 [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch Ard Biesheuvel @ 2020-01-15 12:50 ` Laszlo Ersek 2020-01-15 13:02 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2020-01-15 12:50 UTC (permalink / raw) To: Ard Biesheuvel, devel; +Cc: leif.lindholm, ray.ni, afish Hi Ard, 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. > > 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. (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. > @@ -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(). Thanks! Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch 2020-01-15 12:50 ` Laszlo Ersek @ 2020-01-15 13:02 ` Ard Biesheuvel 2020-01-15 14:52 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2020-01-15 13:02 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-groups-io, Leif Lindholm, Ni, Ray, Andrew Fish 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch 2020-01-15 13:02 ` Ard Biesheuvel @ 2020-01-15 14:52 ` Laszlo Ersek 2020-01-15 14:58 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2020-01-15 14:52 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel-groups-io, Leif Lindholm, Ni, Ray, Andrew Fish On 01/15/20 14:02, Ard Biesheuvel wrote: > On Wed, 15 Jan 2020 at 13:50, Laszlo Ersek <lersek@redhat.com> wrote: >> (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. Looking at BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c], admittedly quite little happens between the calls to PlatformBootManagerBeforeConsole() and PlatformBootManagerAfterConsole(). What if a UEFI driver, loaded from a Driver#### option, prints a message to the UEFI console, in its entry point function? Do we want such messages to appear on platform framebuffers? ... Hmmm, I felt that the Driver Writer's Guide spoke some words on this, and indeed. In 3.15.3 "Connecting consoles", the Note at the end includes the following sentence: UEFI Drivers should never directly access console devices except for the few UEFI driver related services that explicitly allow user interaction. In most cases, UEFI drivers use HII infrastructure to present information to users. I don't know what those "few UEFI driver related services" are supposed to be "that explicitly allow user interaction". So with the (somewhat incomplete) information I seem to have, I must agree that simply moving those two function calls should be safe enough. Thanks Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch 2020-01-15 14:52 ` Laszlo Ersek @ 2020-01-15 14:58 ` Ard Biesheuvel 0 siblings, 0 replies; 5+ messages in thread From: Ard Biesheuvel @ 2020-01-15 14:58 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-groups-io, Leif Lindholm, Ni, Ray, Andrew Fish , On Wed, 15 Jan 2020 at 15:52, Laszlo Ersek <lersek@redhat.com> wrote: > > On 01/15/20 14:02, Ard Biesheuvel wrote: > > On Wed, 15 Jan 2020 at 13:50, Laszlo Ersek <lersek@redhat.com> wrote: > > >> (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. > > Looking at BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c], > admittedly quite little happens between the calls to > PlatformBootManagerBeforeConsole() and PlatformBootManagerAfterConsole(). > > What if a UEFI driver, loaded from a Driver#### option, prints a message > to the UEFI console, in its entry point function? Do we want such > messages to appear on platform framebuffers? > > ... Hmmm, I felt that the Driver Writer's Guide spoke some words on > this, and indeed. In 3.15.3 "Connecting consoles", the Note at the end > includes the following sentence: > > UEFI Drivers should never directly access console devices except for > the few UEFI driver related services that explicitly allow user > interaction. In most cases, UEFI drivers use HII infrastructure to > present information to users. > > I don't know what those "few UEFI driver related services" are supposed > to be "that explicitly allow user interaction". So with the (somewhat > incomplete) information I seem to have, I must agree that simply moving > those two function calls should be safe enough. > I suppose the main occurrence between BeforeConsole() and AfterConsole() is the invocation of EfiBootManagerConnectAllDefaultConsoles(), which updates the system table with the new values of ConIn, ConOut etc. It looks like that /should/ matter, but in my testing, connecting the GOP driver after that still yields a working graphical console, so I'm not sure I understand what's going on here ... ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-15 14:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-01-15 10:22 [RFC PATCH] ArmPkg/PlatformBmLib: defer GOP console discovery after Driver#### dispatch Ard Biesheuvel 2020-01-15 12:50 ` Laszlo Ersek 2020-01-15 13:02 ` Ard Biesheuvel 2020-01-15 14:52 ` Laszlo Ersek 2020-01-15 14:58 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox