diff mbox series

[v2,7/8] firmware/sysfb: Update screen_info for relocated EFI framebuffers

Message ID 20240202120140.3517-8-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series firmware/sysfb: Track parent device for screen_info | expand

Commit Message

Thomas Zimmermann Feb. 2, 2024, 11:58 a.m. UTC
On ARM PCI systems, the PCI hierarchy might be reconfigured during
boot and the firmware framebuffer might move as a result of that.
The values in screen_info will then be invalid.

Work around this problem by tracking the framebuffer's initial
location before it get relocated; then fix the screen_info state
between reloaction and creating the firmware framebuffer's device.

This functionality has been lifted from efifb. See the commit message
of commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that
covers the framebuffer") for more information.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb.c        |  2 +
 drivers/video/screen_info_pci.c | 88 +++++++++++++++++++++++++++++++++
 include/linux/screen_info.h     | 16 ++++++
 3 files changed, 106 insertions(+)

Comments

Sui Jingfeng Feb. 2, 2024, 5:54 p.m. UTC | #1
Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> +static inline void __screen_info_set_lfb_base(struct screen_info *si, u64 lfb_base)
> +{
> +	si->lfb_base = lfb_base & GENMASK_ULL(31, 0);
> +	si->ext_lfb_base = (lfb_base & GENMASK_ULL(63, 32)) >> 32;
> +
> +	if (si->ext_lfb_base)
> +		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
> +	else
> +		si->capabilities &= ~VIDEO_CAPABILITY_64BIT_BASE;
> +}
> +

Do we really has a need to modify the si->capabilities at here?
Sui Jingfeng Feb. 2, 2024, 6 p.m. UTC | #2
Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> +static inline void __screen_info_set_lfb_base(struct screen_info *si, u64 lfb_base)
> +{
> +	si->lfb_base = lfb_base & GENMASK_ULL(31, 0);
> +	si->ext_lfb_base = (lfb_base & GENMASK_ULL(63, 32)) >> 32;


I want to ask a trivial question: why not simply write it like below?

si->lfb_base = (u32)lfb_base;

si->ext_lfb_base = lfb_base >> 32;

I'm asking because I feel it is a little bit complicated.

> +	if (si->ext_lfb_base)
> +		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
> +	else
> +		si->capabilities &= ~VIDEO_CAPABILITY_64BIT_BASE;
> +}
> +
Thomas Zimmermann Feb. 5, 2024, 10:11 a.m. UTC | #3
Hi

Am 02.02.24 um 18:54 schrieb Sui Jingfeng:
> Hi,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> +static inline void __screen_info_set_lfb_base(struct screen_info *si, 
>> u64 lfb_base)
>> +{
>> +    si->lfb_base = lfb_base & GENMASK_ULL(31, 0);
>> +    si->ext_lfb_base = (lfb_base & GENMASK_ULL(63, 32)) >> 32;
>> +
>> +    if (si->ext_lfb_base)
>> +        si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
>> +    else
>> +        si->capabilities &= ~VIDEO_CAPABILITY_64BIT_BASE;
>> +}
>> +
> 
> Do we really has a need to modify the si->capabilities at here?

We need to set or clear the flag depending on the value of ext_lfb_base. 
Without the flag, decoders will ignore the value in ext_lfb_base.

Best regards
Thomas

>
Thomas Zimmermann Feb. 6, 2024, 4:45 p.m. UTC | #4
Hi

Am 02.02.24 um 19:00 schrieb Sui Jingfeng:
> Hi,
>
>
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> +static inline void __screen_info_set_lfb_base(struct screen_info 
>> *si, u64 lfb_base)
>> +{
>> +    si->lfb_base = lfb_base & GENMASK_ULL(31, 0);
>> +    si->ext_lfb_base = (lfb_base & GENMASK_ULL(63, 32)) >> 32;
>
>
> I want to ask a trivial question: why not simply write it like below?
>
> si->lfb_base = (u32)lfb_base;
>
> si->ext_lfb_base = lfb_base >> 32;
>
> I'm asking because I feel it is a little bit complicated.

Admittedly it's a bit verbose. I've written it like this so that it's 
clear which bits go where.

Best regards
Thomas

>
>> +    if (si->ext_lfb_base)
>> +        si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
>> +    else
>> +        si->capabilities &= ~VIDEO_CAPABILITY_64BIT_BASE;
>> +}
>> +
diff mbox series

Patch

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index ab5cbc0326f6d..87b9236577ca7 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -116,6 +116,8 @@  static __init int sysfb_init(void)
 	bool compatible;
 	int ret = 0;
 
+	screen_info_apply_fixups();
+
 	mutex_lock(&disable_lock);
 	if (disabled)
 		goto unlock_mutex;
