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.
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.
I'd like to move vgaarb.c to drivers/pci eventually, but there's another initcall ordering snag that needs to be resolved first, so this leaves it where it is.
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...
Bjorn Helgaas (8): vgaarb: Factor out vga_select_framebuffer_device() vgaarb: Factor out default VGA device selection vgaarb: Move framebuffer detection to ADD_DEVICE path vgaarb: Move non-legacy VGA detection to ADD_DEVICE path vgaarb: Move disabled VGA device detection to ADD_DEVICE path vgaarb: Remove empty vga_arb_device_card_gone() vgaarb: Use unsigned format string to print lock counts vgaarb: Replace full MIT license text with SPDX identifier
Huacai Chen (2): vgaarb: Move vga_arb_integrated_gpu() earlier in file vgaarb: Log bridge control messages when adding devices
drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++-------------------- 1 file changed, 154 insertions(+), 157 deletions(-)
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 Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/gpu/vga/vgaarb.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 569930552957..ef5ad4c432f5 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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/gpu/vga/vgaarb.c | 99 +++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index ef5ad4c432f5..36d9140c64f6 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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: Bjorn Helgaas bhelgaas@google.com
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.
Link: https://lore.kernel.org/r/20211015061512.2941859-4-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@kernel.org Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/gpu/vga/vgaarb.c | 45 ++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 36d9140c64f6..b0ae0f177c6f 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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: Bjorn Helgaas bhelgaas@google.com
Previously we selected a device 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 framebuffer 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() found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org --- drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b0ae0f177c6f..aefa4f406f7d 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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_framebuffer; /* BAR covers firmware framebuffer */ 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_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; @@ -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 framebuffer, so if we've already + * found one, there's no need to consider vgadev. + */ + if (boot_vga && boot_vga->is_framebuffer) + return false; + + if (vga_is_framebuffer_device(pdev)) { + vgadev->is_framebuffer = 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;
Hi, Bjorn,
On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
Previously we selected a device 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 framebuffer 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() found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org
drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b0ae0f177c6f..aefa4f406f7d 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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_framebuffer; /* BAR covers firmware framebuffer */ 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_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;@@ -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 framebuffer, so if we've already* found one, there's no need to consider vgadev.*/if (boot_vga && boot_vga->is_framebuffer)return false;if (vga_is_framebuffer_device(pdev)) {vgadev->is_framebuffer = true;return true;}
Maybe it is better to rename vga_is_framebuffer_device() to vga_is_firmware_device() and rename is_framebuffer to is_fw_framebuffer?
Huacai
/* * 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;-- 2.25.1
On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas helgaas@kernel.org wrote:
Previously we selected a device 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 framebuffer 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() found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org
drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b0ae0f177c6f..aefa4f406f7d 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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_framebuffer; /* BAR covers firmware framebuffer */ 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_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;@@ -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 framebuffer, so if we've already* found one, there's no need to consider vgadev.*/if (boot_vga && boot_vga->is_framebuffer)return false;if (vga_is_framebuffer_device(pdev)) {vgadev->is_framebuffer = true;return true;}Maybe it is better to rename vga_is_framebuffer_device() to vga_is_firmware_device() and rename is_framebuffer to is_fw_framebuffer?
That's a great point, thanks!
The "framebuffer" term is way too generic. *All* VGA devices have a framebuffer, so it adds no information. This is really about finding the device that was used by firmware.
I renamed:
vga_is_framebuffer_device() -> vga_is_firmware_default() vga_device.is_framebuffer -> vga_device.is_firmware_default
I updated my local branch and pushed it to: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/v... with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with SPDX identifier").
I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for reference. It'll ultimately be up to the DRM folks to handle this.
I'll wait for any other comments or testing reports before reposting.
/* * 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;-- 2.25.1
Hi, Bjorn,
Why this series still missing in 5.17-rc1? :(
Huacai
On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas helgaas@kernel.org wrote:
Previously we selected a device 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 framebuffer 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() found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org
drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b0ae0f177c6f..aefa4f406f7d 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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_framebuffer; /* BAR covers firmware framebuffer */ 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_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;@@ -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 framebuffer, so if we've already* found one, there's no need to consider vgadev.*/if (boot_vga && boot_vga->is_framebuffer)return false;if (vga_is_framebuffer_device(pdev)) {vgadev->is_framebuffer = true;return true;}Maybe it is better to rename vga_is_framebuffer_device() to vga_is_firmware_device() and rename is_framebuffer to is_fw_framebuffer?
That's a great point, thanks!
The "framebuffer" term is way too generic. *All* VGA devices have a framebuffer, so it adds no information. This is really about finding the device that was used by firmware.
I renamed:
vga_is_framebuffer_device() -> vga_is_firmware_default() vga_device.is_framebuffer -> vga_device.is_firmware_default
I updated my local branch and pushed it to: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/v... with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with SPDX identifier").
I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for reference. It'll ultimately be up to the DRM folks to handle this.
I'll wait for any other comments or testing reports before reposting.
/* * 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;-- 2.25.1
[+cc Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Tue, Jan 25, 2022 at 10:51:15AM +0800, Huacai Chen wrote:
Hi, Bjorn,
Why this series still missing in 5.17-rc1? :(
1) It was posted late in the cycle (Jan 6, when the merge window opened Jan 9), so it was too late to expect a significant change like this to be merged for v5.17. Right now is a good time to consider it again so it would some time in -next.
2) As of now, this code is still in drivers/gpu, and I don't maintain that area. It's up to the DRM folks, who are all cc'd here.
I would like to move this from drivers/gpu to drivers/pci, but that requires a little more work to resolve the initcall ordering problem with respect to vga_arb_device_init() and misc_init() [1].
Bjorn
[1] https://lore.kernel.org/linux-pci/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcy...
On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas helgaas@kernel.org wrote:
Previously we selected a device 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 framebuffer 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() found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org
drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b0ae0f177c6f..aefa4f406f7d 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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_framebuffer; /* BAR covers firmware framebuffer */ 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_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;@@ -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 framebuffer, so if we've already* found one, there's no need to consider vgadev.*/if (boot_vga && boot_vga->is_framebuffer)return false;if (vga_is_framebuffer_device(pdev)) {vgadev->is_framebuffer = true;return true;}Maybe it is better to rename vga_is_framebuffer_device() to vga_is_firmware_device() and rename is_framebuffer to is_fw_framebuffer?
That's a great point, thanks!
The "framebuffer" term is way too generic. *All* VGA devices have a framebuffer, so it adds no information. This is really about finding the device that was used by firmware.
I renamed:
vga_is_framebuffer_device() -> vga_is_firmware_default() vga_device.is_framebuffer -> vga_device.is_firmware_default
I updated my local branch and pushed it to: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/v... with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with SPDX identifier").
I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for reference. It'll ultimately be up to the DRM folks to handle this.
I'll wait for any other comments or testing reports before reposting.
/* * 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;-- 2.25.1
On Tue, Jan 25, 2022 at 11:39 PM Bjorn Helgaas helgaas@kernel.org wrote:
[+cc Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Tue, Jan 25, 2022 at 10:51:15AM +0800, Huacai Chen wrote:
Hi, Bjorn,
Why this series still missing in 5.17-rc1? :(
- It was posted late in the cycle (Jan 6, when the merge window
opened Jan 9), so it was too late to expect a significant change like this to be merged for v5.17. Right now is a good time to consider it again so it would some time in -next.
- As of now, this code is still in drivers/gpu, and I don't maintain
that area. It's up to the DRM folks, who are all cc'd here.
I would like to move this from drivers/gpu to drivers/pci, but that requires a little more work to resolve the initcall ordering problem with respect to vga_arb_device_init() and misc_init() [1].
Hmm, to me, keep it in drivers/gpu is just OK.
Huacai
Bjorn
[1] https://lore.kernel.org/linux-pci/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcy...
On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas helgaas@kernel.org wrote:
Previously we selected a device 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 framebuffer 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() found a device owning the boot framebuffer, it unconditionally set it to be the default VGA device, and no subsequent device could replace it.
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Bruno Prémont bonbons@linux-vserver.org
drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b0ae0f177c6f..aefa4f406f7d 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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_framebuffer; /* BAR covers firmware framebuffer */ 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_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;@@ -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 framebuffer, so if we've already* found one, there's no need to consider vgadev.*/if (boot_vga && boot_vga->is_framebuffer)return false;if (vga_is_framebuffer_device(pdev)) {vgadev->is_framebuffer = true;return true;}Maybe it is better to rename vga_is_framebuffer_device() to vga_is_firmware_device() and rename is_framebuffer to is_fw_framebuffer?
That's a great point, thanks!
The "framebuffer" term is way too generic. *All* VGA devices have a framebuffer, so it adds no information. This is really about finding the device that was used by firmware.
I renamed:
vga_is_framebuffer_device() -> vga_is_firmware_default() vga_device.is_framebuffer -> vga_device.is_firmware_default
I updated my local branch and pushed it to: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/v... with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with SPDX identifier").
I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for reference. It'll ultimately be up to the DRM folks to handle this.
I'll wait for any other comments or testing reports before reposting.
/* * 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;-- 2.25.1
From: Bjorn Helgaas bhelgaas@google.com
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
Link: https://lore.kernel.org/r/20211015061512.2941859-5-chenhuacai@loongson.cn Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@kernel.org Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Daniel Axtens dja@axtens.net Cc: Zhou Wang wangzhou1@hisilicon.com --- drivers/gpu/vga/vgaarb.c | 54 ++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index aefa4f406f7d..123b81628061 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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: Bjorn Helgaas bhelgaas@google.com
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.
Link: https://lore.kernel.org/r/20211015061512.2941859-5-chenhuacai@loongson.cn Based-on-patch-by: Huacai Chen chenhuacai@kernel.org Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: Daniel Axtens dja@axtens.net Cc: Zhou Wang wangzhou1@hisilicon.com --- drivers/gpu/vga/vgaarb.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 123b81628061..ad548917e602 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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/gpu/vga/vgaarb.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index ad548917e602..455cf048fae8 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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 Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/gpu/vga/vgaarb.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 455cf048fae8..b7e6c1228fff 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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/gpu/vga/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b7e6c1228fff..95e37817074e 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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/gpu/vga/vgaarb.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 95e37817074e..17e9d2536430 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/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
[+to Maarten, Maxime, Thomas: sorry, I forgot to use get_maintainer.pl so I missed you the first time. Beginning of thread: https://lore.kernel.org/all/20220106000658.243509-1-helgaas@kernel.org/#t Git branch with this v8 + a couple trivial renames, based on v5.16-rc1: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=0f4ca...]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
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.
I'd like to move vgaarb.c to drivers/pci eventually, but there's another initcall ordering snag that needs to be resolved first, so this leaves it where it is.
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...
Bjorn Helgaas (8): vgaarb: Factor out vga_select_framebuffer_device() vgaarb: Factor out default VGA device selection vgaarb: Move framebuffer detection to ADD_DEVICE path vgaarb: Move non-legacy VGA detection to ADD_DEVICE path vgaarb: Move disabled VGA device detection to ADD_DEVICE path vgaarb: Remove empty vga_arb_device_card_gone() vgaarb: Use unsigned format string to print lock counts vgaarb: Replace full MIT license text with SPDX identifier
Huacai Chen (2): vgaarb: Move vga_arb_integrated_gpu() earlier in file vgaarb: Log bridge control messages when adding devices
drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++-------------------- 1 file changed, 154 insertions(+), 157 deletions(-)
-- 2.25.1
For the whole series: Tested-by: Huacai Chen chenhuacai@loongson.cn
On Fri, Jan 7, 2022 at 12:30 AM Bjorn Helgaas helgaas@kernel.org wrote:
[+to Maarten, Maxime, Thomas: sorry, I forgot to use get_maintainer.pl so I missed you the first time. Beginning of thread: https://lore.kernel.org/all/20220106000658.243509-1-helgaas@kernel.org/#t Git branch with this v8 + a couple trivial renames, based on v5.16-rc1: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=0f4ca...]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
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.
I'd like to move vgaarb.c to drivers/pci eventually, but there's another initcall ordering snag that needs to be resolved first, so this leaves it where it is.
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...
Bjorn Helgaas (8): vgaarb: Factor out vga_select_framebuffer_device() vgaarb: Factor out default VGA device selection vgaarb: Move framebuffer detection to ADD_DEVICE path vgaarb: Move non-legacy VGA detection to ADD_DEVICE path vgaarb: Move disabled VGA device detection to ADD_DEVICE path vgaarb: Remove empty vga_arb_device_card_gone() vgaarb: Use unsigned format string to print lock counts vgaarb: Replace full MIT license text with SPDX identifier
Huacai Chen (2): vgaarb: Move vga_arb_integrated_gpu() earlier in file vgaarb: Log bridge control messages when adding devices
drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++-------------------- 1 file changed, 154 insertions(+), 157 deletions(-)
-- 2.25.1
[+to Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
Where are we at with this series? Is there anything I can do to move it forward?
Bjorn
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.
I'd like to move vgaarb.c to drivers/pci eventually, but there's another initcall ordering snag that needs to be resolved first, so this leaves it where it is.
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...
Bjorn Helgaas (8): vgaarb: Factor out vga_select_framebuffer_device() vgaarb: Factor out default VGA device selection vgaarb: Move framebuffer detection to ADD_DEVICE path vgaarb: Move non-legacy VGA detection to ADD_DEVICE path vgaarb: Move disabled VGA device detection to ADD_DEVICE path vgaarb: Remove empty vga_arb_device_card_gone() vgaarb: Use unsigned format string to print lock counts vgaarb: Replace full MIT license text with SPDX identifier
Huacai Chen (2): vgaarb: Move vga_arb_integrated_gpu() earlier in file vgaarb: Log bridge control messages when adding devices
drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++-------------------- 1 file changed, 154 insertions(+), 157 deletions(-)
-- 2.25.1
Hey, Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
[+to Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
Where are we at with this series? Is there anything I can do to move it forward?
Bjorn
Hi Bjorn,
I'm afraid that I don't understand the vga arbiter or the vga code well enough to review.
Could you perhaps find someone who could review?
I see Chen wrote some patches and tested, so perhaps they could?
~Maarten
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.
I'd like to move vgaarb.c to drivers/pci eventually, but there's another initcall ordering snag that needs to be resolved first, so this leaves it where it is.
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...
Bjorn Helgaas (8): vgaarb: Factor out vga_select_framebuffer_device() vgaarb: Factor out default VGA device selection vgaarb: Move framebuffer detection to ADD_DEVICE path vgaarb: Move non-legacy VGA detection to ADD_DEVICE path vgaarb: Move disabled VGA device detection to ADD_DEVICE path vgaarb: Remove empty vga_arb_device_card_gone() vgaarb: Use unsigned format string to print lock counts vgaarb: Replace full MIT license text with SPDX identifier
Huacai Chen (2): vgaarb: Move vga_arb_integrated_gpu() earlier in file vgaarb: Log bridge control messages when adding devices
drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++-------------------- 1 file changed, 154 insertions(+), 157 deletions(-)
-- 2.25.1
On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
[+to Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
Where are we at with this series? Is there anything I can do to move it forward?
I'm afraid that I don't understand the vga arbiter or the vga code well enough to review.
Could you perhaps find someone who could review?
I see Chen wrote some patches and tested, so perhaps they could?
Huacai, any chance you could review this? I'm worried that this series isn't going to go anywhere unless we can find somebody to review it.
Bjorn
Hi, Bjorn,
On Tue, Feb 8, 2022 at 1:59 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
[+to Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
Where are we at with this series? Is there anything I can do to move it forward?
I'm afraid that I don't understand the vga arbiter or the vga code well enough to review.
Could you perhaps find someone who could review?
I see Chen wrote some patches and tested, so perhaps they could?
Huacai, any chance you could review this? I'm worried that this series isn't going to go anywhere unless we can find somebody to review it.
I have reviewed and tested the whole series, it looks good to me (except the naming which has already changed). But I thought I cannot add a "Reviewed-by" because I was originally a co-developer. But if necessary,
Reviewed-by: Huacai Chen chenhuacai@loongson.cn
Huacai
Bjorn
On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
Hey, Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
[+to Maarten, Maxime, Thomas; beginning of thread: https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
On Wed, Jan 05, 2022 at 06:06:48PM -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.
Where are we at with this series? Is there anything I can do to move it forward?
I'm afraid that I don't understand the vga arbiter or the vga code well enough to review.
Could you perhaps find someone who could review?
I see Chen wrote some patches and tested, so perhaps they could?
Hi Maarten,
Huacai Chen did provide his Reviewed-by (although as he noted, the content initially came from him anyway and my contribution was mainly rearranging things into separate patches for each specific case).
Anything else we can to do help here?
Bjorn
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Huacai Chen -
Huacai Chen -
Maarten Lankhorst