[PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

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.

Link: https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12 [1]
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
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(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 268c69daa4d57..c314ee1b3f9ac 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -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;
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 78748e8d2dbae..0eb1274d52169 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -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");
 	pci_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)
+{
+	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;
+					break;
+				}
+			} else
+				candidate = vgadev->pdev;
+		}
+	}
+
+	if (candidate)
+		vga_set_default_device(candidate);
+}
+
 /*
  * 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);
 
 	/* 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",
 		vga_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__);
 
-	/* Only deal with VGA class devices */
-	if (!pci_is_vga(pdev))
+	/* Only deal with display devices */
+	if (!pci_is_display(pdev))
 		return 0;
 
 	/*
@@ -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);
 
 	pr_info("loaded\n");
 	return rc;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f3923..e77754e43c629 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
-- 
2.43.0

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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?

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

What determines whether the APU is the "correct" device?

> Link: https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12 [1]
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 268c69daa4d57..c314ee1b3f9ac 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -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;
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 78748e8d2dbae..0eb1274d52169 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -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.

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

> +{
> +	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;
> +					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.

>  /*
>   * 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);
>  
>  	/* 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.

>  		vga_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__);
>  
> -	/* 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()?

>  	/*
> @@ -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).

>  	pr_info("loaded\n");
>  	return rc;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f3923..e77754e43c629 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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;
> +}

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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello]

On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
> On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> 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.

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

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

> 
>> Link: https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12 [1]
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 268c69daa4d57..c314ee1b3f9ac 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -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;
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 78748e8d2dbae..0eb1274d52169 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -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.

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.

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

Ack

>> +{
>> +	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;
>> +					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.

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.

> 
>>   /*
>>    * 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);
>>   
>>   	/* 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.

> 
>>   		vga_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__);
>>   
>> -	/* 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()?

+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.
> 
>>   	/*
>> @@ -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).

pci_get_base_class() is a search function.  It only really makese sense 
for iterating.


> 
>>   	pr_info("loaded\n");
>>   	return rc;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 05e68f35f3923..e77754e43c629 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -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;
>> +}
> 
> 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

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.

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Bjorn Helgaas]

On Fri, Jun 13, 2025 at 02:31:10PM -0500, Mario Limonciello wrote:
> On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > 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.

So I guess "APU is not being selected" refers to the AMD 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?
> 
> 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.
> 
> > > 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.
> > 
> > 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.

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

> > > +{
> > > +	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;
> > > +					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.
> 
> 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?

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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello]

+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

On 6/13/2025 3:31 PM, Bjorn Helgaas wrote:
> On Fri, Jun 13, 2025 at 02:31:10PM -0500, Mario Limonciello wrote:
>> On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
>>> On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> 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.
> 
> So I guess "APU is not being selected" refers to the AMD APU?

Yeah that's right.

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

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.

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

> 
>> 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.
> 
>>>> +{
>>>> +	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;
>>>> +					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.
>>
>> 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.

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

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

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

Got it; will KISS.

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Alex Williamson]

On Fri, 13 Jun 2025 14:31:10 -0500
Mario Limonciello <superm1@kernel.org> wrote:

> On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:  
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> 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.
> 
> > 
> > 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.
> 
> >   
> >> 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.  
> > 
> > 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.
> 
> >   
> >> Link: https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12 [1]
> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> index 268c69daa4d57..c314ee1b3f9ac 100644
> >> --- a/drivers/pci/pci-sysfs.c
> >> +++ b/drivers/pci/pci-sysfs.c
> >> @@ -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;
> >> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> >> index 78748e8d2dbae..0eb1274d52169 100644
> >> --- a/drivers/pci/vgaarb.c
> >> +++ b/drivers/pci/vgaarb.c
> >> @@ -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.  
> 
> 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.
> 
> >   
> >>   	pci_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.
> >   
> 
> Ack
> 
> >> +{
> >> +	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;
> >> +					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.  
> 
> 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.
> 
> >   
> >>   /*
> >>    * 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);
> >>   
> >>   	/* 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.
> 
> >   
> >>   		vga_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__);
> >>   
> >> -	/* 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()?  
> 
> +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

> 
> 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.
> >   
> >>   	/*
> >> @@ -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).  
> 
> pci_get_base_class() is a search function.  It only really makese sense 
> for iterating.
> 
> 
> >   
> >>   	pr_info("loaded\n");
> >>   	return rc;
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 05e68f35f3923..e77754e43c629 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -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;
> >> +}  
> > 
> > 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  
> 
> 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.
> 

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello]

On 6/13/2025 4:19 PM, Alex Williamson wrote:
> On Fri, 13 Jun 2025 14:31:10 -0500
> Mario Limonciello <superm1@kernel.org> wrote:
> 
>> On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
>>> On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> 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.
>>
>>>
>>> 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.
>>
>>>    
>>>> 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.
>>>
>>> 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.
>>
>>>    
>>>> Link: https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12 [1]
>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>> index 268c69daa4d57..c314ee1b3f9ac 100644
>>>> --- a/drivers/pci/pci-sysfs.c
>>>> +++ b/drivers/pci/pci-sysfs.c
>>>> @@ -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;
>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>>> index 78748e8d2dbae..0eb1274d52169 100644
>>>> --- a/drivers/pci/vgaarb.c
>>>> +++ b/drivers/pci/vgaarb.c
>>>> @@ -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.
>>
>> 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.
>>
>>>    
>>>>    	pci_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.
>>>    
>>
>> Ack
>>
>>>> +{
>>>> +	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;
>>>> +					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.
>>
>> 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.
>>
>>>    
>>>>    /*
>>>>     * 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);
>>>>    
>>>>    	/* 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.
>>
>>>    
>>>>    		vga_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__);
>>>>    
>>>> -	/* 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()?
>>
>> +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

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

> 
>>
>> 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.
>>>    
>>>>    	/*
>>>> @@ -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).
>>
>> pci_get_base_class() is a search function.  It only really makese sense
>> for iterating.
>>
>>
>>>    
>>>>    	pr_info("loaded\n");
>>>>    	return rc;
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 05e68f35f3923..e77754e43c629 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -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;
>>>> +}
>>>
>>> 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
>>
>> 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.
>>
> 

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Daniel Dadap]

Hi Mario!

On Fri, Jun 13, 2025 at 03:56:03PM -0500, Mario Limonciello wrote:
> +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
> 
> On 6/13/2025 3:31 PM, Bjorn Helgaas wrote:
> > On Fri, Jun 13, 2025 at 02:31:10PM -0500, Mario Limonciello wrote:
> > > On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
> > > > On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
> > > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > > 
> > > > > 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.
> > 
> > So I guess "APU is not being selected" refers to the AMD APU?
> 
> Yeah that's right.
> 
> > 
> > > > 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.
> > > 
> > > > > 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.
> > > > 
> > > > 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.
> 
> 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.
> 

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.

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

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.
 
> > 
> > > 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.
> > 
> > > > > +{
> > > > > +	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;
> > > > > +					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.
> > > 
> > > 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.
>

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.
 
> > 
> > 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.
> 
> > 
> > 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.
> 
> > 
> > > > > -	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).
> > > 
> > > 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
> 
> Got it; will KISS.
> 

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Lukas Wunner]

On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
> 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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Thomas Zimmermann]

Hi

Am 13.06.25 um 23:29 schrieb Mario Limonciello:
> On 6/13/2025 4:19 PM, Alex Williamson wrote:
>> On Fri, 13 Jun 2025 14:31:10 -0500
>> Mario Limonciello <superm1@kernel.org> wrote:
>>
>>> On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
>>>> On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>
>>>>> 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.
>>>
>>>>
>>>> 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.
>>>
>>>>> 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.
>>>>
>>>> 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.
>>>
>>>>> Link: 
>>>>> https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12 
>>>>> [1]
>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>>> 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(-)
>>>>>
>>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>>> index 268c69daa4d57..c314ee1b3f9ac 100644
>>>>> --- a/drivers/pci/pci-sysfs.c
>>>>> +++ b/drivers/pci/pci-sysfs.c
>>>>> @@ -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;
>>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>>>> index 78748e8d2dbae..0eb1274d52169 100644
>>>>> --- a/drivers/pci/vgaarb.c
>>>>> +++ b/drivers/pci/vgaarb.c
>>>>> @@ -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.
>>>
>>> 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.
>>>
>>>>>        pci_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.
>>>
>>> Ack
>>>
>>>>> +{
>>>>> +    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;
>>>>> +                    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.
>>>
>>> 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.
>>>
>>>>>    /*
>>>>>     * 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);
>>>>>           /* 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.
>>>
>>>>> vga_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__);
>>>>>    -    /* 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()?
>>>
>>> +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
>
> 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

>
>>
>>>
>>> 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.
>>>>>        /*
>>>>> @@ -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).
>>>
>>> pci_get_base_class() is a search function.  It only really makese sense
>>> for iterating.
>>>
>>>
>>>>>        pr_info("loaded\n");
>>>>>        return rc;
>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>> index 05e68f35f3923..e77754e43c629 100644
>>>>> --- a/include/linux/pci.h
>>>>> +++ b/include/linux/pci.h
>>>>> @@ -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;
>>>>> +}
>>>>
>>>> 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
>>>
>>> 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.
>>>
>>
>

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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[David Airlie]

On Mon, Jun 16, 2025 at 4:42 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 13.06.25 um 23:29 schrieb Mario Limonciello:
> > On 6/13/2025 4:19 PM, Alex Williamson wrote:
> >> On Fri, 13 Jun 2025 14:31:10 -0500
> >> Mario Limonciello <superm1@kernel.org> wrote:
> >>
> >>> On 6/13/2025 2:07 PM, Bjorn Helgaas wrote:
> >>>> On Thu, Jun 12, 2025 at 10:12:14PM -0500, Mario Limonciello wrote:
> >>>>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>>>
> >>>>> 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.
> >>>
> >>>>
> >>>> 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.
> >>>
> >>>>> 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.
> >>>>
> >>>> 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.
> >>>
> >>>>> Link:
> >>>>> https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12
> >>>>> [1]
> >>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>>>> ---
> >>>>> 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(-)
> >>>>>
> >>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >>>>> index 268c69daa4d57..c314ee1b3f9ac 100644
> >>>>> --- a/drivers/pci/pci-sysfs.c
> >>>>> +++ b/drivers/pci/pci-sysfs.c
> >>>>> @@ -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;
> >>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> >>>>> index 78748e8d2dbae..0eb1274d52169 100644
> >>>>> --- a/drivers/pci/vgaarb.c
> >>>>> +++ b/drivers/pci/vgaarb.c
> >>>>> @@ -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.
> >>>
> >>> 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.
> >>>
> >>>>>        pci_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.
> >>>
> >>> Ack
> >>>
> >>>>> +{
> >>>>> +    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;
> >>>>> +                    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.
> >>>
> >>> 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.
> >>>
> >>>>>    /*
> >>>>>     * 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);
> >>>>>           /* 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.
> >>>
> >>>>> vga_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__);
> >>>>>    -    /* 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()?
> >>>
> >>> +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
> >
> > 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.

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Thomas Zimmermann]

Hi

Am 16.06.25 um 08:50 schrieb David Airlie:
[...]
>>>>
>>>> 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
>> 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?


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

The proposed user-space patch looks way less intrusive than the kernel 
change.

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

>
> Dave.
>

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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[David Airlie]

On Mon, Jun 16, 2025 at 5:21 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 16.06.25 um 08:50 schrieb David Airlie:
> [...]
> >>>>
> >>>> 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
> >> 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?

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.

> > 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.
>
> The proposed user-space patch looks way less intrusive than the kernel
> change.

I don't care for intrusive, I care for correct.
>
> >
> > 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.
>

We should work on that then, since fbcon also needs this info to be
correct, it should go in the kernel.

Dave.

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Gerd Hoffmann]

  Hi,

> > >> -	/* 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()?  
> > 
> > +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.

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

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.

> 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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Daniel Dadap]

On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
> On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
> > 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.

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.

> 
> Thanks,
> 
> Lukas

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Lukas Wunner]

On Mon, Jun 16, 2025 at 10:05:48AM -0500, Daniel Dadap wrote:
> On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
> > On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
> > > 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.
> 
> 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.

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

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Daniel Dadap]

On Mon, Jun 16, 2025 at 09:14:04PM +0200, Lukas Wunner wrote:
> On Mon, Jun 16, 2025 at 10:05:48AM -0500, Daniel Dadap wrote:
> > On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
> > > On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
> > > > 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.
> > 
> > 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.
> 
> 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.

Yeah, that's what I was afraid of.

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

> Thanks,
> 
> Lukas

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello]

On 6/17/25 10:53 AM, Daniel Dadap wrote:
> On Mon, Jun 16, 2025 at 09:14:04PM +0200, Lukas Wunner wrote:
>> On Mon, Jun 16, 2025 at 10:05:48AM -0500, Daniel Dadap wrote:
>>> On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
>>>> On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
>>>>> 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.
>>>
>>> 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.
>>
>> 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.
> 
> Yeah, that's what I was afraid of.
> 
>>
>> 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.)
> 

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.

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Daniel Dadap]

On Tue, Jun 17, 2025 at 11:06:00AM -0500, Mario Limonciello wrote:
> On 6/17/25 10:53 AM, Daniel Dadap wrote:
> > On Mon, Jun 16, 2025 at 09:14:04PM +0200, Lukas Wunner wrote:
> > > On Mon, Jun 16, 2025 at 10:05:48AM -0500, Daniel Dadap wrote:
> > > > On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
> > > > > On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
> > > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > Yeah, that's what I was afraid of.
> > 
> > > 
> > > 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.)
> > 
> 
> 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().

Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello]

On 6/17/25 11:36 AM, Daniel Dadap wrote:
> On Tue, Jun 17, 2025 at 11:06:00AM -0500, Mario Limonciello wrote:
>> On 6/17/25 10:53 AM, Daniel Dadap wrote:
>>> On Mon, Jun 16, 2025 at 09:14:04PM +0200, Lukas Wunner wrote:
>>>> On Mon, Jun 16, 2025 at 10:05:48AM -0500, Daniel Dadap wrote:
>>>>> On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
>>>>>> On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
>>>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> 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.
>>>
>>> Yeah, that's what I was afraid of.
>>>
>>>>
>>>> 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.)
>>>
>>
>> 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().
> 

That's actually a really good point.  Here's the diff I tried.

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 78748e8d2dba..b4e085806544 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -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);
         }

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f392..077e90a2af37 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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.