From: Bjorn Helgaas bhelgaas@google.com
Current default VGA device selection fails in some cases because part of it is done in the vga_arb_device_init() subsys_initcall, and some arches enumerate PCI devices in pcibios_init(), which runs *after* that.
The big change from the v8 posting is that this moves vgaarb.c from drivers/gpu/vga to drivers/pci because it really has nothing to do with GPUs or DRM.
For example:
- On BMC system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA. This is perfectly legal but means the legacy VGA resources won't reach downstream devices unless they're included in the usual bridge windows.
- vga_arb_select_default_device() will set a device below such a bridge as the default VGA device as long as it has PCI_COMMAND_IO and PCI_COMMAND_MEMORY enabled.
- vga_arbiter_add_pci_device() is called for every VGA device, either at boot-time or at hot-add time, and it will also set the device as the default VGA device, but ONLY if all bridges leading to it implement PCI_BRIDGE_CTL_VGA.
- This difference between vga_arb_select_default_device() and vga_arbiter_add_pci_device() means that a device below an AST2500 or similar bridge can only be set as the default if it is enumerated before vga_arb_device_init().
- On ACPI-based systems, PCI devices are enumerated by acpi_init(), which runs before vga_arb_device_init().
- On non-ACPI systems, like on MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
This series consolidates all the default VGA device selection in vga_arbiter_add_pci_device(), which is always called after enumerating a PCI device.
Almost all the work here is Huacai's. I restructured it a little bit and added a few trivial patches on top.
Bjorn
Version history: V0 original implementation as final quirk to set default device. https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn
V1 rework vgaarb to do all default device selection in vga_arbiter_add_pci_device(). https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn
V2 move arbiter to PCI subsystem, fix nits. https://lore.kernel.org/r/20210722212920.347118-1-helgaas@kernel.org
V3 rewrite the commit log of the last patch (which is also summarized by Bjorn). https://lore.kernel.org/r/20210820100832.663931-1-chenhuacai@loongson.cn
V4 split the last patch to two steps. https://lore.kernel.org/r/20210827083129.2781420-1-chenhuacai@loongson.cn
V5 split Patch-9 again and sort the patches. https://lore.kernel.org/r/20210911093056.1555274-1-chenhuacai@loongson.cn
V6 split Patch-5 again and sort the patches again. https://lore.kernel.org/r/20210916082941.3421838-1-chenhuacai@loongson.cn
V7 stop moving vgaarb to drivers/pci because of ordering issues with misc_init(). https://lore.kernel.org/r/20211015061512.2941859-1-chenhuacai@loongson.cn https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEs...
V8 expand commit logs, rework boot VGA device selection https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org
V9 (this posting) move to drivers/pci, resolve initcall ordering, rename vga_is_framebuffer_device() to vga_is_firmware_default()
Bjorn Helgaas (5): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Factor out vga_select_framebuffer_device() PCI/VGA: Remove empty vga_arb_device_card_gone() PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Replace full MIT license text with SPDX identifier
Huacai Chen (6): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Factor out default VGA device selection PCI/VGA: Move firmware default device detection to ADD_DEVICE path PCI/VGA: Move non-legacy VGA detection to ADD_DEVICE path PCI/VGA: Move disabled VGA device detection to ADD_DEVICE path PCI/VGA: Log bridge control messages when adding devices
drivers/gpu/vga/Kconfig | 19 -- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 ++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 313 +++++++++++++++--------------- 5 files changed, 175 insertions(+), 178 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
From: Bjorn Helgaas bhelgaas@google.com
The VGA arbiter is really PCI-specific and doesn't depend on any GPU things. Move it to the PCI subsystem.
Note that misc_init() must be called before vga_arb_device_init(). These are both subsys_initcalls, so this ordering depends on the link order, which is determined by drivers/Makefile:
obj-y += pci/ obj-y += char/ <-- misc_init() obj-y += gpu/ <-- vga_arb_device_init() (before this commit)
The drivers/pci/ subsys_initcalls are called *before* misc_init(), so convert vga_arb_device_init() to subsys_initcall_sync(), which is called after *all* subsys_initcalls.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/gpu/vga/Kconfig | 19 ------------------- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++++++++++++++++++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 2 +- 5 files changed, 21 insertions(+), 21 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (99%)
diff --git a/drivers/gpu/vga/Kconfig b/drivers/gpu/vga/Kconfig index 1ad4c4ef0b5e..eb8b14ab22c3 100644 --- a/drivers/gpu/vga/Kconfig +++ b/drivers/gpu/vga/Kconfig @@ -1,23 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only -config VGA_ARB - bool "VGA Arbitration" if EXPERT - default y - depends on (PCI && !S390) - help - Some "legacy" VGA devices implemented on PCI typically have the same - hard-decoded addresses as they did on ISA. When multiple PCI devices - are accessed at same time they need some kind of coordination. Please - see Documentation/gpu/vgaarbiter.rst for more details. Select this to - enable VGA arbiter. - -config VGA_ARB_MAX_GPUS - int "Maximum number of GPUs" - default 16 - depends on VGA_ARB - help - Reserves space in the kernel to maintain resource locking for - multiple GPUS. The overhead for each GPU is very small. - config VGA_SWITCHEROO bool "Laptop Hybrid Graphics - GPU switching support" depends on X86 diff --git a/drivers/gpu/vga/Makefile b/drivers/gpu/vga/Makefile index e92064442d60..9800620deda3 100644 --- a/drivers/gpu/vga/Makefile +++ b/drivers/gpu/vga/Makefile @@ -1,3 +1,2 @@ # SPDX-License-Identifier: GPL-2.0-only -obj-$(CONFIG_VGA_ARB) += vgaarb.o obj-$(CONFIG_VGA_SWITCHEROO) += vga_switcheroo.o diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index d98fafdd0f99..133c73207782 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -252,6 +252,25 @@ config PCIE_BUS_PEER2PEER
endchoice
+config VGA_ARB + bool "VGA Arbitration" if EXPERT + default y + depends on (PCI && !S390) + help + Some "legacy" VGA devices implemented on PCI typically have the same + hard-decoded addresses as they did on ISA. When multiple PCI devices + are accessed at same time they need some kind of coordination. Please + see Documentation/gpu/vgaarbiter.rst for more details. Select this to + enable VGA arbiter. + +config VGA_ARB_MAX_GPUS + int "Maximum number of GPUs" + default 16 + depends on VGA_ARB + help + Reserves space in the kernel to maintain resource locking for + multiple GPUS. The overhead for each GPU is very small. + source "drivers/pci/hotplug/Kconfig" source "drivers/pci/controller/Kconfig" source "drivers/pci/endpoint/Kconfig" diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 37be95adf169..0da6b1ebc694 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o obj-$(CONFIG_PCI_ECAM) += ecam.o obj-$(CONFIG_PCI_P2PDMA) += p2pdma.o obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o +obj-$(CONFIG_VGA_ARB) += vgaarb.o
# Endpoint library must be initialized before its users obj-$(CONFIG_PCI_ENDPOINT) += endpoint/ diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/pci/vgaarb.c similarity index 99% rename from drivers/gpu/vga/vgaarb.c rename to drivers/pci/vgaarb.c index 569930552957..8320385a487b 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -1564,4 +1564,4 @@ static int __init vga_arb_device_init(void) pr_info("loaded\n"); return rc; } -subsys_initcall(vga_arb_device_init); +subsys_initcall_sync(vga_arb_device_init);
From: Huacai Chen chenhuacai@loongson.cn
Move vga_arb_integrated_gpu() earlier in file to prepare for future patch. No functional change intended.
[bhelgaas: pull #ifdefs inside function] Link: https://lore.kernel.org/r/20211015061512.2941859-3-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn --- drivers/pci/vgaarb.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 8320385a487b..6c57ffb77770 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -565,6 +565,17 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) } EXPORT_SYMBOL(vga_put);
+static bool vga_arb_integrated_gpu(struct device *dev) +{ +#if defined(CONFIG_ACPI) + struct acpi_device *adev = ACPI_COMPANION(dev); + + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID); +#else + return false; +#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 @@ -1430,20 +1441,6 @@ 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;
From: Bjorn Helgaas bhelgaas@google.com
On x86 and ia64, if a VGA device BARs include a framebuffer reported by platform firmware, we select the device as the default VGA device. Factor this code to a separate function. No functional change intended.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org --- drivers/pci/vgaarb.c | 99 ++++++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 46 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 6c57ffb77770..3f8fead49197 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -565,6 +565,58 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) } EXPORT_SYMBOL(vga_put);
+static void __init vga_select_framebuffer_device(struct pci_dev *pdev) +{ +#if defined(CONFIG_X86) || defined(CONFIG_IA64) + struct device *dev = &pdev->dev; + u64 base = screen_info.lfb_base; + u64 size = screen_info.lfb_size; + u64 limit; + resource_size_t start, end; + unsigned long flags; + int i; + + /* Select the device owning the boot framebuffer if there is one */ + + 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(pdev, i); + + if ((flags & IORESOURCE_MEM) == 0) + continue; + + start = pci_resource_start(pdev, i); + end = pci_resource_end(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 (pdev != vga_default_device()) + vgaarb_info(dev, "overriding boot device\n"); + vga_set_default_device(pdev); + } +#endif +} + static bool vga_arb_integrated_gpu(struct device *dev) { #if defined(CONFIG_ACPI) @@ -1446,54 +1498,9 @@ 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); - } + vga_select_framebuffer_device(vgadev->pdev); } -#endif
if (!vga_default_device()) { list_for_each_entry_reverse(vgadev, &vga_list, list) {
From: Huacai Chen chenhuacai@loongson.cn
Default VGA device selection fails when PCI devices are enumerated after the vga_arb_device_init() subsys_initcall.
vga_arbiter_add_pci_device() selects the first fully enabled device to which legacy VGA resources are routed as the default VGA device. This is an ADD_DEVICE notifier, so it runs after every PCI device is enumerated.
vga_arb_select_default_device() may select framebuffer devices, partially enabled GPUs, or non-legacy devices that don't have legacy VGA resources routed to them as the default VGA device. But this only happens once, from the vga_arb_device_init() subsys_initcall, so it doesn't consider devices enumerated after that:
acpi_init acpi_scan_init acpi_pci_root_init # PCI device enumeration (ACPI systems)
vga_arb_device_init for_each_pci_device vga_arbiter_add_pci_device # ADD_DEVICE notifier if (VGA-owner) vga_set_default_device <-- set default VGA vga_arb_select_default_device # only called ONCE for_each_vga_device if (framebuffer) vga_set_default_device <-- set default VGA to framebuffer if (!vga_default_device()) if (non-legacy, integrated GPU, etc) vga_set_default_device <-- set default VGA if (!vga_default_device()) vga_set_default_device <-- set default VGA
pcibios_init pcibios_scanbus # PCI device enumeration (non-ACPI systems) ... vga_arbiter_add_pci_device # ADD_DEVICE notification if (VGA-owner) vga_set_default_device <-- set default VGA
Note that on non-ACPI systems, vga_arb_select_default_device() runs before pcibios_init(), so it sees no VGA devices and can never set a framebuffer device, a non-legacy integrated GPU, etc., as the default device.
Factor out the default VGA device selection to vga_is_boot_device(), called from vga_arbiter_add_pci_device().
Then we can migrate the default device selection from vga_arb_select_default_device() to the vga_arbiter_add_pci_device() path.
[bhelgaas: commit log, split to separate patch] Link: https://lore.kernel.org/r/20211015061512.2941859-4-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn --- drivers/pci/vgaarb.c | 45 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 3f8fead49197..58e0a12e623b 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -628,6 +628,41 @@ static bool vga_arb_integrated_gpu(struct device *dev) #endif }
+/* + * Return true if vgadev is a better default VGA device than the best one + * we've seen so far. + */ +static bool vga_is_boot_device(struct vga_device *vgadev) +{ + struct vga_device *boot_vga = vgadev_find(vga_default_device()); + + /* + * We select the default VGA device in this order: + * Firmware framebuffer (see vga_arb_select_default_device()) + * Legacy VGA device (owns VGA_RSRC_LEGACY_MASK) + * Non-legacy integrated device (see vga_arb_select_default_device()) + * Non-legacy discrete device (see vga_arb_select_default_device()) + * Other device (see vga_arb_select_default_device()) + */ + + /* + * A legacy VGA device has MEM and IO enabled and any bridges + * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy + * resources ([mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], etc) are + * routed to it. + * + * We use the first one we find, so if we've already found one, + * vgadev is no better. + */ + if (boot_vga) + return false; + + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) + return true; + + return false; +} + /* * 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 @@ -755,12 +790,10 @@ 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"); + if (vga_is_boot_device(vgadev)) { + vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n", + vga_default_device() ? + " (overriding previous)" : ""); vga_set_default_device(pdev); }
From: Huacai Chen chenhuacai@loongson.cn
Previously we selected the firmware default device, i.e., one that owns the boot framebuffer, as the default device in vga_arb_select_default_device(). This was only done in the vga_arb_device_init() subsys_initcall, so devices enumerated later, e.g., by pcibios_init(), were not eligible.
Fix this by moving the firmware default device selection from vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is called after every PCI device is enumerated, either by the vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
Note that if vga_arb_select_default_device() previously found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
[bhelgaas: commit log, restructure slightly] Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn Cc: Bruno Prémont bonbons@linux-vserver.org --- drivers/pci/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 58e0a12e623b..c3323ab4f98e 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -72,6 +72,7 @@ struct vga_device { unsigned int io_norm_cnt; /* normal IO count */ unsigned int mem_norm_cnt; /* normal MEM count */ bool bridge_has_one_vga; + bool is_firmware_default; /* device selected by firmware */ unsigned int (*set_decode)(struct pci_dev *pdev, bool decode); };
@@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) } EXPORT_SYMBOL(vga_put);
-static void __init vga_select_framebuffer_device(struct pci_dev *pdev) +static bool vga_is_firmware_default(struct pci_dev *pdev) { #if defined(CONFIG_X86) || defined(CONFIG_IA64) - struct device *dev = &pdev->dev; u64 base = screen_info.lfb_base; u64 size = screen_info.lfb_size; u64 limit; @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
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(pdev, i); @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev) if (base < start || limit >= end) continue;
- if (!vga_default_device()) - vgaarb_info(dev, "setting as boot device\n"); - else if (pdev != vga_default_device()) - vgaarb_info(dev, "overriding boot device\n"); - vga_set_default_device(pdev); + return true; } #endif + return false; }
static bool vga_arb_integrated_gpu(struct device *dev) @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev) static bool vga_is_boot_device(struct vga_device *vgadev) { struct vga_device *boot_vga = vgadev_find(vga_default_device()); + struct pci_dev *pdev = vgadev->pdev;
/* * We select the default VGA device in this order: @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev) * Other device (see vga_arb_select_default_device()) */
+ /* + * We always prefer a firmware default device, so if we've already + * found one, there's no need to consider vgadev. + */ + if (boot_vga && boot_vga->is_firmware_default) + return false; + + if (vga_is_firmware_default(pdev)) { + vgadev->is_firmware_default = true; + return true; + } + /* * A legacy VGA device has MEM and IO enabled and any bridges * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void) struct pci_dev *pdev, *found = NULL; struct vga_device *vgadev;
- list_for_each_entry(vgadev, &vga_list, list) { - vga_select_framebuffer_device(vgadev->pdev); - } - if (!vga_default_device()) { list_for_each_entry_reverse(vgadev, &vga_list, list) { struct device *dev = &vgadev->pdev->dev;
From: Huacai Chen chenhuacai@loongson.cn
a37c0f48950b ("vgaarb: Select a default VGA device even if there's no legacy VGA") extended the vga_arb_device_init() subsys_initcall so it could select a non-legacy VGA device as the default.
That failed to consider that PCI devices may be enumerated after vga_arb_device_init(), e.g., hot-added devices or non-ACPI systems that do PCI enumeration in pcibios_init(). Devices found then could never be selected as the default.
One system where this is a problem is the MIPS-based Loongson where an ASpeed AST2500 VGA device is behind a bridge that doesn't implement the VGA Enable bit, so legacy resources are not routed to the VGA device. [1]
Fix this by moving the non-legacy VGA device selection from vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is called after every PCI device is enumerated, either by the vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
[1] https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn
[bhelgaas: commit log, restructure] Link: https://lore.kernel.org/r/20211015061512.2941859-5-chenhuacai@loongson.cn Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn Cc: Daniel Axtens dja@axtens.net Cc: Zhou Wang wangzhou1@hisilicon.com --- drivers/pci/vgaarb.c | 54 ++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index c3323ab4f98e..e36ccbfdbd89 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -624,6 +624,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev) { struct vga_device *boot_vga = vgadev_find(vga_default_device()); struct pci_dev *pdev = vgadev->pdev; + u16 cmd, boot_cmd;
/* * We select the default VGA device in this order: @@ -661,6 +662,37 @@ static bool vga_is_boot_device(struct vga_device *vgadev) if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) return true;
+ /* + * If we haven't found a legacy VGA device, accept a non-legacy + * device. It may have either IO or MEM enabled, and bridges may + * not have PCI_BRIDGE_CTL_VGA enabled, so it may not be able to + * use legacy VGA resources. Prefer an integrated GPU over others. + */ + pci_read_config_word(pdev, PCI_COMMAND, &cmd); + if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { + + /* + * An integrated GPU overrides a previous non-legacy + * device. We expect only a single integrated GPU, but if + * there are more, we use the *last* because that was the + * previous behavior. + */ + if (vga_arb_integrated_gpu(&pdev->dev)) + return true; + + /* + * We prefer the first non-legacy discrete device we find. + * If we already found one, vgadev is no better. + */ + if (boot_vga) { + pci_read_config_word(boot_vga->pdev, PCI_COMMAND, + &boot_cmd); + if (boot_cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) + return false; + } + return true; + } + return false; }
@@ -1529,30 +1561,8 @@ static struct miscdevice vga_arb_device = {
static void __init vga_arb_select_default_device(void) { - struct pci_dev *pdev, *found = NULL; struct vga_device *vgadev;
- 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);
From: Huacai Chen chenhuacai@loongson.cn
a37c0f48950b ("vgaarb: Select a default VGA device even if there's no legacy VGA") extended the vga_arb_device_init() subsys_initcall so that if there are no other eligible devices, it could select a disabled VGA device as the default.
Move this detection from vga_arb_select_default_device() to vga_arbiter_add_pci_device() so every device, even those hot-added or enumerated after vga_arb_device_init() is eligible for selection as the default VGA device.
[bhelgaas: commit log, restructure] Link: https://lore.kernel.org/r/20211015061512.2941859-5-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@kernel.org Cc: Daniel Axtens dja@axtens.net Cc: Zhou Wang wangzhou1@hisilicon.com --- drivers/pci/vgaarb.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index e36ccbfdbd89..e8d5efd85ba6 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -656,7 +656,8 @@ static bool vga_is_boot_device(struct vga_device *vgadev) * We use the first one we find, so if we've already found one, * vgadev is no better. */ - if (boot_vga) + if (boot_vga && + (boot_vga->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) return false;
if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) @@ -693,6 +694,13 @@ static bool vga_is_boot_device(struct vga_device *vgadev) return true; }
+ /* + * vgadev has neither IO nor MEM enabled. If we haven't found any + * other VGA devices, it is the best candidate so far. + */ + if (!boot_vga) + return true; + return false; }
@@ -1559,21 +1567,6 @@ static struct miscdevice vga_arb_device = { MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops };
-static void __init vga_arb_select_default_device(void) -{ - struct vga_device *vgadev; - - 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; @@ -1603,8 +1596,6 @@ static int __init vga_arb_device_init(void) vgaarb_info(dev, "no bridge control possible\n"); }
- vga_arb_select_default_device(); - pr_info("loaded\n"); return rc; }
From: Bjorn Helgaas bhelgaas@google.com
vga_arb_device_card_gone() has always been empty. Remove it.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/pci/vgaarb.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index e8d5efd85ba6..582f0054b71a 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -123,8 +123,6 @@ static int vga_str_to_iostate(char *buf, int str_size, int *io_state) /* this is only used a cookie - it should not be dereferenced */ static struct pci_dev *vga_default;
-static void vga_arb_device_card_gone(struct pci_dev *pdev); - /* Find somebody in our list */ static struct vga_device *vgadev_find(struct pci_dev *pdev) { @@ -878,10 +876,6 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) /* Remove entry from list */ list_del(&vgadev->list); vga_count--; - /* Notify userland driver that the device is gone so it discards - * it's copies of the pci_dev pointer - */ - vga_arb_device_card_gone(pdev);
/* Wake up all possible waiters */ wake_up_all(&vga_wait_queue); @@ -1131,9 +1125,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf, if (lbuf == NULL) return -ENOMEM;
- /* Shields against vga_arb_device_card_gone (pci_dev going - * away), and allows access to vga list - */ + /* Protects vga_list */ spin_lock_irqsave(&vga_lock, flags);
/* If we are targeting the default, use it */ @@ -1150,8 +1142,6 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf, /* Wow, it's not in the list, that shouldn't happen, * let's fix us up and return invalid card */ - if (pdev == priv->target) - vga_arb_device_card_gone(pdev); spin_unlock_irqrestore(&vga_lock, flags); len = sprintf(lbuf, "invalid"); goto done; @@ -1495,10 +1485,6 @@ static int vga_arb_release(struct inode *inode, struct file *file) return 0; }
-static void vga_arb_device_card_gone(struct pci_dev *pdev) -{ -} - /* * callback any registered clients to let them know we have a * change in VGA cards
From: Huacai Chen chenhuacai@loongson.cn
Previously vga_arb_device_init() iterated through all VGA devices and indicated whether legacy VGA routing to each could be controlled by an upstream bridge.
But we determine that information in vga_arbiter_add_pci_device(), which we call for every device, so we can log it there without iterating through the VGA devices again.
Note that we call vga_arbiter_check_bridge_sharing() before adding the device to vga_list, so we have to handle the very first device separately.
Signed-off-by: Huacai Chen chenhuacai@loongson.cn --- drivers/pci/vgaarb.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 582f0054b71a..7b1bfdea9d10 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -719,8 +719,10 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
vgadev->bridge_has_one_vga = true;
- if (list_empty(&vga_list)) + if (list_empty(&vga_list)) { + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n"); return; + }
/* okay iterate the new devices bridge hierarachy */ new_bus = vgadev->pdev->bus; @@ -759,6 +761,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) } new_bus = new_bus->parent; } + + if (vgadev->bridge_has_one_vga) + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n"); + else + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n"); }
/* @@ -1557,7 +1564,6 @@ 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) @@ -1573,15 +1579,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"); - } - pr_info("loaded\n"); return rc; }
From: Bjorn Helgaas bhelgaas@google.com
In struct vga_device, io_lock_cnt and mem_lock_cnt are unsigned, but we previously printed them with "%d", the signed decimal format. Print them with the unsigned format "%u" instead.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/pci/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 7b1bfdea9d10..d6e77c6a9a1a 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -1156,7 +1156,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
/* Fill the buffer with infos */ len = snprintf(lbuf, 1024, - "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%d:%d)\n", + "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%u:%u)\n", vga_decode_count, pci_name(pdev), vga_iostate_to_str(vgadev->decodes), vga_iostate_to_str(vgadev->owns),
From: Bjorn Helgaas bhelgaas@google.com
Per Documentation/process/license-rules.rst, the SPDX MIT identifier is equivalent to including the entire MIT license text from LICENSES/preferred/MIT.
Replace the MIT license text with the equivalent SPDX identifier.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/pci/vgaarb.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index d6e77c6a9a1a..f80b6ec88dc3 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -1,32 +1,11 @@ +// SPDX-License-Identifier: MIT /* * vgaarb.c: Implements the VGA arbitration. For details refer to * Documentation/gpu/vgaarbiter.rst * - * * (C) Copyright 2005 Benjamin Herrenschmidt benh@kernel.crashing.org * (C) Copyright 2007 Paulo R. Zanoni przanoni@gmail.com * (C) Copyright 2007, 2009 Tiago Vignatti vignatti@freedesktop.org - * - * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice (including the next - * paragraph) shall be included in all copies or substantial portions of the - * Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER - * DEALINGS - * IN THE SOFTWARE. - * */
#define pr_fmt(fmt) "vgaarb: " fmt
On Thu, Feb 24, 2022 at 04:47:42PM -0600, Bjorn Helgaas wrote:
From: Bjorn Helgaas bhelgaas@google.com
Current default VGA device selection fails in some cases because part of it is done in the vga_arb_device_init() subsys_initcall, and some arches enumerate PCI devices in pcibios_init(), which runs *after* that.
The big change from the v8 posting is that this moves vgaarb.c from drivers/gpu/vga to drivers/pci because it really has nothing to do with GPUs or DRM.
I provisionally applied this to pci/vga and put it into -next just to get a little runtime on it.
But I'd prefer not to unilaterally yank this out of drivers/gpu without a consensus from the GPU folks that this is the right thing to do.
Any thoughts? If it seems OK to you, I think patch 1/11 (the move itself) is all you would need to look at, although of course I would still be grateful for any review and feedback on the rest.
After it's in drivers/pci, all the blame for any issues would come my way.
Bjorn
On Fri, Feb 25, 2022 at 04:15:23PM -0600, Bjorn Helgaas wrote:
On Thu, Feb 24, 2022 at 04:47:42PM -0600, Bjorn Helgaas wrote:
From: Bjorn Helgaas bhelgaas@google.com
Current default VGA device selection fails in some cases because part of it is done in the vga_arb_device_init() subsys_initcall, and some arches enumerate PCI devices in pcibios_init(), which runs *after* that.
The big change from the v8 posting is that this moves vgaarb.c from drivers/gpu/vga to drivers/pci because it really has nothing to do with GPUs or DRM.
I provisionally applied this to pci/vga and put it into -next just to get a little runtime on it.
But I'd prefer not to unilaterally yank this out of drivers/gpu without a consensus from the GPU folks that this is the right thing to do.
Any thoughts? If it seems OK to you, I think patch 1/11 (the move itself) is all you would need to look at, although of course I would still be grateful for any review and feedback on the rest.
After it's in drivers/pci, all the blame for any issues would come my way.
Ping? This has been in -next since the Feb 28 tree, and I plan to ask Linus to include it during the v5.18 merge window (which will probably open Mar 13) unless somebody objects.
Bjorn
I have no objection :)
Huacai
On Thu, Mar 10, 2022 at 12:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Feb 25, 2022 at 04:15:23PM -0600, Bjorn Helgaas wrote:
On Thu, Feb 24, 2022 at 04:47:42PM -0600, Bjorn Helgaas wrote:
From: Bjorn Helgaas bhelgaas@google.com
Current default VGA device selection fails in some cases because part of it is done in the vga_arb_device_init() subsys_initcall, and some arches enumerate PCI devices in pcibios_init(), which runs *after* that.
The big change from the v8 posting is that this moves vgaarb.c from drivers/gpu/vga to drivers/pci because it really has nothing to do with GPUs or DRM.
I provisionally applied this to pci/vga and put it into -next just to get a little runtime on it.
But I'd prefer not to unilaterally yank this out of drivers/gpu without a consensus from the GPU folks that this is the right thing to do.
Any thoughts? If it seems OK to you, I think patch 1/11 (the move itself) is all you would need to look at, although of course I would still be grateful for any review and feedback on the rest.
After it's in drivers/pci, all the blame for any issues would come my way.
Ping? This has been in -next since the Feb 28 tree, and I plan to ask Linus to include it during the v5.18 merge window (which will probably open Mar 13) unless somebody objects.
Bjorn
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Huacai Chen