From: Mario Limonciello <mario.limonciello@amd.com>

The x86 specific check for whether a framebuffer belongs to a device
works for display devices as well as VGA devices.  Callers to
video_is_primary_device() can benefit from checking non-VGA display
devices.

Move the x86 specific check into x86 specific code, and adjust VGA
arbiter to call that code as well. This allows fbcon to find the
right PCI device on systems that don't have VGA devices.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/video/video-common.c | 28 +++++++++++++++++++++++++++
 drivers/pci/vgaarb.c          | 36 ++---------------------------------
 2 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/arch/x86/video/video-common.c b/arch/x86/video/video-common.c
index 81fc97a2a837a..718116e35e450 100644
--- a/arch/x86/video/video-common.c
+++ b/arch/x86/video/video-common.c
@@ -9,6 +9,7 @@
 
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/screen_info.h>
 #include <linux/vgaarb.h>
 
 #include <asm/video.h>
@@ -27,13 +28,40 @@ EXPORT_SYMBOL(pgprot_framebuffer);
 
 bool video_is_primary_device(struct device *dev)
 {
+	u64 base = screen_info.lfb_base;
+	u64 size = screen_info.lfb_size;
 	struct pci_dev *pdev;
+	struct resource *r;
+	u64 limit;
 
 	if (!dev_is_pci(dev))
 		return false;
 
 	pdev = to_pci_dev(dev);
 
+	if (!pci_is_display(pdev))
+		return false;
+
+	/* Select the device owning the boot framebuffer if there is one */
+	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+		base |= (u64)screen_info.ext_lfb_base << 32;
+
+	limit = base + size;
+
+	/* Does firmware framebuffer belong to us? */
+	pci_dev_for_each_resource(pdev, r) {
+		if (resource_type(r) != IORESOURCE_MEM)
+			continue;
+
+		if (!r->start || !r->end)
+			continue;
+
+		if (base < r->start || limit >= r->end)
+			continue;
+
+		return true;
+	}
+
 	return (pdev == vga_default_device());
 }
 EXPORT_SYMBOL(video_is_primary_device);
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 78748e8d2dbae..15ab58c70b016 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -26,12 +26,12 @@
 #include <linux/poll.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
-#include <linux/screen_info.h>
 #include <linux/vt.h>
 #include <linux/console.h>
 #include <linux/acpi.h>
 #include <linux/uaccess.h>
 #include <linux/vgaarb.h>
+#include <asm/video.h>
 
 static void vga_arbiter_notify_clients(void);
 
@@ -554,38 +554,6 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
 }
 EXPORT_SYMBOL(vga_put);
 
