[+cc linux-pci] On Mon, Jul 5, 2021 at 5:04 AM Huacai Chen <chenhuacai@xxxxxxxxxxx> wrote: > > Currently, vga_arb_device_init() selects the first probed VGA device > with VGA legacy resources enabled as the default device. However, some > BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA > legacy resources because their built-in upstream bridges don't support > PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org > auto-detection. > > Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even > if there's no legacy VGA") try to solve this problem but fails on some > platforms, because it relies on the initcall order: > > We should call vga_arb_device_init() after PCI enumeration, otherwise it > may fail to select the default VGA device. Since vga_arb_device_init() > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are > both wrapped by subsys_initcall(), their sequence is not assured. So, it > is possible to use subsys_initcall_sync() instead of subsys_initcall() > to wrap vga_arb_device_init(). > > However, the above approach still has drawbacks, it cannot handle the > cases that a VGA card is hot-plugged, or the gpu driver is compiled as a > module. > > So, as suggested by Bjorn Helgaas, this patch rework the selection: > 1, Remove direct vga_arb_select_default_device() calls in vga_arb_ > device_init(). > 2, Rename vga_arb_select_default_device() to vga_arb_update_default_ > device(), which selects the first probed VGA device as the default > (whether legacy resources enabled or not), and update the default > device if a better one is found (device with legacy resources enabled > is better, device owns the firmware framebuffer is even better). > 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device() > is called, and vga_arb_update_default_device() is also called. So the > hotplug case and the module case can also be handled. > > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx> > --- > drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++---------------------- > 1 file changed, 97 insertions(+), 122 deletions(-) > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > index 949fde433ea2..07770aae3aaf 100644 > --- a/drivers/gpu/vga/vgaarb.c > +++ b/drivers/gpu/vga/vgaarb.c > @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) > } > EXPORT_SYMBOL(vga_put); > > +#if defined(CONFIG_ACPI) > +static bool vga_arb_integrated_gpu(struct device *dev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(dev); > + > + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID); > +} > +#else > +static bool vga_arb_integrated_gpu(struct device *dev) > +{ > + return false; > +} > +#endif > + > +static void vga_arb_update_default_device(struct vga_device *vgadev) > +{ > + struct pci_dev *pdev = vgadev->pdev; > + struct device *dev = &pdev->dev; > + struct vga_device *vgadev_default; > +#if defined(CONFIG_X86) || defined(CONFIG_IA64) > + int i; > + unsigned long flags; > + u64 base = screen_info.lfb_base; > + u64 size = screen_info.lfb_size; > + u64 limit; > + resource_size_t start, end; > +#endif > + > + /* Deal with VGA default device. Use first enabled one > + * by default if arch doesn't have it's own hook > + */ > + if (!vga_default_device()) { > + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) > + vgaarb_info(dev, "setting as boot VGA device\n"); > + else > + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); > + vga_set_default_device(pdev); > + } > + > + vgadev_default = vgadev_find(vga_default); > + > + /* Overridden by a better device */ > + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0) > + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { > + vgaarb_info(dev, "overriding boot VGA device\n"); > + vga_set_default_device(pdev); > + } > + > + if (vga_arb_integrated_gpu(dev)) { > + vgaarb_info(dev, "overriding boot VGA device\n"); > + vga_set_default_device(pdev); > + } > + > +#if defined(CONFIG_X86) || defined(CONFIG_IA64) > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > + base |= (u64)screen_info.ext_lfb_base << 32; > + > + limit = base + size; > + > + /* > + * Override vga_arbiter_add_pci_device()'s I/O based detection > + * as it may take the wrong device (e.g. on Apple system under > + * EFI). > + * > + * Select the device owning the boot framebuffer if there is > + * one. > + */ > + > + /* Does firmware framebuffer belong to us? */ > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > + flags = pci_resource_flags(vgadev->pdev, i); > + > + if ((flags & IORESOURCE_MEM) == 0) > + continue; > + > + start = pci_resource_start(vgadev->pdev, i); > + end = pci_resource_end(vgadev->pdev, i); > + > + if (!start || !end) > + continue; > + > + if (base < start || limit >= end) > + continue; > + > + if (vgadev->pdev != vga_default_device()) > + vgaarb_info(dev, "overriding boot device\n"); > + vga_set_default_device(vgadev->pdev); > + } > +#endif > +} > + > /* > * Rules for using a bridge to control a VGA descendant decoding: if a bridge > * has only one VGA descendant then it can be used to control the VGA routing > @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) > } > new_bus = new_bus->parent; > } > + > + if (vgadev->bridge_has_one_vga == true) > + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n"); > + else > + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n"); > } > > /* > @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > bus = bus->parent; > } > > - /* Deal with VGA default device. Use first enabled one > - * by default if arch doesn't have it's own hook > - */ > - if (vga_default == NULL && > - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { > - vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); > - vga_set_default_device(pdev); > - } > - > + vga_arb_update_default_device(vgadev); > vga_arbiter_check_bridge_sharing(vgadev); > > /* Add to the list */ > @@ -1451,111 +1539,10 @@ static struct miscdevice vga_arb_device = { > MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops > }; > > -#if defined(CONFIG_ACPI) > -static bool vga_arb_integrated_gpu(struct device *dev) > -{ > - struct acpi_device *adev = ACPI_COMPANION(dev); > - > - return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID); > -} > -#else > -static bool vga_arb_integrated_gpu(struct device *dev) > -{ > - return false; > -} > -#endif > - > -static void __init vga_arb_select_default_device(void) > -{ > - struct pci_dev *pdev, *found = NULL; > - struct vga_device *vgadev; > - > -#if defined(CONFIG_X86) || defined(CONFIG_IA64) > - u64 base = screen_info.lfb_base; > - u64 size = screen_info.lfb_size; > - u64 limit; > - resource_size_t start, end; > - unsigned long flags; > - int i; > - > - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > - base |= (u64)screen_info.ext_lfb_base << 32; > - > - limit = base + size; > - > - list_for_each_entry(vgadev, &vga_list, list) { > - struct device *dev = &vgadev->pdev->dev; > - /* > - * Override vga_arbiter_add_pci_device()'s I/O based detection > - * as it may take the wrong device (e.g. on Apple system under > - * EFI). > - * > - * Select the device owning the boot framebuffer if there is > - * one. > - */ > - > - /* Does firmware framebuffer belong to us? */ > - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > - flags = pci_resource_flags(vgadev->pdev, i); > - > - if ((flags & IORESOURCE_MEM) == 0) > - continue; > - > - start = pci_resource_start(vgadev->pdev, i); > - end = pci_resource_end(vgadev->pdev, i); > - > - if (!start || !end) > - continue; > - > - if (base < start || limit >= end) > - continue; > - > - if (!vga_default_device()) > - vgaarb_info(dev, "setting as boot device\n"); > - else if (vgadev->pdev != vga_default_device()) > - vgaarb_info(dev, "overriding boot device\n"); > - vga_set_default_device(vgadev->pdev); > - } > - } > -#endif > - > - if (!vga_default_device()) { > - list_for_each_entry_reverse(vgadev, &vga_list, list) { > - struct device *dev = &vgadev->pdev->dev; > - u16 cmd; > - > - pdev = vgadev->pdev; > - pci_read_config_word(pdev, PCI_COMMAND, &cmd); > - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { > - found = pdev; > - if (vga_arb_integrated_gpu(dev)) > - break; > - } > - } > - } > - > - if (found) { > - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n"); > - vga_set_default_device(found); > - return; > - } > - > - if (!vga_default_device()) { > - vgadev = list_first_entry_or_null(&vga_list, > - struct vga_device, list); > - if (vgadev) { > - struct device *dev = &vgadev->pdev->dev; > - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); > - vga_set_default_device(vgadev->pdev); > - } > - } > -} > - > static int __init vga_arb_device_init(void) > { > int rc; > struct pci_dev *pdev; > - struct vga_device *vgadev; > > rc = misc_register(&vga_arb_device); > if (rc < 0) > @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void) > PCI_ANY_ID, pdev)) != NULL) > vga_arbiter_add_pci_device(pdev); > > - list_for_each_entry(vgadev, &vga_list, list) { > - struct device *dev = &vgadev->pdev->dev; > - > - if (vgadev->bridge_has_one_vga) > - vgaarb_info(dev, "bridge control possible\n"); > - else > - vgaarb_info(dev, "no bridge control possible\n"); > - } > - > - vga_arb_select_default_device(); > - > - pr_info("loaded\n"); > return rc; > } > subsys_initcall(vga_arb_device_init); > -- > 2.27.0 >