Re: [PATCH] PCI/VGA: Make the vga_is_firmware_default() less arch-independent
On Tue, Aug 08, 2023 at 10:58:59PM +0800, Sui Jingfeng wrote:
> Currently, the vga_is_firmware_default() function works only on x86 and
> IA64 architectures, but it is a no-op on ARM64, PPC, RISC-V, etc. This
> patch completes the implementation by tracking the firmware framebuffer's
> address range. The added code is trying to identify the VRAM aperture that
> contains the firmware framebuffer. Once found, related information about
> the VRAM aperture will be tracked.
>
> Note that we need to identify the VRAM aperture before it get moved. We
> achieve this by using DECLARE_PCI_FIXUP_CLASS_HEADER(), which ensures that
> vga_arb_firmware_fb_addr_tracker() gets called before PCI resource
> allocation. Once we found the VRAM aperture that contains firmware fb, we
> are able to monitor the address changes of it. If the VRAM aperture of the
> primary GPU do moved, we will update our cached firmware framebuffer's
> address range accordingly. This approach overcomes the VRAM bar relocation
> issue successfully. Hence, this patch make the vga_is_firmware_default()
> function works on whatever arch that has UEFI GOP support, including x86
> and IA64. But, at the first step, we make it available only on platforms
> which PCI resource relocation do happens. Once provided method proved to
> be effective and reliable, it can be expanded to other arch easily.
I think this patch tries to solve two problems, and it should be split
into two patches:
1) Identify firmware framebuffer on arches other than x86 and ia64
2) Deal with VGA devices where the PCI core has moved the BAR
containing the framebuffer
For x86 and ia64, vga_is_firmware_default() currently gets the
framebuffer base and size from screen_info. Whenever
vga_arbiter_add_pci_device() adds a VGA device, we check to see if it
has a BAR containing the framebuffer.
It looks like this patch retains that for x86 and ia64, but only if
CONFIG_EFI=y. I think CONFIG_EFI is optional for both x86 and ia64,
so it looks like this will break systems where CONFIG_X86=y and
CONFIG_EFI is not set.
> This patch is tested on
> 1) LS3A5000+LS7A2000 and LS3A5000+LS7A1000 platform.
> 2) Intel i3-8100 CPU + H110 D4L motherboard with triple video cards:
>
> $ lspci | grep VGA
>
> Intel Corporation CoffeeLake-S GT2 [UHD Graphics 630]
> Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 470] (rev cf)
> ASPEED Technology, Inc. ASPEED Graphics Family (rev 52)
>
> Note that on x86, in order to testing the new approach this patch provided,
> we remove the vga_arb_get_fb_range_from_screen_info() call in
> vga_is_firmware_default() function, as following.
>
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - ret = vga_arb_get_fb_range_from_screen_info(_start, _end);
> -#else
> ret = vga_arb_get_fb_range_from_tracker(_start, _end);
> -#endif
>
> It is just that we don't observe the case which VRAM Bar of VGA compatible
> controller moves, so there just no need to unify it. But on LoongArch,
> the VRAM Bar of AMDGPU do moves.
>
> v2:
> * Fix test robot warnnings and fix typos
>
> v3:
> * Fix linkage problems if the global screen_info is not exported
This doesn't build on x86:
$ git checkout -b wip/sui-vga-default-arch-independent v6.5-rc1
$ b4 am 20230808145859.1590625-1-suijingf...@loongson.cn
$ git am
./20230808_suijingfeng_pci_vga_make_the_vga_is_firmware_default_less_arch_independent.mbx
$ make drivers/pci/vgaarb.o
CC drivers/pci/vgaarb.o
drivers/pci/vgaarb.c:114:13: error: ‘vga_arb_get_fb_range_from_tracker’
defined but not used [-Werror=unused-function]
114 | static bool vga_arb_get_fb_range_from_tracker(resource_size_t *start,
| ^
> Signed-off-by: Sui Jingfeng
> ---
> drivers/pci/vgaarb.c | 154 ++-
> 1 file changed, 139 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5a696078b382..e0919a70af3e 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -61,6 +61,92 @@ static bool vga_arbiter_used;
> static DEFINE_SPINLOCK(vga_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>
> +static struct firmware_fb_tracker {
> + /* The PCI(e) device who owns the firmware framebuffer */
> + struct pci_dev *pdev;
> + /* The index of the VRAM Bar */
> + unsigned int bar;
> + /* Firmware fb's offset from the VRAM aperture start */
> + resource_size_t offset;
> + /* The firmware fb's size, in bytes */
> + resource_size_t size;
> +
> + /* Firmware fb's address range, suffer from change */
> + resource_size_t start;
> + resource_size_t end;
It's redundant to save start, size, and end. Start and end should be
enough, and maybe you could use a struct resource for that.
It's not clear to me why you need to save the start/end/etc anyway.
All we need to know is which pci_dev is the firmware device. Doesn't
[PATCH] PCI/VGA: Make the vga_is_firmware_default() less arch-independent
Currently, the vga_is_firmware_default() function works only on x86 and
IA64 architectures, but it is a no-op on ARM64, PPC, RISC-V, etc. This
patch completes the implementation by tracking the firmware framebuffer's
address range. The added code is trying to identify the VRAM aperture that
contains the firmware framebuffer. Once found, related information about
the VRAM aperture will be tracked.
Note that we need to identify the VRAM aperture before it get moved. We
achieve this by using DECLARE_PCI_FIXUP_CLASS_HEADER(), which ensures that
vga_arb_firmware_fb_addr_tracker() gets called before PCI resource
allocation. Once we found the VRAM aperture that contains firmware fb, we
are able to monitor the address changes of it. If the VRAM aperture of the
primary GPU do moved, we will update our cached firmware framebuffer's
address range accordingly. This approach overcomes the VRAM bar relocation
issue successfully. Hence, this patch make the vga_is_firmware_default()
function works on whatever arch that has UEFI GOP support, including x86
and IA64. But, at the first step, we make it available only on platforms
which PCI resource relocation do happens. Once provided method proved to
be effective and reliable, it can be expanded to other arch easily.
This patch is tested on
1) LS3A5000+LS7A2000 and LS3A5000+LS7A1000 platform.
2) Intel i3-8100 CPU + H110 D4L motherboard with triple video cards:
$ lspci | grep VGA
Intel Corporation CoffeeLake-S GT2 [UHD Graphics 630]
Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 470] (rev cf)
ASPEED Technology, Inc. ASPEED Graphics Family (rev 52)
Note that on x86, in order to testing the new approach this patch provided,
we remove the vga_arb_get_fb_range_from_screen_info() call in
vga_is_firmware_default() function, as following.
-#if defined(CONFIG_X86) || defined(CONFIG_IA64)
- ret = vga_arb_get_fb_range_from_screen_info(_start, _end);
-#else
ret = vga_arb_get_fb_range_from_tracker(_start, _end);
-#endif
It is just that we don't observe the case which VRAM Bar of VGA compatible
controller moves, so there just no need to unify it. But on LoongArch,
the VRAM Bar of AMDGPU do moves.
v2:
* Fix test robot warnnings and fix typos
v3:
* Fix linkage problems if the global screen_info is not exported
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 154 ++-
1 file changed, 139 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5a696078b382..e0919a70af3e 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -61,6 +61,92 @@ static bool vga_arbiter_used;
static DEFINE_SPINLOCK(vga_lock);
static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
+static struct firmware_fb_tracker {
+ /* The PCI(e) device who owns the firmware framebuffer */
+ struct pci_dev *pdev;
+ /* The index of the VRAM Bar */
+ unsigned int bar;
+ /* Firmware fb's offset from the VRAM aperture start */
+ resource_size_t offset;
+ /* The firmware fb's size, in bytes */
+ resource_size_t size;
+
+ /* Firmware fb's address range, suffer from change */
+ resource_size_t start;
+ resource_size_t end;
+} firmware_fb;
+
+/*
+ * Get the physical address range that the firmware framebuffer occupies.
+ *
+ * The global screen_info is arch-specific; it will not be exported if the
+ * CONFIG_EFI is not selected on Arm64. Hence, CONFIG_EFI is chosen as
+ * compile-time conditional to suppress linkage problems. This guard can be
+ * removed if the global screen_info became arch-independent one day.
+ */
+static bool vga_arb_get_fb_range_from_screen_info(resource_size_t *start,
+ resource_size_t *end)
+{
+ resource_size_t fb_start = 0;
+ resource_size_t fb_size = 0;
+ resource_size_t fb_end;
+
+#if defined(CONFIG_EFI)
+ 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;
+}
+
+static bool vga_arb_get_fb_range_from_tracker(resource_size_t *start,
+ resource_size_t *end)
+{
+ struct pci_dev *pdev = firmware_fb.pdev;
+ resource_size_t new_vram_base;
+ resource_size_t new_fb_start;
+ resource_size_t old_fb_start;
+ resource_size_t old_fb_end;
+
+ /*
+* No firmware framebuffer support or no aperture that contains the
+* firmware FB is found. In this case, the firmware_fb.pdev will be
+* NULL. We will return immediately.
+*/
+ if (!pdev)
+
