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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.06.25 um 17:37 schrieb 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@amd.com>
>>>
>>> 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@amd.com>
>>> ---
>>> 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.

Ok, makes sense. With the kernel test robot's issues fixed:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

I guess that interface also needs some sort of OK from user-space devs?

Best regards
Thomas

>
>>
>> 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@vger.kernel.org>
>>> +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);
>>>   }
>>
>

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