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

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

[Aaron Ma]

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

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.

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.

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

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

[Mario Limonciello]

+Thomas Zimmermann

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

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

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

[Thomas Zimmermann]

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

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

Makes sense.

Here is an updated kernel patch:

https://lore.kernel.org/linux-pci/20260623141505.1816786-1-mario.limonciello@amd.com/T/#u

Here is a change to xserver:

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