linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 00/10] vgaarb: Rework default VGA device selection
@ 2022-01-06  0:06 Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 01/10] vgaarb: Move vga_arb_integrated_gpu() earlier in file Bjorn Helgaas
                   ` (11 more replies)
  0 siblings, 12 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

Current default VGA device selection fails in some cases because part of it
is done in the vga_arb_device_init() subsys_initcall, and some arches
enumerate PCI devices in pcibios_init(), which runs *after* that.

For example:

  - On BMC system, the AST2500 bridge [1a03:1150] does not implement
    PCI_BRIDGE_CTL_VGA.  This is perfectly legal but means the legacy VGA
    resources won't reach downstream devices unless they're included in the
    usual bridge windows.

  - vga_arb_select_default_device() will set a device below such a bridge
    as the default VGA device as long as it has PCI_COMMAND_IO and
    PCI_COMMAND_MEMORY enabled.

  - vga_arbiter_add_pci_device() is called for every VGA device, either at
    boot-time or at hot-add time, and it will also set the device as the
    default VGA device, but ONLY if all bridges leading to it implement
    PCI_BRIDGE_CTL_VGA.

  - This difference between vga_arb_select_default_device() and
    vga_arbiter_add_pci_device() means that a device below an AST2500 or
    similar bridge can only be set as the default if it is enumerated
    before vga_arb_device_init().

  - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
    runs before vga_arb_device_init().

  - On non-ACPI systems, like on MIPS system, they are enumerated by
    pcibios_init(), which typically runs *after* vga_arb_device_init().

This series consolidates all the default VGA device selection in
vga_arbiter_add_pci_device(), which is always called after enumerating a
PCI device.

Almost all the work here is Huacai's.  I restructured it a little bit and
added a few trivial patches on top.

I'd like to move vgaarb.c to drivers/pci eventually, but there's another
initcall ordering snag that needs to be resolved first, so this leaves 
it where it is.

Bjorn

Version history:
V0 original implementation as final quirk to set default device.
https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn

V1 rework vgaarb to do all default device selection in
vga_arbiter_add_pci_device().
https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn

V2 move arbiter to PCI subsystem, fix nits.
https://lore.kernel.org/r/20210722212920.347118-1-helgaas@kernel.org

V3 rewrite the commit log of the last patch (which is also summarized
by Bjorn).
https://lore.kernel.org/r/20210820100832.663931-1-chenhuacai@loongson.cn

V4 split the last patch to two steps.
https://lore.kernel.org/r/20210827083129.2781420-1-chenhuacai@loongson.cn

V5 split Patch-9 again and sort the patches.
https://lore.kernel.org/r/20210911093056.1555274-1-chenhuacai@loongson.cn

V6 split Patch-5 again and sort the patches again.
https://lore.kernel.org/r/20210916082941.3421838-1-chenhuacai@loongson.cn

V7 stop moving vgaarb to drivers/pci because of ordering issues with
misc_init().
https://lore.kernel.org/r/20211015061512.2941859-1-chenhuacai@loongson.cn
https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com


Bjorn Helgaas (8):
  vgaarb: Factor out vga_select_framebuffer_device()
  vgaarb: Factor out default VGA device selection
  vgaarb: Move framebuffer detection to ADD_DEVICE path
  vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
  vgaarb: Move disabled VGA device detection to ADD_DEVICE path
  vgaarb: Remove empty vga_arb_device_card_gone()
  vgaarb: Use unsigned format string to print lock counts
  vgaarb: Replace full MIT license text with SPDX identifier

Huacai Chen (2):
  vgaarb: Move vga_arb_integrated_gpu() earlier in file
  vgaarb: Log bridge control messages when adding devices

 drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
 1 file changed, 154 insertions(+), 157 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v8 01/10] vgaarb: Move vga_arb_integrated_gpu() earlier in file
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 02/10] vgaarb: Factor out vga_select_framebuffer_device() Bjorn Helgaas
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, 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: pull #ifdefs inside function]
Link: https://lore.kernel.org/r/20211015061512.2941859-3-chenhuacai@loongson.cn
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/gpu/vga/vgaarb.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 569930552957..ef5ad4c432f5 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -565,6 +565,17 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
 }
 EXPORT_SYMBOL(vga_put);
 
+static bool vga_arb_integrated_gpu(struct device *dev)
+{
+#if defined(CONFIG_ACPI)
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
+#else
+	return false;
+#endif
+}
+
 /*
  * Rules for using a bridge to control a VGA descendant decoding: if a bridge
  * has only one VGA descendant then it can be used to control the VGA routing
@@ -1430,20 +1441,6 @@ static struct miscdevice vga_arb_device = {
 	MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
 };
 
-#if defined(CONFIG_ACPI)
-static bool vga_arb_integrated_gpu(struct device *dev)
-{
-	struct acpi_device *adev = ACPI_COMPANION(dev);
-
-	return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
-}
-#else
-static bool vga_arb_integrated_gpu(struct device *dev)
-{
-	return false;
-}
-#endif
-
 static void __init vga_arb_select_default_device(void)
 {
 	struct pci_dev *pdev, *found = NULL;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 02/10] vgaarb: Factor out vga_select_framebuffer_device()
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 01/10] vgaarb: Move vga_arb_integrated_gpu() earlier in file Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 03/10] vgaarb: Factor out default VGA device selection Bjorn Helgaas
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas, Bruno Prémont

From: Bjorn Helgaas <bhelgaas@google.com>

On x86 and ia64, if a VGA device BARs include a framebuffer reported by
platform firmware, we select the device as the default VGA device.  Factor
this code to a separate function.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/gpu/vga/vgaarb.c | 99 +++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index ef5ad4c432f5..36d9140c64f6 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -565,6 +565,58 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
 }
 EXPORT_SYMBOL(vga_put);
 
+static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
+{
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+	struct device *dev = &pdev->dev;
+	u64 base = screen_info.lfb_base;
+	u64 size = screen_info.lfb_size;
+	u64 limit;
+	resource_size_t start, end;
+	unsigned long flags;
+	int i;
+
+	/* Select the device owning the boot framebuffer if there is one */
+
+	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+		base |= (u64)screen_info.ext_lfb_base << 32;
+
+	limit = base + size;
+
+	/*
+	 * Override vga_arbiter_add_pci_device()'s I/O based detection
+	 * as it may take the wrong device (e.g. on Apple system under
+	 * EFI).
+	 *
+	 * Select the device owning the boot framebuffer if there is
+	 * one.
+	 */
+
+	/* Does firmware framebuffer belong to us? */
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		flags = pci_resource_flags(pdev, i);
+
+		if ((flags & IORESOURCE_MEM) == 0)
+			continue;
+
+		start = pci_resource_start(pdev, i);
+		end  = pci_resource_end(pdev, i);
+
+		if (!start || !end)
+			continue;
+
+		if (base < start || limit >= end)
+			continue;
+
+		if (!vga_default_device())
+			vgaarb_info(dev, "setting as boot device\n");
+		else if (pdev != vga_default_device())
+			vgaarb_info(dev, "overriding boot device\n");
+		vga_set_default_device(pdev);
+	}
+#endif
+}
+
 static bool vga_arb_integrated_gpu(struct device *dev)
 {
 #if defined(CONFIG_ACPI)
@@ -1446,54 +1498,9 @@ static void __init vga_arb_select_default_device(void)
 	struct pci_dev *pdev, *found = NULL;
 	struct vga_device *vgadev;
 
-#if defined(CONFIG_X86) || defined(CONFIG_IA64)
-	u64 base = screen_info.lfb_base;
-	u64 size = screen_info.lfb_size;
-	u64 limit;
-	resource_size_t start, end;
-	unsigned long flags;
-	int i;
-
-	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
-		base |= (u64)screen_info.ext_lfb_base << 32;
-
-	limit = base + size;
-
 	list_for_each_entry(vgadev, &vga_list, list) {
-		struct device *dev = &vgadev->pdev->dev;
-		/*
-		 * Override vga_arbiter_add_pci_device()'s I/O based detection
-		 * as it may take the wrong device (e.g. on Apple system under
-		 * EFI).
-		 *
-		 * Select the device owning the boot framebuffer if there is
-		 * one.
-		 */
-
-		/* Does firmware framebuffer belong to us? */
-		for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
-			flags = pci_resource_flags(vgadev->pdev, i);
-
-			if ((flags & IORESOURCE_MEM) == 0)
-				continue;
-
-			start = pci_resource_start(vgadev->pdev, i);
-			end  = pci_resource_end(vgadev->pdev, i);
-
-			if (!start || !end)
-				continue;
-
-			if (base < start || limit >= end)
-				continue;
-
-			if (!vga_default_device())
-				vgaarb_info(dev, "setting as boot device\n");
-			else if (vgadev->pdev != vga_default_device())
-				vgaarb_info(dev, "overriding boot device\n");
-			vga_set_default_device(vgadev->pdev);
-		}
+		vga_select_framebuffer_device(vgadev->pdev);
 	}
-#endif
 
 	if (!vga_default_device()) {
 		list_for_each_entry_reverse(vgadev, &vga_list, list) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 03/10] vgaarb: Factor out default VGA device selection
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 01/10] vgaarb: Move vga_arb_integrated_gpu() earlier in file Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 02/10] vgaarb: Factor out vga_select_framebuffer_device() Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path Bjorn Helgaas
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas, Huacai Chen

From: Bjorn Helgaas <bhelgaas@google.com>

Default VGA device selection fails when PCI devices are enumerated after
the vga_arb_device_init() subsys_initcall.

vga_arbiter_add_pci_device() selects the first fully enabled device to
which legacy VGA resources are routed as the default VGA device.  This is
an ADD_DEVICE notifier, so it runs after every PCI device is enumerated.

vga_arb_select_default_device() may select framebuffer devices, partially
enabled GPUs, or non-legacy devices that don't have legacy VGA resources
routed to them as the default VGA device.  But this only happens once, from
the vga_arb_device_init() subsys_initcall, so it doesn't consider devices
enumerated after that:

  acpi_init
    acpi_scan_init
      acpi_pci_root_init         # PCI device enumeration (ACPI systems)

  vga_arb_device_init
    for_each_pci_device
      vga_arbiter_add_pci_device      # ADD_DEVICE notifier
        if (VGA-owner)
          vga_set_default_device      <-- set default VGA
    vga_arb_select_default_device     # only called ONCE
      for_each_vga_device
        if (framebuffer)
          vga_set_default_device      <-- set default VGA to framebuffer
      if (!vga_default_device())
        if (non-legacy, integrated GPU, etc)
          vga_set_default_device      <-- set default VGA
      if (!vga_default_device())
        vga_set_default_device        <-- set default VGA

  pcibios_init
    pcibios_scanbus              # PCI device enumeration (non-ACPI systems)
      ...
        vga_arbiter_add_pci_device    # ADD_DEVICE notification
          if (VGA-owner)
            vga_set_default_device    <-- set default VGA

Note that on non-ACPI systems, vga_arb_select_default_device() runs before
pcibios_init(), so it sees no VGA devices and can never set a framebuffer
device, a non-legacy integrated GPU, etc., as the default device.

Factor out the default VGA device selection to vga_is_boot_device(), called
from vga_arbiter_add_pci_device().

Then we can migrate the default device selection from
vga_arb_select_default_device() to the vga_arbiter_add_pci_device() path.

Link: https://lore.kernel.org/r/20211015061512.2941859-4-chenhuacai@loongson.cn
Based-on-patch-by: Huacai Chen <chenhuacai@kernel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/gpu/vga/vgaarb.c | 45 ++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 36d9140c64f6..b0ae0f177c6f 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -628,6 +628,41 @@ static bool vga_arb_integrated_gpu(struct device *dev)
 #endif
 }
 
+/*
+ * Return true if vgadev is a better default VGA device than the best one
+ * we've seen so far.
+ */
+static bool vga_is_boot_device(struct vga_device *vgadev)
+{
+	struct vga_device *boot_vga = vgadev_find(vga_default_device());
+
+	/*
+	 * We select the default VGA device in this order:
+	 *   Firmware framebuffer (see vga_arb_select_default_device())
+	 *   Legacy VGA device (owns VGA_RSRC_LEGACY_MASK)
+	 *   Non-legacy integrated device (see vga_arb_select_default_device())
+	 *   Non-legacy discrete device (see vga_arb_select_default_device())
+	 *   Other device (see vga_arb_select_default_device())
+	 */
+
+	/*
+	 * A legacy VGA device has MEM and IO enabled and any bridges
+	 * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
+	 * resources ([mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], etc) are
+	 * routed to it.
+	 *
+	 * We use the first one we find, so if we've already found one,
+	 * vgadev is no better.
+	 */
+	if (boot_vga)
+		return false;
+
+	if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
+		return true;
+
+	return false;
+}
+
 /*
  * Rules for using a bridge to control a VGA descendant decoding: if a bridge
  * has only one VGA descendant then it can be used to control the VGA routing
@@ -755,12 +790,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 		bus = bus->parent;
 	}
 
-	/* Deal with VGA default device. Use first enabled one
-	 * by default if arch doesn't have it's own hook
-	 */
-	if (vga_default == NULL &&
-	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
-		vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
+	if (vga_is_boot_device(vgadev)) {
+		vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n",
+			    vga_default_device() ?
+			    " (overriding previous)" : "");
 		vga_set_default_device(pdev);
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (2 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 03/10] vgaarb: Factor out default VGA device selection Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  6:44   ` Huacai Chen
  2022-01-06  0:06 ` [PATCH v8 05/10] vgaarb: Move non-legacy VGA " Bjorn Helgaas
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas, Bruno Prémont

From: Bjorn Helgaas <bhelgaas@google.com>

Previously we selected a device that owns the boot framebuffer as the
default device in vga_arb_select_default_device().  This was only done in
the vga_arb_device_init() subsys_initcall, so devices enumerated later,
e.g., by pcibios_init(), were not eligible.

Fix this by moving the framebuffer device selection from
vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
called after every PCI device is enumerated, either by the
vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.

Note that if vga_arb_select_default_device() found a device owning the boot
framebuffer, it unconditionally set it to be the default VGA device, and no
subsequent device could replace it.

Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
Based-on-patch-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index b0ae0f177c6f..aefa4f406f7d 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -72,6 +72,7 @@ struct vga_device {
 	unsigned int io_norm_cnt;	/* normal IO count */
 	unsigned int mem_norm_cnt;	/* normal MEM count */
 	bool bridge_has_one_vga;
+	bool is_framebuffer;	/* BAR covers firmware framebuffer */
 	unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
 };
 
@@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
 }
 EXPORT_SYMBOL(vga_put);
 
-static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
+static bool vga_is_framebuffer_device(struct pci_dev *pdev)
 {
 #if defined(CONFIG_X86) || defined(CONFIG_IA64)
-	struct device *dev = &pdev->dev;
 	u64 base = screen_info.lfb_base;
 	u64 size = screen_info.lfb_size;
 	u64 limit;
@@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
 
 	limit = base + size;
 
-	/*
-	 * Override vga_arbiter_add_pci_device()'s I/O based detection
-	 * as it may take the wrong device (e.g. on Apple system under
-	 * EFI).
-	 *
-	 * Select the device owning the boot framebuffer if there is
-	 * one.
-	 */
-
 	/* Does firmware framebuffer belong to us? */
 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
 		flags = pci_resource_flags(pdev, i);
@@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
 		if (base < start || limit >= end)
 			continue;
 
-		if (!vga_default_device())
-			vgaarb_info(dev, "setting as boot device\n");
-		else if (pdev != vga_default_device())
-			vgaarb_info(dev, "overriding boot device\n");
-		vga_set_default_device(pdev);
+		return true;
 	}
 #endif
+	return false;
 }
 
 static bool vga_arb_integrated_gpu(struct device *dev)
@@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
 static bool vga_is_boot_device(struct vga_device *vgadev)
 {
 	struct vga_device *boot_vga = vgadev_find(vga_default_device());
+	struct pci_dev *pdev = vgadev->pdev;
 
 	/*
 	 * We select the default VGA device in this order:
@@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 	 *   Other device (see vga_arb_select_default_device())
 	 */
 
+	/*
+	 * We always prefer a firmware framebuffer, so if we've already
+	 * found one, there's no need to consider vgadev.
+	 */
+	if (boot_vga && boot_vga->is_framebuffer)
+		return false;
+
+	if (vga_is_framebuffer_device(pdev)) {
+		vgadev->is_framebuffer = true;
+		return true;
+	}
+
 	/*
 	 * A legacy VGA device has MEM and IO enabled and any bridges
 	 * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
@@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
 	struct pci_dev *pdev, *found = NULL;
 	struct vga_device *vgadev;
 
-	list_for_each_entry(vgadev, &vga_list, list) {
-		vga_select_framebuffer_device(vgadev->pdev);
-	}
-
 	if (!vga_default_device()) {
 		list_for_each_entry_reverse(vgadev, &vga_list, list) {
 			struct device *dev = &vgadev->pdev->dev;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 05/10] vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (3 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 06/10] vgaarb: Move disabled VGA device " Bjorn Helgaas
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas, Huacai Chen, Daniel Axtens,
	Zhou Wang

From: Bjorn Helgaas <bhelgaas@google.com>

a37c0f48950b ("vgaarb: Select a default VGA device even if there's no
legacy VGA") extended the vga_arb_device_init() subsys_initcall so it could
select a non-legacy VGA device as the default.

That failed to consider that PCI devices may be enumerated after
vga_arb_device_init(), e.g., hot-added devices or non-ACPI systems that do
PCI enumeration in pcibios_init().  Devices found then could never be
selected as the default.

One system where this is a problem is the MIPS-based Loongson where an
ASpeed AST2500 VGA device is behind a bridge that doesn't implement the VGA
Enable bit, so legacy resources are not routed to the VGA device. [1]

Fix this by moving the non-legacy VGA device selection from
vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
called after every PCI device is enumerated, either by the
vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.

[1] https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn

Link: https://lore.kernel.org/r/20211015061512.2941859-5-chenhuacai@loongson.cn
Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
Based-on-patch-by: Huacai Chen <chenhuacai@kernel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Daniel Axtens <dja@axtens.net>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/gpu/vga/vgaarb.c | 54 ++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index aefa4f406f7d..123b81628061 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -624,6 +624,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 {
 	struct vga_device *boot_vga = vgadev_find(vga_default_device());
 	struct pci_dev *pdev = vgadev->pdev;
+	u16 cmd, boot_cmd;
 
 	/*
 	 * We select the default VGA device in this order:
@@ -661,6 +662,37 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 	if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
 		return true;
 
+	/*
+	 * If we haven't found a legacy VGA device, accept a non-legacy
+	 * device.  It may have either IO or MEM enabled, and bridges may
+	 * not have PCI_BRIDGE_CTL_VGA enabled, so it may not be able to
+	 * use legacy VGA resources.  Prefer an integrated GPU over others.
+	 */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+
+		/*
+		 * An integrated GPU overrides a previous non-legacy
+		 * device.  We expect only a single integrated GPU, but if
+		 * there are more, we use the *last* because that was the
+		 * previous behavior.
+		 */
+		if (vga_arb_integrated_gpu(&pdev->dev))
+			return true;
+
+		/*
+		 * We prefer the first non-legacy discrete device we find.
+		 * If we already found one, vgadev is no better.
+		 */
+		if (boot_vga) {
+			pci_read_config_word(boot_vga->pdev, PCI_COMMAND,
+					     &boot_cmd);
+			if (boot_cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
+				return false;
+		}
+		return true;
+	}
+
 	return false;
 }
 
@@ -1529,30 +1561,8 @@ static struct miscdevice vga_arb_device = {
 
 static void __init vga_arb_select_default_device(void)
 {
-	struct pci_dev *pdev, *found = NULL;
 	struct vga_device *vgadev;
 
-	if (!vga_default_device()) {
-		list_for_each_entry_reverse(vgadev, &vga_list, list) {
-			struct device *dev = &vgadev->pdev->dev;
-			u16 cmd;
-
-			pdev = vgadev->pdev;
-			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
-			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
-				found = pdev;
-				if (vga_arb_integrated_gpu(dev))
-					break;
-			}
-		}
-	}
-
-	if (found) {
-		vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
-		vga_set_default_device(found);
-		return;
-	}
-
 	if (!vga_default_device()) {
 		vgadev = list_first_entry_or_null(&vga_list,
 						  struct vga_device, list);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 06/10] vgaarb: Move disabled VGA device detection to ADD_DEVICE path
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (4 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 05/10] vgaarb: Move non-legacy VGA " Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 07/10] vgaarb: Remove empty vga_arb_device_card_gone() Bjorn Helgaas
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas, Huacai Chen, Daniel Axtens,
	Zhou Wang

From: Bjorn Helgaas <bhelgaas@google.com>

a37c0f48950b ("vgaarb: Select a default VGA device even if there's no
legacy VGA") extended the vga_arb_device_init() subsys_initcall so that if
there are no other eligible devices, it could select a disabled VGA device
as the default.

Move this detection from vga_arb_select_default_device() to
vga_arbiter_add_pci_device() so every device, even those hot-added or
enumerated after vga_arb_device_init() is eligible for selection as the
default VGA device.

Link: https://lore.kernel.org/r/20211015061512.2941859-5-chenhuacai@loongson.cn
Based-on-patch-by: Huacai Chen <chenhuacai@kernel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Daniel Axtens <dja@axtens.net>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/gpu/vga/vgaarb.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 123b81628061..ad548917e602 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -656,7 +656,8 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 	 * We use the first one we find, so if we've already found one,
 	 * vgadev is no better.
 	 */
-	if (boot_vga)
+	if (boot_vga &&
+	    (boot_vga->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
 		return false;
 
 	if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
@@ -693,6 +694,13 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 		return true;
 	}
 
+	/*
+	 * vgadev has neither IO nor MEM enabled.  If we haven't found any
+	 * other VGA devices, it is the best candidate so far.
+	 */
+	if (!boot_vga)
+		return true;
+
 	return false;
 }
 
@@ -1559,21 +1567,6 @@ static struct miscdevice vga_arb_device = {
 	MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
 };
 
-static void __init vga_arb_select_default_device(void)
-{
-	struct vga_device *vgadev;
-
-	if (!vga_default_device()) {
-		vgadev = list_first_entry_or_null(&vga_list,
-						  struct vga_device, list);
-		if (vgadev) {
-			struct device *dev = &vgadev->pdev->dev;
-			vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
-			vga_set_default_device(vgadev->pdev);
-		}
-	}
-}
-
 static int __init vga_arb_device_init(void)
 {
 	int rc;
@@ -1603,8 +1596,6 @@ static int __init vga_arb_device_init(void)
 			vgaarb_info(dev, "no bridge control possible\n");
 	}
 
-	vga_arb_select_default_device();
-
 	pr_info("loaded\n");
 	return rc;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 07/10] vgaarb: Remove empty vga_arb_device_card_gone()
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (5 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 06/10] vgaarb: Move disabled VGA device " Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 08/10] vgaarb: Log bridge control messages when adding devices Bjorn Helgaas
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, 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/gpu/vga/vgaarb.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index ad548917e602..455cf048fae8 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -123,8 +123,6 @@ static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
 /* this is only used a cookie - it should not be dereferenced */
 static struct pci_dev *vga_default;
 
-static void vga_arb_device_card_gone(struct pci_dev *pdev);
-
 /* Find somebody in our list */
 static struct vga_device *vgadev_find(struct pci_dev *pdev)
 {
@@ -878,10 +876,6 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 	/* Remove entry from list */
 	list_del(&vgadev->list);
 	vga_count--;
-	/* Notify userland driver that the device is gone so it discards
-	 * it's copies of the pci_dev pointer
-	 */
-	vga_arb_device_card_gone(pdev);
 
 	/* Wake up all possible waiters */
 	wake_up_all(&vga_wait_queue);
@@ -1131,9 +1125,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
 	if (lbuf == NULL)
 		return -ENOMEM;
 
-	/* Shields against vga_arb_device_card_gone (pci_dev going
-	 * away), and allows access to vga list
-	 */
+	/* Protects vga_list */
 	spin_lock_irqsave(&vga_lock, flags);
 
 	/* If we are targeting the default, use it */
@@ -1150,8 +1142,6 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
 		/* Wow, it's not in the list, that shouldn't happen,
 		 * let's fix us up and return invalid card
 		 */
-		if (pdev == priv->target)
-			vga_arb_device_card_gone(pdev);
 		spin_unlock_irqrestore(&vga_lock, flags);
 		len = sprintf(lbuf, "invalid");
 		goto done;
@@ -1495,10 +1485,6 @@ static int vga_arb_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static void vga_arb_device_card_gone(struct pci_dev *pdev)
-{
-}
-
 /*
  * callback any registered clients to let them know we have a
  * change in VGA cards
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 08/10] vgaarb: Log bridge control messages when adding devices
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (6 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 07/10] vgaarb: Remove empty vga_arb_device_card_gone() Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 09/10] vgaarb: Use unsigned format string to print lock counts Bjorn Helgaas
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, 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.

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/gpu/vga/vgaarb.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 455cf048fae8..b7e6c1228fff 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -719,8 +719,10 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
 
 	vgadev->bridge_has_one_vga = true;
 
-	if (list_empty(&vga_list))
+	if (list_empty(&vga_list)) {
+		vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
 		return;
+	}
 
 	/* okay iterate the new devices bridge hierarachy */
 	new_bus = vgadev->pdev->bus;
@@ -759,6 +761,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
 		}
 		new_bus = new_bus->parent;
 	}
+
+	if (vgadev->bridge_has_one_vga)
+		vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
+	else
+		vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
 }
 
 /*
@@ -1557,7 +1564,6 @@ static int __init vga_arb_device_init(void)
 {
 	int rc;
 	struct pci_dev *pdev;
-	struct vga_device *vgadev;
 
 	rc = misc_register(&vga_arb_device);
 	if (rc < 0)
@@ -1573,15 +1579,6 @@ static int __init vga_arb_device_init(void)
 			       PCI_ANY_ID, pdev)) != NULL)
 		vga_arbiter_add_pci_device(pdev);
 
-	list_for_each_entry(vgadev, &vga_list, list) {
-		struct device *dev = &vgadev->pdev->dev;
-
-		if (vgadev->bridge_has_one_vga)
-			vgaarb_info(dev, "bridge control possible\n");
-		else
-			vgaarb_info(dev, "no bridge control possible\n");
-	}
-
 	pr_info("loaded\n");
 	return rc;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 09/10] vgaarb: Use unsigned format string to print lock counts
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (7 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 08/10] vgaarb: Log bridge control messages when adding devices Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06  0:06 ` [PATCH v8 10/10] vgaarb: Replace full MIT license text with SPDX identifier Bjorn Helgaas
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, 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/gpu/vga/vgaarb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index b7e6c1228fff..95e37817074e 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1156,7 +1156,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
 
 	/* Fill the buffer with infos */
 	len = snprintf(lbuf, 1024,
-		       "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%d:%d)\n",
+		       "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%u:%u)\n",
 		       vga_decode_count, pci_name(pdev),
 		       vga_iostate_to_str(vgadev->decodes),
 		       vga_iostate_to_str(vgadev->owns),
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v8 10/10] vgaarb: Replace full MIT license text with SPDX identifier
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (8 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 09/10] vgaarb: Use unsigned format string to print lock counts Bjorn Helgaas
@ 2022-01-06  0:06 ` Bjorn Helgaas
  2022-01-06 16:30 ` [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
  2022-01-31 22:23 ` Bjorn Helgaas
  11 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06  0:06 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, 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/gpu/vga/vgaarb.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 95e37817074e..17e9d2536430 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1,32 +1,11 @@
+// SPDX-License-Identifier: MIT
 /*
  * vgaarb.c: Implements the VGA arbitration. For details refer to
  * Documentation/gpu/vgaarbiter.rst
  *
- *
  * (C) Copyright 2005 Benjamin Herrenschmidt <benh@kernel.crashing.org>
  * (C) Copyright 2007 Paulo R. Zanoni <przanoni@gmail.com>
  * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@freedesktop.org>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS
- * IN THE SOFTWARE.
- *
  */
 
 #define pr_fmt(fmt) "vgaarb: " fmt
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path
  2022-01-06  0:06 ` [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path Bjorn Helgaas
@ 2022-01-06  6:44   ` Huacai Chen
  2022-01-06 16:20     ` Bjorn Helgaas
  0 siblings, 1 reply; 23+ messages in thread
From: Huacai Chen @ 2022-01-06  6:44 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: David Airlie, Daniel Vetter, Xuefeng Li, Huacai Chen, linux-pci,
	Maling list - DRI developers, LKML, Bjorn Helgaas,
	Bruno Prémont

Hi, Bjorn,

On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Previously we selected a device that owns the boot framebuffer as the
> default device in vga_arb_select_default_device().  This was only done in
> the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> e.g., by pcibios_init(), were not eligible.
>
> Fix this by moving the framebuffer device selection from
> vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> called after every PCI device is enumerated, either by the
> vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
>
> Note that if vga_arb_select_default_device() found a device owning the boot
> framebuffer, it unconditionally set it to be the default VGA device, and no
> subsequent device could replace it.
>
> Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
> Based-on-patch-by: Huacai Chen <chenhuacai@loongson.cn>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index b0ae0f177c6f..aefa4f406f7d 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -72,6 +72,7 @@ struct vga_device {
>         unsigned int io_norm_cnt;       /* normal IO count */
>         unsigned int mem_norm_cnt;      /* normal MEM count */
>         bool bridge_has_one_vga;
> +       bool is_framebuffer;    /* BAR covers firmware framebuffer */
>         unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
>  };
>
> @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
>  }
>  EXPORT_SYMBOL(vga_put);
>
> -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
>  {
>  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> -       struct device *dev = &pdev->dev;
>         u64 base = screen_info.lfb_base;
>         u64 size = screen_info.lfb_size;
>         u64 limit;
> @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
>
>         limit = base + size;
>
> -       /*
> -        * Override vga_arbiter_add_pci_device()'s I/O based detection
> -        * as it may take the wrong device (e.g. on Apple system under
> -        * EFI).
> -        *
> -        * Select the device owning the boot framebuffer if there is
> -        * one.
> -        */
> -
>         /* Does firmware framebuffer belong to us? */
>         for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>                 flags = pci_resource_flags(pdev, i);
> @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
>                 if (base < start || limit >= end)
>                         continue;
>
> -               if (!vga_default_device())
> -                       vgaarb_info(dev, "setting as boot device\n");
> -               else if (pdev != vga_default_device())
> -                       vgaarb_info(dev, "overriding boot device\n");
> -               vga_set_default_device(pdev);
> +               return true;
>         }
>  #endif
> +       return false;
>  }
>
>  static bool vga_arb_integrated_gpu(struct device *dev)
> @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
>  static bool vga_is_boot_device(struct vga_device *vgadev)
>  {
>         struct vga_device *boot_vga = vgadev_find(vga_default_device());
> +       struct pci_dev *pdev = vgadev->pdev;
>
>         /*
>          * We select the default VGA device in this order:
> @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>          *   Other device (see vga_arb_select_default_device())
>          */
>
> +       /*
> +        * We always prefer a firmware framebuffer, so if we've already
> +        * found one, there's no need to consider vgadev.
> +        */
> +       if (boot_vga && boot_vga->is_framebuffer)
> +               return false;
> +
> +       if (vga_is_framebuffer_device(pdev)) {
> +               vgadev->is_framebuffer = true;
> +               return true;
> +       }
Maybe it is better to rename vga_is_framebuffer_device() to
vga_is_firmware_device() and rename is_framebuffer to
is_fw_framebuffer?

Huacai
> +
>         /*
>          * A legacy VGA device has MEM and IO enabled and any bridges
>          * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
>         struct pci_dev *pdev, *found = NULL;
>         struct vga_device *vgadev;
>
> -       list_for_each_entry(vgadev, &vga_list, list) {
> -               vga_select_framebuffer_device(vgadev->pdev);
> -       }
> -
>         if (!vga_default_device()) {
>                 list_for_each_entry_reverse(vgadev, &vga_list, list) {
>                         struct device *dev = &vgadev->pdev->dev;
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path
  2022-01-06  6:44   ` Huacai Chen
@ 2022-01-06 16:20     ` Bjorn Helgaas
  2022-01-25  2:51       ` Huacai Chen
  0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06 16:20 UTC (permalink / raw)
  To: Huacai Chen
  Cc: David Airlie, Daniel Vetter, Xuefeng Li, Huacai Chen, linux-pci,
	dri-devel, linux-kernel, Bjorn Helgaas, Bruno Prémont

On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > Previously we selected a device that owns the boot framebuffer as the
> > default device in vga_arb_select_default_device().  This was only done in
> > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > e.g., by pcibios_init(), were not eligible.
> >
> > Fix this by moving the framebuffer device selection from
> > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > called after every PCI device is enumerated, either by the
> > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> >
> > Note that if vga_arb_select_default_device() found a device owning the boot
> > framebuffer, it unconditionally set it to be the default VGA device, and no
> > subsequent device could replace it.
> >
> > Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
> > Based-on-patch-by: Huacai Chen <chenhuacai@loongson.cn>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> >  drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> >  1 file changed, 17 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index b0ae0f177c6f..aefa4f406f7d 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -72,6 +72,7 @@ struct vga_device {
> >         unsigned int io_norm_cnt;       /* normal IO count */
> >         unsigned int mem_norm_cnt;      /* normal MEM count */
> >         bool bridge_has_one_vga;
> > +       bool is_framebuffer;    /* BAR covers firmware framebuffer */
> >         unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> >  };
> >
> > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> >  }
> >  EXPORT_SYMBOL(vga_put);
> >
> > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> >  {
> >  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > -       struct device *dev = &pdev->dev;
> >         u64 base = screen_info.lfb_base;
> >         u64 size = screen_info.lfb_size;
> >         u64 limit;
> > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> >
> >         limit = base + size;
> >
> > -       /*
> > -        * Override vga_arbiter_add_pci_device()'s I/O based detection
> > -        * as it may take the wrong device (e.g. on Apple system under
> > -        * EFI).
> > -        *
> > -        * Select the device owning the boot framebuffer if there is
> > -        * one.
> > -        */
> > -
> >         /* Does firmware framebuffer belong to us? */
> >         for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> >                 flags = pci_resource_flags(pdev, i);
> > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> >                 if (base < start || limit >= end)
> >                         continue;
> >
> > -               if (!vga_default_device())
> > -                       vgaarb_info(dev, "setting as boot device\n");
> > -               else if (pdev != vga_default_device())
> > -                       vgaarb_info(dev, "overriding boot device\n");
> > -               vga_set_default_device(pdev);
> > +               return true;
> >         }
> >  #endif
> > +       return false;
> >  }
> >
> >  static bool vga_arb_integrated_gpu(struct device *dev)
> > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> >  static bool vga_is_boot_device(struct vga_device *vgadev)
> >  {
> >         struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > +       struct pci_dev *pdev = vgadev->pdev;
> >
> >         /*
> >          * We select the default VGA device in this order:
> > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> >          *   Other device (see vga_arb_select_default_device())
> >          */
> >
> > +       /*
> > +        * We always prefer a firmware framebuffer, so if we've already
> > +        * found one, there's no need to consider vgadev.
> > +        */
> > +       if (boot_vga && boot_vga->is_framebuffer)
> > +               return false;
> > +
> > +       if (vga_is_framebuffer_device(pdev)) {
> > +               vgadev->is_framebuffer = true;
> > +               return true;
> > +       }
> Maybe it is better to rename vga_is_framebuffer_device() to
> vga_is_firmware_device() and rename is_framebuffer to
> is_fw_framebuffer?

That's a great point, thanks!

The "framebuffer" term is way too generic.  *All* VGA devices have a
framebuffer, so it adds no information.  This is really about finding
the device that was used by firmware.

I renamed:

  vga_is_framebuffer_device() -> vga_is_firmware_default()
  vga_device.is_framebuffer   -> vga_device.is_firmware_default

I updated my local branch and pushed it to:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
SPDX identifier").

I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
reference.  It'll ultimately be up to the DRM folks to handle this.

I'll wait for any other comments or testing reports before reposting.

> >         /*
> >          * A legacy VGA device has MEM and IO enabled and any bridges
> >          * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> >         struct pci_dev *pdev, *found = NULL;
> >         struct vga_device *vgadev;
> >
> > -       list_for_each_entry(vgadev, &vga_list, list) {
> > -               vga_select_framebuffer_device(vgadev->pdev);
> > -       }
> > -
> >         if (!vga_default_device()) {
> >                 list_for_each_entry_reverse(vgadev, &vga_list, list) {
> >                         struct device *dev = &vgadev->pdev->dev;
> > --
> > 2.25.1
> >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (9 preceding siblings ...)
  2022-01-06  0:06 ` [PATCH v8 10/10] vgaarb: Replace full MIT license text with SPDX identifier Bjorn Helgaas
@ 2022-01-06 16:30 ` Bjorn Helgaas
  2022-01-08  3:26   ` Huacai Chen
  2022-01-31 22:23 ` Bjorn Helgaas
  11 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-06 16:30 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: Xuefeng Li, Huacai Chen, Huacai Chen, linux-pci, dri-devel,
	linux-kernel, Bjorn Helgaas

[+to Maarten, Maxime, Thomas: sorry, I forgot to use
get_maintainer.pl so I missed you the first time.  Beginning of thread:
https://lore.kernel.org/all/20220106000658.243509-1-helgaas@kernel.org/#t
Git branch with this v8 + a couple trivial renames, based on v5.16-rc1:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=0f4caffa1297]

On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Current default VGA device selection fails in some cases because part of it
> is done in the vga_arb_device_init() subsys_initcall, and some arches
> enumerate PCI devices in pcibios_init(), which runs *after* that.
> 
> For example:
> 
>   - On BMC system, the AST2500 bridge [1a03:1150] does not implement
>     PCI_BRIDGE_CTL_VGA.  This is perfectly legal but means the legacy VGA
>     resources won't reach downstream devices unless they're included in the
>     usual bridge windows.
> 
>   - vga_arb_select_default_device() will set a device below such a bridge
>     as the default VGA device as long as it has PCI_COMMAND_IO and
>     PCI_COMMAND_MEMORY enabled.
> 
>   - vga_arbiter_add_pci_device() is called for every VGA device, either at
>     boot-time or at hot-add time, and it will also set the device as the
>     default VGA device, but ONLY if all bridges leading to it implement
>     PCI_BRIDGE_CTL_VGA.
> 
>   - This difference between vga_arb_select_default_device() and
>     vga_arbiter_add_pci_device() means that a device below an AST2500 or
>     similar bridge can only be set as the default if it is enumerated
>     before vga_arb_device_init().
> 
>   - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
>     runs before vga_arb_device_init().
> 
>   - On non-ACPI systems, like on MIPS system, they are enumerated by
>     pcibios_init(), which typically runs *after* vga_arb_device_init().
> 
> This series consolidates all the default VGA device selection in
> vga_arbiter_add_pci_device(), which is always called after enumerating a
> PCI device.
> 
> Almost all the work here is Huacai's.  I restructured it a little bit and
> added a few trivial patches on top.
> 
> I'd like to move vgaarb.c to drivers/pci eventually, but there's another
> initcall ordering snag that needs to be resolved first, so this leaves 
> it where it is.
> 
> Bjorn
> 
> Version history:
> V0 original implementation as final quirk to set default device.
> https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn
> 
> V1 rework vgaarb to do all default device selection in
> vga_arbiter_add_pci_device().
> https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn
> 
> V2 move arbiter to PCI subsystem, fix nits.
> https://lore.kernel.org/r/20210722212920.347118-1-helgaas@kernel.org
> 
> V3 rewrite the commit log of the last patch (which is also summarized
> by Bjorn).
> https://lore.kernel.org/r/20210820100832.663931-1-chenhuacai@loongson.cn
> 
> V4 split the last patch to two steps.
> https://lore.kernel.org/r/20210827083129.2781420-1-chenhuacai@loongson.cn
> 
> V5 split Patch-9 again and sort the patches.
> https://lore.kernel.org/r/20210911093056.1555274-1-chenhuacai@loongson.cn
> 
> V6 split Patch-5 again and sort the patches again.
> https://lore.kernel.org/r/20210916082941.3421838-1-chenhuacai@loongson.cn
> 
> V7 stop moving vgaarb to drivers/pci because of ordering issues with
> misc_init().
> https://lore.kernel.org/r/20211015061512.2941859-1-chenhuacai@loongson.cn
> https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
> 
> 
> Bjorn Helgaas (8):
>   vgaarb: Factor out vga_select_framebuffer_device()
>   vgaarb: Factor out default VGA device selection
>   vgaarb: Move framebuffer detection to ADD_DEVICE path
>   vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
>   vgaarb: Move disabled VGA device detection to ADD_DEVICE path
>   vgaarb: Remove empty vga_arb_device_card_gone()
>   vgaarb: Use unsigned format string to print lock counts
>   vgaarb: Replace full MIT license text with SPDX identifier
> 
> Huacai Chen (2):
>   vgaarb: Move vga_arb_integrated_gpu() earlier in file
>   vgaarb: Log bridge control messages when adding devices
> 
>  drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
>  1 file changed, 154 insertions(+), 157 deletions(-)
> 
> -- 
> 2.25.1
> 
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-01-06 16:30 ` [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
@ 2022-01-08  3:26   ` Huacai Chen
  0 siblings, 0 replies; 23+ messages in thread
From: Huacai Chen @ 2022-01-08  3:26 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Xuefeng Li, Huacai Chen, linux-pci,
	Maling list - DRI developers, LKML, Bjorn Helgaas

For the whole series:
Tested-by: Huacai Chen <chenhuacai@loongson.cn>

On Fri, Jan 7, 2022 at 12:30 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+to Maarten, Maxime, Thomas: sorry, I forgot to use
> get_maintainer.pl so I missed you the first time.  Beginning of thread:
> https://lore.kernel.org/all/20220106000658.243509-1-helgaas@kernel.org/#t
> Git branch with this v8 + a couple trivial renames, based on v5.16-rc1:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=0f4caffa1297]
>
> On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Current default VGA device selection fails in some cases because part of it
> > is done in the vga_arb_device_init() subsys_initcall, and some arches
> > enumerate PCI devices in pcibios_init(), which runs *after* that.
> >
> > For example:
> >
> >   - On BMC system, the AST2500 bridge [1a03:1150] does not implement
> >     PCI_BRIDGE_CTL_VGA.  This is perfectly legal but means the legacy VGA
> >     resources won't reach downstream devices unless they're included in the
> >     usual bridge windows.
> >
> >   - vga_arb_select_default_device() will set a device below such a bridge
> >     as the default VGA device as long as it has PCI_COMMAND_IO and
> >     PCI_COMMAND_MEMORY enabled.
> >
> >   - vga_arbiter_add_pci_device() is called for every VGA device, either at
> >     boot-time or at hot-add time, and it will also set the device as the
> >     default VGA device, but ONLY if all bridges leading to it implement
> >     PCI_BRIDGE_CTL_VGA.
> >
> >   - This difference between vga_arb_select_default_device() and
> >     vga_arbiter_add_pci_device() means that a device below an AST2500 or
> >     similar bridge can only be set as the default if it is enumerated
> >     before vga_arb_device_init().
> >
> >   - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
> >     runs before vga_arb_device_init().
> >
> >   - On non-ACPI systems, like on MIPS system, they are enumerated by
> >     pcibios_init(), which typically runs *after* vga_arb_device_init().
> >
> > This series consolidates all the default VGA device selection in
> > vga_arbiter_add_pci_device(), which is always called after enumerating a
> > PCI device.
> >
> > Almost all the work here is Huacai's.  I restructured it a little bit and
> > added a few trivial patches on top.
> >
> > I'd like to move vgaarb.c to drivers/pci eventually, but there's another
> > initcall ordering snag that needs to be resolved first, so this leaves
> > it where it is.
> >
> > Bjorn
> >
> > Version history:
> > V0 original implementation as final quirk to set default device.
> > https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn
> >
> > V1 rework vgaarb to do all default device selection in
> > vga_arbiter_add_pci_device().
> > https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn
> >
> > V2 move arbiter to PCI subsystem, fix nits.
> > https://lore.kernel.org/r/20210722212920.347118-1-helgaas@kernel.org
> >
> > V3 rewrite the commit log of the last patch (which is also summarized
> > by Bjorn).
> > https://lore.kernel.org/r/20210820100832.663931-1-chenhuacai@loongson.cn
> >
> > V4 split the last patch to two steps.
> > https://lore.kernel.org/r/20210827083129.2781420-1-chenhuacai@loongson.cn
> >
> > V5 split Patch-9 again and sort the patches.
> > https://lore.kernel.org/r/20210911093056.1555274-1-chenhuacai@loongson.cn
> >
> > V6 split Patch-5 again and sort the patches again.
> > https://lore.kernel.org/r/20210916082941.3421838-1-chenhuacai@loongson.cn
> >
> > V7 stop moving vgaarb to drivers/pci because of ordering issues with
> > misc_init().
> > https://lore.kernel.org/r/20211015061512.2941859-1-chenhuacai@loongson.cn
> > https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
> >
> >
> > Bjorn Helgaas (8):
> >   vgaarb: Factor out vga_select_framebuffer_device()
> >   vgaarb: Factor out default VGA device selection
> >   vgaarb: Move framebuffer detection to ADD_DEVICE path
> >   vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
> >   vgaarb: Move disabled VGA device detection to ADD_DEVICE path
> >   vgaarb: Remove empty vga_arb_device_card_gone()
> >   vgaarb: Use unsigned format string to print lock counts
> >   vgaarb: Replace full MIT license text with SPDX identifier
> >
> > Huacai Chen (2):
> >   vgaarb: Move vga_arb_integrated_gpu() earlier in file
> >   vgaarb: Log bridge control messages when adding devices
> >
> >  drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
> >  1 file changed, 154 insertions(+), 157 deletions(-)
> >
> > --
> > 2.25.1
> >
> >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path
  2022-01-06 16:20     ` Bjorn Helgaas
@ 2022-01-25  2:51       ` Huacai Chen
  2022-01-25 15:38         ` Bjorn Helgaas
  0 siblings, 1 reply; 23+ messages in thread
From: Huacai Chen @ 2022-01-25  2:51 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: David Airlie, Daniel Vetter, Xuefeng Li, Huacai Chen, linux-pci,
	Maling list - DRI developers, LKML, Bjorn Helgaas,
	Bruno Prémont

Hi, Bjorn,

Why this series still missing in 5.17-rc1? :(

Huacai

On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> > On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > Previously we selected a device that owns the boot framebuffer as the
> > > default device in vga_arb_select_default_device().  This was only done in
> > > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > > e.g., by pcibios_init(), were not eligible.
> > >
> > > Fix this by moving the framebuffer device selection from
> > > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > > called after every PCI device is enumerated, either by the
> > > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> > >
> > > Note that if vga_arb_select_default_device() found a device owning the boot
> > > framebuffer, it unconditionally set it to be the default VGA device, and no
> > > subsequent device could replace it.
> > >
> > > Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
> > > Based-on-patch-by: Huacai Chen <chenhuacai@loongson.cn>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Bruno Prémont <bonbons@linux-vserver.org>
> > > ---
> > >  drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > index b0ae0f177c6f..aefa4f406f7d 100644
> > > --- a/drivers/gpu/vga/vgaarb.c
> > > +++ b/drivers/gpu/vga/vgaarb.c
> > > @@ -72,6 +72,7 @@ struct vga_device {
> > >         unsigned int io_norm_cnt;       /* normal IO count */
> > >         unsigned int mem_norm_cnt;      /* normal MEM count */
> > >         bool bridge_has_one_vga;
> > > +       bool is_framebuffer;    /* BAR covers firmware framebuffer */
> > >         unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > >  };
> > >
> > > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > >  }
> > >  EXPORT_SYMBOL(vga_put);
> > >
> > > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > >  {
> > >  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > -       struct device *dev = &pdev->dev;
> > >         u64 base = screen_info.lfb_base;
> > >         u64 size = screen_info.lfb_size;
> > >         u64 limit;
> > > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > >
> > >         limit = base + size;
> > >
> > > -       /*
> > > -        * Override vga_arbiter_add_pci_device()'s I/O based detection
> > > -        * as it may take the wrong device (e.g. on Apple system under
> > > -        * EFI).
> > > -        *
> > > -        * Select the device owning the boot framebuffer if there is
> > > -        * one.
> > > -        */
> > > -
> > >         /* Does firmware framebuffer belong to us? */
> > >         for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > >                 flags = pci_resource_flags(pdev, i);
> > > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > >                 if (base < start || limit >= end)
> > >                         continue;
> > >
> > > -               if (!vga_default_device())
> > > -                       vgaarb_info(dev, "setting as boot device\n");
> > > -               else if (pdev != vga_default_device())
> > > -                       vgaarb_info(dev, "overriding boot device\n");
> > > -               vga_set_default_device(pdev);
> > > +               return true;
> > >         }
> > >  #endif
> > > +       return false;
> > >  }
> > >
> > >  static bool vga_arb_integrated_gpu(struct device *dev)
> > > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > >  static bool vga_is_boot_device(struct vga_device *vgadev)
> > >  {
> > >         struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > > +       struct pci_dev *pdev = vgadev->pdev;
> > >
> > >         /*
> > >          * We select the default VGA device in this order:
> > > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > >          *   Other device (see vga_arb_select_default_device())
> > >          */
> > >
> > > +       /*
> > > +        * We always prefer a firmware framebuffer, so if we've already
> > > +        * found one, there's no need to consider vgadev.
> > > +        */
> > > +       if (boot_vga && boot_vga->is_framebuffer)
> > > +               return false;
> > > +
> > > +       if (vga_is_framebuffer_device(pdev)) {
> > > +               vgadev->is_framebuffer = true;
> > > +               return true;
> > > +       }
> > Maybe it is better to rename vga_is_framebuffer_device() to
> > vga_is_firmware_device() and rename is_framebuffer to
> > is_fw_framebuffer?
>
> That's a great point, thanks!
>
> The "framebuffer" term is way too generic.  *All* VGA devices have a
> framebuffer, so it adds no information.  This is really about finding
> the device that was used by firmware.
>
> I renamed:
>
>   vga_is_framebuffer_device() -> vga_is_firmware_default()
>   vga_device.is_framebuffer   -> vga_device.is_firmware_default
>
> I updated my local branch and pushed it to:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
> with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
> SPDX identifier").
>
> I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
> reference.  It'll ultimately be up to the DRM folks to handle this.
>
> I'll wait for any other comments or testing reports before reposting.
>
> > >         /*
> > >          * A legacy VGA device has MEM and IO enabled and any bridges
> > >          * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > >         struct pci_dev *pdev, *found = NULL;
> > >         struct vga_device *vgadev;
> > >
> > > -       list_for_each_entry(vgadev, &vga_list, list) {
> > > -               vga_select_framebuffer_device(vgadev->pdev);
> > > -       }
> > > -
> > >         if (!vga_default_device()) {
> > >                 list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > >                         struct device *dev = &vgadev->pdev->dev;
> > > --
> > > 2.25.1
> > >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path
  2022-01-25  2:51       ` Huacai Chen
@ 2022-01-25 15:38         ` Bjorn Helgaas
  2022-01-26  2:25           ` Huacai Chen
  0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-25 15:38 UTC (permalink / raw)
  To: Huacai Chen
  Cc: David Airlie, linux-pci, LKML, Maling list - DRI developers,
	Bruno Prémont, Bjorn Helgaas, Xuefeng Li, Huacai Chen,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann

[+cc Maarten, Maxime, Thomas; beginning of thread:
https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]

On Tue, Jan 25, 2022 at 10:51:15AM +0800, Huacai Chen wrote:
> Hi, Bjorn,
> 
> Why this series still missing in 5.17-rc1? :(

1) It was posted late in the cycle (Jan 6, when the merge window
opened Jan 9), so it was too late to expect a significant change like
this to be merged for v5.17.  Right now is a good time to consider it
again so it would some time in -next.

2) As of now, this code is still in drivers/gpu, and I don't maintain
that area.  It's up to the DRM folks, who are all cc'd here.

I would like to move this from drivers/gpu to drivers/pci, but that
requires a little more work to resolve the initcall ordering problem
with respect to vga_arb_device_init() and misc_init() [1].

Bjorn

[1] https://lore.kernel.org/linux-pci/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com/

> On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> > > On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > Previously we selected a device that owns the boot framebuffer as the
> > > > default device in vga_arb_select_default_device().  This was only done in
> > > > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > > > e.g., by pcibios_init(), were not eligible.
> > > >
> > > > Fix this by moving the framebuffer device selection from
> > > > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > > > called after every PCI device is enumerated, either by the
> > > > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> > > >
> > > > Note that if vga_arb_select_default_device() found a device owning the boot
> > > > framebuffer, it unconditionally set it to be the default VGA device, and no
> > > > subsequent device could replace it.
> > > >
> > > > Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
> > > > Based-on-patch-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Bruno Prémont <bonbons@linux-vserver.org>
> > > > ---
> > > >  drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > > index b0ae0f177c6f..aefa4f406f7d 100644
> > > > --- a/drivers/gpu/vga/vgaarb.c
> > > > +++ b/drivers/gpu/vga/vgaarb.c
> > > > @@ -72,6 +72,7 @@ struct vga_device {
> > > >         unsigned int io_norm_cnt;       /* normal IO count */
> > > >         unsigned int mem_norm_cnt;      /* normal MEM count */
> > > >         bool bridge_has_one_vga;
> > > > +       bool is_framebuffer;    /* BAR covers firmware framebuffer */
> > > >         unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > > >  };
> > > >
> > > > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > > >  }
> > > >  EXPORT_SYMBOL(vga_put);
> > > >
> > > > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > > >  {
> > > >  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > > -       struct device *dev = &pdev->dev;
> > > >         u64 base = screen_info.lfb_base;
> > > >         u64 size = screen_info.lfb_size;
> > > >         u64 limit;
> > > > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > >
> > > >         limit = base + size;
> > > >
> > > > -       /*
> > > > -        * Override vga_arbiter_add_pci_device()'s I/O based detection
> > > > -        * as it may take the wrong device (e.g. on Apple system under
> > > > -        * EFI).
> > > > -        *
> > > > -        * Select the device owning the boot framebuffer if there is
> > > > -        * one.
> > > > -        */
> > > > -
> > > >         /* Does firmware framebuffer belong to us? */
> > > >         for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > >                 flags = pci_resource_flags(pdev, i);
> > > > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > >                 if (base < start || limit >= end)
> > > >                         continue;
> > > >
> > > > -               if (!vga_default_device())
> > > > -                       vgaarb_info(dev, "setting as boot device\n");
> > > > -               else if (pdev != vga_default_device())
> > > > -                       vgaarb_info(dev, "overriding boot device\n");
> > > > -               vga_set_default_device(pdev);
> > > > +               return true;
> > > >         }
> > > >  #endif
> > > > +       return false;
> > > >  }
> > > >
> > > >  static bool vga_arb_integrated_gpu(struct device *dev)
> > > > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > > >  static bool vga_is_boot_device(struct vga_device *vgadev)
> > > >  {
> > > >         struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > > > +       struct pci_dev *pdev = vgadev->pdev;
> > > >
> > > >         /*
> > > >          * We select the default VGA device in this order:
> > > > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > > >          *   Other device (see vga_arb_select_default_device())
> > > >          */
> > > >
> > > > +       /*
> > > > +        * We always prefer a firmware framebuffer, so if we've already
> > > > +        * found one, there's no need to consider vgadev.
> > > > +        */
> > > > +       if (boot_vga && boot_vga->is_framebuffer)
> > > > +               return false;
> > > > +
> > > > +       if (vga_is_framebuffer_device(pdev)) {
> > > > +               vgadev->is_framebuffer = true;
> > > > +               return true;
> > > > +       }
> > > Maybe it is better to rename vga_is_framebuffer_device() to
> > > vga_is_firmware_device() and rename is_framebuffer to
> > > is_fw_framebuffer?
> >
> > That's a great point, thanks!
> >
> > The "framebuffer" term is way too generic.  *All* VGA devices have a
> > framebuffer, so it adds no information.  This is really about finding
> > the device that was used by firmware.
> >
> > I renamed:
> >
> >   vga_is_framebuffer_device() -> vga_is_firmware_default()
> >   vga_device.is_framebuffer   -> vga_device.is_firmware_default
> >
> > I updated my local branch and pushed it to:
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
> > with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
> > SPDX identifier").
> >
> > I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
> > reference.  It'll ultimately be up to the DRM folks to handle this.
> >
> > I'll wait for any other comments or testing reports before reposting.
> >
> > > >         /*
> > > >          * A legacy VGA device has MEM and IO enabled and any bridges
> > > >          * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > > > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > > >         struct pci_dev *pdev, *found = NULL;
> > > >         struct vga_device *vgadev;
> > > >
> > > > -       list_for_each_entry(vgadev, &vga_list, list) {
> > > > -               vga_select_framebuffer_device(vgadev->pdev);
> > > > -       }
> > > > -
> > > >         if (!vga_default_device()) {
> > > >                 list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > > >                         struct device *dev = &vgadev->pdev->dev;
> > > > --
> > > > 2.25.1
> > > >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path
  2022-01-25 15:38         ` Bjorn Helgaas
@ 2022-01-26  2:25           ` Huacai Chen
  0 siblings, 0 replies; 23+ messages in thread
From: Huacai Chen @ 2022-01-26  2:25 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: David Airlie, linux-pci, LKML, Maling list - DRI developers,
	Bruno Prémont, Bjorn Helgaas, Xuefeng Li, Huacai Chen,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann

On Tue, Jan 25, 2022 at 11:39 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Maarten, Maxime, Thomas; beginning of thread:
> https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
>
> On Tue, Jan 25, 2022 at 10:51:15AM +0800, Huacai Chen wrote:
> > Hi, Bjorn,
> >
> > Why this series still missing in 5.17-rc1? :(
>
> 1) It was posted late in the cycle (Jan 6, when the merge window
> opened Jan 9), so it was too late to expect a significant change like
> this to be merged for v5.17.  Right now is a good time to consider it
> again so it would some time in -next.
>
> 2) As of now, this code is still in drivers/gpu, and I don't maintain
> that area.  It's up to the DRM folks, who are all cc'd here.
>
> I would like to move this from drivers/gpu to drivers/pci, but that
> requires a little more work to resolve the initcall ordering problem
> with respect to vga_arb_device_init() and misc_init() [1].
Hmm, to me, keep it in drivers/gpu is just OK.

Huacai

>
> Bjorn
>
> [1] https://lore.kernel.org/linux-pci/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com/
>
> > On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> > > > On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > Previously we selected a device that owns the boot framebuffer as the
> > > > > default device in vga_arb_select_default_device().  This was only done in
> > > > > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > > > > e.g., by pcibios_init(), were not eligible.
> > > > >
> > > > > Fix this by moving the framebuffer device selection from
> > > > > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > > > > called after every PCI device is enumerated, either by the
> > > > > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> > > > >
> > > > > Note that if vga_arb_select_default_device() found a device owning the boot
> > > > > framebuffer, it unconditionally set it to be the default VGA device, and no
> > > > > subsequent device could replace it.
> > > > >
> > > > > Link: https://lore.kernel.org/r/20211015061512.2941859-7-chenhuacai@loongson.cn
> > > > > Based-on-patch-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > Cc: Bruno Prémont <bonbons@linux-vserver.org>
> > > > > ---
> > > > >  drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > > > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > > > index b0ae0f177c6f..aefa4f406f7d 100644
> > > > > --- a/drivers/gpu/vga/vgaarb.c
> > > > > +++ b/drivers/gpu/vga/vgaarb.c
> > > > > @@ -72,6 +72,7 @@ struct vga_device {
> > > > >         unsigned int io_norm_cnt;       /* normal IO count */
> > > > >         unsigned int mem_norm_cnt;      /* normal MEM count */
> > > > >         bool bridge_has_one_vga;
> > > > > +       bool is_framebuffer;    /* BAR covers firmware framebuffer */
> > > > >         unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > > > >  };
> > > > >
> > > > > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > > > >  }
> > > > >  EXPORT_SYMBOL(vga_put);
> > > > >
> > > > > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > > > >  {
> > > > >  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > > > -       struct device *dev = &pdev->dev;
> > > > >         u64 base = screen_info.lfb_base;
> > > > >         u64 size = screen_info.lfb_size;
> > > > >         u64 limit;
> > > > > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > >
> > > > >         limit = base + size;
> > > > >
> > > > > -       /*
> > > > > -        * Override vga_arbiter_add_pci_device()'s I/O based detection
> > > > > -        * as it may take the wrong device (e.g. on Apple system under
> > > > > -        * EFI).
> > > > > -        *
> > > > > -        * Select the device owning the boot framebuffer if there is
> > > > > -        * one.
> > > > > -        */
> > > > > -
> > > > >         /* Does firmware framebuffer belong to us? */
> > > > >         for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > > >                 flags = pci_resource_flags(pdev, i);
> > > > > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > >                 if (base < start || limit >= end)
> > > > >                         continue;
> > > > >
> > > > > -               if (!vga_default_device())
> > > > > -                       vgaarb_info(dev, "setting as boot device\n");
> > > > > -               else if (pdev != vga_default_device())
> > > > > -                       vgaarb_info(dev, "overriding boot device\n");
> > > > > -               vga_set_default_device(pdev);
> > > > > +               return true;
> > > > >         }
> > > > >  #endif
> > > > > +       return false;
> > > > >  }
> > > > >
> > > > >  static bool vga_arb_integrated_gpu(struct device *dev)
> > > > > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > > > >  static bool vga_is_boot_device(struct vga_device *vgadev)
> > > > >  {
> > > > >         struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > > > > +       struct pci_dev *pdev = vgadev->pdev;
> > > > >
> > > > >         /*
> > > > >          * We select the default VGA device in this order:
> > > > > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > > > >          *   Other device (see vga_arb_select_default_device())
> > > > >          */
> > > > >
> > > > > +       /*
> > > > > +        * We always prefer a firmware framebuffer, so if we've already
> > > > > +        * found one, there's no need to consider vgadev.
> > > > > +        */
> > > > > +       if (boot_vga && boot_vga->is_framebuffer)
> > > > > +               return false;
> > > > > +
> > > > > +       if (vga_is_framebuffer_device(pdev)) {
> > > > > +               vgadev->is_framebuffer = true;
> > > > > +               return true;
> > > > > +       }
> > > > Maybe it is better to rename vga_is_framebuffer_device() to
> > > > vga_is_firmware_device() and rename is_framebuffer to
> > > > is_fw_framebuffer?
> > >
> > > That's a great point, thanks!
> > >
> > > The "framebuffer" term is way too generic.  *All* VGA devices have a
> > > framebuffer, so it adds no information.  This is really about finding
> > > the device that was used by firmware.
> > >
> > > I renamed:
> > >
> > >   vga_is_framebuffer_device() -> vga_is_firmware_default()
> > >   vga_device.is_framebuffer   -> vga_device.is_firmware_default
> > >
> > > I updated my local branch and pushed it to:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
> > > with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
> > > SPDX identifier").
> > >
> > > I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
> > > reference.  It'll ultimately be up to the DRM folks to handle this.
> > >
> > > I'll wait for any other comments or testing reports before reposting.
> > >
> > > > >         /*
> > > > >          * A legacy VGA device has MEM and IO enabled and any bridges
> > > > >          * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > > > > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > > > >         struct pci_dev *pdev, *found = NULL;
> > > > >         struct vga_device *vgadev;
> > > > >
> > > > > -       list_for_each_entry(vgadev, &vga_list, list) {
> > > > > -               vga_select_framebuffer_device(vgadev->pdev);
> > > > > -       }
> > > > > -
> > > > >         if (!vga_default_device()) {
> > > > >                 list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > > > >                         struct device *dev = &vgadev->pdev->dev;
> > > > > --
> > > > > 2.25.1
> > > > >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
                   ` (10 preceding siblings ...)
  2022-01-06 16:30 ` [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
@ 2022-01-31 22:23 ` Bjorn Helgaas
  2022-02-01 15:46   ` Maarten Lankhorst
  11 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2022-01-31 22:23 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann
  Cc: linux-pci, linux-kernel, dri-devel, Bjorn Helgaas, Xuefeng Li,
	Huacai Chen

[+to Maarten, Maxime, Thomas; beginning of thread:
https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]

On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Current default VGA device selection fails in some cases because part of it
> is done in the vga_arb_device_init() subsys_initcall, and some arches
> enumerate PCI devices in pcibios_init(), which runs *after* that.

Where are we at with this series?  Is there anything I can do to move
it forward?

Bjorn

> For example:
> 
>   - On BMC system, the AST2500 bridge [1a03:1150] does not implement
>     PCI_BRIDGE_CTL_VGA.  This is perfectly legal but means the legacy VGA
>     resources won't reach downstream devices unless they're included in the
>     usual bridge windows.
> 
>   - vga_arb_select_default_device() will set a device below such a bridge
>     as the default VGA device as long as it has PCI_COMMAND_IO and
>     PCI_COMMAND_MEMORY enabled.
> 
>   - vga_arbiter_add_pci_device() is called for every VGA device, either at
>     boot-time or at hot-add time, and it will also set the device as the
>     default VGA device, but ONLY if all bridges leading to it implement
>     PCI_BRIDGE_CTL_VGA.
> 
>   - This difference between vga_arb_select_default_device() and
>     vga_arbiter_add_pci_device() means that a device below an AST2500 or
>     similar bridge can only be set as the default if it is enumerated
>     before vga_arb_device_init().
> 
>   - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
>     runs before vga_arb_device_init().
> 
>   - On non-ACPI systems, like on MIPS system, they are enumerated by
>     pcibios_init(), which typically runs *after* vga_arb_device_init().
> 
> This series consolidates all the default VGA device selection in
> vga_arbiter_add_pci_device(), which is always called after enumerating a
> PCI device.
> 
> Almost all the work here is Huacai's.  I restructured it a little bit and
> added a few trivial patches on top.
> 
> I'd like to move vgaarb.c to drivers/pci eventually, but there's another
> initcall ordering snag that needs to be resolved first, so this leaves 
> it where it is.
> 
> Bjorn
> 
> Version history:
> V0 original implementation as final quirk to set default device.
> https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn
> 
> V1 rework vgaarb to do all default device selection in
> vga_arbiter_add_pci_device().
> https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn
> 
> V2 move arbiter to PCI subsystem, fix nits.
> https://lore.kernel.org/r/20210722212920.347118-1-helgaas@kernel.org
> 
> V3 rewrite the commit log of the last patch (which is also summarized
> by Bjorn).
> https://lore.kernel.org/r/20210820100832.663931-1-chenhuacai@loongson.cn
> 
> V4 split the last patch to two steps.
> https://lore.kernel.org/r/20210827083129.2781420-1-chenhuacai@loongson.cn
> 
> V5 split Patch-9 again and sort the patches.
> https://lore.kernel.org/r/20210911093056.1555274-1-chenhuacai@loongson.cn
> 
> V6 split Patch-5 again and sort the patches again.
> https://lore.kernel.org/r/20210916082941.3421838-1-chenhuacai@loongson.cn
> 
> V7 stop moving vgaarb to drivers/pci because of ordering issues with
> misc_init().
> https://lore.kernel.org/r/20211015061512.2941859-1-chenhuacai@loongson.cn
> https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
> 
> 
> Bjorn Helgaas (8):
>   vgaarb: Factor out vga_select_framebuffer_device()
>   vgaarb: Factor out default VGA device selection
>   vgaarb: Move framebuffer detection to ADD_DEVICE path
>   vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
>   vgaarb: Move disabled VGA device detection to ADD_DEVICE path
>   vgaarb: Remove empty vga_arb_device_card_gone()
>   vgaarb: Use unsigned format string to print lock counts
>   vgaarb: Replace full MIT license text with SPDX identifier
> 
> Huacai Chen (2):
>   vgaarb: Move vga_arb_integrated_gpu() earlier in file
>   vgaarb: Log bridge control messages when adding devices
> 
>  drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
>  1 file changed, 154 insertions(+), 157 deletions(-)
> 
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-01-31 22:23 ` Bjorn Helgaas
@ 2022-02-01 15:46   ` Maarten Lankhorst
  2022-02-07 17:59     ` Bjorn Helgaas
  2022-02-14 16:19     ` Bjorn Helgaas
  0 siblings, 2 replies; 23+ messages in thread
From: Maarten Lankhorst @ 2022-02-01 15:46 UTC (permalink / raw)
  To: Bjorn Helgaas, David Airlie, Daniel Vetter, Maxime Ripard,
	Thomas Zimmermann
  Cc: linux-pci, linux-kernel, dri-devel, Bjorn Helgaas, Xuefeng Li,
	Huacai Chen

Hey,
 
Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> [+to Maarten, Maxime, Thomas; beginning of thread:
> https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
>
> On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>>
>> Current default VGA device selection fails in some cases because part of it
>> is done in the vga_arb_device_init() subsys_initcall, and some arches
>> enumerate PCI devices in pcibios_init(), which runs *after* that.
> Where are we at with this series?  Is there anything I can do to move
> it forward?
>
> Bjorn

Hi Bjorn,


I'm afraid that I don't understand the vga arbiter or the vga code well enough to review.

Could you perhaps find someone who could review?

I see Chen wrote some patches and tested, so perhaps they could?

~Maarten

>> For example:
>>
>>   - On BMC system, the AST2500 bridge [1a03:1150] does not implement
>>     PCI_BRIDGE_CTL_VGA.  This is perfectly legal but means the legacy VGA
>>     resources won't reach downstream devices unless they're included in the
>>     usual bridge windows.
>>
>>   - vga_arb_select_default_device() will set a device below such a bridge
>>     as the default VGA device as long as it has PCI_COMMAND_IO and
>>     PCI_COMMAND_MEMORY enabled.
>>
>>   - vga_arbiter_add_pci_device() is called for every VGA device, either at
>>     boot-time or at hot-add time, and it will also set the device as the
>>     default VGA device, but ONLY if all bridges leading to it implement
>>     PCI_BRIDGE_CTL_VGA.
>>
>>   - This difference between vga_arb_select_default_device() and
>>     vga_arbiter_add_pci_device() means that a device below an AST2500 or
>>     similar bridge can only be set as the default if it is enumerated
>>     before vga_arb_device_init().
>>
>>   - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
>>     runs before vga_arb_device_init().
>>
>>   - On non-ACPI systems, like on MIPS system, they are enumerated by
>>     pcibios_init(), which typically runs *after* vga_arb_device_init().
>>
>> This series consolidates all the default VGA device selection in
>> vga_arbiter_add_pci_device(), which is always called after enumerating a
>> PCI device.
>>
>> Almost all the work here is Huacai's.  I restructured it a little bit and
>> added a few trivial patches on top.
>>
>> I'd like to move vgaarb.c to drivers/pci eventually, but there's another
>> initcall ordering snag that needs to be resolved first, so this leaves 
>> it where it is.
>>
>> Bjorn
>>
>> Version history:
>> V0 original implementation as final quirk to set default device.
>> https://lore.kernel.org/r/20210514080025.1828197-6-chenhuacai@loongson.cn
>>
>> V1 rework vgaarb to do all default device selection in
>> vga_arbiter_add_pci_device().
>> https://lore.kernel.org/r/20210705100503.1120643-1-chenhuacai@loongson.cn
>>
>> V2 move arbiter to PCI subsystem, fix nits.
>> https://lore.kernel.org/r/20210722212920.347118-1-helgaas@kernel.org
>>
>> V3 rewrite the commit log of the last patch (which is also summarized
>> by Bjorn).
>> https://lore.kernel.org/r/20210820100832.663931-1-chenhuacai@loongson.cn
>>
>> V4 split the last patch to two steps.
>> https://lore.kernel.org/r/20210827083129.2781420-1-chenhuacai@loongson.cn
>>
>> V5 split Patch-9 again and sort the patches.
>> https://lore.kernel.org/r/20210911093056.1555274-1-chenhuacai@loongson.cn
>>
>> V6 split Patch-5 again and sort the patches again.
>> https://lore.kernel.org/r/20210916082941.3421838-1-chenhuacai@loongson.cn
>>
>> V7 stop moving vgaarb to drivers/pci because of ordering issues with
>> misc_init().
>> https://lore.kernel.org/r/20211015061512.2941859-1-chenhuacai@loongson.cn
>> https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
>>
>>
>> Bjorn Helgaas (8):
>>   vgaarb: Factor out vga_select_framebuffer_device()
>>   vgaarb: Factor out default VGA device selection
>>   vgaarb: Move framebuffer detection to ADD_DEVICE path
>>   vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
>>   vgaarb: Move disabled VGA device detection to ADD_DEVICE path
>>   vgaarb: Remove empty vga_arb_device_card_gone()
>>   vgaarb: Use unsigned format string to print lock counts
>>   vgaarb: Replace full MIT license text with SPDX identifier
>>
>> Huacai Chen (2):
>>   vgaarb: Move vga_arb_integrated_gpu() earlier in file
>>   vgaarb: Log bridge control messages when adding devices
>>
>>  drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
>>  1 file changed, 154 insertions(+), 157 deletions(-)
>>
>> -- 
>> 2.25.1
>>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-02-01 15:46   ` Maarten Lankhorst
@ 2022-02-07 17:59     ` Bjorn Helgaas
  2022-02-08  2:14       ` Huacai Chen
  2022-02-14 16:19     ` Bjorn Helgaas
  1 sibling, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2022-02-07 17:59 UTC (permalink / raw)
  To: Maarten Lankhorst
  Cc: David Airlie, Daniel Vetter, Maxime Ripard, Thomas Zimmermann,
	linux-pci, linux-kernel, dri-devel, Bjorn Helgaas, Xuefeng Li,
	Huacai Chen

On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
> Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> > [+to Maarten, Maxime, Thomas; beginning of thread:
> > https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
> >
> > On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> >> From: Bjorn Helgaas <bhelgaas@google.com>
> >>
> >> Current default VGA device selection fails in some cases because part of it
> >> is done in the vga_arb_device_init() subsys_initcall, and some arches
> >> enumerate PCI devices in pcibios_init(), which runs *after* that.
> > Where are we at with this series?  Is there anything I can do to move
> > it forward?
> 
> I'm afraid that I don't understand the vga arbiter or the vga code
> well enough to review.
> 
> Could you perhaps find someone who could review?
> 
> I see Chen wrote some patches and tested, so perhaps they could?

Huacai, any chance you could review this?  I'm worried that this
series isn't going to go anywhere unless we can find somebody to
review it.

Bjorn

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-02-07 17:59     ` Bjorn Helgaas
@ 2022-02-08  2:14       ` Huacai Chen
  0 siblings, 0 replies; 23+ messages in thread
From: Huacai Chen @ 2022-02-08  2:14 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Maarten Lankhorst, David Airlie, linux-pci, LKML,
	Maling list - DRI developers, Thomas Zimmermann, Bjorn Helgaas,
	Xuefeng Li, Huacai Chen

Hi, Bjorn,

On Tue, Feb 8, 2022 at 1:59 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
> > Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> > > [+to Maarten, Maxime, Thomas; beginning of thread:
> > > https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
> > >
> > > On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> > >> From: Bjorn Helgaas <bhelgaas@google.com>
> > >>
> > >> Current default VGA device selection fails in some cases because part of it
> > >> is done in the vga_arb_device_init() subsys_initcall, and some arches
> > >> enumerate PCI devices in pcibios_init(), which runs *after* that.
> > > Where are we at with this series?  Is there anything I can do to move
> > > it forward?
> >
> > I'm afraid that I don't understand the vga arbiter or the vga code
> > well enough to review.
> >
> > Could you perhaps find someone who could review?
> >
> > I see Chen wrote some patches and tested, so perhaps they could?
>
> Huacai, any chance you could review this?  I'm worried that this
> series isn't going to go anywhere unless we can find somebody to
> review it.
I have reviewed and tested the whole series, it looks good to me
(except the naming which has already changed). But I thought I cannot
add a "Reviewed-by" because I was originally a co-developer. But if
necessary,

Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>

Huacai
>
> Bjorn

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 00/10] vgaarb: Rework default VGA device selection
  2022-02-01 15:46   ` Maarten Lankhorst
  2022-02-07 17:59     ` Bjorn Helgaas
@ 2022-02-14 16:19     ` Bjorn Helgaas
  1 sibling, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2022-02-14 16:19 UTC (permalink / raw)
  To: Maarten Lankhorst
  Cc: David Airlie, Daniel Vetter, Maxime Ripard, Thomas Zimmermann,
	linux-pci, linux-kernel, dri-devel, Bjorn Helgaas, Xuefeng Li,
	Huacai Chen

On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
> Hey,
>  
> Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> > [+to Maarten, Maxime, Thomas; beginning of thread:
> > https://lore.kernel.org/r/20220106000658.243509-1-helgaas@kernel.org]
> >
> > On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> >> From: Bjorn Helgaas <bhelgaas@google.com>
> >>
> >> Current default VGA device selection fails in some cases because
> >> part of it is done in the vga_arb_device_init() subsys_initcall,
> >> and some arches enumerate PCI devices in pcibios_init(), which
> >> runs *after* that.
> >
> > Where are we at with this series?  Is there anything I can do to
> > move it forward?
> 
> I'm afraid that I don't understand the vga arbiter or the vga code
> well enough to review.
> 
> Could you perhaps find someone who could review?
> 
> I see Chen wrote some patches and tested, so perhaps they could?

Hi Maarten,

Huacai Chen did provide his Reviewed-by (although as he noted, the
content initially came from him anyway and my contribution was mainly
rearranging things into separate patches for each specific case).

Anything else we can to do help here?

Bjorn

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-02-14 16:19 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-06  0:06 [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 01/10] vgaarb: Move vga_arb_integrated_gpu() earlier in file Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 02/10] vgaarb: Factor out vga_select_framebuffer_device() Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 03/10] vgaarb: Factor out default VGA device selection Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 04/10] vgaarb: Move framebuffer detection to ADD_DEVICE path Bjorn Helgaas
2022-01-06  6:44   ` Huacai Chen
2022-01-06 16:20     ` Bjorn Helgaas
2022-01-25  2:51       ` Huacai Chen
2022-01-25 15:38         ` Bjorn Helgaas
2022-01-26  2:25           ` Huacai Chen
2022-01-06  0:06 ` [PATCH v8 05/10] vgaarb: Move non-legacy VGA " Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 06/10] vgaarb: Move disabled VGA device " Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 07/10] vgaarb: Remove empty vga_arb_device_card_gone() Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 08/10] vgaarb: Log bridge control messages when adding devices Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 09/10] vgaarb: Use unsigned format string to print lock counts Bjorn Helgaas
2022-01-06  0:06 ` [PATCH v8 10/10] vgaarb: Replace full MIT license text with SPDX identifier Bjorn Helgaas
2022-01-06 16:30 ` [PATCH v8 00/10] vgaarb: Rework default VGA device selection Bjorn Helgaas
2022-01-08  3:26   ` Huacai Chen
2022-01-31 22:23 ` Bjorn Helgaas
2022-02-01 15:46   ` Maarten Lankhorst
2022-02-07 17:59     ` Bjorn Helgaas
2022-02-08  2:14       ` Huacai Chen
2022-02-14 16:19     ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).