Re: [PATCH v3 6/7] PCI/VGA: Move check for firmware default out of VGA arbiter

[Thomas Zimmermann <tzimmermann@suse.de>]

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)