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

[Daniel Axtens]

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

}

[Benjamin Herrenschmidt]

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

Ack.

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

Signed-off-by: Daniel Axtens <***@axtens.net>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/kernel/pci-common.c | 2 ++
drivers/gpu/vga/Kconfig | 8 ++++++++
drivers/gpu/vga/vgaarb.c | 21 +++++++++++++++++++++
4 files changed, 32 insertions(+)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 524f71104b75..f86e4c8a9cc6 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -83,6 +83,7 @@ config PPC
select BUILDTIME_EXTABLE_SORT
select ARCH_MIGHT_HAVE_PC_PARPORT
select ARCH_MIGHT_HAVE_PC_SERIO
+ select ARCH_WANT_VGA_ARB_FALLBACK
select BINFMT_ELF
select ARCH_HAS_ELF_RANDOMIZE
select OF
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 07f05a0f59a2..e0f29a594aa1 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1740,6 +1740,7 @@ 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);

+#if !defined(CONFIG_ARCH_WANT_VGA_ARB_FALLBACK)
static void fixup_vga(struct pci_dev *pdev)
{
u16 cmd;
@@ -1754,3 +1755,4 @@ static void fixup_vga(struct pci_dev *pdev)
}
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);
+#endif
diff --git a/drivers/gpu/vga/Kconfig b/drivers/gpu/vga/Kconfig
index 29437eabe095..20f6c5a9a159 100644
--- a/drivers/gpu/vga/Kconfig
+++ b/drivers/gpu/vga/Kconfig
@@ -17,6 +17,14 @@ 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
+ depends on !(X86 || IA64)
+ 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 0f5b2dd24507..02424dc3a58d 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

