[RFC PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter
Mario Limonciello
superm1 at kernel.org
Tue Jun 10 13:56:19 UTC 2025
On 6/10/2025 6:35 AM, Thomas Zimmermann wrote:
> Hi
>
> Am 09.06.25 um 04:24 schrieb Mario Limonciello:
>> From: Mario Limonciello <mario.limonciello at 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)
>
> My understanding of vgaarb is that it manages concurrent usage of the
> fixed VGA I/O ports. So are these actually VGA devices? I'm concerned
> about vgaarb handling devices that aren't VGA and possible side effects
> of that.
No; neither is a VGA device. There was a suggestion to do this from
userspace in libpciaccess [1] but Dave Airlie suggested it would be
better to adjust the "meaning" of boot_vga, which is essentially what
this RFC does.
[1] https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/37
>
> As a side note, there's also video_is_primary_device(), which we use for
> fbcon and which also uses vga_default_device() on x86. [1] This helper
> should likely return the same value as sysfs' boot_vga attribute.
>
> [1] https://elixir.bootlin.com/linux/v6.15.1/C/ident/
> video_is_primary_device
>
>>
>> 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]
>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>> ---
>> drivers/pci/pci-sysfs.c | 2 +-
>> drivers/pci/vgaarb.c | 53 ++++++++++++++++++++++++++---------------
>> include/linux/pci.h | 15 ++++++++++++
>> 3 files changed, 50 insertions(+), 20 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..8281144747487 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -139,7 +139,7 @@ void vga_set_default_device(struct pci_dev *pdev)
>> {
>> if (vga_default == pdev)
>> return;
>> -
>> + vgaarb_info(&pdev->dev, "selecting as VGA boot device\n");
>
> vgaarb_dbg() please.
>
>> pci_dev_put(vga_default);
>> vga_default = pci_dev_get(pdev);
>> }
>> @@ -676,9 +676,9 @@ static bool vga_is_boot_device(struct vga_device
>> *vgadev)
>> }
>> /*
>> - * Vgadev has neither IO nor MEM enabled. If we haven't found any
>> - * other VGA devices, it is the best candidate so far.
>> - */
>> + * Vgadev has neither IO nor MEM enabled. If we haven't found any
>> + * other VGA devices, it is the best candidate so far.
>> + */
>
> This should be a separate patch.
>
> Best regards
> Thomas
>
>> if (!boot_vga)
>> return true;
>> @@ -751,6 +751,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,13 +841,6 @@ 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 */
>> @@ -833,6 +851,7 @@ static bool vga_arbiter_add_pci_device(struct
>> pci_dev *pdev)
>> 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 +1518,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 +1567,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
>
More information about the dri-devel
mailing list