-static bool vga_is_firmware_default(struct pci_dev *pdev)
-{
-#if defined(CONFIG_X86)
-	u64 base = screen_info.lfb_base;
-	u64 size = screen_info.lfb_size;
-	struct resource *r;
-	u64 limit;
-
-	/* Select the device owning the boot framebuffer if there is one */
-
-	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
-		base |= (u64)screen_info.ext_lfb_base << 32;
-
-	limit = base + size;
-
-	/* Does firmware framebuffer belong to us? */
-	pci_dev_for_each_resource(pdev, r) {
-		if (resource_type(r) != IORESOURCE_MEM)
-			continue;
-
-		if (!r->start || !r->end)
-			continue;
-
-		if (base < r->start || limit >= r->end)
-			continue;
-
-		return true;
-	}
-#endif
-	return false;
-}
-
 static bool vga_arb_integrated_gpu(struct device *dev)
 {
 #if defined(CONFIG_ACPI)
@@ -623,7 +591,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 	if (boot_vga && boot_vga->is_firmware_default)
 		return false;
 
-	if (vga_is_firmware_default(pdev)) {
+	if (video_is_primary_device(&pdev->dev)) {
 		vgadev->is_firmware_default = true;
 		return true;
 	}
-- 
2.43.0

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus tiwai-sound/for-next tiwai-sound/for-linus awilliam-vfio/next awilliam-vfio/for-linus tip/x86/core linus/master v6.16-rc2 next-20250620]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PCI-Add-helper-for-checking-if-a-PCI-device-is-a-display-controller/20250620-105220
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250620024943.3415685-7-superm1%40kernel.org
patch subject: [PATCH v3 6/7] PCI/VGA: Move check for firmware default out of VGA arbiter
config: sparc-defconfig (https://download.01.org/0day-ci/archive/20250622/202506221312.49Fy1aNA-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250622/202506221312.49Fy1aNA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506221312.49Fy1aNA-lkp@intel.com/

All errors (new ones prefixed by >>):

   sparc-linux-ld: drivers/pci/vgaarb.o: in function `vga_arbiter_add_pci_device':
>> vgaarb.c:(.text+0x14ec): undefined reference to `video_is_primary_device'
>> sparc-linux-ld: vgaarb.c:(.text+0x174c): undefined reference to `video_is_primary_device'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Hi

Am 20.06.25 um 04:49 schrieb Mario Limonciello:
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> The x86 specific check for whether a framebuffer belongs to a device
> works for display devices as well as VGA devices.  Callers to
> video_is_primary_device() can benefit from checking non-VGA display
> devices.
>
> Move the x86 specific check into x86 specific code, and adjust VGA
> arbiter to call that code as well. This allows fbcon to find the
> right PCI device on systems that don't have VGA devices.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   arch/x86/video/video-common.c | 28 +++++++++++++++++++++++++++
>   drivers/pci/vgaarb.c          | 36 ++---------------------------------
>   2 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/video/video-common.c b/arch/x86/video/video-common.c
> index 81fc97a2a837a..718116e35e450 100644
> --- a/arch/x86/video/video-common.c
> +++ b/arch/x86/video/video-common.c
> @@ -9,6 +9,7 @@
>   
>   #include <linux/module.h>
>   #include <linux/pci.h>
> +#include <linux/screen_info.h>
>   #include <linux/vgaarb.h>
>   
>   #include <asm/video.h>
> @@ -27,13 +28,40 @@ EXPORT_SYMBOL(pgprot_framebuffer);
>   
>   bool video_is_primary_device(struct device *dev)

I'm not sure I understand this patch. video_is_primary_device() already 
exists for 3 architectures, including x86. [1] Adding it here should 
produce an error. (?)

[1] https://elixir.bootlin.com/linux/v6.15.2/A/ident/video_is_primary_device

The code on x86 is

bool 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/bool>video_is_primary_device 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/video_is_primary_device>(structdevice 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { 
structpci_dev 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; 
if(!dev_is_pci 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/dev_is_pci>(dev)) 
returnfalse <https://elixir.bootlin.com/linux/v6.15.2/C/ident/false>; 
pdev=to_pci_dev 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/to_pci_dev>(dev); 
return(pdev==vga_default_device 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>()); }

I was thinking about extending it to test for additional properties, 
like this

bool 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/bool>video_is_primary_device 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/video_is_primary_device>(structdevice 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { 
structpci_dev 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; 
if(!dev_is_pci 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/dev_is_pci>(dev)) 
returnfalse <https://elixir.bootlin.com/linux/v6.15.2/C/ident/false>; 
pdev=to_pci_dev 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/to_pci_dev>(dev); 
if(pdev==vga_default_device 
<https://elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>()) 
return true for_each_pci_dev() { // test if display and could be 
primary. } return false; // nothing found }


This would then be called from per-device sysfs code that export a 
property similar to boot_vga (such as boot_display).


The issue is currently just an x86 problem, but I can imagine something 
similar happening on ARM. There we'd have to go through the DT tree to 
figure out the primary device. That's a problem for a later patch set, 
but we should keep this in mind.

>   {
> +	u64 base = screen_info.lfb_base;
> +	u64 size = screen_info.lfb_size;
>   	struct pci_dev *pdev;
> +	struct resource *r;
> +	u64 limit;
>   
>   	if (!dev_is_pci(dev))
>   		return false;
>   
>   	pdev = to_pci_dev(dev);
>   
> +	if (!pci_is_display(pdev))
> +		return false;
> +
> +	/* Select the device owning the boot framebuffer if there is one */
> +	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> +		base |= (u64)screen_info.ext_lfb_base << 32;
> +
> +	limit = base + size;
> +
> +	/* Does firmware framebuffer belong to us? */
> +	pci_dev_for_each_resource(pdev, r) {
> +		if (resource_type(r) != IORESOURCE_MEM)
> +			continue;
> +
> +		if (!r->start || !r->end)
> +			continue;
> +
> +		if (base < r->start || limit >= r->end)
> +			continue;
> +
> +		return true;
> +	}
> +

You can drop all this code and call screen_info_pci_dev() instead. I 
simply never got to update vgaarb to use it.

[2] 
https://elixir.bootlin.com/linux/v6.15.2/source/drivers/video/screen_info_pci.c#L109

>   	return (pdev == vga_default_device());
>   }
>   EXPORT_SYMBOL(video_is_primary_device);
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 78748e8d2dbae..15ab58c70b016 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -26,12 +26,12 @@
>   #include <linux/poll.h>
>   #include <linux/miscdevice.h>
>   #include <linux/slab.h>
> -#include <linux/screen_info.h>
>   #include <linux/vt.h>
>   #include <linux/console.h>
>   #include <linux/acpi.h>
>   #include <linux/uaccess.h>
>   #include <linux/vgaarb.h>
> +#include <asm/video.h>
>   
>   static void vga_arbiter_notify_clients(void);
>   
> @@ -554,38 +554,6 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
>   }
>   EXPORT_SYMBOL(vga_put);
>   
> -static bool vga_is_firmware_default(struct pci_dev *pdev)
> -{
> -#if defined(CONFIG_X86)
> -	u64 base = screen_info.lfb_base;
> -	u64 size = screen_info.lfb_size;
> -	struct resource *r;
> -	u64 limit;
> -
> -	/* Select the device owning the boot framebuffer if there is one */
> -
> -	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> -		base |= (u64)screen_info.ext_lfb_base << 32;
> -
> -	limit = base + size;
> -
> -	/* Does firmware framebuffer belong to us? */
> -	pci_dev_for_each_resource(pdev, r) {
> -		if (resource_type(r) != IORESOURCE_MEM)
> -			continue;
> -
> -		if (!r->start || !r->end)
> -			continue;
> -
> -		if (base < r->start || limit >= r->end)
> -			continue;
> -
> -		return true;
> -	}
> -#endif
> -	return false;
> -}
> -
>   static bool vga_arb_integrated_gpu(struct device *dev)
>   {
>   #if defined(CONFIG_ACPI)
> @@ -623,7 +591,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>   	if (boot_vga && boot_vga->is_firmware_default)
>   		return false;
>   
> -	if (vga_is_firmware_default(pdev)) {
> +	if (video_is_primary_device(&pdev->dev)) {

Maybe not change this because you don't want to end up with non-VGA 
devices here.

Best regards
Thomas

>   		vgadev->is_firmware_default = true;
>   		return true;
>   	}

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

On 6/20/2025 3:45 AM, Thomas Zimmermann wrote:
> Hi
> 
> Am 20.06.25 um 04:49 schrieb Mario Limonciello:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> The x86 specific check for whether a framebuffer belongs to a device
>> works for display devices as well as VGA devices.  Callers to
>> video_is_primary_device() can benefit from checking non-VGA display
>> devices.
>>
>> Move the x86 specific check into x86 specific code, and adjust VGA
>> arbiter to call that code as well. This allows fbcon to find the
>> right PCI device on systems that don't have VGA devices.
>>
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   arch/x86/video/video-common.c | 28 +++++++++++++++++++++++++++
>>   drivers/pci/vgaarb.c          | 36 ++---------------------------------
>>   2 files changed, 30 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/x86/video/video-common.c b/arch/x86/video/video- 
>> common.c
>> index 81fc97a2a837a..718116e35e450 100644
>> --- a/arch/x86/video/video-common.c
>> +++ b/arch/x86/video/video-common.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>> +#include <linux/screen_info.h>
>>   #include <linux/vgaarb.h>
>>   #include <asm/video.h>
>> @@ -27,13 +28,40 @@ EXPORT_SYMBOL(pgprot_framebuffer);
>>   bool video_is_primary_device(struct device *dev)
> 
> I'm not sure I understand this patch. video_is_primary_device() already 
> exists for 3 architectures, including x86. [1] Adding it here should 
> produce an error. (?)

I wasn't adding a new implementation of it, I was augmenting the x86 
implementation.

But I guess based on your below point it just needs to call 
screen_info_pci_dev().

> 
> [1] https://elixir.bootlin.com/linux/v6.15.2/A/ident/ 
> video_is_primary_device
> 
> The code on x86 is
> 
> bool <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
> bool>video_is_primary_device <https://elixir.bootlin.com/linux/v6.15.2/ 
> C/ident/video_is_primary_device>(structdevice <https:// 
> elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { structpci_dev 
> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; if(! 
> dev_is_pci <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
> dev_is_pci>(dev)) returnfalse <https://elixir.bootlin.com/linux/v6.15.2/ 
> C/ident/false>; pdev=to_pci_dev <https://elixir.bootlin.com/linux/ 
> v6.15.2/C/ident/to_pci_dev>(dev); return(pdev==vga_default_device 
> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>()); }
> 
> I was thinking about extending it to test for additional properties, 
> like this
> 
> bool <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
> bool>video_is_primary_device <https://elixir.bootlin.com/linux/v6.15.2/ 
> C/ident/video_is_primary_device>(structdevice <https:// 
> elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { structpci_dev 
> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; if(! 
> dev_is_pci <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
> dev_is_pci>(dev)) returnfalse <https://elixir.bootlin.com/linux/v6.15.2/ 
> C/ident/false>; pdev=to_pci_dev <https://elixir.bootlin.com/linux/ 
> v6.15.2/C/ident/to_pci_dev>(dev); if(pdev==vga_default_device <https:// 
> elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>()) return 
> true for_each_pci_dev() { // test if display and could be primary. } 
> return false; // nothing found }
> 

The above looks like some bad copy / paste.  Could you clarify?

> 
> This would then be called from per-device sysfs code that export a 
> property similar to boot_vga (such as boot_display).

Here's the other idea I had in mind.

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 268c69daa4d57..8535950b4c0f3 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_console_show(struct device *dev, struct 
device_attribute *attr,
+                                char *buf)
+{
+       return sysfs_emit(buf, "%u\n", video_is_primary_device(dev));
+}
+static DEVICE_ATTR_RO(boot_console);
+
  static ssize_t boot_vga_show(struct device *dev, struct 
device_attribute *attr,
                              char *buf)
  {
@@ -1698,6 +1706,7 @@ late_initcall(pci_sysfs_init);

  static struct attribute *pci_dev_dev_attrs[] = {
         &dev_attr_boot_vga.attr,
+       &dev_attr_boot_console.attr,
         NULL,
  };

@@ -1710,6 +1719,9 @@ static umode_t pci_dev_attrs_are_visible(struct 
kobject *kobj,
         if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
                 return a->mode;

+       if (a == &dev_attr_boot_console.attr && pci_is_display(pdev))
+               return a->mode;
+
         return 0;
  }


> 
> 
> The issue is currently just an x86 problem, but I can imagine something 
> similar happening on ARM. There we'd have to go through the DT tree to 
> figure out the primary device. That's a problem for a later patch set, 
> but we should keep this in mind.

I think that the sysfs file idea above would work for any arch.

> 
>>   {
>> +    u64 base = screen_info.lfb_base;
>> +    u64 size = screen_info.lfb_size;
>>       struct pci_dev *pdev;
>> +    struct resource *r;
>> +    u64 limit;
>>       if (!dev_is_pci(dev))
>>           return false;
>>       pdev = to_pci_dev(dev);
>> +    if (!pci_is_display(pdev))
>> +        return false;
>> +
>> +    /* Select the device owning the boot framebuffer if there is one */
>> +    if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>> +        base |= (u64)screen_info.ext_lfb_base << 32;
>> +
>> +    limit = base + size;
>> +
>> +    /* Does firmware framebuffer belong to us? */
>> +    pci_dev_for_each_resource(pdev, r) {
>> +        if (resource_type(r) != IORESOURCE_MEM)
>> +            continue;
>> +
>> +        if (!r->start || !r->end)
>> +            continue;
>> +
>> +        if (base < r->start || limit >= r->end)
>> +            continue;
>> +
>> +        return true;
>> +    }
>> +
> 
> You can drop all this code and call screen_info_pci_dev() instead. I 
> simply never got to update vgaarb to use it.

👍

> 
> [2] https://elixir.bootlin.com/linux/v6.15.2/source/drivers/video/ 
> screen_info_pci.c#L109
> 
>>       return (pdev == vga_default_device());
>>   }
>>   EXPORT_SYMBOL(video_is_primary_device);
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 78748e8d2dbae..15ab58c70b016 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -26,12 +26,12 @@
>>   #include <linux/poll.h>
>>   #include <linux/miscdevice.h>
>>   #include <linux/slab.h>
>> -#include <linux/screen_info.h>
>>   #include <linux/vt.h>
>>   #include <linux/console.h>
>>   #include <linux/acpi.h>
>>   #include <linux/uaccess.h>
>>   #include <linux/vgaarb.h>
>> +#include <asm/video.h>
>>   static void vga_arbiter_notify_clients(void);
>> @@ -554,38 +554,6 @@ void vga_put(struct pci_dev *pdev, unsigned int 
>> rsrc)
>>   }
>>   EXPORT_SYMBOL(vga_put);
>> -static bool vga_is_firmware_default(struct pci_dev *pdev)
>> -{
>> -#if defined(CONFIG_X86)
>> -    u64 base = screen_info.lfb_base;
>> -    u64 size = screen_info.lfb_size;
>> -    struct resource *r;
>> -    u64 limit;
>> -
>> -    /* Select the device owning the boot framebuffer if there is one */
>> -
>> -    if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>> -        base |= (u64)screen_info.ext_lfb_base << 32;
>> -
>> -    limit = base + size;
>> -
>> -    /* Does firmware framebuffer belong to us? */
>> -    pci_dev_for_each_resource(pdev, r) {
>> -        if (resource_type(r) != IORESOURCE_MEM)
>> -            continue;
>> -
>> -        if (!r->start || !r->end)
>> -            continue;
>> -
>> -        if (base < r->start || limit >= r->end)
>> -            continue;
>> -
>> -        return true;
>> -    }
>> -#endif
>> -    return false;
>> -}
>> -
>>   static bool vga_arb_integrated_gpu(struct device *dev)
>>   {
>>   #if defined(CONFIG_ACPI)
>> @@ -623,7 +591,7 @@ static bool vga_is_boot_device(struct vga_device 
>> *vgadev)
>>       if (boot_vga && boot_vga->is_firmware_default)
>>           return false;
>> -    if (vga_is_firmware_default(pdev)) {
>> +    if (video_is_primary_device(&pdev->dev)) {
> 
> Maybe not change this because you don't want to end up with non-VGA 
> devices here.

👍

> 
> Best regards
> Thomas
> 
>>           vgadev->is_firmware_default = true;
>>           return true;
>>       }
> 

Hi

Am 21.06.25 um 00:17 schrieb Mario Limonciello:
> On 6/20/2025 3:45 AM, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 20.06.25 um 04:49 schrieb Mario Limonciello:
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> The x86 specific check for whether a framebuffer belongs to a device
>>> works for display devices as well as VGA devices.  Callers to
>>> video_is_primary_device() can benefit from checking non-VGA display
>>> devices.
>>>
>>> Move the x86 specific check into x86 specific code, and adjust VGA
>>> arbiter to call that code as well. This allows fbcon to find the
>>> right PCI device on systems that don't have VGA devices.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   arch/x86/video/video-common.c | 28 +++++++++++++++++++++++++++
>>>   drivers/pci/vgaarb.c          | 36 
>>> ++---------------------------------
>>>   2 files changed, 30 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/arch/x86/video/video-common.c b/arch/x86/video/video- 
>>> common.c
>>> index 81fc97a2a837a..718116e35e450 100644
>>> --- a/arch/x86/video/video-common.c
>>> +++ b/arch/x86/video/video-common.c
>>> @@ -9,6 +9,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/pci.h>
>>> +#include <linux/screen_info.h>
>>>   #include <linux/vgaarb.h>
>>>   #include <asm/video.h>
>>> @@ -27,13 +28,40 @@ EXPORT_SYMBOL(pgprot_framebuffer);
>>>   bool video_is_primary_device(struct device *dev)
>>
>> I'm not sure I understand this patch. video_is_primary_device() 
>> already exists for 3 architectures, including x86. [1] Adding it here 
>> should produce an error. (?)
>
> I wasn't adding a new implementation of it, I was augmenting the x86 
> implementation.

Indeed. Apologies, I must have somehow misread the patch. So this is 
essentially doing what I proposed.

>
> But I guess based on your below point it just needs to call 
> screen_info_pci_dev().

Yeah, the helper already does everything necessary.


>>
>> [1] https://elixir.bootlin.com/linux/v6.15.2/A/ident/ 
>> video_is_primary_device
>>
>> The code on x86 is
>>
>> bool <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
>> bool>video_is_primary_device 
>> <https://elixir.bootlin.com/linux/v6.15.2/ 
>> C/ident/video_is_primary_device>(structdevice <https:// 
>> elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { structpci_dev 
>> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; if(! 
>> dev_is_pci <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
>> dev_is_pci>(dev)) returnfalse 
>> <https://elixir.bootlin.com/linux/v6.15.2/ C/ident/false>; 
>> pdev=to_pci_dev <https://elixir.bootlin.com/linux/ 
>> v6.15.2/C/ident/to_pci_dev>(dev); return(pdev==vga_default_device 
>> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>()); 
>> }
>>
>> I was thinking about extending it to test for additional properties, 
>> like this
>>
>> bool <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
>> bool>video_is_primary_device 
>> <https://elixir.bootlin.com/linux/v6.15.2/ 
>> C/ident/video_is_primary_device>(structdevice <https:// 
>> elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { structpci_dev 
>> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; if(! 
>> dev_is_pci <https://elixir.bootlin.com/linux/v6.15.2/C/ident/ 
>> dev_is_pci>(dev)) returnfalse 
>> <https://elixir.bootlin.com/linux/v6.15.2/ C/ident/false>; 
>> pdev=to_pci_dev <https://elixir.bootlin.com/linux/ 
>> v6.15.2/C/ident/to_pci_dev>(dev); if(pdev==vga_default_device 
>> <https:// 
>> elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>()) 
>> return true for_each_pci_dev() { // test if display and could be 
>> primary. } return false; // nothing found }
>>
>
> The above looks like some bad copy / paste.  Could you clarify?

Oh, well. I really messed up my reply. :D

What I meant is what you already implemented, but with the existing helper:

bool video_is_primary_device(dev)
{
     if (dev == vga_default_device())
       return true

     if (dev == screen_info_pci_device())
       return true

     return false
}


One thing to keep in minds is that video_is_primary_device() currently 
returns false by default. IDK if that's a problem for user space, but 
user space should at least pick a reasonable fallback in that case.

Best regards
Thomas


>
>>
>> This would then be called from per-device sysfs code that export a 
>> property similar to boot_vga (such as boot_display).
>
> Here's the other idea I had in mind.
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 268c69daa4d57..8535950b4c0f3 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_console_show(struct device *dev, struct 
> device_attribute *attr,
> +                                char *buf)
> +{
> +       return sysfs_emit(buf, "%u\n", video_is_primary_device(dev));
> +}
> +static DEVICE_ATTR_RO(boot_console);
> +
>  static ssize_t boot_vga_show(struct device *dev, struct 
> device_attribute *attr,
>                              char *buf)
>  {
> @@ -1698,6 +1706,7 @@ late_initcall(pci_sysfs_init);
>
>  static struct attribute *pci_dev_dev_attrs[] = {
>         &dev_attr_boot_vga.attr,
> +       &dev_attr_boot_console.attr,
>         NULL,
>  };
>
> @@ -1710,6 +1719,9 @@ static umode_t pci_dev_attrs_are_visible(struct 
> kobject *kobj,
>         if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
>                 return a->mode;
>
> +       if (a == &dev_attr_boot_console.attr && pci_is_display(pdev))
> +               return a->mode;
> +
>         return 0;
>  }
>
>
>>
>>
>> The issue is currently just an x86 problem, but I can imagine 
>> something similar happening on ARM. There we'd have to go through the 
>> DT tree to figure out the primary device. That's a problem for a 
>> later patch set, but we should keep this in mind.
>
> I think that the sysfs file idea above would work for any arch.
>
>>
>>>   {
>>> +    u64 base = screen_info.lfb_base;
>>> +    u64 size = screen_info.lfb_size;
>>>       struct pci_dev *pdev;
>>> +    struct resource *r;
>>> +    u64 limit;
>>>       if (!dev_is_pci(dev))
>>>           return false;
>>>       pdev = to_pci_dev(dev);
>>> +    if (!pci_is_display(pdev))
>>> +        return false;
>>> +
>>> +    /* Select the device owning the boot framebuffer if there is 
>>> one */
>>> +    if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>>> +        base |= (u64)screen_info.ext_lfb_base << 32;
>>> +
>>> +    limit = base + size;
>>> +
>>> +    /* Does firmware framebuffer belong to us? */
>>> +    pci_dev_for_each_resource(pdev, r) {
>>> +        if (resource_type(r) != IORESOURCE_MEM)
>>> +            continue;
>>> +
>>> +        if (!r->start || !r->end)
>>> +            continue;
>>> +
>>> +        if (base < r->start || limit >= r->end)
>>> +            continue;
>>> +
>>> +        return true;
>>> +    }
>>> +
>>
>> You can drop all this code and call screen_info_pci_dev() instead. I 
>> simply never got to update vgaarb to use it.
>
> 👍
>
>>
>> [2] https://elixir.bootlin.com/linux/v6.15.2/source/drivers/video/ 
>> screen_info_pci.c#L109
>>
>>>       return (pdev == vga_default_device());
>>>   }
>>>   EXPORT_SYMBOL(video_is_primary_device);
>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>> index 78748e8d2dbae..15ab58c70b016 100644
>>> --- a/drivers/pci/vgaarb.c
>>> +++ b/drivers/pci/vgaarb.c
>>> @@ -26,12 +26,12 @@
>>>   #include <linux/poll.h>
>>>   #include <linux/miscdevice.h>
>>>   #include <linux/slab.h>
>>> -#include <linux/screen_info.h>
>>>   #include <linux/vt.h>
>>>   #include <linux/console.h>
>>>   #include <linux/acpi.h>
>>>   #include <linux/uaccess.h>
>>>   #include <linux/vgaarb.h>
>>> +#include <asm/video.h>
>>>   static void vga_arbiter_notify_clients(void);
>>> @@ -554,38 +554,6 @@ void vga_put(struct pci_dev *pdev, unsigned int 
>>> rsrc)
>>>   }
>>>   EXPORT_SYMBOL(vga_put);
>>> -static bool vga_is_firmware_default(struct pci_dev *pdev)
>>> -{
>>> -#if defined(CONFIG_X86)
>>> -    u64 base = screen_info.lfb_base;
>>> -    u64 size = screen_info.lfb_size;
>>> -    struct resource *r;
>>> -    u64 limit;
>>> -
>>> -    /* Select the device owning the boot framebuffer if there is 
>>> one */
>>> -
>>> -    if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>>> -        base |= (u64)screen_info.ext_lfb_base << 32;
>>> -
>>> -    limit = base + size;
>>> -
>>> -    /* Does firmware framebuffer belong to us? */
>>> -    pci_dev_for_each_resource(pdev, r) {
>>> -        if (resource_type(r) != IORESOURCE_MEM)
>>> -            continue;
>>> -
>>> -        if (!r->start || !r->end)
>>> -            continue;
>>> -
>>> -        if (base < r->start || limit >= r->end)
>>> -            continue;
>>> -
>>> -        return true;
>>> -    }
>>> -#endif
>>> -    return false;
>>> -}
>>> -
>>>   static bool vga_arb_integrated_gpu(struct device *dev)
>>>   {
>>>   #if defined(CONFIG_ACPI)
>>> @@ -623,7 +591,7 @@ static bool vga_is_boot_device(struct vga_device 
>>> *vgadev)
>>>       if (boot_vga && boot_vga->is_firmware_default)
>>>           return false;
>>> -    if (vga_is_firmware_default(pdev)) {
>>> +    if (video_is_primary_device(&pdev->dev)) {
>>
>> Maybe not change this because you don't want to end up with non-VGA 
>> devices here.
>
> 👍
>
>>
>> Best regards
>> Thomas
>>
>>>           vgadev->is_firmware_default = true;
>>>           return true;
>>>       }
>>
>

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