diff --git a/drivers/video/screen_info_pci.c b/drivers/video/screen_info_pci.c
index d959a4c6ba3d5..afd568486dc24 100644
--- a/drivers/video/screen_info_pci.c
+++ b/drivers/video/screen_info_pci.c
@@ -1,7 +1,95 @@ 
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/pci.h>
+#include <linux/printk.h>
 #include <linux/screen_info.h>
+#include <linux/string.h>
+
+static struct pci_dev *screen_info_lfb_pdev;
+static size_t screen_info_lfb_bar;
+static resource_size_t screen_info_lfb_offset;
+static struct resource screen_info_lfb_res = DEFINE_RES_MEM(0, 0);
+
+static bool __screen_info_relocation_is_valid(const struct screen_info *si, struct resource *pr)
+{
+	u64 size = __screen_info_lfb_size(si, screen_info_video_type(si));
+
+	if (screen_info_lfb_offset > resource_size(pr))
+		return false;
+	if (size > resource_size(pr))
+		return false;
+	if (resource_size(pr) - size < screen_info_lfb_offset)
+		return false;
+
+	return true;
+}
+
+void screen_info_apply_fixups(void)
+{
+	struct screen_info *si = &screen_info;
+
+	if (screen_info_lfb_pdev) {
+		struct resource *pr = &screen_info_lfb_pdev->resource[screen_info_lfb_bar];
+
+		if (pr->start != screen_info_lfb_res.start) {
+			if (__screen_info_relocation_is_valid(si, pr)) {
+				/*
+				 * Only update base if we have an actual
+				 * relocation to a valid I/O range.
+				 */
+				__screen_info_set_lfb_base(si, pr->start + screen_info_lfb_offset);
+				pr_info("Relocating firmware framebuffer to offset %pa[d] within %pr\n",
+					&screen_info_lfb_offset, pr);
+			} else {
+				pr_warn("Invalid relocating, disabling firmware framebuffer\n");
+			}
+		}
+	}
+}
+
+static void screen_info_fixup_lfb(struct pci_dev *pdev)
+{
+	unsigned int type;
+	struct resource res[SCREEN_INFO_MAX_RESOURCES];
+	size_t i, numres;
+	int ret;
+	const struct screen_info *si = &screen_info;
+
+	if (screen_info_lfb_pdev)
+		return; // already found
+
+	type = screen_info_video_type(si);
+	if (type != VIDEO_TYPE_EFI)
+		return; // only applies to EFI
+
+	ret = screen_info_resources(si, res, ARRAY_SIZE(res));
+	if (ret < 0)
+		return;
+	numres = ret;
+
+	for (i = 0; i < numres; ++i) {
+		struct resource *r = &res[i];
+		const struct resource *pr;
+
+		if (!(r->flags & IORESOURCE_MEM))
+			continue;
+		pr = pci_find_resource(pdev, r);
+		if (!pr)
+			continue;
+
+		/*
+		 * We've found a PCI device with the framebuffer
+		 * resource. Store away the parameters to track
+		 * relocation of the framebuffer aperture.
+		 */
+		screen_info_lfb_pdev = pdev;
+		screen_info_lfb_bar = pr - pdev->resource;
+		screen_info_lfb_offset = r->start - pr->start;
+		memcpy(&screen_info_lfb_res, r, sizeof(screen_info_lfb_res));
+	}
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY, 16,
+			       screen_info_fixup_lfb);
 
 static struct pci_dev *__screen_info_pci_dev(struct resource *res)
 {
diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
index 0eae08e3c6f90..75303c126285a 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -4,6 +4,8 @@ 
 
 #include <uapi/linux/screen_info.h>
 
+#include <linux/bits.h>
+
 /**
  * SCREEN_INFO_MAX_RESOURCES - maximum number of resources per screen_info
  */
@@ -27,6 +29,17 @@  static inline u64 __screen_info_lfb_base(const struct screen_info *si)
 	return lfb_base;
 }
 
+static inline void __screen_info_set_lfb_base(struct screen_info *si, u64 lfb_base)
+{
+	si->lfb_base = lfb_base & GENMASK_ULL(31, 0);
+	si->ext_lfb_base = (lfb_base & GENMASK_ULL(63, 32)) >> 32;
+
+	if (si->ext_lfb_base)
+		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
+	else
+		si->capabilities &= ~VIDEO_CAPABILITY_64BIT_BASE;
+}
+
 static inline u64 __screen_info_lfb_size(const struct screen_info *si, unsigned int type)
 {
 	u64 lfb_size = si->lfb_size;
@@ -106,8 +119,11 @@  static inline unsigned int screen_info_video_type(const struct screen_info *si)
 ssize_t screen_info_resources(const struct screen_info *si, struct resource *r, size_t num);
 
 #if defined(CONFIG_PCI)
+void screen_info_apply_fixups(void);
 struct pci_dev *screen_info_pci_dev(const struct screen_info *si);
 #else
+static inline void screen_info_apply_fixups(void)
+{ }
 static inline struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
 {
 	return NULL;