Re: [PATCH v2] PCI: vgaarb: Include 0x0380 devices in boot VGA selection

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/23/26 00:32, Thomas Zimmermann wrote:
> (cc dri-devel)
> 
> Hi Mario et al
> 
> Am 22.06.26 um 18:09 schrieb Mario Limonciello:
>> +Thomas Zimmermann
> 
> Thanks.
> 
>>
>> Here is the lore link for you for full context.
>>
>> Aaron's v1: https://lore.kernel.org/linux- 
>> pci/20260618081803.2790848-1-aaron.ma@canonical.com/
>> My proposal: https://lore.kernel.org/linux- 
>> pci/20260621175230.1453114-1-mario.limonciello@amd.com/
> 
> I saw this patch. I was slightly confused, because it says 'fbcon'.
> 
>> Aaron's v2: https://lore.kernel.org/linux- 
>> pci/20260622061339.3306046-1-aaron.ma@canonical.com/
>>
>> On 6/21/26 23:13, Aaron Ma wrote:
>>> Some firmware boot displays use PCI class 0x0380
>>> (PCI_CLASS_DISPLAY_OTHER). vgaarb only registers pci_is_vga()
>>> devices, so those devices are skipped by vga_is_firmware_default()
>>> and cannot become vga_default_device().
> 
> I think the current behavior is correct. The vgaarb is for mediating 
> VGA's hard-coded I/O addresses among multiple devices. IMHO the module 
> should not manage other graphics devices.
> 
> IOW, I think a fix should mostly be outside of vgaarb.c.
> 
>>>
>>> On hybrid systems this can leave a discrete VGA GPU as
>>> vga_default_device() even when the firmware framebuffer is on the
>>> 0x0380 device. That makes boot_vga point at the wrong GPU and can
>>> make boot_display report multiple devices through different paths.
> 
> I assume that boot_display is true for both cards because one device has 
> the screen_info settings,  and the other is the 'vga_default_device', 
> Right?
> 
>>>
>>> Register legacy VGA and 0x0380 display devices for default selection
>>> and boot_vga visibility. Keep legacy VGA decode ownership limited to
>>> pci_is_vga() devices so 0x0380 devices are not treated as legacy VGA
>>> decoders.
>>>
>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>>> ---
>>> Changes in v2:
>>> - Keep legacy VGA decodes and owns limited to pci_is_vga() devices.
>>> - Keep 0x0380 devices eligible for firmware/default selection and
>>>    boot_vga visibility without treating them as legacy VGA decoders.
>>> - Update the commit message to describe the boot_vga compatibility goal.
>>
>> I really think dropping the fallback path (we found nothing, so this 
>> VGA thing must be it) is the right direction.
>>
>> I'd like Thomas' comments about that though.
>>
>> If we must keep that path, how about instead modify how 
>> vga_is_firmware_default() works.  We could instead have 
>> vga_has_firmware_default() and vga_is_firmware_default() both.
>>
>> If the system vga_has_firmware_default() then no longer activate the 
>> fallback path.
>>
>>>
>>>   drivers/pci/pci-sysfs.c |  3 ++-
>>>   drivers/pci/vgaarb.c    | 27 +++++++++++++++------------
>>>   include/linux/pci.h     | 14 ++++++++++++++
>>>   3 files changed, 31 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>> index d37860841260c..843d83ec9550a 100644
>>> --- a/drivers/pci/pci-sysfs.c
>>> +++ b/drivers/pci/pci-sysfs.c
>>> @@ -1717,7 +1717,8 @@ 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_vga_or_other_display(pdev))
>>>           return a->mode;
>>
>> I really don't think we want boot_vga exported on a non-VGA device.
>> Even if the rest of the PR hangs around, this hunk I think should go.
> 
> I agree. Boot_vga does not mean that there's a mode installed, but 
> rather that the VGA device got posted by the BIOS/UEFI. That's clearly 
> not the case for the other-display device.
> 
> 
>>
>>>       if (a == &dev_attr_serial_number.attr && pci_get_dsn(pdev))
>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>> index c360eee11dd9e..56a86779e6d28 100644
>>> --- a/drivers/pci/vgaarb.c
>>> +++ b/drivers/pci/vgaarb.c
>>> @@ -741,6 +741,7 @@ static bool vga_arbiter_add_pci_device(struct 
>>> pci_dev *pdev)
>>>       unsigned long flags;
>>>       struct pci_bus *bus;
>>>       struct pci_dev *bridge;
>>> +    bool legacy_vga = pci_is_vga(pdev);
> 
> Just a word on terminology. AFAIU in this context 'legacy' does not mean 
> that VGA is legacy. Legacy refers to VGA's original hard-coded I/O 
> ranges (e.g, 0xa0000, 0x3c2, etc.) .
> 
> 
> 
> I think Mario's suggestion at [1] might be a better way forward than 
> modifying vgaarb. For boot_vga, we want a VGA device. For the primary 
> device, we want the device with the pre-configured framebuffer on it. So 
> I'd do something like this in video_is_primary_device():
> 
> if (screen_info is_valid) {
>      // do the screen_info test from [2]
> } else {
>      // fall back to the default vga device. [3]
> }
> 
> This should get us at most one primary graphics device; preferably the 
> one with the framebuffer. With no framebuffer set up, using the default 
> VGA seems a sensible choice. We can still have distinct boot_display and 
> boot_vga flags, because this is what the system looks like.
> 
> And with that, another patch has to go into the Xorg server. Support for 
> boot_display got added in commit 41c58693c ("Add check for 
> `pci_device_linux_sysfs_boot_display()`") [4] AFAICT.  This needs to be 
> reworks a bit, so that it first tests for boot_display; and then falls 
> back to boot_vga if the former did not exist.

Makes sense.

Here is an updated kernel patch:

https://lore.kernel.org/linux-pci/20260623141505.1816786-1-mario.limonciello@amd.com/T/#u

Here is a change to xserver:

https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2242