[pci-next,v6,1/2] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent

  Currently, the vga_is_firmware_default() function only works on x86 and
ia64, it is a no-op on the rest of the architectures. This patch completes
the implementation for it, the added code tries to capture the PCI (e) VGA
device that owns the firmware framebuffer, since only one GPU could own
the firmware fb, things are almost done once we have determined the boot
VGA device. As the PCI resource relocation do have a influence on the
results of identification, we make it available on architectures where PCI
resource relocation does happen at first. Because this patch is more
important for those architectures(such as arm, arm64, loongarch, mips and
risc-v etc).

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)
  

On Mon, Oct 02, 2023 at 08:05:10PM +0800, Sui Jingfeng wrote:
> Currently, the vga_is_firmware_default() function only works on x86 and
> ia64, it is a no-op on the rest of the architectures. This patch completes
> the implementation for it, the added code tries to capture the PCI (e) VGA
> device that owns the firmware framebuffer, since only one GPU could own
> the firmware fb, things are almost done once we have determined the boot
> VGA device. As the PCI resource relocation do have a influence on the
> results of identification, we make it available on architectures where PCI
> resource relocation does happen at first. Because this patch is more
> important for those architectures(such as arm, arm64, loongarch, mips and
> risc-v etc).

There's a little too much going on this this patch.  The problem is
very simple: currently we compare firmware BAR assignments with BARs
that may have been reassigned by Linux.

What if we did something like the patch below?  I think it will be
less confusing if we only have one copy of the code related to
screen_info.

