PCI/VGA: Look at all PCI display devices in VGA arbiter
PATCH 1/1 PCI/VGA: Look at all PCI display devices in VGA arbiter
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "boot_vga" but that is what some environments use to select a GPU [1].
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)
So neither device gets the boot_vga sysfs file. The vga_is_boot_device() function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.
Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
This will cause all GPUs to gain a boot_vga file, but the correct device (APU in this case) will now show 1 and the incorrect device shows 0. Userspace then picks the right device as well.
---RFC->v1:
* Add tag
* Drop unintended whitespace change
* Use vgaarb_dbg instead of vgaarb_info
--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/vgaarb.c | 48 +++++++++++++++++++++++++++-------------- include/linux/pci.h | 15 +++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)
drivers/pci/pci-sysfs.c +1 -1
@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,struct device *dev =kobj_to_dev (kobj);struct pci_dev *pdev =to_pci_dev (dev); -if (a == &dev_attr_boot_vga.attr &&pci_is_vga (pdev)) +if (a == &dev_attr_boot_vga.attr &&pci_is_display (pdev))return a->mode;return 0 ;
drivers/pci/vgaarb.c +32 -16
87 lines not on this page · raw message
include/linux/pci.h +15 -0
@@ -744,6 +744,21 @@ static inline bool pci_is_vga(struct pci_dev *pdev)return false; } +/** +* pci_is_display - Check if a PCI device is a display controller +* @pdev: Pointer to the PCI device structure +* +* This function determines whether the given PCI device corresponds +* to a display controller. Display controllers are typically used +* for graphical output and are identified based on their class code. +* +* Return: true if the PCI device is a display controller, false otherwise. +*/ +static inline bool pci_is_display (struct pci_dev *pdev) +{ +return (pdev->class >>16 ) ==PCI_BASE_CLASS_DISPLAY ; +} +#define for_each_pci_bridge (dev, bus) \ list_for_each_entry(dev, &bus->devices, bus_list) \ if (!pci_is_bridge(dev)) {} else
Signature · 1 line
2.43.0
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].
What is "A+N" and "APU"?
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?
Quoted 13 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.
Quoted 7 lines
Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.
What determines whether the APU is the "correct" device?
Quoted 32 lines
Signed-off-by Mario Limonciello <[email protected]>---
RFC->v1:
* Add tag
* Drop unintended whitespace change
* Usevgaarb_dbginstead ofvgaarb_info--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/vgaarb.c | 48 +++++++++++++++++++++++++++-------------- include/linux/pci.h | 15 +++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)drivers/pci/pci-sysfs.c +1
-1@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,struct device *dev =kobj_to_dev (kobj);struct pci_dev *pdev =to_pci_dev (dev); -if (a == &dev_attr_boot_vga.attr &&pci_is_vga (pdev)) +if (a == &dev_attr_boot_vga.attr &&pci_is_display (pdev))return a->mode;return 0 ;drivers/pci/vgaarb.c +0
-0
drivers/pci/vgaarb.c void vga_set_default_device(struct pci_dev *pdev) · line 143
Quoted 4 lines
drivers/pci/vgaarb.c +1
-0@@ -140,6 +140,7 @@ void vga_set_default_device(struct pci_dev *pdev)if (vga_default == pdev)return ; +vgaarb_dbg (&pdev->dev,"selecting as VGA boot device\n" );
I guess this is essentially a move of the vgaarb_info() message from vga_arbiter_add_pci_device() to here? If so, I think I would preserve the _info() level. Including non-VGA devices is fairly subtle and I don't think we need to discard potentially useful information about what we're doing.
drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 756
Quoted 9 lines
drivers/pci/vgaarb.c +2
-0pci_dev_put (vga_default); vga_default =pci_dev_get (pdev); }@@ -751,6 +752,31 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)vgaarb_info (&vgadev->pdev->dev,"no bridge control possible\n" ); } +static +void vga_arbiter_select_default_device (void )
Signature on one line.
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;
drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}
It looks like this is related to the integrated GPU code in vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?
It seems like this patch would change the selection in a way that makes some of the vga_is_boot_device() comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?
The end of vga_is_boot_device() mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there because vga_arb_device_init() and pci_notify() only call vga_arbiter_add_pci_device() for VGA devices.
Quoted 11 lines
drivers/pci/vgaarb.c +0
-4/* * Currently, we assume that the "initial" setup of the system is not sane, * that is, we come up with conflicting devices and let the arbiter's @@ -816,23 +842,17 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } -if (vga_is_boot_device (vgadev)) { -vgaarb_info (&pdev->dev,"setting as boot VGA device%s\n" , -vga_default_device () ? -" (overriding previous)" :"" );
drivers/pci/vgaarb.c static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) · line 850
Quoted 10 lines
drivers/pci/vgaarb.c +1
-4-vga_set_default_device (pdev); - } -vga_arbiter_check_bridge_sharing (vgadev);/* Add to the list */ list_add_tail (&vgadev->list, &vga_list); vga_count++; - vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", + vgaarb_dbg(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n",
Looks like an unrelated change.
Quoted 9 lines
drivers/pci/vgaarb.c +1
-0vga_iostate_to_str (vgadev->decodes),vga_iostate_to_str (vgadev->owns), vga_iostate_to_str(vgadev->locks)); +vga_arbiter_select_default_device ();spin_unlock_irqrestore (&vga_lock, flags);return true; fail:
drivers/pci/vgaarb.c static int pci_notify(struct notifier_block *nb, unsigned long action, · line 1524
Quoted 8 lines
drivers/pci/vgaarb.c +2
-2@@ -1499,8 +1519,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,vgaarb_dbg (dev,"%s\n" , __func__); -/* Only deal with VGA class devices */ -if (!pci_is_vga (pdev)) +/* Only deal with display devices */ +if (!pci_is_display (pdev))return 0 ;
Are there any other pci_is_vga() users that might want pci_is_display() instead? virtio_gpu_pci_quirk()?
drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 13 lines
drivers/pci/vgaarb.c +2
-6/* @@ -1548,12 +1568,8 @@ static int __init vga_arb_device_init(void)/* Add all VGA class PCI devices by default */ pdev =NULL ; -while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);
I guess pci_get_base_class(PCI_BASE_CLASS_DISPLAY) is sort of a source code optimization and this one-line change would be equivalent?
- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);
If so, I think I prefer the one-liner because then everything in this file would use pci_is_display() and we wouldn't have to figure out the equivalent-ness of pci_get_base_class(PCI_BASE_CLASS_DISPLAY).
Quoted 14 lines
drivers/pci/vgaarb.c +0
-0pr_info ("loaded\n" );return rc;include/linux/pci.h +4
-0@@ -744,6 +744,21 @@ static inline bool pci_is_vga(struct pci_dev *pdev)return false; } +/** +* pci_is_display - Check if a PCI device is a display controller +* @pdev: Pointer to the PCI device structure +*
include/linux/pci.h static inline bool pci_is_vga(struct pci_dev *pdev) · line 760
Quoted 10 lines
include/linux/pci.h +10
-0+* This function determines whether the given PCI device corresponds +* to a display controller. Display controllers are typically used +* for graphical output and are identified based on their class code. +* +* Return: true if the PCI device is a display controller, false otherwise. +*/ +static inline bool pci_is_display (struct pci_dev *pdev) +{ +return (pdev->class >>16 ) ==PCI_BASE_CLASS_DISPLAY ; +}
Could use in vga_switcheroo_client_probe_defer(), IS_GFX_DEVICE(), vfio_pci_is_intel_display(), i915_gfx_present(), get_bound_vga(). Arguable whether it's worth changing them, but I guess it's nice to test for the same thing the same way.
Bjorn
Quoted 9 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.
Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?
Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a boot_vga file. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.
Quoted 18 lines
Quoted 18 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
Quoted 5 lines
Quoted 3 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 36 lines
Quoted 36 lines
Signed-off-by Mario Limonciello <[email protected]>---
RFC->v1:
* Add tag
* Drop unintended whitespace change
* Usevgaarb_dbginstead ofvgaarb_info--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/vgaarb.c | 48 +++++++++++++++++++++++++++-------------- include/linux/pci.h | 15 +++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)drivers/pci/pci-sysfs.c +1
-1@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,struct device *dev =kobj_to_dev (kobj);struct pci_dev *pdev =to_pci_dev (dev); -if (a == &dev_attr_boot_vga.attr &&pci_is_vga (pdev)) +if (a == &dev_attr_boot_vga.attr &&pci_is_display (pdev))return a->mode;return 0 ;drivers/pci/vgaarb.c +0
-0@@ -140,6 +140,7 @@ void vga_set_default_device(struct pci_dev *pdev)if (vga_default == pdev)return ;
Quoted 7 lines
drivers/pci/vgaarb.c void vga_set_default_device(struct pci_dev *pdev) · line 143
Quoted 1 line
drivers/pci/vgaarb.c +1
-0+vgaarb_dbg (&pdev->dev,"selecting as VGA boot device\n" );I guess this is essentially a move of the
vgaarb_info()message fromvga_arbiter_add_pci_device()to here? If so, I think I would preserve the_info()level. Including non-VGA devices is fairly subtle and I don't think we need to discard potentially useful information about what we're doing.
Thanks - that was my original RFC before I sent this as PATCH but Thomas had suggested to decrease to debug. I'll restore in the next spin.
Quoted 8 lines
Quoted 8 lines
drivers/pci/vgaarb.c +0
-0pci_dev_put (vga_default); vga_default =pci_dev_get (pdev); }@@ -751,6 +752,31 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)vgaarb_info (&vgadev->pdev->dev,"no bridge control possible\n" ); }
Quoted 5 lines
drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 756
Quoted 2 lines
drivers/pci/vgaarb.c +2
-0+static +void vga_arbiter_select_default_device (void )Signature on one line.
Ack
Quoted 32 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?
Quoted 4 lines
The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.
Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
Quoted 17 lines
Quoted 17 lines
drivers/pci/vgaarb.c +0
-7/* * Currently, we assume that the "initial" setup of the system is not sane, * that is, we come up with conflicting devices and let the arbiter's @@ -816,23 +842,17 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } -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); - } -vga_arbiter_check_bridge_sharing (vgadev);
Quoted 7 lines
drivers/pci/vgaarb.c static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) · line 850
Quoted 5 lines
drivers/pci/vgaarb.c +1
-1/* Add to the list */ list_add_tail (&vgadev->list, &vga_list); vga_count++; - vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", + vgaarb_dbg(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n",Looks like an unrelated change.
Yeah it was going with the theme from Thomas' comment to decrease to debug. I'll put it back to info.
Quoted 13 lines
Quoted 13 lines
drivers/pci/vgaarb.c +1
-0vga_iostate_to_str (vgadev->decodes),vga_iostate_to_str (vgadev->owns), vga_iostate_to_str(vgadev->locks)); +vga_arbiter_select_default_device ();spin_unlock_irqrestore (&vga_lock, flags);return true; fail:@@ -1499,8 +1519,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,vgaarb_dbg (dev,"%s\n" , __func__);
Quoted 8 lines
drivers/pci/vgaarb.c static int pci_notify(struct notifier_block *nb, unsigned long action, · line 1524
Quoted 5 lines
drivers/pci/vgaarb.c +2
-2-/* Only deal with VGA class devices */ -if (!pci_is_vga (pdev)) +/* Only deal with display devices */ +if (!pci_is_display (pdev))return 0 ;Are there any other
pci_is_vga()users that might wantpci_is_display()instead?virtio_gpu_pci_quirk()?
+AlexW for comments on potential virtio changes here.
If there are sensible changes they should be another patch, and also I'll split the creation of pci_is_display() helper to it's own patch.
Quoted 18 lines
Quoted 4 lines
drivers/pci/vgaarb.c +0
-0/* @@ -1548,12 +1568,8 @@ static int __init vga_arb_device_init(void)drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 10 lines
drivers/pci/vgaarb.c +2
-6/* Add all VGA class PCI devices by default */ pdev =NULL ; -while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?
Quoted 7 lines
- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class() is a search function. It only really makese sense for iterating.
Quoted 31 lines
Quoted 15 lines
drivers/pci/vgaarb.c +0
-0pr_info ("loaded\n" );return rc;include/linux/pci.h +4
-0@@ -744,6 +744,21 @@ static inline bool pci_is_vga(struct pci_dev *pdev)return false; } +/** +* pci_is_display - Check if a PCI device is a display controller +* @pdev: Pointer to the PCI device structure +* include/linux/pci.h static inline bool pci_is_vga(struct pci_dev *pdev) · line 760
Quoted 10 lines
include/linux/pci.h +10
-0+* This function determines whether the given PCI device corresponds +* to a display controller. Display controllers are typically used +* for graphical output and are identified based on their class code. +* +* Return: true if the PCI device is a display controller, false otherwise. +*/ +static inline bool pci_is_display (struct pci_dev *pdev) +{ +return (pdev->class >>16 ) ==PCI_BASE_CLASS_DISPLAY ; +}Could use in
vga_switcheroo_client_probe_defer(),IS_GFX_DEVICE(),vfio_pci_is_intel_display(),i915_gfx_present(),get_bound_vga(). Arguable whether it's worth changing them, but I guess it's nice to test for the same thing the same way.
Quoted 1 line
Bjorn
Sure - this makes a stronger argument to add pci_is_display helper in it's own patch instead of with this one. I'll intend to have the first patch introduce the helper and replace all existing users with it.
Quoted 9 lines · Mario Limonciello
Quoted 8 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].
Quoted 5 lines
Quoted 1 line
What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.
So I guess "APU is not being selected" refers to the AMD APU?
Quoted 26 lines
Quoted 3 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.Quoted 17 lines
Quoted 17 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
How does the code figure out which is connected to the eDP panel? I didn't see anything in the patch that I can relate to this. This needs to be something people who are not AMD and NVIDIA experts can figure out in five years.
It feels like we're fixing a point problem and next month's system might have the opposite issue, and we won't know how to make both systems work.
Quoted 37 lines
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 32 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?
Quoted 8 lines
Quoted 4 lines
The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
How do we break the tie? I guess we use the first one we find?
The comment in vga_is_boot_device() says we expect only a single integrated GPU, but I guess this system breaks that assumption?
And in the absence of other clues, vga_is_boot_device() decides that every integrated GPU it finds should be the default, so the last one wins? But now we want the first one to win?
Quoted 16 lines
Quoted 16 lines
drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 8 lines
drivers/pci/vgaarb.c +2
-6-while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);
Quoted 6 lines
Quoted 3 lines
If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class()is a search function. It only really makes sense for iterating.
Right I'm saying that if you do this:
while ((pdev =
pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
PCI_ANY_ID, pdev)) != NULL) {
if (pci_is_display(pdev))
vga_arbiter_add_pci_device(pdev);
}the patch is a bit smaller and we don't have to look up pci_get_base_class() and confirm that it returns things for which pci_is_display() is true. That's just a little more analysis.
Bjorn
+Daniel Dadap from NV
+Hans
Here's the whole thread for you guys for context.
https://lore.kernel.org/linux-pci/20250613203130.GA974345@bhelgaas/T/#m7f7bbc3f048f43e698fec4cccc9e5a3bad6ee645
Quoted 10 lines · Bjorn Helgaas
Quoted 9 lines · Mario Limonciello
Quoted 8 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.So I guess "APU is not being selected" refers to the AMD APU?
Yeah that's right.
Quoted 33 lines
Quoted 33 lines
Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.Quoted 22 lines
Quoted 13 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Quoted 7 lines
Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
Quoted 7 lines
Quoted 2 lines
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
How does the code figure out which is connected to the eDP panel? I didn't see anything in the patch that I can relate to this. This needs to be something people who are not AMD and NVIDIA experts can figure out in five years.
In this case we're using the vga_arb_integrated_gpu() to tell which GPU has ACPI_VIDEO_HID added to it. That is; it's a proxy from ACPI.
Quoted 4 lines
It feels like we're fixing a point problem and next month's system might have the opposite issue, and we won't know how to make both systems work.
Actually quite the contrary. I wrote this patch for a Strix A+N system from 2025, but I just got a bug report at drm/amd that is showing very high power consumption on an A+N system from 2023.
I'm still waiting for further details from the reporter (including testing this patch) but I want to call out a few things:
* It was on Ubuntu - which is known to have code to default to Xorg when it sees NVIDIA.
* In that case the N GPU comes first on the PCI hierarchy (just like this one).
* The NVIDIA GPU never goes into a low power state.
* That case has both the integrated GPU and NVIDIA GPU as "VGA" devices.
So I feel it's actually a really good litmus test for the logic introduced in this patch.
If you would like to look more on it:
https://gitlab.freedesktop.org/drm/amd/-/issues/4303
Quoted 43 lines
Quoted 43 lines
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 36 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.
Quoted 5 lines
Quoted 3 lines
Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
How do we break the tie? I guess we use the first one we find?
My expectation is that only one will be given the HID ACPI_VIDEO_HID by ACPI; that is vga_arb_integrated_gpu() should only return for one of them.
Quoted 3 lines
The comment in
vga_is_boot_device()says we expect only a single integrated GPU, but I guess this system breaks that assumption?
No; only the integrated GPU in the APU has ACPI_VIDEO_HID.
Quoted 4 lines
And in the absence of other clues,
vga_is_boot_device()decides that every integrated GPU it finds should be the default, so the last one wins? But now we want the first one to win?
Hopefully you see exactly why I don't see a way to do this without a tie breaker round. vga_is_boot_device() looks for things that it thinks are "better" and will mark one as the default device based on heuristics.
Now that we're looking at more devices (display devices) there are more things that will meet those heuristics and thus we need a second pass.
Quoted 33 lines
Quoted 17 lines
Quoted 17 lines
drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 9 lines
drivers/pci/vgaarb.c +2
-6-while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);Quoted 6 lines
Quoted 3 lines
If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class()is a search function. It only really makes sense for iterating.Right I'm saying that if you do this:
while ((pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pdev)) != NULL) { if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev); }
Quoted 5 lines
the patch is a bit smaller and we don't have to look up
pci_get_base_class()and confirm that it returns things for whichpci_is_display()is true. That's just a little more analysis.Bjorn
Got it; will KISS.
Hi Mario!
Quoted 63 lines · Mario Limonciello
(quoted diff) +2
-0+Daniel Dadap from NV +HansHere's the whole thread for you guys for context.
https://lore.kernel.org/linux-pci/20250613203130.GA974345@bhelgaas/T/#m7f7bbc3f048f43e698fec4cccc9e5a3bad6ee645Quoted 10 lines · Bjorn Helgaas
Quoted 9 lines · Mario Limonciello
Quoted 8 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].Quoted 7 lines
Quoted 5 lines
Quoted 1 line
What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.So I guess "APU is not being selected" refers to the AMD APU?
Yeah that's right.
Quoted 36 lines
Quoted 35 lines
Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.Quoted 17 lines
Quoted 17 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
Quoted 5 lines
Quoted 3 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
Quoted 8 lines
Quoted 4 lines
How does the code figure out which is connected to the eDP panel? I didn't see anything in the patch that I can relate to this. This needs to be something people who are not AMD and NVIDIA experts can figure out in five years.
In this case we're using the
vga_arb_integrated_gpu()to tell which GPU hasACPI_VIDEO_HIDadded to it. That is; it's a proxy from ACPI.
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place, but theoretically it is possible. It does seem like this may cause a behavior change if a system has the dGPU exposed as a VGA controller and the iGPU exposed as a 3D controller (as opposed to them both being 3D or both being VGA), but I have never seen such a system, and would be highly skeptical of how the GPUs were labeled if I did. Such a behavior change would most likely be for the better. It may be worth expanding the comment that says we're checking for an iGPU to justify the reason for using it as a heuristic.
Quoted 19 lines
Quoted 4 lines
It feels like we're fixing a point problem and next month's system might have the opposite issue, and we won't know how to make both systems work.
Actually quite the contrary. I wrote this patch for a Strix A+N system from 2025, but I just got a bug report at drm/amd that is showing very high power consumption on an A+N system from 2023.
I'm still waiting for further details from the reporter (including testing this patch) but I want to call out a few things:
* It was on Ubuntu - which is known to have code to default to Xorg when it sees NVIDIA.
* In that case the N GPU comes first on the PCI hierarchy (just like this one).
* The NVIDIA GPU never goes into a low power state.
* That case has both the integrated GPU and NVIDIA GPU as "VGA" devices.
Quoted 6 lines
So I feel it's actually a really good litmus test for the logic introduced in this patch.
If you would like to look more on it:
https://gitlab.freedesktop.org/drm/amd/-/issues/4303
Indeed, I just left a comment on this issue after you directed my attention to it, with some suggested experiments. My expectation is that on this system the iGPU should be driving the panel by default when it's in dynamic mode. Assuming that's so, the dGPU should be able to runtime suspend while idle, as long as it's been configured to do so. If power usage still looks high after confirming that the iGPU is driving the panel and the dGPU is configured to runtime suspend, then maybe there's a bug preventing it from runtime suspending, but hopefully it's just a configuration issue.
Quoted 43 lines
Quoted 43 lines
Quoted 43 lines
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 36 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
How do we break the tie? I guess we use the first one we find?
My expectation is that only one will be given the HID
ACPI_VIDEO_HIDby ACPI; that isvga_arb_integrated_gpu()should only return for one of them.
Yeah, it is not as good of a tie breaker as an actual direct query for connectedness, but like I said I don't think we have a way to do that. It's a very good heuristic though IMHO.
Quoted 59 lines
Quoted 3 lines
The comment in
vga_is_boot_device()says we expect only a single integrated GPU, but I guess this system breaks that assumption?No; only the integrated GPU in the APU has
ACPI_VIDEO_HID.Quoted 4 lines
And in the absence of other clues,
vga_is_boot_device()decides that every integrated GPU it finds should be the default, so the last one wins? But now we want the first one to win?Hopefully you see exactly why I don't see a way to do this without a tie breaker round.
vga_is_boot_device()looks for things that it thinks are "better" and will mark one as the default device based on heuristics.Now that we're looking at more devices (display devices) there are more things that will meet those heuristics and thus we need a second pass.
Quoted 33 lines
Quoted 17 lines
Quoted 17 lines
drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 9 lines
drivers/pci/vgaarb.c +2
-6-while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);Quoted 6 lines
Quoted 3 lines
If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class()is a search function. It only really makes sense for iterating.Right I'm saying that if you do this:
while ((pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pdev)) != NULL) { if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev); }Quoted 5 lines
the patch is a bit smaller and we don't have to look up
pci_get_base_class()and confirm that it returns things for whichpci_is_display()is true. That's just a little more analysis.Bjorn
Got it; will KISS.
Quoted 6 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since 71e49eccdca6.
I don't pretend to fully understand the consequences of the proposed patch, just want to highlight the regression potential on Apple machines and probably others.
Thanks,
Lukas
Quoted 15 lines · Lukas Wunner
Quoted 6 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
Quoted 7 lines
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since
71e49eccdca6.I don't pretend to fully understand the consequences of the proposed patch, just want to highlight the regression potential on Apple machines and probably others.
This is good to know. Is vga_switcheroo initialized by the time the code in this patch runs? If so, maybe we should query switcheroo to determine the GPU which is connected to the panel and favor that one, then fall back to the "iGPU is probably right" heuristic otherwise.
Quoted 4 lines
Thanks,
Lukas
Quoted 17 lines · Daniel Dadap
Quoted 16 lines · Lukas Wunner
Quoted 7 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
Quoted 8 lines
Quoted 3 lines
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since
71e49eccdca6.This is good to know. Is
vga_switcherooinitialized by the time the code in this patch runs? If so, maybe we should query switcheroo to determine the GPU which is connected to the panel and favor that one, then fall back to the "iGPU is probably right" heuristic otherwise.
Right now vga_switcheroo doesn't seem to provide a function to query the current mux state.
The driver for the mux on MacBook Pros, apple_gmux.c, can be modular, so may be loaded fairly late.
Personally I'm booting my MacBook Pro via EFI, so the GPU in use is whatever efifb is talking to. However it is possible to boot these machines in a legacy CSM mode and I don't know what the situation looks like in that case.
Thanks,
Lukas
Quoted 27 lines · Lukas Wunner
Quoted 17 lines · Daniel Dadap
Quoted 16 lines · Lukas Wunner
Quoted 7 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
Quoted 8 lines
Quoted 3 lines
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since
71e49eccdca6.This is good to know. Is
vga_switcherooinitialized by the time the code in this patch runs? If so, maybe we should query switcheroo to determine the GPU which is connected to the panel and favor that one, then fall back to the "iGPU is probably right" heuristic otherwise.
Quoted 5 lines
Right now
vga_switcheroodoesn't seem to provide a function to query the current mux state.The driver for the mux on MacBook Pros,
apple_gmux.c, can be modular, so may be loaded fairly late.
Yeah, that's what I was afraid of.
Quoted 6 lines
Personally I'm booting my MacBook Pro via EFI, so the GPU in use is whatever efifb is talking to. However it is possible to boot these machines in a legacy CSM mode and I don't know what the situation looks like in that case.
Skimming through the code, this seems like the sort of thing that the existing check in vga_is_firmware_default() is testing. I'm not familiar enough with the relevant code to intuitively know whether it is supposed to work for UEFI or legacy VGA or both (I think both?).
Mario, on the affected system, what does vga_is_firmware_default() return on each of the GPUs? (I'd expect it to return true for the iGPU and false for the dGPU, but I think the (boot_vga && boot_vga->is_firmware_default) test would cause vga_is_boot_device() to return false if the vga_default is passed into it a second time. That made sense for the way that the function was originally called, to test if the passed-in vgadev is any "better" than vga_default, and from a quick skim it doesn't seem like it would get called multiple times in the new code either, but I worry that if someone else decides they need to call vga_is_boot_device() a second time in the future it might return a false result for vga_default.)
Quoted 3 lines
Thanks,
Lukas
Quoted 48 lines · Daniel Dadap
Quoted 27 lines · Lukas Wunner
Quoted 17 lines · Daniel Dadap
Quoted 16 lines · Lukas Wunner
Quoted 7 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
Quoted 8 lines
Quoted 3 lines
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since
71e49eccdca6.This is good to know. Is
vga_switcherooinitialized by the time the code in this patch runs? If so, maybe we should query switcheroo to determine the GPU which is connected to the panel and favor that one, then fall back to the "iGPU is probably right" heuristic otherwise.Quoted 5 lines
Right now
vga_switcheroodoesn't seem to provide a function to query the current mux state.The driver for the mux on MacBook Pros,
apple_gmux.c, can be modular, so may be loaded fairly late.Yeah, that's what I was afraid of.
Quoted 6 lines
Personally I'm booting my MacBook Pro via EFI, so the GPU in use is whatever efifb is talking to. However it is possible to boot these machines in a legacy CSM mode and I don't know what the situation looks like in that case.
Skimming through the code, this seems like the sort of thing that the existing check in
vga_is_firmware_default()is testing. I'm not familiar enough with the relevant code to intuitively know whether it is supposed to work for UEFI or legacy VGA or both (I think both?).
Quoted 11 lines
Mario, on the affected system, what does
vga_is_firmware_default()return on each of the GPUs? (I'd expect it to return true for the iGPU and false for the dGPU, but I think the (boot_vga&&boot_vga->is_firmware_default) test would causevga_is_boot_device()to return false if thevga_defaultis passed into it a second time. That made sense for the way that the function was originally called, to test if the passed-in vgadev is any "better" thanvga_default, and from a quick skim it doesn't seem like it would get called multiple times in the new code either, but I worry that if someone else decides they need to callvga_is_boot_device()a second time in the future it might return a false result forvga_default.)
Right "now" on an unpatched kernel it won't run 'at all' because vga_arb_device_init() only matches VGA class devices.
Both GPUs are not VGA compatible, which is what lead to the patch in this thread starting to match display class devices too.
Quoted 61 lines · Mario Limonciello
Quoted 48 lines · Daniel Dadap
Quoted 27 lines · Lukas Wunner
Quoted 17 lines · Daniel Dadap
Quoted 16 lines · Lukas Wunner
Quoted 7 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
Quoted 8 lines
Quoted 3 lines
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since
71e49eccdca6.This is good to know. Is
vga_switcherooinitialized by the time the code in this patch runs? If so, maybe we should query switcheroo to determine the GPU which is connected to the panel and favor that one, then fall back to the "iGPU is probably right" heuristic otherwise.Quoted 5 lines
Right now
vga_switcheroodoesn't seem to provide a function to query the current mux state.The driver for the mux on MacBook Pros,
apple_gmux.c, can be modular, so may be loaded fairly late.Yeah, that's what I was afraid of.
Quoted 6 lines
Personally I'm booting my MacBook Pro via EFI, so the GPU in use is whatever efifb is talking to. However it is possible to boot these machines in a legacy CSM mode and I don't know what the situation looks like in that case.
Skimming through the code, this seems like the sort of thing that the existing check in
vga_is_firmware_default()is testing. I'm not familiar enough with the relevant code to intuitively know whether it is supposed to work for UEFI or legacy VGA or both (I think both?).Quoted 11 lines
Mario, on the affected system, what does
vga_is_firmware_default()return on each of the GPUs? (I'd expect it to return true for the iGPU and false for the dGPU, but I think the (boot_vga&&boot_vga->is_firmware_default) test would causevga_is_boot_device()to return false if thevga_defaultis passed into it a second time. That made sense for the way that the function was originally called, to test if the passed-in vgadev is any "better" thanvga_default, and from a quick skim it doesn't seem like it would get called multiple times in the new code either, but I worry that if someone else decides they need to callvga_is_boot_device()a second time in the future it might return a false result forvga_default.)
Quoted 7 lines
Right "now" on an unpatched kernel it won't run 'at all' because
vga_arb_device_init()only matches VGA class devices.Both GPUs are not VGA compatible, which is what lead to the patch in this thread starting to match display class devices too.
Sure, I was curious what vga_is_firmware_default() returns for each of the GPUs when run, regardless of whether or not it's currently being run in the existing code - I'm wondering if vga_is_firmware_default() can be a better tie breaker (or at least a first line tie breaker) than the one you have now which tests for iGPU. I kind of went off on a tangent about vga_is_boot_device() being possibly unreliable for a second time caller when I was checking the callers of vga_is_firmware_default().
Quoted 70 lines · Daniel Dadap
Quoted 61 lines · Mario Limonciello
Quoted 48 lines · Daniel Dadap
Quoted 27 lines · Lukas Wunner
Quoted 17 lines · Daniel Dadap
Quoted 16 lines · Lukas Wunner
Quoted 7 lines · Daniel Dadap
Ideally we'd be able to actually query which GPU is connected to the panel at the time we're making this determination, but I don't think there's a uniform way to do this at the moment. Selecting the integrated GPU seems like a reasonable heuristic, since I'm not aware of any systems where the internal panel defaults to dGPU connection, since that would defeat the purpose of having a hybrid GPU system in the first place
Intel-based dual-GPU MacBook Pros boot with the panel switched to the dGPU by default. This is done for Windows compatibility because Apple never bothered to implement dynamic GPU switching on Windows.
The boot firmware can be told to switch the panel to the iGPU via an EFI variable, but that's not something the kernel can or should depend on.
Quoted 8 lines
Quoted 3 lines
MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is switched to the dGPU on boot, but the kernel is now unhiding it since
71e49eccdca6.This is good to know. Is
vga_switcherooinitialized by the time the code in this patch runs? If so, maybe we should query switcheroo to determine the GPU which is connected to the panel and favor that one, then fall back to the "iGPU is probably right" heuristic otherwise.Quoted 5 lines
Right now
vga_switcheroodoesn't seem to provide a function to query the current mux state.The driver for the mux on MacBook Pros,
apple_gmux.c, can be modular, so may be loaded fairly late.Yeah, that's what I was afraid of.
Quoted 6 lines
Personally I'm booting my MacBook Pro via EFI, so the GPU in use is whatever efifb is talking to. However it is possible to boot these machines in a legacy CSM mode and I don't know what the situation looks like in that case.
Skimming through the code, this seems like the sort of thing that the existing check in
vga_is_firmware_default()is testing. I'm not familiar enough with the relevant code to intuitively know whether it is supposed to work for UEFI or legacy VGA or both (I think both?).Quoted 11 lines
Mario, on the affected system, what does
vga_is_firmware_default()return on each of the GPUs? (I'd expect it to return true for the iGPU and false for the dGPU, but I think the (boot_vga&&boot_vga->is_firmware_default) test would causevga_is_boot_device()to return false if thevga_defaultis passed into it a second time. That made sense for the way that the function was originally called, to test if the passed-in vgadev is any "better" thanvga_default, and from a quick skim it doesn't seem like it would get called multiple times in the new code either, but I worry that if someone else decides they need to callvga_is_boot_device()a second time in the future it might return a false result forvga_default.)Quoted 7 lines
Right "now" on an unpatched kernel it won't run 'at all' because
vga_arb_device_init()only matches VGA class devices.Both GPUs are not VGA compatible, which is what lead to the patch in this thread starting to match display class devices too.
Quoted 8 lines
Sure, I was curious what
vga_is_firmware_default()returns for each of the GPUs when run, regardless of whether or not it's currently being run in the existing code - I'm wondering ifvga_is_firmware_default()can be a better tie breaker (or at least a first line tie breaker) than the one you have now which tests for iGPU. I kind of went off on a tangent aboutvga_is_boot_device()being possibly unreliable for a second time caller when I was checking the callers ofvga_is_firmware_default().
That's actually a really good point. Here's the diff I tried.
drivers/pci/vgaarb.c +4 -2
@@ -816,6 +816,8 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } +pci_info (pdev,"is firmware default: %d\n" ,vga_is_firmware_default (pdev)); +if (vga_is_boot_device (vgadev)) { vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n", vga_default_device() ?
@@ -1500,7 +1502,7 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,vgaarb_dbg (dev,"%s\n" , __func__);/* Only deal with VGA class devices */ -if (!pci_is_vga (pdev)) +if (!pci_is_display (pdev))return 0 ;/*
@@ -1551,7 +1553,7 @@ static int __init vga_arb_device_init(void)while ((pdev =pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) +if (pci_is_display (pdev))vga_arbiter_add_pci_device (pdev); }
include/linux/pci.h +16 -0
@@ -744,6 +744,22 @@ static inline bool pci_is_vga(struct pci_dev *pdev)return false; } + +/** +* pci_is_display - Check if a PCI device is a display controller +* @pdev: Pointer to the PCI device structure +* +* This function determines whether the given PCI device corresponds +* to a display controller. Display controllers are typically used +* for graphical output and are identified based on their class code. +* +* Return: true if the PCI device is a display controller, false otherwise. +*/ +static inline bool pci_is_display (struct pci_dev *pdev) +{ +return (pdev->class >>16 ) ==PCI_BASE_CLASS_DISPLAY ; +} +#define for_each_pci_bridge (dev, bus) \ list_for_each_entry(dev, &bus->devices, bus_list) \ if (!pci_is_bridge(dev)) {} else
On 6.16-rc2 with that applied:
$ lspci | grep "\[03" c6:00.0 3D controller [0302]: NVIDIA Corporation Device (rev a1) c7:00.0 Display controller [0380]: Advanced Micro Devices, Inc. [AMD/ATI] Device (rev d1)
$ sudo dmesg | grep "firmware default" [ 0.379664] pci 0000:c6:00.0: is firmware default: 0 [ 0.379664] pci 0000:c7:00.0: is firmware default: 1
Which actually means that the existing code when making a second pass the correct GPU *is* getting set using that as a tie breaker.
$ sudo dmesg | grep arb [ 0.379664] pci 0000:c6:00.0: vgaarb: setting as boot VGA device [ 0.379664] pci 0000:c6:00.0: vgaarb: bridge control possible [ 0.379664] pci 0000:c6:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 0.379664] pci 0000:c7:00.0: vgaarb: setting as boot VGA device (overriding previous) [ 0.379664] pci 0000:c7:00.0: vgaarb: bridge control possible [ 0.379664] pci 0000:c7:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 0.379664] vgaarb: loaded [ 4.108302] amdgpu 0000:c7:00.0: vgaarb: deactivate vga console
IE if changing pci_dev_attrs_are_visible() to show for all display devices it /should/ work without the logic changes I had.
Quoted 211 lines · On Fri, 13 Jun 2025 14:31:10 -0500 Mario Limonciello <[email protected]> wrote:
Quoted 9 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.Quoted 18 lines
Quoted 18 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
Quoted 5 lines
Quoted 3 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 36 lines
Quoted 36 lines
Signed-off-by Mario Limonciello <[email protected]>---
RFC->v1:
* Add tag
* Drop unintended whitespace change
* Usevgaarb_dbginstead ofvgaarb_info--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/vgaarb.c | 48 +++++++++++++++++++++++++++-------------- include/linux/pci.h | 15 +++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)drivers/pci/pci-sysfs.c +1
-1@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,struct device *dev =kobj_to_dev (kobj);struct pci_dev *pdev =to_pci_dev (dev); -if (a == &dev_attr_boot_vga.attr &&pci_is_vga (pdev)) +if (a == &dev_attr_boot_vga.attr &&pci_is_display (pdev))return a->mode;return 0 ;drivers/pci/vgaarb.c +0
-0@@ -140,6 +140,7 @@ void vga_set_default_device(struct pci_dev *pdev)if (vga_default == pdev)return ;Quoted 7 lines
drivers/pci/vgaarb.c void vga_set_default_device(struct pci_dev *pdev) · line 143
Quoted 1 line
drivers/pci/vgaarb.c +1
-0+vgaarb_dbg (&pdev->dev,"selecting as VGA boot device\n" );I guess this is essentially a move of the
vgaarb_info()message fromvga_arbiter_add_pci_device()to here? If so, I think I would preserve the_info()level. Including non-VGA devices is fairly subtle and I don't think we need to discard potentially useful information about what we're doing.Thanks - that was my original RFC before I sent this as PATCH but Thomas had suggested to decrease to debug. I'll restore in the next spin.
Quoted 8 lines
Quoted 8 lines
drivers/pci/vgaarb.c +0
-0pci_dev_put (vga_default); vga_default =pci_dev_get (pdev); }@@ -751,6 +752,31 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)vgaarb_info (&vgadev->pdev->dev,"no bridge control possible\n" ); }Quoted 5 lines
drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 756
Quoted 2 lines
drivers/pci/vgaarb.c +2
-0+static +void vga_arbiter_select_default_device (void )Signature on one line.
Ack
Quoted 32 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?Quoted 4 lines
The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
Quoted 17 lines
Quoted 17 lines
drivers/pci/vgaarb.c +0
-7/* * Currently, we assume that the "initial" setup of the system is not sane, * that is, we come up with conflicting devices and let the arbiter's @@ -816,23 +842,17 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } -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); - } -vga_arbiter_check_bridge_sharing (vgadev);Quoted 7 lines
drivers/pci/vgaarb.c static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) · line 850
Quoted 5 lines
drivers/pci/vgaarb.c +1
-1/* Add to the list */ list_add_tail (&vgadev->list, &vga_list); vga_count++; - vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", + vgaarb_dbg(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n",Looks like an unrelated change.
Yeah it was going with the theme from Thomas' comment to decrease to debug. I'll put it back to info.
Quoted 13 lines
Quoted 13 lines
drivers/pci/vgaarb.c +1
-0vga_iostate_to_str (vgadev->decodes),vga_iostate_to_str (vgadev->owns), vga_iostate_to_str(vgadev->locks)); +vga_arbiter_select_default_device ();spin_unlock_irqrestore (&vga_lock, flags);return true; fail:@@ -1499,8 +1519,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,vgaarb_dbg (dev,"%s\n" , __func__);Quoted 8 lines
drivers/pci/vgaarb.c static int pci_notify(struct notifier_block *nb, unsigned long action, · line 1524
Quoted 5 lines
drivers/pci/vgaarb.c +2
-2-/* Only deal with VGA class devices */ -if (!pci_is_vga (pdev)) +/* Only deal with display devices */ +if (!pci_is_display (pdev))return 0 ;Are there any other
pci_is_vga()users that might wantpci_is_display()instead?virtio_gpu_pci_quirk()?
Quoted 1 line
+AlexW for comments on potential virtio changes here.
I can't comment on virtio_gpu, Cc +Gerd for that.
I do however take issue with the premise of this patch. The VGA arbiter is for adjusting VGA routing. If neither device is VGA, why are they registering with the VGA arbiter and what sense does it make to create a boot_vga sysfs attribute of a non-VGA device?
It seems like we're trying to adapt the kernel to create a facade for userspace when userspace should be falling back to some other means of determining a primary graphical device. For instance, is there something in UEFI that could indicate the console? ACPI? DT?
It's also a bit concerning that we're making a policy decision here that the integrated graphics is the "boot VGA" device, among two non-VGA devices. I think vgaarb has a similar legacy policy decision for VGA devices that it's stuck with, but it's not clear to me that reiterating the policy selection here is still correct. Thanks,
Alex
Quoted 70 lines
If there are sensible changes they should be another patch, and also I'll split the creation of
pci_is_display()helper to it's own patch.Quoted 18 lines
Quoted 4 lines
drivers/pci/vgaarb.c +0
-0/* @@ -1548,12 +1568,8 @@ static int __init vga_arb_device_init(void)drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 10 lines
drivers/pci/vgaarb.c +2
-6/* Add all VGA class PCI devices by default */ pdev =NULL ; -while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?Quoted 7 lines
- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class()is a search function. It only really makese sense for iterating.Quoted 31 lines
Quoted 15 lines
drivers/pci/vgaarb.c +0
-0pr_info ("loaded\n" );return rc;include/linux/pci.h +4
-0@@ -744,6 +744,21 @@ static inline bool pci_is_vga(struct pci_dev *pdev)return false; } +/** +* pci_is_display - Check if a PCI device is a display controller +* @pdev: Pointer to the PCI device structure +* include/linux/pci.h static inline bool pci_is_vga(struct pci_dev *pdev) · line 760
Quoted 10 lines
include/linux/pci.h +10
-0+* This function determines whether the given PCI device corresponds +* to a display controller. Display controllers are typically used +* for graphical output and are identified based on their class code. +* +* Return: true if the PCI device is a display controller, false otherwise. +*/ +static inline bool pci_is_display (struct pci_dev *pdev) +{ +return (pdev->class >>16 ) ==PCI_BASE_CLASS_DISPLAY ; +}Could use in
vga_switcheroo_client_probe_defer(),IS_GFX_DEVICE(),vfio_pci_is_intel_display(),i915_gfx_present(),get_bound_vga(). Arguable whether it's worth changing them, but I guess it's nice to test for the same thing the same way.Quoted 1 line
Bjorn
Sure - this makes a stronger argument to add
pci_is_displayhelper in it's own patch instead of with this one. I'll intend to have the first patch introduce the helper and replace all existing users with it.
Quoted 228 lines · Alex Williamson
Quoted 211 lines · On Fri, 13 Jun 2025 14:31:10 -0500 Mario Limonciello <[email protected]> wrote:
Quoted 9 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.Quoted 18 lines
Quoted 18 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
Quoted 5 lines
Quoted 3 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 36 lines
Quoted 36 lines
Signed-off-by Mario Limonciello <[email protected]>---
RFC->v1:
* Add tag
* Drop unintended whitespace change
* Usevgaarb_dbginstead ofvgaarb_info--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/vgaarb.c | 48 +++++++++++++++++++++++++++-------------- include/linux/pci.h | 15 +++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)drivers/pci/pci-sysfs.c +1
-1@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,struct device *dev =kobj_to_dev (kobj);struct pci_dev *pdev =to_pci_dev (dev); -if (a == &dev_attr_boot_vga.attr &&pci_is_vga (pdev)) +if (a == &dev_attr_boot_vga.attr &&pci_is_display (pdev))return a->mode;return 0 ;drivers/pci/vgaarb.c +0
-0@@ -140,6 +140,7 @@ void vga_set_default_device(struct pci_dev *pdev)if (vga_default == pdev)return ;Quoted 7 lines
drivers/pci/vgaarb.c void vga_set_default_device(struct pci_dev *pdev) · line 143
Quoted 1 line
drivers/pci/vgaarb.c +1
-0+vgaarb_dbg (&pdev->dev,"selecting as VGA boot device\n" );I guess this is essentially a move of the
vgaarb_info()message fromvga_arbiter_add_pci_device()to here? If so, I think I would preserve the_info()level. Including non-VGA devices is fairly subtle and I don't think we need to discard potentially useful information about what we're doing.Thanks - that was my original RFC before I sent this as PATCH but Thomas had suggested to decrease to debug. I'll restore in the next spin.
Quoted 8 lines
Quoted 8 lines
drivers/pci/vgaarb.c +0
-0pci_dev_put (vga_default); vga_default =pci_dev_get (pdev); }@@ -751,6 +752,31 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)vgaarb_info (&vgadev->pdev->dev,"no bridge control possible\n" ); }Quoted 5 lines
drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 756
Quoted 2 lines
drivers/pci/vgaarb.c +2
-0+static +void vga_arbiter_select_default_device (void )Signature on one line.
Ack
Quoted 32 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?Quoted 4 lines
The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
Quoted 17 lines
Quoted 17 lines
drivers/pci/vgaarb.c +0
-7/* * Currently, we assume that the "initial" setup of the system is not sane, * that is, we come up with conflicting devices and let the arbiter's @@ -816,23 +842,17 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } -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); - } -vga_arbiter_check_bridge_sharing (vgadev);Quoted 7 lines
drivers/pci/vgaarb.c static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) · line 850
Quoted 5 lines
drivers/pci/vgaarb.c +1
-1/* Add to the list */ list_add_tail (&vgadev->list, &vga_list); vga_count++; - vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", + vgaarb_dbg(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n",Looks like an unrelated change.
Yeah it was going with the theme from Thomas' comment to decrease to debug. I'll put it back to info.
Quoted 13 lines
Quoted 13 lines
drivers/pci/vgaarb.c +1
-0vga_iostate_to_str (vgadev->decodes),vga_iostate_to_str (vgadev->owns), vga_iostate_to_str(vgadev->locks)); +vga_arbiter_select_default_device ();spin_unlock_irqrestore (&vga_lock, flags);return true; fail:@@ -1499,8 +1519,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,vgaarb_dbg (dev,"%s\n" , __func__);Quoted 8 lines
drivers/pci/vgaarb.c static int pci_notify(struct notifier_block *nb, unsigned long action, · line 1524
Quoted 5 lines
drivers/pci/vgaarb.c +2
-2-/* Only deal with VGA class devices */ -if (!pci_is_vga (pdev)) +/* Only deal with display devices */ +if (!pci_is_display (pdev))return 0 ;Are there any other
pci_is_vga()users that might wantpci_is_display()instead?virtio_gpu_pci_quirk()?Quoted 1 line
+AlexW for comments on potential virtio changes here.
I can't comment on
virtio_gpu, Cc +Gerd for that.I do however take issue with the premise of this patch. The VGA arbiter is for adjusting VGA routing. If neither device is VGA, why are they registering with the VGA arbiter and what sense does it make to create a
boot_vgasysfs attribute of a non-VGA device?It seems like we're trying to adapt the kernel to create a facade for userspace when userspace should be falling back to some other means of determining a primary graphical device. For instance, is there something in UEFI that could indicate the console? ACPI? DT?
Quoted 7 lines
It's also a bit concerning that we're making a policy decision here that the integrated graphics is the "boot VGA" device, among two non-VGA devices. I think vgaarb has a similar legacy policy decision for VGA devices that it's stuck with, but it's not clear to me that reiterating the policy selection here is still correct. Thanks,
Alex
I'm with you there. That's actually exactly why the first swing at this was with a patch to userspace.
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/37
Quoted 72 lines
Quoted 71 lines
If there are sensible changes they should be another patch, and also I'll split the creation of
pci_is_display()helper to it's own patch.Quoted 18 lines
Quoted 4 lines
drivers/pci/vgaarb.c +0
-0/* @@ -1548,12 +1568,8 @@ static int __init vga_arb_device_init(void)drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 10 lines
drivers/pci/vgaarb.c +2
-6/* Add all VGA class PCI devices by default */ pdev =NULL ; -while ((pdev = -pci_get_subsys (PCI_ANY_ID ,PCI_ANY_ID ,PCI_ANY_ID , -PCI_ANY_ID , pdev)) !=NULL ) { -if (pci_is_vga (pdev)) -vga_arbiter_add_pci_device (pdev); - } +while ((pdev =pci_get_base_class (PCI_BASE_CLASS_DISPLAY , pdev))) +vga_arbiter_add_pci_device (pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?Quoted 7 lines
- if (pci_is_vga(pdev)) + if (pci_is_display(pdev)) vga_arbiter_add_pci_device(pdev);If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class()is a search function. It only really makese sense for iterating.Quoted 31 lines
Quoted 15 lines
drivers/pci/vgaarb.c +0
-0pr_info ("loaded\n" );return rc;include/linux/pci.h +4
-0@@ -744,6 +744,21 @@ static inline bool pci_is_vga(struct pci_dev *pdev)return false; } +/** +* pci_is_display - Check if a PCI device is a display controller +* @pdev: Pointer to the PCI device structure +* include/linux/pci.h static inline bool pci_is_vga(struct pci_dev *pdev) · line 760
Quoted 10 lines
include/linux/pci.h +10
-0+* This function determines whether the given PCI device corresponds +* to a display controller. Display controllers are typically used +* for graphical output and are identified based on their class code. +* +* Return: true if the PCI device is a display controller, false otherwise. +*/ +static inline bool pci_is_display (struct pci_dev *pdev) +{ +return (pdev->class >>16 ) ==PCI_BASE_CLASS_DISPLAY ; +}Could use in
vga_switcheroo_client_probe_defer(),IS_GFX_DEVICE(),vfio_pci_is_intel_display(),i915_gfx_present(),get_bound_vga(). Arguable whether it's worth changing them, but I guess it's nice to test for the same thing the same way.Quoted 1 line
Bjorn
Sure - this makes a stronger argument to add
pci_is_displayhelper in it's own patch instead of with this one. I'll intend to have the first patch introduce the helper and replace all existing users with it.
Hi
Am 13.06.25 um 23:29 schrieb Mario Limonciello:
Quoted 227 lines · Alex Williamson
Quoted 226 lines
Quoted 203 lines · On Fri, 13 Jun 2025 14:31:10 -0500 Mario Limonciello <[email protected]> wrote:
Quoted 9 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy which happens to be the NVIDIA GPU.Quoted 17 lines
Quoted 17 lines
The VGA arbiter driver only looks at devices that report as PCI display VGA class devices. Neither GPU on the system is a display VGA class device:
c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()function already has some handling for integrated GPUs by looking at the ACPI HID entries, so if all PCI display class devices are looked at it actually can detect properly with this device ordering. However if there is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all PCI display class devices. After every device is added to the arbiter list make a pass on all devices and explicitly check whether one is integrated.
Quoted 5 lines
Quoted 3 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device (APU in this case) will now show1and the incorrect device shows0. Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU stays powered on at all times. It never enters into runtime PM.
Quoted 33 lines
Quoted 33 lines
Signed-off-by Mario Limonciello <[email protected]>---
RFC->v1:
* Add tag
* Drop unintended whitespace change
* Usevgaarb_dbginstead ofvgaarb_info
---
drivers/pci/pci-sysfs.c| 2 +-
drivers/pci/vgaarb.c | 48
+++++++++++++++++++++++++++--------------
include/linux/pci.h | 15 +++++++++++++3 files changed, 48 insertions(+), 17 deletions(-)drivers/pci/pci-sysfs.c +1
-0@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj, struct device *dev = kobj_to_dev(kobj); struct pci_dev *pdev = to_pci_dev(dev); - if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev)) + if (a == &dev_attr_boot_vga.attr && pci_is_display(pdev)) return a->mode; return 0;drivers/pci/vgaarb.c +0
-0@@ -140,6 +140,7 @@ void vga_set_default_device(struct pci_dev *pdev) if (vga_default == pdev)Quoted 8 lines
drivers/pci/vgaarb.c void vga_set_default_device(struct pci_dev *pdev) · line 142
Quoted 2 lines
drivers/pci/vgaarb.c +0
-0return; + vgaarb_dbg(&pdev->dev, "selecting as VGA boot device\n");I guess this is essentially a move of the
vgaarb_info()message fromvga_arbiter_add_pci_device()to here? If so, I think I would preserve the_info()level. Including non-VGA devices is fairly subtle and I don't think we need to discard potentially useful information about what we're doing.Thanks - that was my original RFC before I sent this as PATCH but Thomas had suggested to decrease to debug. I'll restore in the next spin.
Quoted 11 lines
Quoted 9 lines
drivers/pci/vgaarb.c +0
-0pci_dev_put(vga_default); vga_default = pci_dev_get(pdev); }@@ -751,6 +752,31 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n"); }+static
+voidvga_arbiter_select_default_device(void)Signature on one line.
Ack
Quoted 32 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ + struct pci_dev *candidate = vga_default_device(); + struct vga_device *vgadev; + + list_for_each_entry(vgadev, &vga_list, list) { + if (vga_is_boot_device(vgadev)) { + /* check if one is an integrated GPU, use that if so */ + if (candidate) { + if (vga_arb_integrated_gpu(&candidate->dev)) + break; + if (vga_arb_integrated_gpu(&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+ break; + } + } else + candidate = vgadev->pdev; + } + } + + if (candidate) + vga_set_default_device(candidate); +}It looks like this is related to the integrated GPU code in vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?
It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?Quoted 4 lines
The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
Quoted 16 lines
Quoted 16 lines
/*
* Currently, we assume that the "initial" setup of the system is not sane,
* that is, we come up with conflicting devices and let the arbiter'sdrivers/pci/vgaarb.c +0
-6@@ -816,23 +842,17 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } - 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); - } - vga_arbiter_check_bridge_sharing(vgadev); /* Add to the list */Quoted 8 lines
drivers/pci/vgaarb.c static bool · line 852
Quoted 6 lines
drivers/pci/vgaarb.c +1
-1list_add_tail(&vgadev->list, &vga_list); vga_count++; - vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", + vgaarb_dbg(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n",Looks like an unrelated change.
Yeah it was going with the theme from Thomas' comment to decrease to debug. I'll put it back to info.
Quoted 10 lines
Quoted 10 lines
drivers/pci/vgaarb.c +0
-0vga_iostate_to_str (vgadev->decodes), vga_iostate_to_str(vgadev->owns), vga_iostate_to_str(vgadev->locks)); + vga_arbiter_select_default_device(); spin_unlock_irqrestore(&vga_lock, flags); return true;fail:
drivers/pci/vgaarb.c +0
-0@@ -1499,8 +1519,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action, vgaarb_dbg(dev, "%s\n", __func__);Quoted 8 lines
Quoted 5 lines
- /* Only deal with VGA class devices */
- if (!pci_is_vga(pdev))
+ /* Only deal with display devices */
+ if (!pci_is_display(pdev))
return 0;Are there any other
pci_is_vga()users that might wantpci_is_display()instead?virtio_gpu_pci_quirk()?Quoted 1 line
+AlexW for comments on potential virtio changes here.
I can't comment on
virtio_gpu, Cc +Gerd for that.I do however take issue with the premise of this patch. The VGA arbiter is for adjusting VGA routing. If neither device is VGA, why are they registering with the VGA arbiter and what sense does it make to create a
boot_vgasysfs attribute of a non-VGA device?It seems like we're trying to adapt the kernel to create a facade for userspace when userspace should be falling back to some other means of determining a primary graphical device. For instance, is there something in UEFI that could indicate the console? ACPI? DT?
It's also a bit concerning that we're making a policy decision here that the integrated graphics is the "boot VGA" device, among two non-VGA devices. I think vgaarb has a similar legacy policy decision for VGA devices that it's stuck with, but it's not clear to me that reiterating the policy selection here is still correct. Thanks,
Quoted 5 lines
Quoted 1 line
Alex
I'm with you there. That's actually exactly why the first swing at this was with a patch to userspace.
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/37
I second Alex' point. I acked the patch purely on my understanding of how it does this, but as I noted, I'd also prefer to solve this in user space. I think we should push back on this change.
Best regards
Thomas
Quoted 74 lines
Quoted 73 lines
Quoted 72 lines
If there are sensible changes they should be another patch, and also I'll split the creation of
pci_is_display()helper to it's own patch.Quoted 17 lines
drivers/pci/vgaarb.c static int __init vga_arb_device_init(void) · line 1572
Quoted 13 lines
/*
drivers/pci/vgaarb.c +2
-6@@ -1548,12 +1568,8 @@ static int __init vga_arb_device_init(void) /* Add all VGA class PCI devices by default */ pdev = NULL; - while ((pdev = - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, - PCI_ANY_ID, pdev)) != NULL) { - if (pci_is_vga(pdev)) - vga_arbiter_add_pci_device(pdev); - } + while ((pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev))) + vga_arbiter_add_pci_device(pdev);I guess
pci_get_base_class(PCI_BASE_CLASS_DISPLAY)is sort of a source code optimization and this one-line change would be equivalent?Quoted 7 lines
- if (
pci_is_vga(pdev))
+ if (pci_is_display(pdev))
vga_arbiter_add_pci_device(pdev);If so, I think I prefer the one-liner because then everything in this file would use
pci_is_display()and we wouldn't have to figure out the equivalent-ness ofpci_get_base_class(PCI_BASE_CLASS_DISPLAY).
pci_get_base_class()is a search function. It only really makese sense for iterating.Quoted 32 lines
Quoted 22 lines
drivers/pci/vgaarb.c +0
-0pr_info("loaded\n"); return rc;include/linux/pci.h +0
-0@@ -744,6 +744,21 @@ static inline bool pci_is_vga(struct pci_dev *pdev) return false; }+/**
+ *pci_is_display- Check if a PCI device is a display controller
+ * @pdev: Pointer to the PCI device structure
+ *
+ * This function determines whether the given PCI device corresponds
+ * to a display controller. Display controllers are typically used
+ * for graphical output and are identified based on their class code.
+ *
+ * Return: true if the PCI device is a display controller, false otherwise.
+ */include/linux/pci.h static inline bool pci_is_vga(struct pci_dev *pdev) · line 760
Quoted 4 lines
include/linux/pci.h +4
-0+static inline bool pci_is_display (struct pci_dev *pdev) +{ + return (pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY; +}Could use in
vga_switcheroo_client_probe_defer(),IS_GFX_DEVICE(),vfio_pci_is_intel_display(),i915_gfx_present(),get_bound_vga(). Arguable whether it's worth changing them, but I guess it's nice to test for the same thing the same way.Quoted 1 line
Bjorn
Sure - this makes a stronger argument to add
pci_is_displayhelper in it's own patch instead of with this one. I'll intend to have the first patch introduce the helper and replace all existing users with it.
Signature · 7 lines
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Quoted 253 lines · Thomas Zimmermann
Hi
Am 13.06.25 um 23:29 schrieb Mario Limonciello:
Quoted 249 lines · Alex Williamson
Quoted 247 lines
Quoted 223 lines · On Fri, 13 Jun 2025 14:31:10 -0500 Mario Limonciello <[email protected]> wrote:
Quoted 9 lines · Bjorn Helgaas
Quoted 6 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
On an A+N mobile system the APU is not being selected by some desktop environments for any rendering tasks. This is because the neither GPU is being treated as "
boot_vga" but that is what some environments use to select a GPU [1].What is "A+N" and "APU"?
A+N is meant to refer to an AMD APU + NVIDIA dGPU.
APU is an SoC that contains a lot more IP than just x86 cores. In this context it contains both integrated graphics and display IP.Quoted 4 lines
I didn't quite follow the second sentence. I guess you're saying some userspace environments use the "
boot_vga" sysfs file to select a GPU? And on this A+N system, neither device has the file?Yeah. Specifically this problem happens in Xorg that the library it uses (libpciaccess) looks for a
boot_vgafile. That file isn't found on
either GPU and so Xorg picks the first GPU it finds in the PCI heirarchy
which happens to be the NVIDIA GPU.Quoted 26 lines
Quoted 26 lines
The VGA arbiter driver only looks at devices that report as PCI display
VGA class devices. Neither GPU on the system is a display VGA class device:c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1) c6:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Device 150e (rev d1)
So neither device gets the
boot_vgasysfs file. Thevga_is_boot_device()
function already has some handling for integrated GPUs by looking at the
ACPI HID entries, so if all PCI display class devices are looked at it
actually can detect properly with this device ordering. However if there
is a different ordering it could flag the wrong device.Modify the VGA arbiter code and matching sysfs file entries to examine all
PCI display class devices. After every device is added to the arbiter list
make a pass on all devices and explicitly check whether one is integrated.Quoted 7 lines
Quoted 5 lines
This will cause all GPUs to gain a
boot_vgafile, but the correct device
(APU in this case) will now show1and the incorrect device shows0.
Userspace then picks the right device as well.What determines whether the APU is the "correct" device?
In this context is the one that is physically connected to the eDP panel. When the "wrong" one is selected then it is used for rendering.
Without this patch the net outcome ends up being that the APU display hardware drives the eDP but the desktop is rendered using the N dGPU. There is a lot of latency in doing it this way, and worse off the N dGPU
stays powered on at all times. It never enters into runtime PM.Quoted 35 lines
Quoted 35 lines
Link:
https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12[1]Signed-off-by Mario Limonciello <[email protected]>---
RFC->v1:
* Add tag
* Drop unintended whitespace change
* Usevgaarb_dbginstead ofvgaarb_info--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/vgaarb.c | 48 +++++++++++++++++++++++++++-------------- include/linux/pci.h | 15 +++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)drivers/pci/pci-sysfs.c +1
-0@@ -1707,7 +1707,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj, struct device *dev = kobj_to_dev(kobj);struct pci_dev *pdev =to_pci_dev (dev); - if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev)) +if (a == &dev_attr_boot_vga.attr &&pci_is_display (pdev))return a->mode;return 0 ;drivers/pci/vgaarb.c +0
-0@@ -140,6 +140,7 @@ void vga_set_default_device(struct pci_dev *pdev)if (vga_default == pdev)Quoted 8 lines
drivers/pci/vgaarb.c void vga_set_default_device(struct pci_dev *pdev) · line 142
Quoted 2 lines
drivers/pci/vgaarb.c +0
-0return ; +vgaarb_dbg (&pdev->dev,"selecting as VGA boot device\n" );I guess this is essentially a move of the
vgaarb_info()message fromvga_arbiter_add_pci_device()to here? If so, I think I would preserve the_info()level. Including non-VGA devices is fairly subtle and I don't think we need to discard potentially useful information about what we're doing.Thanks - that was my original RFC before I sent this as PATCH but Thomas
had suggested to decrease to debug. I'll restore in the next spin.Quoted 4 lines
Quoted 4 lines
drivers/pci/vgaarb.c +0
-0pci_dev_put (vga_default); vga_default =pci_dev_get (pdev); }@@ -751,6 +752,31 @@ static voidQuoted 8 lines
drivers/pci/vgaarb.c static void · line 757
Quoted 6 lines
drivers/pci/vgaarb.c +1
-0vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n"); } +static +void vga_arbiter_select_default_device (void )Signature on one line.
Ack
Quoted 32 lines
Quoted 12 lines
drivers/pci/vgaarb.c +12
-0+{ +struct pci_dev *candidate =vga_default_device (); +struct vga_device *vgadev; + +list_for_each_entry (vgadev, &vga_list, list) { +if (vga_is_boot_device (vgadev)) { +/* check if one is an integrated GPU, use that if so */ +if (candidate) { +if (vga_arb_integrated_gpu (&candidate->dev)) +break ; +if (vga_arb_integrated_gpu (&vgadev->pdev->dev)) { + candidate = vgadev->pdev;drivers/pci/vgaarb.c static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) · line 778
Quoted 10 lines
drivers/pci/vgaarb.c +10
-0+break ; + } + }else + candidate = vgadev->pdev; + } + } + +if (candidate) +vga_set_default_device (candidate); +}It looks like this is related to the integrated GPU code in
vga_is_boot_device(). Can this be done by updating the logic there, so it's more clear what change this patch makes?It seems like this patch would change the selection in a way that makes some of the
vga_is_boot_device()comments obsolete. E.g., "We select the default VGA device in this order"? Or "we use the *last* [integrated GPU] because that was the previous behavior"?Quoted 4 lines
The end of
vga_is_boot_device()mentions non-legacy (non-VGA) devices, but I don't remember now how we ever got there becausevga_arb_device_init()andpci_notify()only callvga_arbiter_add_pci_device()for VGA devices.Sure I'll review the comments and update. As for moving the logic I didn't see an obvious way to do this. This code is "tie-breaker" code in the case that two GPUs are otherwise ranked equally.
Quoted 18 lines
Quoted 18 lines
drivers/pci/vgaarb.c +0
-0/* * Currently, we assume that the "initial" setup of the system is not sane,
* that is, we come up with conflicting devices and let the
arbiter'sdrivers/pci/vgaarb.c +0
-6@@ -816,23 +842,17 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; } - 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); - } -vga_arbiter_check_bridge_sharing (vgadev);/* Add to the list */ Quoted 8 lines
drivers/pci/vgaarb.c static bool · line 852
Quoted 6 lines
drivers/pci/vgaarb.c +1
-1list_add_tail (&vgadev->list, &vga_list); vga_count++; - vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", + vgaarb_dbg(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n",Looks like an unrelated change.
Yeah it was going with the theme from Thomas' comment to decrease to debug. I'll put it back to info.
Quoted 10 lines
Quoted 10 lines
drivers/pci/vgaarb.c +0
-0vga_iostate_to_str (vgadev->decodes),vga_iostate_to_str (vgadev->owns), vga_iostate_to_str(vgadev->locks)); +vga_arbiter_select_default_device ();spin_unlock_irqrestore (&vga_lock, flags);return true;fail:
drivers/pci/vgaarb.c +0
-0@@ -1499,8 +1519,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,vgaarb_dbg (dev,"%s\n" , __func__);Quoted 8 lines
drivers/pci/vgaarb.c static int pci_notify(struct notifier_block *nb, unsigned long action, · line 1524
Quoted 5 lines
drivers/pci/vgaarb.c +2
-1-/* Only deal with VGA class devices */ - if (!pci_is_vga(pdev)) +/* Only deal with display devices */ + if (!pci_is_display(pdev))return 0 ;Are there any other
pci_is_vga()users that might wantpci_is_display()instead?virtio_gpu_pci_quirk()?Quoted 1 line
+AlexW for comments on potential virtio changes here.
I can't comment on
virtio_gpu, Cc +Gerd for that.I do however take issue with the premise of this patch. The VGA arbiter is for adjusting VGA routing. If neither device is VGA, why are they registering with the VGA arbiter and what sense does it make to create a
boot_vgasysfs attribute of a non-VGA device?It seems like we're trying to adapt the kernel to create a facade for userspace when userspace should be falling back to some other means of determining a primary graphical device. For instance, is there something in UEFI that could indicate the console? ACPI? DT?
It's also a bit concerning that we're making a policy decision here that the integrated graphics is the "boot VGA" device, among two non-VGA devices. I think vgaarb has a similar legacy policy decision for VGA devices that it's stuck with, but it's not clear to me that reiterating the policy selection here is still correct. Thanks,
Alex
Quoted 9 lines
Quoted 4 lines
I'm with you there. That's actually exactly why the first swing at this was with a patch to userspace.
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/37
I second Alex' point. I acked the patch purely on my understanding of how it does this, but as I noted, I'd also prefer to solve this in user space. I think we should push back on this change.
If we are solving this in userspace I think fixing it in libpciaccess is probably also the wrong place, which leaves open where is the right place?
To be honest the what GPU is driving the display at boot hint should come from the kernel, since it knows already, whether boot_vga is the best method of doing that is open to questions.
It might be better to have a bit somewhere on the drm device that shows this.
hello new UAPI.
Dave.
Hi
Am 16.06.25 um 08:50 schrieb David Airlie:
[...]
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
It's also a bit concerning that we're making a policy decision here that the integrated graphics is the "boot VGA" device, among two non-VGA devices. I think vgaarb has a similar legacy policy decision for VGA devices that it's stuck with, but it's not clear to me that reiterating the policy selection here is still correct. Thanks,
Alex
I'm with you there. That's actually exactly why the first swing at this was with a patch to userspace.
Quoted 8 lines
Quoted 5 lines
I second Alex' point. I acked the patch purely on my understanding of how it does this, but as I noted, I'd also prefer to solve this in user space. I think we should push back on this change.
If we are solving this in userspace I think fixing it in libpciaccess is probably also the wrong place, which leaves open where is the right place?
Why is libpciaccess the wrong place? I see that the boot display is not necessarily on the PCI bus. But if its not handled in libpciaccess, should it rather go into Xorg directly?
Quoted 4 lines
To be honest the what GPU is driving the display at boot hint should come from the kernel, since it knows already, whether
boot_vgais the best method of doing that is open to questions.
The proposed user-space patch looks way less intrusive than the kernel change.
Quoted 4 lines
It might be better to have a bit somewhere on the drm device that shows this.
hello new UAPI.
In the kernel, there's already video_is_primary_device. [1] On x86, it looks at vga_default_device(), on other architectures, it looks at firmware settings. If there's no default vga on x86, it could do more heuristics to figure out the boot display. The result can be exported by a boot_display file via sysfs. video_is_primary_device() is currently used by fbcon. Any changes to the helper would likely benefit fbcon as well.
Best regards
Thomas
[1] https://elixir.bootlin.com/linux/v6.15.2/source/include/asm-generic/video.h#L28
Quoted 3 lines
Dave.
Signature · 7 lines
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Quoted 21 lines · Thomas Zimmermann
Hi
Am 16.06.25 um 08:50 schrieb David Airlie:
[...]Quoted 16 lines
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
It's also a bit concerning that we're making a policy decision here that the integrated graphics is the "boot VGA" device, among two non-VGA devices. I think vgaarb has a similar legacy policy decision for VGA devices that it's stuck with, but it's not clear to me that reiterating the policy selection here is still correct. Thanks,
Quoted 5 lines
Quoted 1 line
Alex
I'm with you there. That's actually exactly why the first swing at this was with a patch to userspace.
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/37
I second Alex' point. I acked the patch purely on my understanding of how it does this, but as I noted, I'd also prefer to solve this in user space. I think we should push back on this change.
Quoted 7 lines
Quoted 3 lines
If we are solving this in userspace I think fixing it in libpciaccess is probably also the wrong place, which leaves open where is the right place?
Why is libpciaccess the wrong place? I see that the boot display is not necessarily on the PCI bus. But if its not handled in libpciaccess, should it rather go into Xorg directly?
Because this isn't generally a PCI problem, and it involves accessing ACPI stuff, who says it won't need DT stuff on other hardware?
Just seems like libpciaccess is convenient but so is the kernel, I don't think userspace is right at all. The kernel should abstract the hardware.
Quoted 6 lines
Quoted 3 lines
To be honest the what GPU is driving the display at boot hint should come from the kernel, since it knows already, whether
boot_vgais the best method of doing that is open to questions.The proposed user-space patch looks way less intrusive than the kernel change.
I don't care for intrusive, I care for correct.
Quoted 4 lines
Quoted 4 lines
It might be better to have a bit somewhere on the drm device that shows this.
Quoted 9 lines
Quoted 1 line
hello new UAPI.
In the kernel, there's already
video_is_primary_device. [1] On x86, it looks atvga_default_device(), on other architectures, it looks at firmware settings. If there's no default vga on x86, it could do more heuristics to figure out the boot display. The result can be exported by aboot_displayfile via sysfs.video_is_primary_device()is currently used by fbcon. Any changes to the helper would likely benefit fbcon as well.
We should work on that then, since fbcon also needs this info to be correct, it should go in the kernel.
Dave.
Hi,
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/pci/vgaarb.c static int pci_notify(struct notifier_block *nb, unsigned long action, · line 1524
Quoted 5 lines
drivers/pci/vgaarb.c +2
-2-/* Only deal with VGA class devices */ -if (!pci_is_vga (pdev)) +/* Only deal with display devices */ +if (!pci_is_display (pdev))return 0 ;Are there any other
pci_is_vga()users that might wantpci_is_display()instead?virtio_gpu_pci_quirk()?
Quoted 3 lines
Quoted 1 line
+AlexW for comments on potential virtio changes here.
I can't comment on
virtio_gpu, Cc +Gerd for that.
virtio-gpu comes in two variants, with and without vga compatibility mode, and they advertise them self as "display/other" and "display/vga".
The driver has some pci_is_vga() checks and they need to stay that way because only the variant with vga compatibility mode has to care about vga compat things.
Quoted 4 lines
I do however take issue with the premise of this patch. The VGA arbiter is for adjusting VGA routing. If neither device is VGA, why are they registering with the VGA arbiter and what sense does it make to create a
boot_vgasysfs attribute of a non-VGA device?
100% agree. If a device is not vga compatible, why register it with the vga arbiter in the first place?
With BIOS support starting to disappear I expect VGA compatibility in GPU will be less and less common, and the concept "primary VGA" will slowly disappear too.
Quoted 4 lines
It seems like we're trying to adapt the kernel to create a facade for userspace when userspace should be falling back to some other means of determining a primary graphical device. For instance, is there something in UEFI that could indicate the console? ACPI? DT?
I don't think there is the one source of truth here. I expect userspace (with the help of the kernel) needs collect information from multiple sources, then try make a good default choice, and of course offer ways to override that of needed.
The VGA arbiter can be one of these sources, but using it as the *only* source looks wrong to me.
From UEFI one hint is the firmware framebuffer (GOP aka efi graphics output protocol, also known as efifb).
aperture_remove_conflicting_pci_devices() will disable firmware framebuffers before the native gpu driver takes over and could record the fact as hint, so userspace can figure later which device was used to display the boot screen.
There also is the ConOut EFI variable, with the console output devices. That isn't always present though. When booting something based on edk2 it is there, when booting for example with u-boot in EFI mode it is not.
take care,
Gerd