diff mbox series

PCI/VGA: Make the vga_is_firmware_default() arch-independent

Message ID 20230803081758.968742-1-suijingfeng@loongson.cn
State New
Headers
Series PCI/VGA: Make the vga_is_firmware_default() arch-independent |

Commit Message

Sui Jingfeng Aug. 3, 2023, 8:17 a.m. UTC 
  Currently, the vga_is_firmware_default() function works 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 the initial VRAM aperture (the one that contains firmware fb)
identification should be done before the PCI resource relocation. This is
because we need to lock the VRAM aperture before it is 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.

This patch overcame the VRAM bar relocation issue by updating the cached
firmware framebuffer's address range accordingly if the VRAM bar of the
primary GPU do moved. We achieve that by monitoring the address changes of
the VRAM aperture.

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 to be effective and reliable, it can be expanded
to other arch easily.

This patch is tested on LS3A5000+LS7A2000 platform and LS3A5000+LS7A1000
platform. This patch can be applied on pci-next (6.5.0-rc1+) branch cleanly

v2:
	* Fix test robot warnnings and fix typos

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

Comments

kernel test robot Aug. 3, 2023, 12:25 p.m. UTC | #1 
Hi Sui,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.5-rc4 next-20230803]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Make-the-vga_is_firmware_default-arch-independent/20230803-161838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20230803081758.968742-1-suijingfeng%40loongson.cn
patch subject: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent
config: arm64-randconfig-r026-20230731 (https://download.01.org/0day-ci/archive/20230803/202308032022.yiZngbbk-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230803/202308032022.yiZngbbk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308032022.yiZngbbk-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: screen_info
   >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86)
   >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
   >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86)
   >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
   >>> referenced by vgaarb.c:88 (drivers/pci/vgaarb.c:88)
   >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
   >>> referenced 3 more times
Sui Jingfeng Aug. 4, 2023, 3:11 a.m. UTC | #2 
Hi,

On 2023/8/3 20:25, kernel test robot wrote:
> Hi Sui,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on pci/next]
> [also build test ERROR on pci/for-linus linus/master v6.5-rc4 next-20230803]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Make-the-vga_is_firmware_default-arch-independent/20230803-161838
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:    https://lore.kernel.org/r/20230803081758.968742-1-suijingfeng%40loongson.cn
> patch subject: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent
> config: arm64-randconfig-r026-20230731 (https://download.01.org/0day-ci/archive/20230803/202308032022.yiZngbbk-lkp@intel.com/config)
> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> reproduce: (https://download.01.org/0day-ci/archive/20230803/202308032022.yiZngbbk-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202308032022.yiZngbbk-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>>> ld.lld: error: undefined symbol: screen_info
>     >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86)
>     >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
>     >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86)
>     >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
>     >>> referenced by vgaarb.c:88 (drivers/pci/vgaarb.c:88)
>     >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
>     >>> referenced 3 more times
>
This is a more like arch-specific problem, It will be pain at many places on platforms
that do not export the screen_info symbol. Not only here.

I have already explained that screen_info is arch-dependent many times, but no one cares about me.
By using (looking at) screen_info, vgaarb gets infected, and becomes arch-dependent as well.
vgaarb deals with VGA class (pdev->class == 0x0300XX) devices only, This makes it device-dependent.
Hence, It only works correctly for a small set of PCIe devices on x86.

arch-dependent, device-dependent, subsystem-dependent (part of it rely on ACPI) and
loading order dependent, those dependent itself are the problems.
It results in various undefined (uncertain) behaviors on non-x86 architectures.

Even on x86, some platform choose to relay on the firmware to solve the multiple GPU coexist problem.
so it is also firmware-dependent.

