From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 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.
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 | 0 5 files changed, 20 insertions(+), 20 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (100%)
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 0c473d75e625..7c9e56d7b857 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 d62c4ac4ae1b..ebe720f69b15 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -29,6 +29,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 100% rename from drivers/gpu/vga/vgaarb.c rename to drivers/pci/vgaarb.c
On 7/22/21 2:29 PM, Bjorn Helgaas wrote:
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.
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 | 0 5 files changed, 20 insertions(+), 20 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (100%)
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 0c473d75e625..7c9e56d7b857 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)
You can drop the PCI part above.
- help
Some "legacy" VGA devices implemented on PCI typically have the samehard-decoded addresses as they did on ISA. When multiple PCI devicesare accessed at same time they need some kind of coordination. Pleasesee Documentation/gpu/vgaarbiter.rst for more details. Select this to
Move that Doc file also...
enable VGA arbiter.
On Thu, Jul 22, 2021 at 02:38:43PM -0700, Randy Dunlap wrote:
On 7/22/21 2:29 PM, Bjorn Helgaas wrote:
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.
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 | 0 5 files changed, 20 insertions(+), 20 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (100%)
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 0c473d75e625..7c9e56d7b857 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)
You can drop the PCI part above.
- help
Some "legacy" VGA devices implemented on PCI typically have the samehard-decoded addresses as they did on ISA. When multiple PCI devicesare accessed at same time they need some kind of coordination. Pleasesee Documentation/gpu/vgaarbiter.rst for more details. Select this toMove that Doc file also...
Thanks! Fixed both locally.
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 949fde433ea2..61b57abcb014 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
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 61b57abcb014..e4153ab70481 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -1022,7 +1022,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
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 e4153ab70481..c984c76b3fd7 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -104,8 +104,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) { @@ -741,10 +739,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); @@ -994,9 +988,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 */ @@ -1013,8 +1005,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; @@ -1358,10 +1348,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
Move vga_arb_integrated_gpu() earlier in file to prepare for future patch. No functional change intended.
[bhelgaas: split to separate patch] Link: https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/pci/vgaarb.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index c984c76b3fd7..1f8fb37be5fa 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -563,6 +563,20 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) } EXPORT_SYMBOL(vga_put);
+#if defined(CONFIG_ACPI) +static bool vga_arb_integrated_gpu(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID); +} +#else +static bool vga_arb_integrated_gpu(struct device *dev) +{ + return false; +} +#endif + /* * 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 @@ -1416,20 +1430,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: Huacai Chen chenhuacai@loongson.cn
Use the vga_default_device() interface consistently instead of directly testing vga_default. No functional change intended.
[bhelgaas: split to separate patch and extended] Link: https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/pci/vgaarb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index 1f8fb37be5fa..a6a5864ff538 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -173,7 +173,7 @@ int vga_remove_vgacon(struct pci_dev *pdev) { int ret = 0;
- if (pdev != vga_default) + if (pdev != vga_default_device()) return 0; vgaarb_info(&pdev->dev, "deactivate vga console\n");
@@ -707,7 +707,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) /* Deal with VGA default device. Use first enabled one * by default if arch doesn't have it's own hook */ - if (vga_default == NULL && + if (!vga_default_device() && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); vga_set_default_device(pdev); @@ -744,7 +744,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) goto bail; }
- if (vga_default == pdev) + if (vga_default_device() == pdev) vga_set_default_device(NULL);
if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
From: Huacai Chen chenhuacai@loongson.cn
If there's no default VGA device, and we find a VGA device that owns the legacy VGA resources, we make that device the default. Split this logic out from vga_arbiter_add_pci_device() into a new function, vga_arb_update_default_device().
[bhelgaas: split another piece to separate patch] Link: https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/pci/vgaarb.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index a6a5864ff538..4cecb599f5ed 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -577,6 +577,21 @@ static bool vga_arb_integrated_gpu(struct device *dev) } #endif
+static void vga_arb_update_default_device(struct vga_device *vgadev) +{ + struct pci_dev *pdev = vgadev->pdev; + + /* + * If we don't have a default VGA device yet, and this device owns + * the legacy VGA resources, make it the default. + */ + if (!vga_default_device() && + ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { + vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); + vga_set_default_device(pdev); + } +} + /* * 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 @@ -704,15 +719,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; }
- /* Deal with VGA default device. Use first enabled one - * by default if arch doesn't have it's own hook - */ - if (!vga_default_device() && - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { - vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); - vga_set_default_device(pdev); - } - + vga_arb_update_default_device(vgadev); vga_arbiter_check_bridge_sharing(vgadev);
/* Add to the list */
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.
[bhelgaas: commit log, split another piece to separate patch, fix list_empty() issue] Link: https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn Signed-off-by: Huacai Chen chenhuacai@loongson.cn Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- 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 4cecb599f5ed..dd07b1c3205f 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -609,8 +609,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; @@ -649,6 +651,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"); }
/* @@ -1527,7 +1534,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) @@ -1543,15 +1549,6 @@ static int __init vga_arb_device_init(void) PCI_ANY_ID, pdev)) != NULL) vga_arbiter_add_pci_device(pdev);
- list_for_each_entry(vgadev, &vga_list, list) { - struct device *dev = &vgadev->pdev->dev; - - if (vgadev->bridge_has_one_vga) - vgaarb_info(dev, "bridge control possible\n"); - else - vgaarb_info(dev, "no bridge control possible\n"); - } - vga_arb_select_default_device();
pr_info("loaded\n");
From: Huacai Chen chenhuacai@loongson.cn
Needs a better subject and a commit log.
Link: https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn --- drivers/pci/vgaarb.c | 158 ++++++++++++++++++------------------------- 1 file changed, 66 insertions(+), 92 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index dd07b1c3205f..0b059a2fc749 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -580,16 +580,79 @@ static bool vga_arb_integrated_gpu(struct device *dev) static void vga_arb_update_default_device(struct vga_device *vgadev) { struct pci_dev *pdev = vgadev->pdev; + struct device *dev = &pdev->dev; + struct vga_device *vgadev_default; +#if defined(CONFIG_X86) || defined(CONFIG_IA64) + int i; + unsigned long flags; + u64 base = screen_info.lfb_base; + u64 size = screen_info.lfb_size; + u64 limit; + resource_size_t start, end; +#endif
/* * If we don't have a default VGA device yet, and this device owns * the legacy VGA resources, make it the default. */ - if (!vga_default_device() && - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { - vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); + if (!vga_default_device()) { + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) + vgaarb_info(dev, "setting as boot VGA device\n"); + else + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); vga_set_default_device(pdev); } + + vgadev_default = vgadev_find(vga_default); + + /* Overridden by a better device */ + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0) + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { + vgaarb_info(dev, "overriding boot VGA device\n"); + vga_set_default_device(pdev); + } + + if (vga_arb_integrated_gpu(dev)) { + vgaarb_info(dev, "overriding boot VGA device\n"); + vga_set_default_device(pdev); + } + +#if defined(CONFIG_X86) || defined(CONFIG_IA64) + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) + base |= (u64)screen_info.ext_lfb_base << 32; + + limit = base + size; + + /* + * Override vga_arbiter_add_pci_device()'s I/O based detection + * as it may take the wrong device (e.g. on Apple system under + * EFI). + * + * Select the device owning the boot framebuffer if there is + * one. + */ + + /* Does firmware framebuffer belong to us? */ + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { + flags = pci_resource_flags(vgadev->pdev, i); + + if ((flags & IORESOURCE_MEM) == 0) + continue; + + start = pci_resource_start(vgadev->pdev, i); + end = pci_resource_end(vgadev->pdev, i); + + if (!start || !end) + continue; + + if (base < start || limit >= end) + continue; + + if (vgadev->pdev != vga_default_device()) + vgaarb_info(dev, "overriding boot device\n"); + vga_set_default_device(vgadev->pdev); + } +#endif }
/* @@ -1444,92 +1507,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 pci_dev *pdev, *found = NULL; - struct vga_device *vgadev; - -#if defined(CONFIG_X86) || defined(CONFIG_IA64) - u64 base = screen_info.lfb_base; - u64 size = screen_info.lfb_size; - u64 limit; - resource_size_t start, end; - unsigned long flags; - int i; - - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) - base |= (u64)screen_info.ext_lfb_base << 32; - - limit = base + size; - - list_for_each_entry(vgadev, &vga_list, list) { - struct device *dev = &vgadev->pdev->dev; - /* - * Override vga_arbiter_add_pci_device()'s I/O based detection - * as it may take the wrong device (e.g. on Apple system under - * EFI). - * - * Select the device owning the boot framebuffer if there is - * one. - */ - - /* Does firmware framebuffer belong to us? */ - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { - flags = pci_resource_flags(vgadev->pdev, i); - - if ((flags & IORESOURCE_MEM) == 0) - continue; - - start = pci_resource_start(vgadev->pdev, i); - end = pci_resource_end(vgadev->pdev, i); - - if (!start || !end) - continue; - - if (base < start || limit >= end) - continue; - - if (!vga_default_device()) - vgaarb_info(dev, "setting as boot device\n"); - else if (vgadev->pdev != vga_default_device()) - vgaarb_info(dev, "overriding boot device\n"); - vga_set_default_device(vgadev->pdev); - } - } -#endif - - if (!vga_default_device()) { - list_for_each_entry_reverse(vgadev, &vga_list, list) { - struct device *dev = &vgadev->pdev->dev; - u16 cmd; - - pdev = vgadev->pdev; - pci_read_config_word(pdev, PCI_COMMAND, &cmd); - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { - found = pdev; - if (vga_arb_integrated_gpu(dev)) - break; - } - } - } - - if (found) { - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n"); - vga_set_default_device(found); - return; - } - - if (!vga_default_device()) { - vgadev = list_first_entry_or_null(&vga_list, - struct vga_device, list); - if (vgadev) { - struct device *dev = &vgadev->pdev->dev; - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); - vga_set_default_device(vgadev->pdev); - } - } -} - static int __init vga_arb_device_init(void) { int rc; @@ -1549,9 +1526,6 @@ static int __init vga_arb_device_init(void) PCI_ANY_ID, pdev)) != NULL) vga_arbiter_add_pci_device(pdev);
- vga_arb_select_default_device(); - - pr_info("loaded\n"); return rc; } subsys_initcall(vga_arb_device_init);
Hi, Bjorn,
On Fri, Jul 23, 2021 at 5:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Thank you for your splitting. Your two questions are answered in the following.
(1) explain why your initcall ordering is unusual. The original problem happens on MIPS. vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(). The order of functions in the same level depends on the Makefile.
TOP level Makefile: drivers-y := drivers/ sound/ .... include arch/$(SRCARCH)/Makefile
drivers/Makefile: obj-$(CONFIG_ACPI) += acpi/ .... obj-y += gpu/
arch/mips/Makefile: drivers-$(CONFIG_PCI) += arch/mips/pci/
This makes pcibios_init() in arch/mips/pci/ placed after vga_arb_device_init() in drivers/gpu. ACPI-based systems have no problems because acpi_init() in drivers/acpi is placed before vga_arb_device_init().
(2) explain the approach, which IIUC is basically to add the vga_arb_select_default_device() functionality to vga_arbiter_add_pci_device(). vga_arb_select_default_device() has only one chance to be called, we want to make it be called every time a new vga device is added. So rename it to vga_arb_update_default_device() and move the callsite to vga_arbiter_add_pci_device().
I think you know all the information which you need now. And you can reorganize the commit message based on the existing one. As English is not my first language, the updated commit message written by me may still not be as good as you want.:)
Huacai
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
-- 2.25.1
On Fri, Jul 23, 2021 at 05:53:36PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Fri, Jul 23, 2021 at 5:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Thank you for your splitting. Your two questions are answered in the following.
(1) explain why your initcall ordering is unusual. The original problem happens on MIPS. vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(). The order of functions in the same level depends on the Makefile.
TOP level Makefile: drivers-y := drivers/ sound/ .... include arch/$(SRCARCH)/Makefile
drivers/Makefile: obj-$(CONFIG_ACPI) += acpi/ .... obj-y += gpu/
arch/mips/Makefile: drivers-$(CONFIG_PCI) += arch/mips/pci/
This makes pcibios_init() in arch/mips/pci/ placed after vga_arb_device_init() in drivers/gpu. ACPI-based systems have no problems because acpi_init() in drivers/acpi is placed before vga_arb_device_init().
Thanks for the above; that was helpful. To summarize:
- On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
[1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eab...
(2) explain the approach, which IIUC is basically to add the vga_arb_select_default_device() functionality to vga_arbiter_add_pci_device(). vga_arb_select_default_device() has only one chance to be called, we want to make it be called every time a new vga device is added. So rename it to vga_arb_update_default_device() and move the callsite to vga_arbiter_add_pci_device().
I think you know all the information which you need now. And you can reorganize the commit message based on the existing one. As English is not my first language, the updated commit message written by me may still not be as good as you want.:)
Huacai
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
-- 2.25.1
Hi, Bjorn,
On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Jul 23, 2021 at 05:53:36PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Fri, Jul 23, 2021 at 5:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Thank you for your splitting. Your two questions are answered in the following.
(1) explain why your initcall ordering is unusual. The original problem happens on MIPS. vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(). The order of functions in the same level depends on the Makefile.
TOP level Makefile: drivers-y := drivers/ sound/ .... include arch/$(SRCARCH)/Makefile
drivers/Makefile: obj-$(CONFIG_ACPI) += acpi/ .... obj-y += gpu/
arch/mips/Makefile: drivers-$(CONFIG_PCI) += arch/mips/pci/
This makes pcibios_init() in arch/mips/pci/ placed after vga_arb_device_init() in drivers/gpu. ACPI-based systems have no problems because acpi_init() in drivers/acpi is placed before vga_arb_device_init().
Thanks for the above; that was helpful. To summarize:
On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
Yes, that's right.
Huacai
[1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eab...
(2) explain the approach, which IIUC is basically to add the vga_arb_select_default_device() functionality to vga_arbiter_add_pci_device(). vga_arb_select_default_device() has only one chance to be called, we want to make it be called every time a new vga device is added. So rename it to vga_arb_update_default_device() and move the callsite to vga_arbiter_add_pci_device().
I think you know all the information which you need now. And you can reorganize the commit message based on the existing one. As English is not my first language, the updated commit message written by me may still not be as good as you want.:)
Huacai
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
-- 2.25.1
On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Jul 23, 2021 at 05:53:36PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Fri, Jul 23, 2021 at 5:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Thank you for your splitting. Your two questions are answered in the following.
(1) explain why your initcall ordering is unusual. The original problem happens on MIPS. vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(). The order of functions in the same level depends on the Makefile.
TOP level Makefile: drivers-y := drivers/ sound/ .... include arch/$(SRCARCH)/Makefile
drivers/Makefile: obj-$(CONFIG_ACPI) += acpi/ .... obj-y += gpu/
arch/mips/Makefile: drivers-$(CONFIG_PCI) += arch/mips/pci/
This makes pcibios_init() in arch/mips/pci/ placed after vga_arb_device_init() in drivers/gpu. ACPI-based systems have no problems because acpi_init() in drivers/acpi is placed before vga_arb_device_init().
Thanks for the above; that was helpful. To summarize:
On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
Yes, that's right.
I think that means I screwed up. I somehow had it in my head that the hot-add path would never set the default VGA device. But that is false.
I still think we should move vgaarb.c to drivers/pci/ and get it more tightly integrated into the PCI core.
BUT that's a lot of churn and obscures the simple change that fixes the problem for you. So I think the first step should be the change to vga_arb_update_default_device() so it sets the default device even when the upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA.
That should be a relatively small change, and I think it's better to make the fix before embarking on major restructuring.
[1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eab...
(2) explain the approach, which IIUC is basically to add the vga_arb_select_default_device() functionality to vga_arbiter_add_pci_device(). vga_arb_select_default_device() has only one chance to be called, we want to make it be called every time a new vga device is added. So rename it to vga_arb_update_default_device() and move the callsite to vga_arbiter_add_pci_device().
I think you know all the information which you need now. And you can reorganize the commit message based on the existing one. As English is not my first language, the updated commit message written by me may still not be as good as you want.:)
Huacai
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
-- 2.25.1
On Tue, Aug 03, 2021 at 12:06:44PM -0500, Bjorn Helgaas wrote:
On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Jul 23, 2021 at 05:53:36PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Fri, Jul 23, 2021 at 5:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Thank you for your splitting. Your two questions are answered in the following.
(1) explain why your initcall ordering is unusual. The original problem happens on MIPS. vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(). The order of functions in the same level depends on the Makefile.
TOP level Makefile: drivers-y := drivers/ sound/ .... include arch/$(SRCARCH)/Makefile
drivers/Makefile: obj-$(CONFIG_ACPI) += acpi/ .... obj-y += gpu/
arch/mips/Makefile: drivers-$(CONFIG_PCI) += arch/mips/pci/
This makes pcibios_init() in arch/mips/pci/ placed after vga_arb_device_init() in drivers/gpu. ACPI-based systems have no problems because acpi_init() in drivers/acpi is placed before vga_arb_device_init().
Thanks for the above; that was helpful. To summarize:
On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
Yes, that's right.
I think that means I screwed up. I somehow had it in my head that the hot-add path would never set the default VGA device. But that is false.
I still think we should move vgaarb.c to drivers/pci/ and get it more tightly integrated into the PCI core.
BUT that's a lot of churn and obscures the simple change that fixes the problem for you. So I think the first step should be the change to vga_arb_update_default_device() so it sets the default device even when the upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA.
That should be a relatively small change, and I think it's better to make the fix before embarking on major restructuring.
To make sure this doesn't get lost: I'm hoping you can separate out and post the small patch to vga_arb_update_default_device().
I can look at the move/restructure stuff later.
[1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eab...
(2) explain the approach, which IIUC is basically to add the vga_arb_select_default_device() functionality to vga_arbiter_add_pci_device(). vga_arb_select_default_device() has only one chance to be called, we want to make it be called every time a new vga device is added. So rename it to vga_arb_update_default_device() and move the callsite to vga_arbiter_add_pci_device().
I think you know all the information which you need now. And you can reorganize the commit message based on the existing one. As English is not my first language, the updated commit message written by me may still not be as good as you want.:)
Huacai
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
-- 2.25.1
On Mon, Aug 09, 2021 at 01:59:01PM -0500, Bjorn Helgaas wrote:
On Tue, Aug 03, 2021 at 12:06:44PM -0500, Bjorn Helgaas wrote:
On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote:
On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas helgaas@kernel.org wrote:
Thanks for the above; that was helpful. To summarize:
On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
Yes, that's right.
I think that means I screwed up. I somehow had it in my head that the hot-add path would never set the default VGA device. But that is false.
I still think we should move vgaarb.c to drivers/pci/ and get it more tightly integrated into the PCI core.
BUT that's a lot of churn and obscures the simple change that fixes the problem for you. So I think the first step should be the change to vga_arb_update_default_device() so it sets the default device even when the upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA.
That should be a relatively small change, and I think it's better to make the fix before embarking on major restructuring.
To make sure this doesn't get lost: I'm hoping you can separate out and post the small patch to vga_arb_update_default_device().
I can look at the move/restructure stuff later.
What's happening with this? I'm still assuming you can post a small patch to vga_arb_update_default_device() that's suitable for v5.15, Huacai.
Otherwise I'm afraid we won't make any forward progress this cycle.
Bjorn
Hi, Bjorn,
On Fri, Aug 20, 2021 at 5:52 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Mon, Aug 09, 2021 at 01:59:01PM -0500, Bjorn Helgaas wrote:
On Tue, Aug 03, 2021 at 12:06:44PM -0500, Bjorn Helgaas wrote:
On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote:
On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas helgaas@kernel.org wrote:
Thanks for the above; that was helpful. To summarize:
On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
Yes, that's right.
I think that means I screwed up. I somehow had it in my head that the hot-add path would never set the default VGA device. But that is false.
I still think we should move vgaarb.c to drivers/pci/ and get it more tightly integrated into the PCI core.
BUT that's a lot of churn and obscures the simple change that fixes the problem for you. So I think the first step should be the change to vga_arb_update_default_device() so it sets the default device even when the upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA.
That should be a relatively small change, and I think it's better to make the fix before embarking on major restructuring.
To make sure this doesn't get lost: I'm hoping you can separate out and post the small patch to vga_arb_update_default_device().
I can look at the move/restructure stuff later.
What's happening with this? I'm still assuming you can post a small patch to vga_arb_update_default_device() that's suitable for v5.15, Huacai.
Otherwise I'm afraid we won't make any forward progress this cycle.
In my opinion these patches (including the last one) are small enough, so can I update the commit message of the last one and keep the patch content as is and send V3?
Huacai
Bjorn
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Huacai Chen -
Randy Dunlap