[PATCH v2 0/4] 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.

Cc: Brian King <redacted>
Signed-off-by: Daniel Axtens <redacted>

---

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

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 341a7469cab8..c95fdda3a2dc 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c

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

Signed-off-by: Daniel Axtens <redacted>
---
 drivers/gpu/vga/Kconfig  |  7 +++++++
 drivers/gpu/vga/vgaarb.c | 21 +++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/vga/Kconfig b/drivers/gpu/vga/Kconfig
index 29437eabe095..1205c6cc1ff5 100644
--- a/drivers/gpu/vga/Kconfig
+++ b/drivers/gpu/vga/Kconfig

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

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 76875f6299b8..2135b04759c5 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c

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

Cc: Brian King <redacted>
Signed-off-by: Daniel Axtens <redacted>

---

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

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 36f858c37ca7..c28b8eb1dce1 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

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

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index c95fdda3a2dc..7b093a4fa85d 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c

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

Signed-off-by: Daniel Axtens <redacted>

---

Tested on a D05 using the hibmc card.
---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index dfd908630631..cefcbd442e4f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig

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

On Wed, Jul 19, 2017 at 12:15:16PM +1000, Daniel Axtens wrote:

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.

Signed-off-by: Daniel Axtens <redacted>

Acked-by: Will Deacon <redacted>

Will

Hi Daniel many thanks for your patch

-----Original Message-----
From: Daniel Axtens [mailto:dja@axtens.net]
Sent: 19 July 2017 03:15
To: linux-pci@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
arm-kernel@lists.infradead.org
Cc: benh@kernel.crashing.org; Liuxinliang (Matthew Liu);
zourongrong@gmail.com; catalin.marinas@arm.com; will.deacon@arm.com;
Gabriele Paoloni; bhelgaas@google.com; airlied@linux.ie;
daniel.vetter@intel.com; alex.williamson@redhat.com; Daniel Axtens
Subject: [PATCH v2 0/4] Allow non-legacy cards to be vgaarb default
=20
[v2, in which I send the right patches. My apologies to you all.]
=20
Hi all,
=20
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.
=20
Here is my suggested solution:
=20
 - Create a Kconfig option ARCH_WANT_VGA_ARB_FALLBACK and

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 firs=
t
   one that is a VGA device, capable to respond to IO and MEM requests
   and that has a driver bound to it=20

=20
 - 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.
=20
 - if there is no default device when the hook fires, and the device
   can control memory and I/O, mark it as default.

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 enume=
ration
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 arbite=
r...
Do you agree?

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

Thanks
Gab

[...]