[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f2209054-4e66-a084-c0bc-d35a7fd2fdad@loongson.cn>
Date: Wed, 4 Oct 2023 20:55:04 +0800
From: Sui Jingfeng <suijingfeng@...ngson.cn>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH pci-next v6 1/2] PCI/VGA: Make the
vga_is_firmware_default() less arch-dependent
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@...ngson.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?
Powered by blists - more mailing lists