Allow non-legacy cards to be vgaarb default

[v2, in which I send the right patches. My apologies to you all.]

Hi all,

Previously I posted a patch that provided a quirk for a hibmc card behind a particular Huawei bridge that allowed it to be marked as the default device in the VGA arbiter.[0] This lead to some discussion.[1] It was broadly suggested that a more generic solution would be better, something in the style of powerpc's fixup_vga() quirk.

Here is my suggested solution:

- Create a Kconfig option ARCH_WANT_VGA_ARB_FALLBACK

- if an arch selects that option, install PCI_FIXUP_CLASS_ENABLE hook. This hook fires when a card is enabled, which will require that a driver has been bound.

- if there is no default device when the hook fires, and the device can control memory and I/O, mark it as default.

The patches are as follows:

(1) powerpc: simplify and fix VGA default device behaviour

This cleans up some quirks in the powerpc implementation of the vga_fixup. It should make the behaviour match the original intention.

(2) vgaarb: allow non-legacy cards to be marked as default

Add the Kconfig option, and create the fixup in vgaarb.c gated behind the option. Nothing happens at this stage because no arch has selected the option yet.

(3) powerpc: replace vga_fixup() with generic code

Select the option on powerpc and remove the old code. The only change is that it moves from being a final fixup to an enable fixup.

(4) arm64: allow non-legacy VGA devices to be default

Select the option on arm64. This solves my problem with the D05, but may cause other cards to be marked as default on other boards. This shouldn't cause any real issues but is worth being aware of.

Regards,
Daniel

[0]: https://patchwork.ozlabs.org/patch/787003/
[1]: https://www.spinics.net/lists/arm-kernel/msg593656.html

Daniel Axtens (4):
powerpc: simplify and fix VGA default device behaviour
vgaarb: allow non-legacy cards to be marked as default
powerpc: replace vga_fixup() with generic code
arm64: allow non-legacy VGA devices to be default

 arch/arm64/Kconfig               |  1 +
 arch/powerpc/Kconfig             |  1 +
 arch/powerpc/kernel/pci-common.c | 12 ------------
 drivers/gpu/vga/Kconfig          |  7 +++++++
 drivers/gpu/vga/vgaarb.c         | 21 +++++++++++++++++++++
 5 files changed, 30 insertions(+), 12 deletions(-)

2.11.0

Some powerpc devices provide a PCI display that isn't picked up by the VGA arbiter, presumably because it doesn't support the PCI legacy VGA ranges.

Commit c2e1d84523ad ("powerpc: Set default VGA device") introduced an arch quirk to mark these devices as default to fix X autoconfig.

The commit message stated that the patch:

Ensures a default VGA is always set if a graphics adapter is present, even if firmware did not initialize it. If more than one graphics adapter is present, ensure the one initialized by firmware is set as the default VGA device.

The patch used the following test to decide whether or not to mark a device as default:

  pci_read_config_word(pdev, PCI_COMMAND, &cmd);
  if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
          vga_set_default_device(pdev);

This doesn't seem like it works quite as intended. Because of the logical OR, the default device will be set in 2 cases:

1) if there is no default device
OR
2) if this device has normal memory/IO decoding turned on

This will work as intended if there is only one device, but if there are multiple devices, we may override the device the VGA arbiter picked.

Instead, set a device as default if there is no default device AND this device decodes.

This will not change behaviour on single-headed systems.

---

Tested in TCG (the card provided by qemu doesn't automatically register with vgaarb, so the relevant code path has been tested) but I would appreciate any tests on real hardware.

---
 arch/powerpc/kernel/pci-common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 341a7469cab8..c95fdda3a2dc 100644
@@ -1746,8 +1746,11 @@ static void fixup_vga(struct pci_dev *pdev)
 {
 	u16 cmd;
 
+	if (vga_default_device())
+		return;
+
 	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
-	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
+	if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
 		vga_set_default_device(pdev);
 
 }

2.11.0

The VGA arbiter currently only selects a card as default if it can decode legacy I/O and memory ranges.

However, on some architectures, legacy PCI resources are not used. This has lead to a powerpc quirk, and issues on arm64.

Allow architectures to select ARCH_WANT_VGA_ARB_FALLBACK. When that symbol is selected, add a PCI_FIXUP_CLASS_ENABLE hook, which will mark the first card that is enabled and that can control I/O and memory as the default card.

Behaviour is unchanged unless arches opt-in by selecting the Kconfig option.

---
 drivers/gpu/vga/Kconfig  |  7 +++++++
 drivers/gpu/vga/vgaarb.c | 21 +++++++++++++++++++++
 2 files changed, 28 insertions(+)

index 29437eabe095..1205c6cc1ff5 100644
@@ -17,6 +17,13 @@ config VGA_ARB_MAX_GPUS
 	  Reserves space in the kernel to maintain resource locking for
 	  multiple GPUS.  The overhead for each GPU is very small.
 
+config ARCH_WANT_VGA_ARB_FALLBACK
+	bool
+	help
+	  Some architectures don't have a concept of "legacy" PCI addresses
+	  which the VGA arbiter relies on. Instead, they can fall back to
+	  selecting the first device that decodes memory and I/O.
+
 config VGA_SWITCHEROO
 	bool "Laptop Hybrid Graphics - GPU switching support"
 	depends on X86

index 76875f6299b8..2135b04759c5 100644
@@ -1472,3 +1472,24 @@ static int __init vga_arb_device_init(void)
 	return rc;
 }
 subsys_initcall(vga_arb_device_init);
