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

[Bjorn Helgaas <helgaas@kernel.org>]

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