This patch solves part of the above problems listed, target at the *device level*, as early as possible.
while they still a few problems could be only solved at the *driver level*.
For an example, The display controller in Intel N2000 and d2000 series don't has a dedicated VRAM bar.
they use the "stolen memory", which is carve out by somebody (either bios or kernel?).
Bjorn Helgaas Aug. 4, 2023, 4:38 p.m. UTC | #3 
On Fri, Aug 04, 2023 at 11:11:12AM +0800, suijingfeng wrote:
> On 2023/8/3 20:25, kernel test robot wrote:
> > Hi Sui,
> > 
> > kernel test robot noticed the following build errors:
> > 
> > [auto build test ERROR on pci/next]
> > [also build test ERROR on pci/for-linus linus/master v6.5-rc4 next-20230803]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Make-the-vga_is_firmware_default-arch-independent/20230803-161838
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> > patch link:    https://lore.kernel.org/r/20230803081758.968742-1-suijingfeng%40loongson.cn
> > patch subject: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent
> > config: arm64-randconfig-r026-20230731 (https://download.01.org/0day-ci/archive/20230803/202308032022.yiZngbbk-lkp@intel.com/config)
> > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> > reproduce: (https://download.01.org/0day-ci/archive/20230803/202308032022.yiZngbbk-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202308032022.yiZngbbk-lkp@intel.com/
> > 
> > All errors (new ones prefixed by >>):
> > 
> > > > ld.lld: error: undefined symbol: screen_info
> >     >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86)
> >     >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
> >     >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86)
> >     >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
> >     >>> referenced by vgaarb.c:88 (drivers/pci/vgaarb.c:88)
> >     >>>               drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a
> >     >>> referenced 3 more times
> > 
> This is a more like arch-specific problem, It will be pain at many places on platforms
> that do not export the screen_info symbol. Not only here.
> 
> I have already explained that screen_info is arch-dependent many times, but no one cares about me.
> By using (looking at) screen_info, vgaarb gets infected, and becomes arch-dependent as well.
> vgaarb deals with VGA class (pdev->class == 0x0300XX) devices only, This makes it device-dependent.
> Hence, It only works correctly for a small set of PCIe devices on x86.

This build error report is from an automated service; there's nothing
personal about it and the automated service isn't going to respond to
you.

The build issue is just something that will have to be resolved before
we can consider merging the patch.

Any explanation needs to go in the commit logs for the relevant
patches.