Bjorn

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/vgaarb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5e6b1eb54c64..02821c0f4cd0 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -60,6 +60,9 @@ static bool vga_arbiter_used;
>  static DEFINE_SPINLOCK(vga_lock);
>  static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>  
> +/* The PCI(e) VGA device which owns the firmware framebuffer */
> +static struct pci_dev *pdev_boot_vga;
> +
>  static const char *vga_iostate_to_str(unsigned int iostate)
>  {
>  	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> @@ -582,6 +585,9 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>  
>  		return true;
>  	}
> +#else
> +	if (pdev_boot_vga && pdev_boot_vga == pdev)
> +		return true;
>  #endif
>  	return false;
>  }
> @@ -1557,3 +1563,73 @@ static int __init vga_arb_device_init(void)
>  	return rc;
>  }
>  subsys_initcall_sync(vga_arb_device_init);
> +
> +/*
> + * Get the physical address range that the firmware framebuffer occupies.
> + *
> + * Note that the global screen_info is arch-specific, thus CONFIG_SYSFB is
> + * chosen as compile-time conditional to suppress linkage problems on non-x86
> + * architectures.
> + *
> + * Returns true on success, otherwise return false.
> + */
> +static bool vga_arb_get_firmware_fb_range(u64 *start, u64 *end)
> +{
> +	u64 fb_start = 0;
> +	u64 fb_size = 0;
> +	u64 fb_end;
> +
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64) || defined(CONFIG_SYSFB)
> +	fb_start = screen_info.lfb_base;
> +	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> +		fb_start |= (u64)screen_info.ext_lfb_base << 32;
> +
> +	fb_size = screen_info.lfb_size;
> +#endif
> +
> +	/* No firmware framebuffer support */
> +	if (!fb_start || !fb_size)
> +		return false;
> +
> +	fb_end = fb_start + fb_size - 1;
> +
> +	*start = fb_start;
> +	*end = fb_end;
> +
> +	return true;
> +}
> +
> +/*
> + * Identify the PCI VGA device that contains the firmware framebuffer
> + */
> +static void pci_boot_vga_capturer(struct pci_dev *pdev)
> +{
> +	u64 fb_start, fb_end;
> +	struct resource *res;
> +	unsigned int i;
> +
> +	if (pdev_boot_vga)
> +		return;
> +
> +	if (!vga_arb_get_firmware_fb_range(&fb_start, &fb_end))
> +		return;
> +
> +	pci_dev_for_each_resource(pdev, res, i) {
> +		if (resource_type(res) != IORESOURCE_MEM)
> +			continue;
> +
> +		if (!res->start || !res->end)
> +			continue;
> +
> +		if (res->start <= fb_start && fb_end <= res->end) {
> +			pdev_boot_vga = pdev;
> +
> +			vgaarb_info(&pdev->dev,
> +				    "BAR %u: %pR contains firmware FB [0x%llx-0x%llx]\n",
> +				    i, res, fb_start, fb_end);
> +			break;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
> +			       8, pci_boot_vga_capturer);


PCI/VGA: Match firmware framebuffer before BAR reassignment

vga_is_firmware_default() decides a device is the firmware default VGA
device if it has a BAR that contains the framebuffer described by
screen_info.

Previously this was unreliable because the screen_info framebuffer address
comes from firmware, and the Linux PCI core may reassign device BARs before
vga_is_firmware_default() runs.  This reassignment means the BAR may not
match the screen_info values, but we still want to select the device as the
firmware default.

Make vga_is_firmware_default() more reliable by running it as a quirk so it
happens before any BAR reassignment.

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5e6b1eb54c64..4a53e76caddd 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -60,6 +60,8 @@ static bool vga_arbiter_used;
 static DEFINE_SPINLOCK(vga_lock);
 static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
 
+static struct pci_dev *vga_firmware_device;
+
 static const char *vga_iostate_to_str(unsigned int iostate)
 {
 	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
@@ -560,6 +562,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
 	u64 base = screen_info.lfb_base;
 	u64 size = screen_info.lfb_size;
 	struct resource *r;
+	unsigned int i;
 	u64 limit;
 
 	/* Select the device owning the boot framebuffer if there is one */
@@ -570,7 +573,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
 	limit = base + size;
 
 	/* Does firmware framebuffer belong to us? */
-	pci_dev_for_each_resource(pdev, r) {
+	pci_dev_for_each_resource(pdev, r, i) {
 		if (resource_type(r) != IORESOURCE_MEM)
 			continue;
 
@@ -580,6 +583,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
 		if (base < r->start || limit >= r->end)
 			continue;
 
+		vgaarb_info(&pdev->dev, "BAR %u: %pR contains firmware framebuffer [%#010llx-%#010llx]\n",
+			    i, r, base, limit - 1);
 		return true;
 	}
 #endif
@@ -623,7 +628,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 (pdev == vga_firmware_device) {
 		vgadev->is_firmware_default = true;
 		return true;
 	}
@@ -1557,3 +1562,14 @@ static int __init vga_arb_device_init(void)
 	return rc;
 }
 subsys_initcall_sync(vga_arb_device_init);
+
+static void vga_match_firmware_framebuffer(struct pci_dev *pdev)
+{
+	if (vga_firmware_device)
+		return;
+
+	if (vga_is_firmware_default(pdev))
+		vga_firmware_device = pdev;
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
+			       8, vga_match_firmware_framebuffer);
  

Hi,


On 2023/10/3 23:54, Bjorn Helgaas wrote:
> On Mon, Oct 02, 2023 at 08:05:10PM +0800, Sui Jingfeng wrote:
>> Currently, the vga_is_firmware_default() function only works on x86 and
>> ia64, it is a no-op on the rest of the architectures. This patch completes
>> the implementation for it, the added code tries to capture the PCI (e) VGA
>> device that owns the firmware framebuffer, since only one GPU could own
>> the firmware fb, things are almost done once we have determined the boot
>> VGA device. As the PCI resource relocation do have a influence on the
>> results of identification, we make it available on architectures where PCI
>> resource relocation does happen at first. Because this patch is more
>> important for those architectures(such as arm, arm64, loongarch, mips and
>> risc-v etc).
> There's a little too much going on this this patch.  The problem is
> very simple: currently we compare firmware BAR assignments with BARs
> that may have been reassigned by Linux.
>
> What if we did something like the patch below?  I think it will be
> less confusing if we only have one copy of the code related to
> screen_info.
>
> Bjorn
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/pci/vgaarb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 5e6b1eb54c64..02821c0f4cd0 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -60,6 +60,9 @@ static bool vga_arbiter_used;
>>   static DEFINE_SPINLOCK(vga_lock);
>>   static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>>   
>> +/* The PCI(e) VGA device which owns the firmware framebuffer */
>> +static struct pci_dev *pdev_boot_vga;
>> +
>>   static const char *vga_iostate_to_str(unsigned int iostate)
>>   {
>>   	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
>> @@ -582,6 +585,9 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>>   
>>   		return true;
>>   	}
>> +#else
>> +	if (pdev_boot_vga && pdev_boot_vga == pdev)
>> +		return true;
>>   #endif
>>   	return false;
>>   }
>> @@ -1557,3 +1563,73 @@ static int __init vga_arb_device_init(void)
>>   	return rc;
>>   }
>>   subsys_initcall_sync(vga_arb_device_init);
>> +
>> +/*
>> + * Get the physical address range that the firmware framebuffer occupies.
>> + *
>> + * Note that the global screen_info is arch-specific, thus CONFIG_SYSFB is
>> + * chosen as compile-time conditional to suppress linkage problems on non-x86
>> + * architectures.
>> + *
>> + * Returns true on success, otherwise return false.
>> + */
>> +static bool vga_arb_get_firmware_fb_range(u64 *start, u64 *end)
>> +{
>> +	u64 fb_start = 0;
>> +	u64 fb_size = 0;
>> +	u64 fb_end;
>> +
>> +#if defined(CONFIG_X86) || defined(CONFIG_IA64) || defined(CONFIG_SYSFB)
>> +	fb_start = screen_info.lfb_base;
>> +	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>> +		fb_start |= (u64)screen_info.ext_lfb_base << 32;
>> +
>> +	fb_size = screen_info.lfb_size;
>> +#endif
>> +
>> +	/* No firmware framebuffer support */
>> +	if (!fb_start || !fb_size)
>> +		return false;
>> +
>> +	fb_end = fb_start + fb_size - 1;
>> +
>> +	*start = fb_start;
>> +	*end = fb_end;
>> +
>> +	return true;
>> +}
>> +
>> +/*
>> + * Identify the PCI VGA device that contains the firmware framebuffer
>> + */
>> +static void pci_boot_vga_capturer(struct pci_dev *pdev)
>> +{
>> +	u64 fb_start, fb_end;
>> +	struct resource *res;
>> +	unsigned int i;
>> +
>> +	if (pdev_boot_vga)
>> +		return;
>> +
>> +	if (!vga_arb_get_firmware_fb_range(&fb_start, &fb_end))
>> +		return;
>> +
>> +	pci_dev_for_each_resource(pdev, res, i) {
>> +		if (resource_type(res) != IORESOURCE_MEM)
>> +			continue;
>> +
>> +		if (!res->start || !res->end)
>> +			continue;
>> +
>> +		if (res->start <= fb_start && fb_end <= res->end) {
>> +			pdev_boot_vga = pdev;
>> +
>> +			vgaarb_info(&pdev->dev,
>> +				    "BAR %u: %pR contains firmware FB [0x%llx-0x%llx]\n",
>> +				    i, res, fb_start, fb_end);
>> +			break;
>> +		}
>> +	}
>> +}
>> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
>> +			       8, pci_boot_vga_capturer);
>
> PCI/VGA: Match firmware framebuffer before BAR reassignment
>
> vga_is_firmware_default() decides a device is the firmware default VGA
> device if it has a BAR that contains the framebuffer described by
> screen_info.
>
> Previously this was unreliable because the screen_info framebuffer address
> comes from firmware, and the Linux PCI core may reassign device BARs before
> vga_is_firmware_default() runs.  This reassignment means the BAR may not
> match the screen_info values, but we still want to select the device as the
> firmware default.
>
> Make vga_is_firmware_default() more reliable by running it as a quirk so it
> happens before any BAR reassignment.
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5e6b1eb54c64..4a53e76caddd 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -60,6 +60,8 @@ static bool vga_arbiter_used;
>   static DEFINE_SPINLOCK(vga_lock);
>   static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>   
> +static struct pci_dev *vga_firmware_device;
> +
>   static const char *vga_iostate_to_str(unsigned int iostate)
>   {
>   	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> @@ -560,6 +562,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>   	u64 base = screen_info.lfb_base;
>   	u64 size = screen_info.lfb_size;
>   	struct resource *r;
> +	unsigned int i;
>   	u64 limit;
>   
>   	/* Select the device owning the boot framebuffer if there is one */
> @@ -570,7 +573,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>   	limit = base + size;
>   
>   	/* Does firmware framebuffer belong to us? */
> -	pci_dev_for_each_resource(pdev, r) {
> +	pci_dev_for_each_resource(pdev, r, i) {
>   		if (resource_type(r) != IORESOURCE_MEM)
>   			continue;
>   
> @@ -580,6 +583,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>   		if (base < r->start || limit >= r->end)
>   			continue;
>   
> +		vgaarb_info(&pdev->dev, "BAR %u: %pR contains firmware framebuffer [%#010llx-%#010llx]\n",
> +			    i, r, base, limit - 1);
>   		return true;
>   	}
>   #endif
> @@ -623,7 +628,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 (pdev == vga_firmware_device) {
>   		vgadev->is_firmware_default = true;
>   		return true;
>   	}
> @@ -1557,3 +1562,14 @@ static int __init vga_arb_device_init(void)
>   	return rc;
>   }
>   subsys_initcall_sync(vga_arb_device_init);
> +
> +static void vga_match_firmware_framebuffer(struct pci_dev *pdev)
> +{
> +	if (vga_firmware_device)
> +		return;
> +
> +	if (vga_is_firmware_default(pdev))
> +		vga_firmware_device = pdev;
> +}
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
> +			       8, vga_match_firmware_framebuffer);


