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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

[1] 
https://lore.kernel.org/linux-pci/20260621175230.1453114-1-mario.limonciello@amd.com/ 

[2] 
https://elixir.bootlin.com/linux/v7.1.1/source/arch/x86/video/video-common.c#L49
[3] 
https://elixir.bootlin.com/linux/v7.1.1/source/arch/x86/video/video-common.c#L46
[4] https://gitlab.freedesktop.org/xorg/xserver/-/commit/41c58693c

Best regards
Thomas

>>       u16 cmd;
>>         /* Allocate structure */
>> @@ -762,21 +763,23 @@ static bool vga_arbiter_add_pci_device(struct 
>> pci_dev *pdev)
>>       }
>>       vgadev->pdev = pdev;
>>   -    /* By default, assume we decode everything */
>> -    vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
>> -              VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
>> +    /* By default, assume VGA devices decode everything */
>> +    vgadev->decodes = VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
>> +    if (legacy_vga)
>> +        vgadev->decodes |= VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
>>   -    /* By default, mark it as decoding */
>> -    vga_decode_count++;
>> +    /* By default, mark legacy VGA devices as decoding */
>> +    if (vgadev->decodes & VGA_RSRC_LEGACY_MASK)
>> +        vga_decode_count++;
>>         /*
>>        * Mark that we "own" resources based on our enables, we will
>>        * clear that below if the bridge isn't forwarding.
>>        */
>>       pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>> -    if (cmd & PCI_COMMAND_IO)
>> +    if (legacy_vga && (cmd & PCI_COMMAND_IO))
>>           vgadev->owns |= VGA_RSRC_LEGACY_IO;
>> -    if (cmd & PCI_COMMAND_MEMORY)
>> +    if (legacy_vga && (cmd & PCI_COMMAND_MEMORY))
>>           vgadev->owns |= VGA_RSRC_LEGACY_MEM;
>>         /* Check if VGA cycles can get down to us */
>> @@ -796,7 +799,7 @@ static bool vga_arbiter_add_pci_device(struct 
>> pci_dev *pdev)
>>       }
>>         if (vga_is_boot_device(vgadev)) {
>> -        vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n",
>> +        vgaarb_info(&pdev->dev, "setting as boot display device%s\n",
>>                   vga_default_device() ?
>>                   " (overriding previous)" : "");
>>           vga_set_default_device(pdev);
>> @@ -1483,8 +1486,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 legacy VGA and other display controller 
>> devices */
>> +    if (!pci_is_vga_or_other_display(pdev))
>>           return 0;
>>         /*
>> @@ -1530,12 +1533,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 legacy VGA and other display controller 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))
>> +        if (pci_is_vga_or_other_display(pdev))
>>               vga_arbiter_add_pci_device(pdev);
>>       }
>>   diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 2c4454583c115..195ec1bdac863 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -792,6 +792,20 @@ static inline bool pci_is_vga(struct pci_dev *pdev)
>>       return false;
>>   }
>>   +/**
>> + * pci_is_vga_or_other_display - check if the PCI device is VGA or 
>> 0x0380
>> + * @pdev: PCI device
>> + *
>> + * Return true for legacy VGA-compatible devices and for "other display
>> + * controller" devices.  Some firmware-selected boot display devices 
>> expose
>> + * class 0x0380 instead of PCI_CLASS_DISPLAY_VGA.
>> + */
>> +static inline bool pci_is_vga_or_other_display(struct pci_dev *pdev)
>> +{
>> +    return pci_is_vga(pdev) ||
>> +        (pdev->class >> 8) == PCI_CLASS_DISPLAY_OTHER;
>> +}
>> +
>>   /**
>>    * pci_is_display - check if the PCI device is a display controller
>>    * @pdev: PCI device
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)