> arch-dependent, device-dependent, subsystem-dependent (part of it rely on ACPI) and
> loading order dependent, those dependent itself are the problems.
> It results in various undefined (uncertain) behaviors on non-x86 architectures.
> 
> Even on x86, some platform choose to relay on the firmware to solve the multiple GPU coexist problem.
> so it is also firmware-dependent.
> 
> This patch solves part of the above problems listed, target at the *device level*, as early as possible.
> while they still a few problems could be only solved at the *driver level*.
> For an example, The display controller in Intel N2000 and d2000 series don't has a dedicated VRAM bar.
> they use the "stolen memory", which is carve out by somebody (either bios or kernel?).
> 
>
kernel test robot Aug. 10, 2023, 8:49 p.m. UTC | #4 
Hi Sui,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.5-rc5 next-20230809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Make-the-vga_is_firmware_default-arch-independent/20230803-161838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20230803081758.968742-1-suijingfeng%40loongson.cn
patch subject: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent
config: arm64-randconfig-r052-20230811 (https://download.01.org/0day-ci/archive/20230811/202308110416.0wAKt0vo-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230811/202308110416.0wAKt0vo-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308110416.0wAKt0vo-lkp@intel.com/

All errors (new ones prefixed by >>):

   aarch64-linux-ld: drivers/pci/vgaarb.o: in function `vga_arb_firmware_fb_addr_tracker':
>> vgaarb.c:(.text+0x138): undefined reference to `screen_info'
   aarch64-linux-ld: drivers/pci/vgaarb.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `screen_info' which may bind externally can not be used when making a shared object; recompile with -fPIC
   vgaarb.c:(.text+0x138): dangerous relocation: unsupported relocation
>> aarch64-linux-ld: vgaarb.c:(.text+0x13c): undefined reference to `screen_info'
diff mbox series

Patch

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5a696078b382..1f2d464e5812 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -61,6 +61,82 @@  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;
+
+static bool vga_arb_get_fb_range_from_screen_info(resource_size_t *start,
+						  resource_size_t *end)
+{
+	resource_size_t fb_start;
+	resource_size_t fb_end;
+	resource_size_t fb_size;
+
+	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;
+
+	/* 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)
+		return false;
+
+	new_vram_base = pdev->resource[firmware_fb.bar].start;
+	new_fb_start = new_vram_base + firmware_fb.offset;
+	old_fb_start = firmware_fb.start;
+	old_fb_end = firmware_fb.end;
+
+	if (new_fb_start != old_fb_start) {
+		firmware_fb.start = new_fb_start;
+		firmware_fb.end = new_fb_start + firmware_fb.size - 1;
+		vgaarb_dbg(&pdev->dev,
+			   "[0x%llx, 0x%llx] -> [0x%llx, 0x%llx]\n",
+			   (u64)old_fb_start, (u64)old_fb_end,
+			   (u64)firmware_fb.start, (u64)firmware_fb.end);
+	}
+
+	*start = firmware_fb.start;
+	*end = firmware_fb.end;
+
+	return true;
+}
 
 static const char *vga_iostate_to_str(unsigned int iostate)
 {
@@ -543,20 +619,21 @@  void vga_put(struct pci_dev *pdev, unsigned int rsrc)
 }
 EXPORT_SYMBOL(vga_put);
 
+/* Select the device owning the boot framebuffer if there is one */
 static bool vga_is_firmware_default(struct pci_dev *pdev)
 {
-#if defined(CONFIG_X86) || defined(CONFIG_IA64)
-	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 */
+	resource_size_t fb_start;
+	resource_size_t fb_end;
+	bool ret;
 
-	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
-		base |= (u64)screen_info.ext_lfb_base << 32;
-
-	limit = base + size;
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+	ret = vga_arb_get_fb_range_from_screen_info(&fb_start, &fb_end);
+#else
+	ret = vga_arb_get_fb_range_from_tracker(&fb_start, &fb_end);
+#endif
+	if (!ret)
+		return false;
 
 	/* Does firmware framebuffer belong to us? */
 	pci_dev_for_each_resource(pdev, r) {
@@ -566,12 +643,10 @@  static bool vga_is_firmware_default(struct pci_dev *pdev)
 		if (!r->start || !r->end)
 			continue;
 
-		if (base < r->start || limit >= r->end)
-			continue;
-
-		return true;
+		if (fb_start >= r->start && fb_end <= r->end)
+			return true;
 	}
-#endif
+
 	return false;
 }
 
@@ -1555,3 +1630,38 @@  static int __init vga_arb_device_init(void)
 	return rc;
 }
 subsys_initcall_sync(vga_arb_device_init);
+
+static void vga_arb_firmware_fb_addr_tracker(struct pci_dev *pdev)
+{
+	resource_size_t fb_start;
+	resource_size_t fb_end;
+	unsigned int i;
+
+	if (!vga_arb_get_fb_range_from_screen_info(&fb_start, &fb_end))
+		return;
+
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		struct resource *ap = &pdev->resource[i];
+
+		if (resource_type(ap) != IORESOURCE_MEM)
+			continue;
+
+		if (!ap->start || !ap->end)
+			continue;
+
+		if (ap->start <= fb_start && fb_end <= ap->end) {
+			firmware_fb.pdev = pdev;
+			firmware_fb.bar = i;
+			firmware_fb.size = fb_end - fb_start + 1;
+			firmware_fb.offset = fb_start - ap->start;
+			firmware_fb.start = fb_start;
+			firmware_fb.end = fb_end;
+
+			vgaarb_dbg(&pdev->dev,
+				   "BAR %u contains firmware FB\n", i);
+			break;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA,
+			       8, vga_arb_firmware_fb_addr_tracker);