Q: What if we did something like the patch below?

A:

But the vga_is_firmware_default() function only works on X86 and IA64,
you patch doesn't solve the problems on ARM64 and LoongArch.

Q:

I think it will be
less confusing if we only have one copy of the code related to
screen_info.

A:

Since you have already know everything, you can do anything with those two patch
if you would like to pick them up. As I have spend enough more time on this, if
two copy of the screen_info really matters, Would you mind to squash those two
patch into one?
  

Hi,


On 2023/10/3 23:54, Bjorn Helgaas wrote:
> PCI/VGA: Match firmware framebuffer before BAR reassignment
>
> vga_is_firmware_default() decides a device is the firmware default VGA
> device if it has a BAR that contains the framebuffer described by
> screen_info.
>
> Previously this was unreliable because the screen_info framebuffer address
> comes from firmware, and the Linux PCI core may reassign device BARs before
> vga_is_firmware_default() runs.  This reassignment means the BAR may not
> match the screen_info values, but we still want to select the device as the
> firmware default.
>
> Make vga_is_firmware_default() more reliable by running it as a quirk so it
> happens before any BAR reassignment.


On a specific architecture, the kernel have its own copy of screen_info.
We may choose to rely on the global screen_info, but I don't hope vgaarb
should completely depend on the firmware which without any flexibility.
After all, there are always have the old BIOS with the newer Kernel or
new graphics card combinations. That is to say that the BIOS(firmware)
is released first, then the new graphics comes after. So, we can't rely
on the BIOS know all of the graphics card in the world at the time of
BIOS release. If a BIOS don't know a specific video card, then then BIOS
is unlikely initialize a firmware framebuffer(UEFI GOP or something like
that) on it successfully.