[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

[Daniel Axtens]

Signed-off-by: Daniel Axtens <***@axtens.net>
---
arch/arm64/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index cae4e677a181..e88081b515d2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -20,6 +20,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

[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

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

Apologies everyone - this got mixed in with another patch set. I'll do a
v2 that isn't completely broken and confusing.

Again, my apologies for the noise.

Regards,
Daniel

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

[Ard Biesheuvel]

(+ Laszlo)

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

Hi Daniel,

Given that the whole point of the VGA arbiter is the ability to share
the legacy mem+io ranges between different cards, why do we care about
the VGA arbiter in the first place on arm64?

AFAIK, there have been some recent changes in Xorg to address the
auto-detection problem. I don't remember the exact details, but I have
added Laszlo, who was involved with this at the time.

[Daniel Axtens]

Hi Ard,

Post by Ard Biesheuvel
(+ Laszlo)

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

Hi Daniel,
Given that the whole point of the VGA arbiter is the ability to share
the legacy mem+io ranges between different cards, why do we care about
the VGA arbiter in the first place on arm64?
AFAIK, there have been some recent changes in Xorg to address the
auto-detection problem. I don't remember the exact details, but I have
added Laszlo, who was involved with this at the time.

I haven't been able to locate those changes - I remember that the call
to pci_device_is_boot_vga() in xf86pciBus.c [0] was critical and that is
still there in the latest git.

Indeed, the reason we care about the vga arbiter at all is because of
that Xorg dependency on the boot VGA card. pci_device_is_boot_vga()
reads a sysfs file, and that sysfs file is populated based on the
vga_default_driver(), so it's very difficult to extricate ourselves from
the vga arbiter and its concept of the default device.

We could make this method an 'either/or' rather than a fallback - so
platforms who didn't care about legacy resources didn't bother with
those tests, but I'm not sure what benefit that would give and I find it
harder to be confident of an absence of unexpected consequences.

Regards,
Daniel

[0] https://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86pciBus.c#n113

Post by Ard Biesheuvel
--
Ard.

[Ard Biesheuvel]

(+ Hans)

Post by Daniel Axtens
Hi Ard,

Post by Ard Biesheuvel
(+ Laszlo)

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

Hi Daniel,
Given that the whole point of the VGA arbiter is the ability to share
the legacy mem+io ranges between different cards, why do we care about
the VGA arbiter in the first place on arm64?
AFAIK, there have been some recent changes in Xorg to address the
auto-detection problem. I don't remember the exact details, but I have
added Laszlo, who was involved with this at the time.

I haven't been able to locate those changes - I remember that the call
to pci_device_is_boot_vga() in xf86pciBus.c [0] was critical and that is
still there in the latest git.
Indeed, the reason we care about the vga arbiter at all is because of
that Xorg dependency on the boot VGA card. pci_device_is_boot_vga()
reads a sysfs file, and that sysfs file is populated based on the
vga_default_driver(), so it's very difficult to extricate ourselves from
the vga arbiter and its concept of the default device.
We could make this method an 'either/or' rather than a fallback - so
platforms who didn't care about legacy resources didn't bother with
those tests, but I'm not sure what benefit that would give and I find it
harder to be confident of an absence of unexpected consequences.

I was referring to this commit

https://cgit.freedesktop.org/xorg/xserver/commit/?id=ca8d88e50310a0d440a127c22a0a383cc149f408

but reading the commit log, it may have less to do with this issue
than I thought originally.

But the fact remains that we are going about this the wrong way.
Whether a graphics card decodes legacy VGA ranges or not has *nothing*
to do with whether or not it is in fact the primary device on a
non-x86 system, and so I still think the VGA arbiter should be omitted
entirely for such platforms, and Xorg should be fixed instead.

[Daniel Axtens]

Hi Ard,

Post by Ard Biesheuvel
But the fact remains that we are going about this the wrong way.
Whether a graphics card decodes legacy VGA ranges or not has *nothing*
to do with whether or not it is in fact the primary device on a
non-x86 system, and so I still think the VGA arbiter should be omitted
entirely for such platforms, and Xorg should be fixed instead.

OK, I see where you're coming from. I've been trying to keep my changes
small as I don't want to end up on the hook for the almost limitless
range of problems that changing this sort of code can have, but I do
take your point that it's a bit of an ugly hack of a solution.

Say we were to change Xorg instead. What would correct Xorg behaviour
look like? Xorg would need to honour the boot_vga file if it existed so
as not to break x86, etc. So your proposed Xorg - if it couldn't find a
default card that way, and if there was no helpful config file info,
would arbitrarily pick a card that has an Xorg driver? In other words,
much like the proposed kernel approach but at a different level of the
stack?

Are there other graphical applications we care about (other than Xorg)
that would need to be patched? I'm happy to do the Xorg patch, but I
don't know if anything other than Xorg keys off the boot_vga file.

I'm not fundamentally opposed to this approach if the Xorg community is
happy with it, the kernel community is happy with it, and no-one expects
me to provide patches to any other user-space applications that depend
on boot_vga.

Regards,
Daniel

[Laszlo Ersek]

Post by Daniel Axtens
Hi Ard,

Post by Ard Biesheuvel
But the fact remains that we are going about this the wrong way.
Whether a graphics card decodes legacy VGA ranges or not has *nothing*
to do with whether or not it is in fact the primary device on a
non-x86 system, and so I still think the VGA arbiter should be omitted
entirely for such platforms, and Xorg should be fixed instead.

OK, I see where you're coming from. I've been trying to keep my changes
small as I don't want to end up on the hook for the almost limitless
range of problems that changing this sort of code can have, but I do
take your point that it's a bit of an ugly hack of a solution.
Say we were to change Xorg instead. What would correct Xorg behaviour
look like? Xorg would need to honour the boot_vga file if it existed so
as not to break x86, etc. So your proposed Xorg - if it couldn't find a
default card that way, and if there was no helpful config file info,
would arbitrarily pick a card that has an Xorg driver? In other words,
much like the proposed kernel approach but at a different level of the
stack?
Are there other graphical applications we care about (other than Xorg)
that would need to be patched? I'm happy to do the Xorg patch, but I
don't know if anything other than Xorg keys off the boot_vga file.
I'm not fundamentally opposed to this approach if the Xorg community is
happy with it, the kernel community is happy with it, and no-one expects
me to provide patches to any other user-space applications that depend
on boot_vga.

Ard both identified the Xorg commit that I would have, and CC'd Hans
which I would have recommended as well.

I assume the symptom is that now there's a class of platform GPU devices
that is neither PCI nor legacy VGA, so neither the kernel's boot_vga
logic matches it, nor Xorg's commit in question.

I agree that it should be possible to add more logic to Xorg to detect
this kind device as primary. However, I share Daniel's worry that it
wouldn't cover all user space apps -- I see "Wayland this, Wayland that"
on reddit every week.

Having practically zero background in gfx development (either kernel or
Xorg), I think the problem is that vga_default_device() /
vga_set_default_device(), which -- apparently -- "boot_vga" is based
upon, come from "drivers/gpu/vga/vgaarb.c". Namely, the concept of
"primary / boot display device" is tied to the VGA arbiter, plus only a
PCI device can currently be marked as primary/boot display device.

Can these concepts be split from each other? (I can fully imagine that
this would result in a userspace visible interface change (or addition),
so that e.g. "/sys/devices/**/boot_gpu" would have to be consulted by
display servers.)

(Sorry if I'm totally wrong.)

... Hm, reading the thread starter at
<https://www.mail-archive.com/linuxppc-***@lists.ozlabs.org/msg120851.html>,
and the references within... It looks like this work is motivated by
hardware that is supposed to be PCI, but actually breaks the specs. Is
that correct? If so, then I don't think I can suggest anything useful.
Specs exist so that hardware vendors and software authors follow them.
If hardware does not conform, then software should either refuse to work
with it, or handle it with quirks, on a case-by-case basis. I guess this
means that I don't agree with the

broad[] suggest[ion] that a more generic solution would be better

which seems to disqualify me from the discussion, as it must have been
suggested by people with incomparably more experience than what I have :)

Thanks
Laszlo

[Gabriele Paoloni]

Hi Laszlo

[...]

Post by Laszlo Ersek
Having practically zero background in gfx development (either kernel or
Xorg), I think the problem is that vga_default_device() /
vga_set_default_device(), which -- apparently -- "boot_vga" is based
upon, come from "drivers/gpu/vga/vgaarb.c". Namely, the concept of
"primary / boot display device" is tied to the VGA arbiter, plus only a
PCI device can currently be marked as primary/boot display device.
Can these concepts be split from each other? (I can fully imagine that
this would result in a userspace visible interface change (or
addition),
so that e.g. "/sys/devices/**/boot_gpu" would have to be consulted by
display servers.)
(Sorry if I'm totally wrong.)
... Hm, reading the thread starter at
<https://www.mail-archive.com/linuxppc-
and the references within... It looks like this work is motivated by
hardware that is supposed to be PCI, but actually breaks the specs. Is
that correct? If so, then I don't think I can suggest anything useful.

My understanding is that the current PCIe HW is specs compliant but the
vgaarb, in order to make a VGA device the default one, requires all the
bridges on top of such device to have the "VGA Enable" bit set (optional
bit in the PCI Express™ to PCI/PCI-X Bridge Spec). I.e. all the bridges
on top have to support legacy VGA devices; and this is not mandatory
from the specs...right?

BTW my VGA experience is limited too...this is just my understanding...

Gab

Post by Laszlo Ersek
Specs exist so that hardware vendors and software authors follow them.
If hardware does not conform, then software should either refuse to work
with it, or handle it with quirks, on a case-by-case basis. I guess this
means that I don't agree with the
broad[] suggest[ion] that a more generic solution would be better
which seems to disqualify me from the discussion, as it must have been
suggested by people with incomparably more experience than what I have :)
Thanks
Laszlo

[Daniel Axtens]

Hi Laszlo,

Thanks for your input!

Post by Laszlo Ersek

Post by Daniel Axtens
Are there other graphical applications we care about (other than Xorg)
that would need to be patched? I'm happy to do the Xorg patch, but I
don't know if anything other than Xorg keys off the boot_vga file.
I'm not fundamentally opposed to this approach if the Xorg community is
happy with it, the kernel community is happy with it, and no-one expects
me to provide patches to any other user-space applications that depend
on boot_vga.

Ard both identified the Xorg commit that I would have, and CC'd Hans
which I would have recommended as well.
I assume the symptom is that now there's a class of platform GPU devices
that is neither PCI nor legacy VGA, so neither the kernel's boot_vga
logic matches it, nor Xorg's commit in question.
I agree that it should be possible to add more logic to Xorg to detect
this kind device as primary. However, I share Daniel's worry that it
wouldn't cover all user space apps -- I see "Wayland this, Wayland that"
on reddit every week.
Having practically zero background in gfx development (either kernel or
Xorg), I think the problem is that vga_default_device() /
vga_set_default_device(), which -- apparently -- "boot_vga" is based
upon, come from "drivers/gpu/vga/vgaarb.c". Namely, the concept of
"primary / boot display device" is tied to the VGA arbiter, plus only a
PCI device can currently be marked as primary/boot display device.

You're right, the issue is that the primary/boot device is tied to the
VGA arbiter.

Post by Laszlo Ersek
Can these concepts be split from each other? (I can fully imagine that
this would result in a userspace visible interface change (or addition),
so that e.g. "/sys/devices/**/boot_gpu" would have to be consulted by
display servers.)

Yes, they can be split or a way of marking the default vga device that
doesn't depend on the arbiter can be added.

(But there is some question about what it actually means to be a boot
vga card - it's better defined on an x86 system, but on a ppc or arm64
system we're reduced to guessing based on the first driver loaded.)

Post by Laszlo Ersek
(Sorry if I'm totally wrong.)
... Hm, reading the thread starter at
and the references within... It looks like this work is motivated by
hardware that is supposed to be PCI, but actually breaks the specs. Is
that correct? If so, then I don't think I can suggest anything useful.
Specs exist so that hardware vendors and software authors follow them.
If hardware does not conform, then software should either refuse to work
with it, or handle it with quirks, on a case-by-case basis. I guess this
means that I don't agree with the
broad[] suggest[ion] that a more generic solution would be better
which seems to disqualify me from the discussion, as it must have been
suggested by people with incomparably more experience than what I have :)

Originally this was brought to the fore through a PCI bridge that wasn't
spec compliant, and originally I proposed a simple quirk: [0]. However,
that highlighted the related issue that platforms that don't use legacy
resources still go through the VGA arbiter process which is built around
legacy resource arbitration. Changing that behaviour also fixes the
issue with the non-spec-compliant bridge because the new model doesn't
rely upon the particular part of the spec that the bridge violates.

I'm not fussy about how we solve this problem, so long as we solve this
problem somehow.

Regards,
Daniel

[0]: https://patchwork.ozlabs.org/patch/787003/

Post by Laszlo Ersek
Thanks
Laszlo