+
+#if defined(CONFIG_ARCH_WANT_VGA_ARB_FALLBACK)
+static void vga_arb_fallback_fixup(struct pci_dev *pdev)
+{
+	u16 cmd;
+
+	if (vga_default_device())
+		return;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+		vgaarb_info(&pdev->dev, "[fallback]"
+			    " setting as default device\n");
+		vga_set_default_device(pdev);
+	}
+
+}
+DECLARE_PCI_FIXUP_CLASS_ENABLE(PCI_ANY_ID, PCI_ANY_ID,
+			       PCI_CLASS_DISPLAY_VGA, 8,
+			       vga_arb_fallback_fixup);
+#endif

2.11.0

Currently, we do a PCI fixup to mark a default card so that Xorg autoconfiguration works.

There is a new generic method to do this sort of vga fixup.

Code-wise, it is identical, however instead of firing at the FIXUP_FINAL stage it fires at the FIXUP_ENABLE stage. This means a card will not be marked as default unless a driver enables it.

---

Tested with xeyes on qemu TCG, which does use this code path.

---
 arch/powerpc/Kconfig             |  1 +
 arch/powerpc/kernel/pci-common.c | 15 ---------------
 2 files changed, 1 insertion(+), 15 deletions(-)

index 36f858c37ca7..c28b8eb1dce1 100644
@@ -140,6 +140,7 @@ config PPC
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if PPC64
 	select ARCH_WANT_IPC_PARSE_VERSION
+	select ARCH_WANT_VGA_ARB_FALLBACK
 	select ARCH_WEAK_RELEASE_ACQUIRE
 	select BINFMT_ELF
 	select BUILDTIME_EXTABLE_SORT

index c95fdda3a2dc..7b093a4fa85d 100644
@@ -1741,18 +1741,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
-
-static void fixup_vga(struct pci_dev *pdev)
-{
-	u16 cmd;
-
-	if (vga_default_device())
-		return;
-
-	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
-	if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
-		vga_set_default_device(pdev);
-
-}
-DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-			      PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);

2.11.0

The VGA arbiter only marks a device as default if it can decode legacy I/O and memory ranges. This is often not the case on arm64, which doesn't use the legacy ranges.

Enable the VGA arbiter to mark the first enabled VGA card as default.

---

Tested on a D05 using the hibmc card.

---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

index dfd908630631..cefcbd442e4f 100644
@@ -28,6 +28,7 @@ config ARM64
 	select ARCH_SUPPORTS_NUMA_BALANCING
 	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
 	select ARCH_WANT_FRAME_POINTERS
+	select ARCH_WANT_VGA_ARB_FALLBACK
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARM_AMBA
 	select ARM_ARCH_TIMER

2.11.0

Will

Hi Daniel many thanks for your patch

In my opinion we could avoid depending on a Kernel config options. I.e. we can have generic code that, after all PCI devs are enumerated:
1) check if we have a default vga device
2) if not check each registered PCI device and make default device the first one that is a VGA device, capable to respond to IO and MEM requests and that has a driver bound to it

I am worried that the patchset you proposed has a race condition with the VGA arbiter. In fact you see:
subsys_initcall(vga_arb_device_init) is not guaranteed to be called before subsys_initcall(acpi_init)

acpi_init->acpi_scan_init->acpi_pci_root_init() at this stage the PCI enumeration is done and as soon as a device is added the Kernel will look for a driver to bind to it and therefore you quirk could be called before the VGA arbiter... Do you agree?

What about modifying vgaarb.c to add a late_initcall() checking what I suggested above?

Thanks
Gab

[...]