[PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

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@loongson.cn/
[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%)

-- 
2.25.1

[PATCH v2 1/9] PCI/VGA: Move vgaarb to drivers/pci

[Bjorn Helgaas]

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
-- 
2.25.1

Re: [PATCH v2 1/9] PCI/VGA: Move vgaarb to drivers/pci

[Randy Dunlap]

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 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

Move that Doc file also...

> +	  enable VGA arbiter.
> +


-- 
~Randy

Re: [PATCH v2 1/9] PCI/VGA: Move vgaarb to drivers/pci

[Bjorn Helgaas]

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 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
> 
> Move that Doc file also...

Thanks!  Fixed both locally.

[PATCH v2 2/9] PCI/VGA: Replace full MIT license text with SPDX identifier

[Bjorn Helgaas]

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
-- 
2.25.1

[PATCH v2 3/9] PCI/VGA: Use unsigned format string to print lock counts

[Bjorn Helgaas]

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),
-- 
2.25.1

[PATCH v2 4/9] PCI/VGA: Remove empty vga_arb_device_card_gone()

[Bjorn Helgaas]

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
-- 
2.25.1

[PATCH v2 5/9] PCI/VGA: Move vga_arb_integrated_gpu() earlier in file

[Bjorn Helgaas]

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;
-- 
2.25.1

[PATCH v2 6/9] PCI/VGA: Prefer vga_default_device()

[Bjorn Helgaas]

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))
-- 
2.25.1

[PATCH v2 7/9] PCI/VGA: Split out vga_arb_update_default_device()

[Bjorn Helgaas]

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 */
-- 
2.25.1

[PATCH v2 8/9] PCI/VGA: Log bridge control messages when adding devices

[Bjorn Helgaas]

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");
-- 
2.25.1

[PATCH v2 9/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

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);
-- 
2.25.1

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Christoph Hellwig]

On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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].

FYI, I have a bunch of changes to this code that the drm maintainers
picked up.  They should show up in the next linux-next I think.

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Daniel Vetter]

On Fri, Jul 23, 2021 at 06:51:59AM +0100, Christoph Hellwig wrote:
> On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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].
> 
> FYI, I have a bunch of changes to this code that the drm maintainers
> picked up.  They should show up in the next linux-next I think.

Yeah I think for merging I think there'll be two options:

- We also merge this series through drm-misc-next to avoid conflicts, but
  anything after that will (i.e. from 5.16-rc1 onwards) will go in through
  the pci tree.

- You also merge Christoph's series, and we tell Linus to ignore the
  vgaarb changes that also come in through drm-next pull.

It's a non-rebasing tree so taking them out isn't an option, and reverting
feels silly. Either of the above is fine with me.

Also I just noticed that the scrip has gone wrong for drm-misc-next and
it's not actually yet in linux-next. I'll sort that out. Ok I just did
sort that out while I forgot this reply draft here, one of our committers
pushed a patch to the wrong branch. Luckily it was a broken one and the
right fix is in the right branch (and already in Linus' tree), so a hard
reset was all it took. So should be all in linux-next on the next update.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

On Fri, Jul 23, 2021 at 10:27:08AM +0200, Daniel Vetter wrote:
> On Fri, Jul 23, 2021 at 06:51:59AM +0100, Christoph Hellwig wrote:
> > On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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].
> > 
> > FYI, I have a bunch of changes to this code that the drm maintainers
> > picked up.  They should show up in the next linux-next I think.
> 
> Yeah I think for merging I think there'll be two options:
> 
> - We also merge this series through drm-misc-next to avoid conflicts, but
>   anything after that will (i.e. from 5.16-rc1 onwards) will go in through
>   the pci tree.
> 
> - You also merge Christoph's series, and we tell Linus to ignore the
>   vgaarb changes that also come in through drm-next pull.
> 
> It's a non-rebasing tree so taking them out isn't an option, and reverting
> feels silly. Either of the above is fine with me.

Seems easiest/cleanest if I just fix this up so it applies on top of
drm-misc-next, e.g., on top of this:

  474596fc749c ("dt-bindings: display: simple-bridge: Add corpro,gm7123 compatible")

I'll post a v3 after that rebase and working on the commit log from
Huacai.

Bjorn

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Daniel Vetter]

On Fri, Jul 23, 2021 at 05:43:35PM -0500, Bjorn Helgaas wrote:
> On Fri, Jul 23, 2021 at 10:27:08AM +0200, Daniel Vetter wrote:
> > On Fri, Jul 23, 2021 at 06:51:59AM +0100, Christoph Hellwig wrote:
> > > On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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].
> > > 
> > > FYI, I have a bunch of changes to this code that the drm maintainers
> > > picked up.  They should show up in the next linux-next I think.
> > 
> > Yeah I think for merging I think there'll be two options:
> > 
> > - We also merge this series through drm-misc-next to avoid conflicts, but
> >   anything after that will (i.e. from 5.16-rc1 onwards) will go in through
> >   the pci tree.

I meant 5.15-rc1 here ofc. Living a bit too much in the future :-)

> > - You also merge Christoph's series, and we tell Linus to ignore the
> >   vgaarb changes that also come in through drm-next pull.
> > 
> > It's a non-rebasing tree so taking them out isn't an option, and reverting
> > feels silly. Either of the above is fine with me.
> 
> Seems easiest/cleanest if I just fix this up so it applies on top of
> drm-misc-next, e.g., on top of this:
> 
>   474596fc749c ("dt-bindings: display: simple-bridge: Add corpro,gm7123 compatible")
> 
> I'll post a v3 after that rebase and working on the commit log from
> Huacai.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Huacai Chen]

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@loongson.cn/
> [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
>

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

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@loongson.cn/
> > [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-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com

> (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
> >

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Huacai Chen]

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@loongson.cn/
> > > [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-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com
>
> > (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
> > >

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

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@loongson.cn/
> > > > [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-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com
> >
> > > (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
> > > >

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

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@loongson.cn/
> > > > > [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-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com
> > >
> > > > (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
> > > > >

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Bjorn Helgaas]

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

Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection

[Huacai Chen]

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