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

[Mario Limonciello]

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@xxxxxxxxxxxxx/
> > My proposal: https://lore.kernel.org/linux- 
> > pci/20260621175230.1453114-1-mario.limonciello@xxxxxxx/
>
> 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@xxxxxxxxxxxxx/
> >
> > 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@xxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxx/T/#u

Here is a change to xserver:

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