Re: [PATCH v6 9/9] PCI: Add a new 'boot_display' attribute

[Mario Limonciello]

On 6/27/2025 2:07 AM, Thomas Zimmermann wrote:
> Hi
>
> Am 27.06.25 um 06:31 schrieb Mario Limonciello:
> > From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >
> > On systems with multiple GPUs there can be uncertainty which GPU is the
> > primary one used to drive the display at bootup. In order to disambiguate
> > this add a new sysfs attribute 'boot_display' that uses the output of
> > video_is_primary_device() to populate whether a PCI device was used for
> > driving the display.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > v6:
> >   * Only show for the device that is boot display
> >   * Only create after PCI device sysfs files are initialized to ensure
> >     that resources are ready.
> > v4:
> >   * new patch
> > ---
> >   Documentation/ABI/testing/sysfs-bus-pci |  8 +++++
> >   drivers/pci/pci-sysfs.c                 | 46 +++++++++++++++++++++++++
>
> The code looks good. Just one more question: could this be added 
> independently from the PCI bus (at a reasonable cost)? There are other 
> busses that can host the boot display. Alternatively, we'd add this 
> attribute per bus as needed.

It depends upon the underlying hardware implementation.  On x86 it's 
always PCI and so I realized there is a requirement that PCI resources 
are setup before screen_info event works.

That is the v5 version of this patch would have had a potential race 
condition with userspace where boot_display didn't always show '1' if 
userspace read it too quickly.

Other architecture's hardware implementation might have similar problem.

So in summary I think it would be better to do it per-bus.  If we 
realize there is indeed code duplication we can always move this to a 
common helper at that point.

> Best regards
> Thomas
>
> >   2 files changed, 54 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ 
> > ABI/testing/sysfs-bus-pci
> > index 69f952fffec72..8b455b1a58852 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -612,3 +612,11 @@ Description:
> >             # ls doe_features
> >             0001:01        0001:02        doe_discovery
> > +
> > +What:        /sys/bus/pci/devices/.../boot_display
> > +Date:        October 2025
> > +Contact:    Linux PCI developers <linux-pci@xxxxxxxxxxxxxxx>
> > +Description:
> > +        This file indicates the device was used as a boot
> > +        display. If the device was used as the boot display, the file
> > +        will be present and contain "1".
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 268c69daa4d57..cc766461de1da 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -30,6 +30,7 @@
> >   #include <linux/msi.h>
> >   #include <linux/of.h>
> >   #include <linux/aperture.h>
> > +#include <asm/video.h>
> >   #include "pci.h"
> >   #ifndef ARCH_PCI_DEV_GROUPS
> > @@ -679,6 +680,13 @@ const struct attribute_group *pcibus_groups[] = {
> >       NULL,
> >   };
> > +static ssize_t boot_display_show(struct device *dev, struct 
> > device_attribute *attr,
> > +                 char *buf)
> > +{
> > +    return sysfs_emit(buf, "1\n");
> > +}
> > +static DEVICE_ATTR_RO(boot_display);
> > +
> >   static ssize_t boot_vga_show(struct device *dev, struct 
> > device_attribute *attr,
> >                    char *buf)
> >   {
> > @@ -1246,6 +1254,37 @@ static int pci_create_attr(struct pci_dev 
> > *pdev, int num, int write_combine)
> >       return 0;
> >   }
> > +/**
> > + * pci_create_boot_display_file - create a file in sysfs for @dev
> > + * @pdev: dev in question
> > + *
> > + * Creates a file `boot_display` in sysfs for the PCI device @pdev
> > + * if it is the boot display device.
> > + */
> > +static int pci_create_boot_display_file(struct pci_dev *pdev)
> > +{
> > +#ifdef CONFIG_VIDEO
> > +    if (video_is_primary_device(&pdev->dev))
> > +        return sysfs_create_file(&pdev->dev.kobj, 
> > &dev_attr_boot_display.attr);
> > +#endif
> > +    return 0;
> > +}
> > +
> > +/**
> > + * pci_remove_boot_display_file - remove the boot display file for @dev
> > + * @pdev: dev in question
> > + *
> > + * Removes the file `boot_display` in sysfs for the PCI device @pdev
> > + * if it is the boot display device.
> > + */
> > +static void pci_remove_boot_display_file(struct pci_dev *pdev)
> > +{
> > +#ifdef CONFIG_VIDEO
> > +    if (video_is_primary_device(&pdev->dev))
> > +        sysfs_remove_file(&pdev->dev.kobj, &dev_attr_boot_display.attr);
> > +#endif
> > +}
> > +
> >   /**
> >    * pci_create_resource_files - create resource files in sysfs for @dev
> >    * @pdev: dev in question
> > @@ -1654,9 +1693,15 @@ static const struct attribute_group 
> > pci_dev_resource_resize_group = {
> >   int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
> >   {
> > +    int retval;
> > +
> >       if (!sysfs_initialized)
> >           return -EACCES;
> > +    retval = pci_create_boot_display_file(pdev);
> > +    if (retval)
> > +        return retval;
> > +
> >       return pci_create_resource_files(pdev);
> >   }
> > @@ -1671,6 +1716,7 @@ void pci_remove_sysfs_dev_files(struct pci_dev 
> > *pdev)
> >       if (!sysfs_initialized)
> >           return;
> > +    pci_remove_boot_display_file(pdev);
> >       pci_remove_resource_files(pdev);
> >   }