I hope vgaarb can work without fully rely on the firmware, don't mention
the word 'firmware' as less as possible. Because, we can even modify the
kernel's screen_info by hardcode or fill it by parsing DT. We are only
rely on the global screen_info here. while how does the global screen_info
being filled is a kernel side and arch specific thing. The global screen_info
may rely on the BIOS, but this is a thing of the kernel side, vgaarb belong
to drivers directory.

Beside, the word 'firmware' have multiple meanings, for the patch at here,
it refer to the UEFI or uboot or something similar. But for the GPU domain,
it may refer to any binary executable on the macro-controller embedded in
the GPU IP. So I think the names 'is_firmware_default' and 'vga_is_firmware_default'
are putting *too much emphasis* the firmware. So I want to remove it.

vgaarb already have been exposed to userspace via sysfs interface (/sys/class/misc/vga_arbiter).
So the original spirit is actually allow user to tune or control. I hope vgaarb become more
standalone and more flexible so that there are some ways to rescue even in the worse case.
If someone (who have a better background or have better understanding toward a specific case only)
don't see this as a problem, then it is not my problem. I'm not good at debating with English.
  

On Wed, Oct 04, 2023 at 08:55:04PM +0800, Sui Jingfeng wrote:
> On 2023/10/3 23:54, Bjorn Helgaas wrote:
> > On Mon, Oct 02, 2023 at 08:05:10PM +0800, Sui Jingfeng wrote:
> > > Currently, the vga_is_firmware_default() function only works on x86 and
> > > ia64, it is a no-op on the rest of the architectures. This patch completes
> > > the implementation for it, the added code tries to capture the PCI (e) VGA
> > > device that owns the firmware framebuffer, since only one GPU could own
> > > the firmware fb, things are almost done once we have determined the boot
> > > VGA device. As the PCI resource relocation do have a influence on the
> > > results of identification, we make it available on architectures where PCI
> > > resource relocation does happen at first. Because this patch is more
> > > important for those architectures(such as arm, arm64, loongarch, mips and
> > > risc-v etc).
> >
> > There's a little too much going on this this patch.  The problem is
> > very simple: currently we compare firmware BAR assignments with BARs
> > that may have been reassigned by Linux.
> > 
> > What if we did something like the patch below?  I think it will be
> > less confusing if we only have one copy of the code related to
> > screen_info.
> > 
> > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > ---
> > >   drivers/pci/vgaarb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 76 insertions(+)
> > > 
> > > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > > index 5e6b1eb54c64..02821c0f4cd0 100644
> > > --- a/drivers/pci/vgaarb.c
> > > +++ b/drivers/pci/vgaarb.c
> > > @@ -60,6 +60,9 @@ static bool vga_arbiter_used;
> > >   static DEFINE_SPINLOCK(vga_lock);
> > >   static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
> > > +/* The PCI(e) VGA device which owns the firmware framebuffer */
> > > +static struct pci_dev *pdev_boot_vga;
> > > +
> > >   static const char *vga_iostate_to_str(unsigned int iostate)
> > >   {
> > >   	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> > > @@ -582,6 +585,9 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
> > >   		return true;
> > >   	}
> > > +#else
> > > +	if (pdev_boot_vga && pdev_boot_vga == pdev)
> > > +		return true;
> > >   #endif
> > >   	return false;
> > >   }
> > > @@ -1557,3 +1563,73 @@ static int __init vga_arb_device_init(void)
> > >   	return rc;
> > >   }
> > >   subsys_initcall_sync(vga_arb_device_init);
> > > +
> > > +/*
> > > + * Get the physical address range that the firmware framebuffer occupies.
> > > + *
> > > + * Note that the global screen_info is arch-specific, thus CONFIG_SYSFB is
> > > + * chosen as compile-time conditional to suppress linkage problems on non-x86
> > > + * architectures.
> > > + *
> > > + * Returns true on success, otherwise return false.
> > > + */
> > > +static bool vga_arb_get_firmware_fb_range(u64 *start, u64 *end)
> > > +{
> > > +	u64 fb_start = 0;
> > > +	u64 fb_size = 0;
> > > +	u64 fb_end;
> > > +
> > > +#if defined(CONFIG_X86) || defined(CONFIG_IA64) || defined(CONFIG_SYSFB)
> > > +	fb_start = screen_info.lfb_base;
> > > +	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> > > +		fb_start |= (u64)screen_info.ext_lfb_base << 32;
> > > +
> > > +	fb_size = screen_info.lfb_size;
> > > +#endif
> > > +
> > > +	/* No firmware framebuffer support */
> > > +	if (!fb_start || !fb_size)
> > > +		return false;
> > > +
> > > +	fb_end = fb_start + fb_size - 1;
> > > +
> > > +	*start = fb_start;
> > > +	*end = fb_end;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +/*
> > > + * Identify the PCI VGA device that contains the firmware framebuffer
> > > + */
> > > +static void pci_boot_vga_capturer(struct pci_dev *pdev)
> > > +{
> > > +	u64 fb_start, fb_end;
> > > +	struct resource *res;
> > > +	unsigned int i;
> > > +
> > > +	if (pdev_boot_vga)
> > > +		return;
> > > +
> > > +	if (!vga_arb_get_firmware_fb_range(&fb_start, &fb_end))
> > > +		return;
> > > +
> > > +	pci_dev_for_each_resource(pdev, res, i) {
> > > +		if (resource_type(res) != IORESOURCE_MEM)
> > > +			continue;
> > > +
> > > +		if (!res->start || !res->end)
> > > +			continue;
> > > +
> > > +		if (res->start <= fb_start && fb_end <= res->end) {
> > > +			pdev_boot_vga = pdev;
> > > +
> > > +			vgaarb_info(&pdev->dev,
> > > +				    "BAR %u: %pR contains firmware FB [0x%llx-0x%llx]\n",
> > > +				    i, res, fb_start, fb_end);
> > > +			break;
> > > +		}
> > > +	}
> > > +}
> > > +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
> > > +			       8, pci_boot_vga_capturer);
> > 
> > PCI/VGA: Match firmware framebuffer before BAR reassignment
> > 
> > vga_is_firmware_default() decides a device is the firmware default VGA
> > device if it has a BAR that contains the framebuffer described by
> > screen_info.
> > 
> > Previously this was unreliable because the screen_info framebuffer address
> > comes from firmware, and the Linux PCI core may reassign device BARs before
> > vga_is_firmware_default() runs.  This reassignment means the BAR may not
> > match the screen_info values, but we still want to select the device as the
> > firmware default.
> > 
> > Make vga_is_firmware_default() more reliable by running it as a quirk so it
> > happens before any BAR reassignment.
> > 
> > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > index 5e6b1eb54c64..4a53e76caddd 100644
> > --- a/drivers/pci/vgaarb.c
> > +++ b/drivers/pci/vgaarb.c
> > @@ -60,6 +60,8 @@ static bool vga_arbiter_used;
> >   static DEFINE_SPINLOCK(vga_lock);
> >   static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
> > +static struct pci_dev *vga_firmware_device;
> > +
> >   static const char *vga_iostate_to_str(unsigned int iostate)
> >   {
> >   	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> > @@ -560,6 +562,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
> >   	u64 base = screen_info.lfb_base;
> >   	u64 size = screen_info.lfb_size;
> >   	struct resource *r;
> > +	unsigned int i;
> >   	u64 limit;
> >   	/* Select the device owning the boot framebuffer if there is one */
> > @@ -570,7 +573,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
> >   	limit = base + size;
> >   	/* Does firmware framebuffer belong to us? */
> > -	pci_dev_for_each_resource(pdev, r) {
> > +	pci_dev_for_each_resource(pdev, r, i) {
> >   		if (resource_type(r) != IORESOURCE_MEM)
> >   			continue;
> > @@ -580,6 +583,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
> >   		if (base < r->start || limit >= r->end)
> >   			continue;
> > +		vgaarb_info(&pdev->dev, "BAR %u: %pR contains firmware framebuffer [%#010llx-%#010llx]\n",
> > +			    i, r, base, limit - 1);
> >   		return true;
> >   	}
> >   #endif
> > @@ -623,7 +628,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 (pdev == vga_firmware_device) {
> >   		vgadev->is_firmware_default = true;
> >   		return true;
> >   	}
> > @@ -1557,3 +1562,14 @@ static int __init vga_arb_device_init(void)
> >   	return rc;
> >   }
> >   subsys_initcall_sync(vga_arb_device_init);
> > +
> > +static void vga_match_firmware_framebuffer(struct pci_dev *pdev)
> > +{
> > +	if (vga_firmware_device)
> > +		return;
> > +
> > +	if (vga_is_firmware_default(pdev))
> > +		vga_firmware_device = pdev;
> > +}
> > +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
> > +			       8, vga_match_firmware_framebuffer);
> 
> 
> Q: What if we did something like the patch below?
> 
> A:
> 
> But the vga_is_firmware_default() function only works on X86 and IA64,
> you patch doesn't solve the problems on ARM64 and LoongArch.

Yes, that's true.  Ideally a patch solves a single problem.  This one
solves an issue for x86 and ia64.  A subsequent patch can solve the
problems on ARM64 and LoongArch.  Doing them separately means that
each patch is easier to understand and if we accidentally break
something, a bisection can give more specific information about what
broke.

Bjorn