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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 17.06.25 um 22:22 schrieb Mario Limonciello:
>
>
> On 6/17/25 2:22 PM, Alex Williamson wrote:
>> On Tue, 17 Jun 2025 12:59:10 -0500
>> Mario Limonciello <superm1@kernel.org> wrote:
>>
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> On a mobile system with an AMD integrated GPU + NVIDIA discrete GPU the
>>> AMD GPU is not being selected by some desktop environments for any
>>> rendering tasks. This is because 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 PCI 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)
>>>
>>> If the GPUs were looked at the vga_is_firmware_default() function 
>>> actually
>>> does do a good job at recognizing the case from the device used for the
>>> firmware framebuffer.
>>>
>>> Modify the VGA arbiter code and matching sysfs file entries to 
>>> examine all
>>> PCI display class devices. The existing logic stays the same.
>>>
>>> This will cause all GPUs to gain a `boot_vga` file, but the correct 
>>> device
>>> (AMD GPU 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]
>>> Suggested-by: Daniel Dadap <ddadap@nvidia.com>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/pci/pci-sysfs.c | 2 +-
>>>   drivers/pci/vgaarb.c    | 8 ++++----
>>>   2 files changed, 5 insertions(+), 5 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..63216e5787d73 100644
>>> --- a/drivers/pci/vgaarb.c
>>> +++ b/drivers/pci/vgaarb.c
>>> @@ -1499,8 +1499,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 PCI display class devices */
>>> +    if (!pci_is_display(pdev))
>>>           return 0;
>>>         /*
>>> @@ -1546,12 +1546,12 @@ static int __init vga_arb_device_init(void)
>>>         bus_register_notifier(&pci_bus_type, &pci_notifier);
>>>   -    /* Add all VGA class PCI devices by default */
>>> +    /* Add all PCI display class 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))
>>> +        if (pci_is_display(pdev))
>>>               vga_arbiter_add_pci_device(pdev);
>>>       }
>>
>> At the very least a non-VGA device should not mark that it decodes
>> legacy resources, marking the boot VGA device is only a part of what
>> the VGA arbiter does.  It seems none of the actual VGA arbitration
>> interfaces have been considered here though.
>>
>> I still think this is a bad idea and I'm not sure Thomas didn't
>> withdraw his ack in the previous round[1].  Thanks,
>
> Ah; I didn't realize that was intended to be a withdrawl.
> If there's another version of this I'll remove it.

Then let me formally withdraw the A-b.

I think this updated patch doesn't address the concerns raised in the 
previous reviews. AFAIU vgaarb is really only about VGA devices.

Best regards
Thomas

>
> Dave,
>
> What is your current temperature on this approach?
>
> Do you still think it's best for something in the kernel or is this 
> better done in libpciaccess?
>
> Mutter, Kwin, and Cosmic all handle this case in the compositor.
>
>
>>
>> Alex
>>
>> [1]https://lore.kernel.org/all/bc0a3ac2-c86c-43b8-b83f-edfdfa5ee184@suse.de/ 
>>
>>
>

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