[PATCH v2 0/4] Allow non-legacy cards to be vgaarb default

[Daniel Axtens]

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

[Daniel Axtens]

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 <***@linux.vnet.ibm.com>
Signed-off-by: Daniel Axtens <***@axtens.net>

---

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

}

[Daniel Axtens]

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 <***@axtens.net>

---

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

[Will Deacon]

Post by Daniel Axtens
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.

Acked-by: Will Deacon <***@arm.com>

Will

Post by Daniel Axtens
---
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

[Daniel Axtens]

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 <***@linux.vnet.ibm.com>
Signed-off-by: Daniel Axtens <***@axtens.net>

---

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

[Daniel Axtens]

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 <***@axtens.net>
---
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

[Gabriele Paoloni]

Hi Daniel many thanks for your patch

-----Original Message-----
Sent: 19 July 2017 03:15
Subject: [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.
- 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 first
one that is a VGA device, capable to respond to IO and MEM requests
and that has a driver bound to it

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

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

[...]