[PATCH v2 0/8] firmware/sysfb: Track parent device for screen_info

[PATCH v2 0/8] firmware/sysfb: Track parent device for screen_info

[Thomas Zimmermann]

Detect the firmware framebuffer's parent device from the global
screen_info state and set up the framebuffer's device accordingly.
Remove the equivalent functionality from efifb. Other drivers for
firmware framebuffers, such as simpledrm or vesafb, now add these
new features.

Patches 1 and 2 provide a set of helper functions to avoid parsing
the screen_info values manually. Decoding screen_info is fragile and
many drivers get it wrong. We should later adopt these helpers in
existing drivers, such as efifb, vesafb, as well.

Patches 3 and 4 set the firmware framebuffer's parent device. There
is code in efifb to do something similar for power management. That
is now obsolete and being cleaned up. Setting the parent device makes
Linux track the power management correctly.

Patches 5 and 6 track the parent device's enable state. We don't
create framebuffer devices if the underlying hardware device has been
disabled. Remove the functionality from efifb.

Patches 7 and 8 track the parent device's PCI BAR location. It can
happen on aarch64 that the firmware framebuffer moves its location
during the kernel's boot. We now fix up the screen_info state to
point to the correct location. Again remove such functionality from
efifb.

v2:
	* small refactorings throughout the patchset

Thomas Zimmermann (8):
  video: Add helpers for decoding screen_info
  video: Provide screen_info_get_pci_dev() to find screen_info's PCI
    device
  firmware/sysfb: Set firmware-framebuffer parent device
  fbdev/efifb: Remove PM for parent device
  firmware/sysfb: Create firmware device only for enabled PCI devices
  fbdev/efifb: Do not track parent device status
  firmware/sysfb: Update screen_info for relocated EFI framebuffers
  fbdev/efifb: Remove framebuffer relocation tracking

 drivers/firmware/Kconfig            |   1 +
 drivers/firmware/sysfb.c            |  49 +++++++++-
 drivers/firmware/sysfb_simplefb.c   |   5 +-
 drivers/video/Kconfig               |   4 +
 drivers/video/Makefile              |   4 +
 drivers/video/fbdev/efifb.c         |  97 +-----------------
 drivers/video/screen_info_generic.c | 146 ++++++++++++++++++++++++++++
 drivers/video/screen_info_pci.c     | 140 ++++++++++++++++++++++++++
 include/linux/screen_info.h         | 126 ++++++++++++++++++++++++
 include/linux/sysfb.h               |   3 +-
 10 files changed, 480 insertions(+), 95 deletions(-)
 create mode 100644 drivers/video/screen_info_generic.c
 create mode 100644 drivers/video/screen_info_pci.c

-- 
2.43.0

[PATCH v2 1/8] video: Add helpers for decoding screen_info

[Thomas Zimmermann]

The plain values as stored in struct screen_info need to be decoded
before being used. Add helpers that decode the type of video output
and the framebuffer I/O aperture.

Old or non-x86 systems may not set the type of video directly, but
only indicate the presence by storing 0x01 in orig_video_isVGA. The
decoding logic in screen_info_video_type() takes this into account.
It then follows similar code in vgacon's vgacon_startup() to detect
the video type from the given values.

A call to screen_info_resources() returns all known resources of the
given screen_info. The resources' values have been taken from existing
code in vgacon and vga16fb. These drivers can later be converted to
use the new interfaces.

v2:
	* return ssize_t from screen_info_resources()
	* don't call __screen_info_has_lfb() unnecessarily

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/Kconfig            |   1 +
 drivers/video/Kconfig               |   4 +
 drivers/video/Makefile              |   3 +
 drivers/video/screen_info_generic.c | 146 ++++++++++++++++++++++++++++
 include/linux/screen_info.h         | 100 +++++++++++++++++++
 5 files changed, 254 insertions(+)
 create mode 100644 drivers/video/screen_info_generic.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index afd38539b92e5..71d8b26c4103b 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -182,6 +182,7 @@ config MTK_ADSP_IPC
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
+	select SCREEN_INFO
 
 config SYSFB_SIMPLEFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 130ebccb83380..44c9ef1435a2d 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -11,6 +11,10 @@ config APERTURE_HELPERS
 	  Support tracking and hand-over of aperture ownership. Required
 	  by graphics drivers for firmware-provided framebuffers.
 
+config SCREEN_INFO
+	bool
+	default n
+
 config STI_CORE
 	bool
 	depends on PARISC
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 9eb5557911de5..117cbdbb58c2c 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -1,11 +1,14 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_APERTURE_HELPERS)    += aperture.o
+obj-$(CONFIG_SCREEN_INFO)         += screen_info.o
 obj-$(CONFIG_STI_CORE)            += sticore.o
 obj-$(CONFIG_VGASTATE)            += vgastate.o
 obj-$(CONFIG_VIDEO)               += cmdline.o nomodeset.o
 obj-$(CONFIG_HDMI)                += hdmi.o
 
+screen_info-y			  := screen_info_generic.o
+
 obj-$(CONFIG_VT)		  += console/
 obj-$(CONFIG_FB_STI)		  += console/
 obj-$(CONFIG_LOGO)		  += logo/
diff --git a/drivers/video/screen_info_generic.c b/drivers/video/screen_info_generic.c
new file mode 100644
index 0000000000000..64117c6367abb
--- /dev/null
+++ b/drivers/video/screen_info_generic.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/export.h>
+#include <linux/ioport.h>
+#include <linux/screen_info.h>
+#include <linux/string.h>
+
+static void resource_init_named(struct resource *r,
+				resource_size_t start, resource_size_t size,
+				const char *name, unsigned int flags)
+{
+	memset(r, 0, sizeof(*r));
+
+	r->start = start;
+	r->end = start + size - 1;
+	r->name = name;
+	r->flags = flags;
+}
+
+static void resource_init_io_named(struct resource *r,
+				   resource_size_t start, resource_size_t size,
+				   const char *name)
+{
+	resource_init_named(r, start, size, name, IORESOURCE_IO);
+}
+
+static void resource_init_mem_named(struct resource *r,
+				   resource_size_t start, resource_size_t size,
+				   const char *name)
+{
+	resource_init_named(r, start, size, name, IORESOURCE_MEM);
+}
+
+static inline bool __screen_info_has_ega_gfx(unsigned int mode)
+{
+	switch (mode) {
+	case 0x0d:	/* 320x200-4 */
+	case 0x0e:	/* 640x200-4 */
+	case 0x0f:	/* 640x350-1 */
+	case 0x10:	/* 640x350-4 */
+		return true;
+	default:
+		return false;
+	}
+}
+
+static inline bool __screen_info_has_vga_gfx(unsigned int mode)
+{
+	switch (mode) {
+	case 0x10:	/* 640x480-1 */
+	case 0x12:	/* 640x480-4 */
+	case 0x13:	/* 320-200-8 */
+	case 0x6a:	/* 800x600-4 (VESA) */
+		return true;
+	default:
+		return __screen_info_has_ega_gfx(mode);
+	}
+}
+
+/**
+ * screen_info_resources() - Get resources from screen_info structure
+ * @si: the screen_info
+ * @r: pointer to an array of resource structures
+ * @num: number of elements in @r:
+ *
+ * Returns:
+ * The number of resources stored in @r on success, or a negative errno code otherwise.
+ *
+ * A call to screen_info_resources() returns the resources consumed by the
+ * screen_info's device or framebuffer. The result is stored in the caller-supplied
+ * array @r with up to @num elements. The function returns the number of
+ * initialized elements.
+ */
+ssize_t screen_info_resources(const struct screen_info *si, struct resource *r, size_t num)
+{
+	struct resource *pos = r;
+	unsigned int type = screen_info_video_type(si);
+	u64 base, size;
+
+	switch (type) {
+	case VIDEO_TYPE_MDA:
+		if (num > 0)
+			resource_init_io_named(pos++, 0x3b0, 12, "mda");
+		if (num > 1)
+			resource_init_io_named(pos++, 0x3bf, 0x01, "mda");
+		if (num > 2)
+			resource_init_mem_named(pos++, 0xb0000, 0x2000, "mda");
+		break;
+	case VIDEO_TYPE_CGA:
+		if (num > 0)
+			resource_init_io_named(pos++, 0x3d4, 0x02, "cga");
+		if (num > 1)
+			resource_init_mem_named(pos++, 0xb8000, 0x2000, "cga");
+		break;
+	case VIDEO_TYPE_EGAM:
+		if (num > 0)
+			resource_init_io_named(pos++, 0x3bf, 0x10, "ega");
+		if (num > 1)
+			resource_init_mem_named(pos++, 0xb0000, 0x8000, "ega");
+		break;
+	case VIDEO_TYPE_EGAC:
+		if (num > 0)
+			resource_init_io_named(pos++, 0x3c0, 0x20, "ega");
+		if (num > 1) {
+			if (__screen_info_has_ega_gfx(si->orig_video_mode))
+				resource_init_mem_named(pos++, 0xa0000, 0x10000, "ega");
+			else
+				resource_init_mem_named(pos++, 0xb8000, 0x8000, "ega");
+		}
+		break;
+	case VIDEO_TYPE_VGAC:
+		if (num > 0)
+			resource_init_io_named(pos++, 0x3c0, 0x20, "vga+");
+		if (num > 1) {
+			if (__screen_info_has_vga_gfx(si->orig_video_mode))
+				resource_init_mem_named(pos++, 0xa0000, 0x10000, "vga+");
+			else
+				resource_init_mem_named(pos++, 0xb8000, 0x8000, "vga+");
+		}
+		break;
+	case VIDEO_TYPE_VLFB:
+	case VIDEO_TYPE_EFI:
+		base = __screen_info_lfb_base(si);
+		if (!base)
+			break;
+		size = __screen_info_lfb_size(si, type);
+		if (!size)
+			break;
+		if (num > 0)
+			resource_init_mem_named(pos++, base, size, "lfb");
+		break;
+	case VIDEO_TYPE_PICA_S3:
+	case VIDEO_TYPE_MIPS_G364:
+	case VIDEO_TYPE_SGI:
+	case VIDEO_TYPE_TGAC:
+	case VIDEO_TYPE_SUN:
+	case VIDEO_TYPE_SUNPCI:
+	case VIDEO_TYPE_PMAC:
+	default:
+		/* not supported */
+		return -EINVAL;
+	}
+
+	return pos - r;
+}
+EXPORT_SYMBOL(screen_info_resources);
diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
index eab7081392d50..e7a02c5609d12 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -4,6 +4,106 @@
 
 #include <uapi/linux/screen_info.h>
 
+/**
+ * SCREEN_INFO_MAX_RESOURCES - maximum number of resources per screen_info
+ */
+#define SCREEN_INFO_MAX_RESOURCES	3
+
+struct resource;
+
+static inline bool __screen_info_has_lfb(unsigned int type)
+{
+	return (type == VIDEO_TYPE_VLFB) || (type == VIDEO_TYPE_EFI);
+}
+
+static inline u64 __screen_info_lfb_base(const struct screen_info *si)
+{
+	u64 lfb_base = si->lfb_base;
+
+	if (si->capabilities & VIDEO_CAPABILITY_64BIT_BASE)
+		lfb_base |= (u64)si->ext_lfb_base << 32;
+
+	return lfb_base;
+}
+
+static inline u64 __screen_info_lfb_size(const struct screen_info *si, unsigned int type)
+{
+	u64 lfb_size = si->lfb_size;
+
+	if (type == VIDEO_TYPE_VLFB)
+		lfb_size <<= 16;
+	return lfb_size;
+}
+
+static inline unsigned int __screen_info_video_type(unsigned int type)
+{
+	switch (type) {
+	case VIDEO_TYPE_MDA:
+	case VIDEO_TYPE_CGA:
+	case VIDEO_TYPE_EGAM:
+	case VIDEO_TYPE_EGAC:
+	case VIDEO_TYPE_VGAC:
+	case VIDEO_TYPE_VLFB:
+	case VIDEO_TYPE_PICA_S3:
+	case VIDEO_TYPE_MIPS_G364:
+	case VIDEO_TYPE_SGI:
+	case VIDEO_TYPE_TGAC:
+	case VIDEO_TYPE_SUN:
+	case VIDEO_TYPE_SUNPCI:
+	case VIDEO_TYPE_PMAC:
+	case VIDEO_TYPE_EFI:
+		return type;
+	default:
+		return 0;
+	}
+}
+
+/**
+ * screen_info_video_type() - Decodes the video type from struct screen_info
+ * @si: an instance of struct screen_info
+ *
+ * Returns:
+ * A VIDEO_TYPE_ constant representing si's type of video display, or 0 otherwise.
+ */
+static inline unsigned int screen_info_video_type(const struct screen_info *si)
+{
+	unsigned int type;
+
+	// check if display output is on
+	if (!si->orig_video_isVGA)
+		return 0;
+
+	// check for a known VIDEO_TYPE_ constant
+	type = __screen_info_video_type(si->orig_video_isVGA);
+	if (type)
+		return si->orig_video_isVGA;
+
+	// check if text mode has been initialized
+	if (!si->orig_video_lines || !si->orig_video_cols)
+		return 0;
+
+	// 80x25 text, mono
+	if (si->orig_video_mode == 0x07) {
+		if ((si->orig_video_ega_bx & 0xff) != 0x10)
+			return VIDEO_TYPE_EGAM;
+		else
+			return VIDEO_TYPE_MDA;
+	}
+
+	// EGA/VGA, 16 colors
+	if ((si->orig_video_ega_bx & 0xff) != 0x10) {
+		if (si->orig_video_isVGA)
+			return VIDEO_TYPE_VGAC;
+		else
+			return VIDEO_TYPE_EGAC;
+	}
+
+	// the rest...
+	return VIDEO_TYPE_CGA;
+}
+
+ssize_t screen_info_resources(const struct screen_info *si, struct resource *r, size_t num);
+
 extern struct screen_info screen_info;
 
 #endif /* _SCREEN_INFO_H */
-- 
2.43.0

[PATCH v2 2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Thomas Zimmermann]

Add screen_info_get_pci_dev() to find the PCI device of an instance
of screen_info. Does nothing on systems without PCI bus.

v2:
	* remove ret from screen_info_pci_dev() (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/Makefile          |  1 +
 drivers/video/screen_info_pci.c | 52 +++++++++++++++++++++++++++++++++
 include/linux/screen_info.h     | 10 +++++++
 3 files changed, 63 insertions(+)
 create mode 100644 drivers/video/screen_info_pci.c

diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 117cbdbb58c2c..ffbac4387c670 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_VIDEO)               += cmdline.o nomodeset.o
 obj-$(CONFIG_HDMI)                += hdmi.o
 
 screen_info-y			  := screen_info_generic.o
+screen_info-$(CONFIG_PCI)         += screen_info_pci.o
 
 obj-$(CONFIG_VT)		  += console/
 obj-$(CONFIG_FB_STI)		  += console/
diff --git a/drivers/video/screen_info_pci.c b/drivers/video/screen_info_pci.c
new file mode 100644
index 0000000000000..d959a4c6ba3d5
--- /dev/null
+++ b/drivers/video/screen_info_pci.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/pci.h>
+#include <linux/screen_info.h>
+
+static struct pci_dev *__screen_info_pci_dev(struct resource *res)
+{
+	struct pci_dev *pdev;
+
+	if (!(res->flags & IORESOURCE_MEM))
+		return NULL;
+
+	for_each_pci_dev(pdev) {
+		const struct resource *r;
+
+		if ((pdev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
+			continue;
+
+		r = pci_find_resource(pdev, res);
+		if (r)
+			return pdev;
+	}
+
+	return NULL;
+}
+
+/**
+ * screen_info_pci_dev() - Return PCI parent device that contains screen_info's framebuffer
+ * @si: the screen_info
+ *
+ * Returns:
+ * The screen_info's parent device on success, or NULL otherwise.
+ */
+struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
+{
+	struct resource res[SCREEN_INFO_MAX_RESOURCES];
+	ssize_t i, numres;
+
+	numres = screen_info_resources(si, res, ARRAY_SIZE(res));
+	if (numres < 0)
+		return ERR_PTR(numres);
+
+	for (i = 0; i < numres; ++i) {
+		struct pci_dev *pdev = __screen_info_pci_dev(&res[i]);
+
+		if (pdev)
+			return pdev;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(screen_info_pci_dev);
diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
index e7a02c5609d12..0eae08e3c6f90 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -9,6 +9,7 @@
  */
 #define SCREEN_INFO_MAX_RESOURCES	3
 
+struct pci_dev;
 struct resource;
 
 static inline bool __screen_info_has_lfb(unsigned int type)
@@ -104,6 +105,15 @@ 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)
+struct pci_dev *screen_info_pci_dev(const struct screen_info *si);
+#else
+static inline struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
+{
+	return NULL;
+}
+#endif
+
 extern struct screen_info screen_info;
 
 #endif /* _SCREEN_INFO_H */
-- 
2.43.0

[PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device

[Thomas Zimmermann]

Set the firmware framebuffer's parent device, which usually is the
graphics hardware's physical device. Integrates the framebuffer in
the Linux device hierarchy and lets Linux handle dependencies among
devices. For example, the graphics hardware won't be suspended while
the firmware device is still active.

v2:
	* detect parent device in sysfb_parent_dev()

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb.c          | 19 ++++++++++++++++++-
 drivers/firmware/sysfb_simplefb.c |  5 ++++-
 include/linux/sysfb.h             |  3 ++-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 3c197db42c9d9..d02945b0d8ea1 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -29,6 +29,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/pci.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
@@ -69,9 +70,21 @@ void sysfb_disable(void)
 }
 EXPORT_SYMBOL_GPL(sysfb_disable);
 
+static __init struct device *sysfb_parent_dev(const struct screen_info *si)
+{
+	struct pci_dev *pdev;
+
+	pdev = screen_info_pci_dev(si);
+	if (pdev)
+		return &pdev->dev;
+
+	return NULL;
+}
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
+	struct device *parent;
 	struct simplefb_platform_data mode;
 	const char *name;
 	bool compatible;
@@ -83,10 +96,12 @@ static __init int sysfb_init(void)
 
 	sysfb_apply_efi_quirks();
 
+	parent = sysfb_parent_dev(si);
+
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);
 	if (compatible) {
-		pd = sysfb_create_simplefb(si, &mode);
+		pd = sysfb_create_simplefb(si, &mode, parent);
 		if (!IS_ERR(pd))
 			goto unlock_mutex;
 	}
@@ -109,6 +124,8 @@ static __init int sysfb_init(void)
 		goto unlock_mutex;
 	}
 
+	pd->dev.parent = parent;
+
 	sysfb_set_efifb_fwnode(pd);
 
 	ret = platform_device_add_data(pd, si, sizeof(*si));
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 74363ed7501f6..75a186bf8f8ec 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -91,7 +91,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 }
 
 __init struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
-						     const struct simplefb_platform_data *mode)
+						     const struct simplefb_platform_data *mode,
+						     struct device *parent)
 {
 	struct platform_device *pd;
 	struct resource res;
@@ -143,6 +144,8 @@ __init struct platform_device *sysfb_create_simplefb(const struct screen_info *s
 	if (!pd)
 		return ERR_PTR(-ENOMEM);
 
+	pd->dev.parent = parent;
+
 	sysfb_set_efifb_fwnode(pd);
 
 	ret = platform_device_add_resources(pd, &res, 1);
diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
index 19cb803dd5ecd..6ee3ade3f8b06 100644
--- a/include/linux/sysfb.h
+++ b/include/linux/sysfb.h
@@ -91,7 +91,8 @@ static inline void sysfb_set_efifb_fwnode(struct platform_device *pd)
 bool sysfb_parse_mode(const struct screen_info *si,
 		      struct simplefb_platform_data *mode);
 struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
-					      const struct simplefb_platform_data *mode);
+					      const struct simplefb_platform_data *mode,
+					      struct device *parent);
 
 #else /* CONFIG_SYSFB_SIMPLE */
 
-- 
2.43.0

[PATCH v2 4/8] fbdev/efifb: Remove PM for parent device

[Thomas Zimmermann]

The EFI device has the correct parent device set. This allows Linux
to handle the power management internally. Hence, remove the manual
PM management for the parent device from efifb.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/efifb.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 10fc14ad5d127..e66ef35fa6b62 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -17,7 +17,6 @@
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/screen_info.h>
-#include <linux/pm_runtime.h>
 #include <video/vga.h>
 #include <asm/efi.h>
 #include <drm/drm_utils.h> /* For drm_get_panel_orientation_quirk */
@@ -258,9 +257,6 @@ static void efifb_destroy(struct fb_info *info)
 {
 	struct efifb_par *par = info->par;
 
-	if (efifb_pci_dev)
-		pm_runtime_put(&efifb_pci_dev->dev);
-
 	if (info->screen_base) {
 		if (mem_flags & (EFI_MEMORY_UC | EFI_MEMORY_WC))
 			iounmap(info->screen_base);
@@ -598,26 +594,20 @@ static int efifb_probe(struct platform_device *dev)
 		goto err_groups;
 	}
 
-	if (efifb_pci_dev)
-		WARN_ON(pm_runtime_get_sync(&efifb_pci_dev->dev) < 0);
-
 	err = devm_aperture_acquire_for_platform_device(dev, par->base, par->size);
 	if (err) {
 		pr_err("efifb: cannot acquire aperture\n");
-		goto err_put_rpm_ref;
+		goto err_fb_dealloc_cmap;
 	}
 	err = register_framebuffer(info);
 	if (err < 0) {
 		pr_err("efifb: cannot register framebuffer\n");
-		goto err_put_rpm_ref;
+		goto err_fb_dealloc_cmap;
 	}
 	fb_info(info, "%s frame buffer device\n", info->fix.id);
 	return 0;
 
-err_put_rpm_ref:
-	if (efifb_pci_dev)
-		pm_runtime_put(&efifb_pci_dev->dev);
-
+err_fb_dealloc_cmap:
 	fb_dealloc_cmap(&info->cmap);
 err_groups:
 	sysfs_remove_groups(&dev->dev.kobj, efifb_groups);
-- 
2.43.0

[PATCH v2 5/8] firmware/sysfb: Create firmware device only for enabled PCI devices

[Thomas Zimmermann]

Test if the firmware framebuffer's parent PCI device, if any, has
been enabled. If not, the firmware framebuffer is most likely not
working. Hence, do not create a device for the firmware framebuffer
on disabled PCI devices.

So far, efifb tracked the status of the PCI parent device internally
and did not bind if it was disabled. This patch implements the
functionality for all firmware framebuffers.

v2:
	* rework sysfb_pci_dev_is_enabled() (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index d02945b0d8ea1..ab5cbc0326f6d 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -70,13 +70,39 @@ void sysfb_disable(void)
 }
 EXPORT_SYMBOL_GPL(sysfb_disable);
 
+#if defined(CONFIG_PCI)
+static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
+{
+	/*
+	 * TODO: Try to integrate this code into the PCI subsystem
+	 */
+	int ret;
+	u16 command;
+
+	ret = pci_read_config_word(pdev, PCI_COMMAND, &command);
+	if (ret != PCIBIOS_SUCCESSFUL)
+		return false;
+	if (!(command & PCI_COMMAND_MEMORY))
+		return false;
+	return true;
+}
+#else
+static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
+{
+	return false;
+}
+#endif
+
 static __init struct device *sysfb_parent_dev(const struct screen_info *si)
 {
 	struct pci_dev *pdev;
 
 	pdev = screen_info_pci_dev(si);
-	if (pdev)
+	if (pdev) {
+		if (!sysfb_pci_dev_is_enabled(pdev))
+			return ERR_PTR(-ENODEV);
 		return &pdev->dev;
+	}
 
 	return NULL;
 }
@@ -97,6 +123,8 @@ static __init int sysfb_init(void)
 	sysfb_apply_efi_quirks();
 
 	parent = sysfb_parent_dev(si);
+	if (IS_ERR(parent))
+		goto unlock_mutex;
 
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);
-- 
2.43.0

[PATCH v2 6/8] fbdev/efifb: Do not track parent device status

[Thomas Zimmermann]

There will be no EFI framebuffer device for disabled parent devices
and thus we never probe efifb in that case. Hence remove the tracking
code from efifb.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/efifb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index e66ef35fa6b62..f76b7ae007518 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -348,8 +348,6 @@ static struct attribute *efifb_attrs[] = {
 };
 ATTRIBUTE_GROUPS(efifb);
 
-static bool pci_dev_disabled;	/* FB base matches BAR of a disabled device */
-
 static struct resource *bar_resource;
 static u64 bar_offset;
 
@@ -377,7 +375,7 @@ static int efifb_probe(struct platform_device *dev)
 	if (!si)
 		return -ENOMEM;
 
-	if (si->orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
+	if (si->orig_video_isVGA != VIDEO_TYPE_EFI)
 		return -ENODEV;
 
 	if (fb_get_options("efifb", &option))
@@ -653,7 +651,6 @@ static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
 
 	pci_read_config_word(dev, PCI_COMMAND, &word);
 	if (!(word & PCI_COMMAND_MEMORY)) {
-		pci_dev_disabled = true;
 		dev_err(&dev->dev,
 			"BAR %d: assigned to efifb but device is disabled!\n",
 			idx);
-- 
2.43.0

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

[Thomas Zimmermann]

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(+)

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;
-- 
2.43.0

[PATCH v2 8/8] fbdev/efifb: Remove framebuffer relocation tracking

[Thomas Zimmermann]

If the firmware framebuffer has been reloacted, the sysfb code
fixes the screen_info state before it creates the framebuffer's
platform device. Efifb will automatically receive a screen_info
with updated values. Hence remove the tracking from efifb.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/efifb.c | 76 +------------------------------------
 1 file changed, 1 insertion(+), 75 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index f76b7ae007518..8dd82afb3452b 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -13,7 +13,6 @@
 #include <linux/efi-bgrt.h>
 #include <linux/errno.h>
 #include <linux/fb.h>
-#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/screen_info.h>
@@ -47,8 +46,6 @@ static bool use_bgrt = true;
 static bool request_mem_succeeded = false;
 static u64 mem_flags = EFI_MEMORY_WC | EFI_MEMORY_UC;
 
-static struct pci_dev *efifb_pci_dev;	/* dev with BAR covering the efifb */
-
 struct efifb_par {
 	u32 pseudo_palette[16];
 	resource_size_t base;
@@ -348,9 +345,6 @@ static struct attribute *efifb_attrs[] = {
 };
 ATTRIBUTE_GROUPS(efifb);
 
-static struct resource *bar_resource;
-static u64 bar_offset;
-
 static int efifb_probe(struct platform_device *dev)
 {
 	struct screen_info *si;
@@ -411,21 +405,7 @@ static int efifb_probe(struct platform_device *dev)
 		si->rsvd_pos = 24;
 	}
 
-	efifb_fix.smem_start = si->lfb_base;
-
-	if (si->capabilities & VIDEO_CAPABILITY_64BIT_BASE) {
-		u64 ext_lfb_base;
-
-		ext_lfb_base = (u64)(unsigned long)si->ext_lfb_base << 32;
-		efifb_fix.smem_start |= ext_lfb_base;
-	}
-
-	if (bar_resource &&
-	    bar_resource->start + bar_offset != efifb_fix.smem_start) {
-		dev_info(&efifb_pci_dev->dev,
-			 "BAR has moved, updating efifb address\n");
-		efifb_fix.smem_start = bar_resource->start + bar_offset;
-	}
+	efifb_fix.smem_start = __screen_info_lfb_base(si);
 
 	efifb_defined.bits_per_pixel = si->lfb_depth;
 	efifb_defined.xres = si->lfb_width;
@@ -640,57 +620,3 @@ static struct platform_driver efifb_driver = {
 };
 
 builtin_platform_driver(efifb_driver);
-
-#if defined(CONFIG_PCI)
-
-static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
-{
-	u16 word;
-
-	efifb_pci_dev = dev;
-
-	pci_read_config_word(dev, PCI_COMMAND, &word);
-	if (!(word & PCI_COMMAND_MEMORY)) {
-		dev_err(&dev->dev,
-			"BAR %d: assigned to efifb but device is disabled!\n",
-			idx);
-		return;
-	}
-
-	bar_resource = &dev->resource[idx];
-	bar_offset = offset;
-
-	dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
-}
-
-static void efifb_fixup_resources(struct pci_dev *dev)
-{
-	u64 base = screen_info.lfb_base;
-	u64 size = screen_info.lfb_size;
-	int i;
-
-	if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
-		return;
-
-	if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
-		base |= (u64)screen_info.ext_lfb_base << 32;
-
-	if (!base)
-		return;
-
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		struct resource *res = &dev->resource[i];
-
-		if (!(res->flags & IORESOURCE_MEM))
-			continue;
-
-		if (res->start <= base && res->end >= base + size - 1) {
-			record_efifb_bar_resource(dev, i, base - res->start);
-			break;
-		}
-	}
-}
-DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY,
-			       16, efifb_fixup_resources);
-
-#endif
-- 
2.43.0

Re: [v2,3/8] firmware/sysfb: Set firmware-framebuffer parent device

[Sui Jingfeng]

Hi, Thomas:

I love your patch, yet it can cause the linux kernel fail to compile
if the CONFIG_SYSFB_SIMPLEFB option is not selected. I paste the log
at below for easier to read and debug. Please fix this. :-)

drivers/firmware/sysfb.c: In function ‘sysfb_init’:
drivers/firmware/sysfb.c:134:22: error: too many arguments to function 
‘sysfb_create_simplefb’
   134 |                 pd = sysfb_create_simplefb(si, &mode, parent);
       |                      ^~~~~~~~~~~~~~~~~~~~~
In file included from drivers/firmware/sysfb.c:36:
./include/linux/sysfb.h:105:39: note: declared here
   105 | static inline struct platform_device 
*sysfb_create_simplefb(const struct screen_info *si,
       | ^~~~~~~~~~~~~~~~~~~~~
make[4]: *** [scripts/Makefile.build:243: drivers/firmware/sysfb.o] Error 1
make[3]: *** [scripts/Makefile.build:481: drivers/firmware] Error 2
make[3]: *** Waiting for unfinished jobs....

This is because you forget to modify prototype of sysfb_create_simplefb() function,
Please see it in include/linux/sysfb.h, at the 106 line of the file.


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> Set the firmware framebuffer's parent device, which usually is the
> graphics hardware's physical device. Integrates the framebuffer in
> the Linux device hierarchy and lets Linux handle dependencies among
> devices. For example, the graphics hardware won't be suspended while
> the firmware device is still active.
>
> v2:
> 	* detect parent device in sysfb_parent_dev()
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/firmware/sysfb.c          | 19 ++++++++++++++++++-
>   drivers/firmware/sysfb_simplefb.c |  5 ++++-
>   include/linux/sysfb.h             |  3 ++-
>   3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 3c197db42c9d9..d02945b0d8ea1 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -29,6 +29,7 @@
>   #include <linux/init.h>
>   #include <linux/kernel.h>
>   #include <linux/mm.h>
> +#include <linux/pci.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
>   #include <linux/screen_info.h>
> @@ -69,9 +70,21 @@ void sysfb_disable(void)
>   }
>   EXPORT_SYMBOL_GPL(sysfb_disable);
>   
> +static __init struct device *sysfb_parent_dev(const struct screen_info *si)
> +{
> +	struct pci_dev *pdev;
> +
> +	pdev = screen_info_pci_dev(si);
> +	if (pdev)
> +		return &pdev->dev;
> +
> +	return NULL;
> +}
> +
>   static __init int sysfb_init(void)
>   {
>   	struct screen_info *si = &screen_info;
> +	struct device *parent;
>   	struct simplefb_platform_data mode;
>   	const char *name;
>   	bool compatible;
> @@ -83,10 +96,12 @@ static __init int sysfb_init(void)
>   
>   	sysfb_apply_efi_quirks();
>   
> +	parent = sysfb_parent_dev(si);
> +
>   	/* try to create a simple-framebuffer device */
>   	compatible = sysfb_parse_mode(si, &mode);
>   	if (compatible) {
> -		pd = sysfb_create_simplefb(si, &mode);
> +		pd = sysfb_create_simplefb(si, &mode, parent);
>   		if (!IS_ERR(pd))
>   			goto unlock_mutex;
>   	}
> @@ -109,6 +124,8 @@ static __init int sysfb_init(void)
>   		goto unlock_mutex;
>   	}
>   
> +	pd->dev.parent = parent;
> +
>   	sysfb_set_efifb_fwnode(pd);
>   
>   	ret = platform_device_add_data(pd, si, sizeof(*si));
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 74363ed7501f6..75a186bf8f8ec 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -91,7 +91,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   }
>   
>   __init struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
> -						     const struct simplefb_platform_data *mode)
> +						     const struct simplefb_platform_data *mode,
> +						     struct device *parent)
>   {
>   	struct platform_device *pd;
>   	struct resource res;
> @@ -143,6 +144,8 @@ __init struct platform_device *sysfb_create_simplefb(const struct screen_info *s
>   	if (!pd)
>   		return ERR_PTR(-ENOMEM);
>   
> +	pd->dev.parent = parent;
> +
>   	sysfb_set_efifb_fwnode(pd);
>   
>   	ret = platform_device_add_resources(pd, &res, 1);
> diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
> index 19cb803dd5ecd..6ee3ade3f8b06 100644
> --- a/include/linux/sysfb.h
> +++ b/include/linux/sysfb.h
> @@ -91,7 +91,8 @@ static inline void sysfb_set_efifb_fwnode(struct platform_device *pd)
>   bool sysfb_parse_mode(const struct screen_info *si,
>   		      struct simplefb_platform_data *mode);
>   struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
> -					      const struct simplefb_platform_data *mode);
> +					      const struct simplefb_platform_data *mode,
> +					      struct device *parent);
>   
>   #else /* CONFIG_SYSFB_SIMPLE */
>   

Re: [v2,3/8] firmware/sysfb: Set firmware-framebuffer parent device

[Sui Jingfeng]

Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> Set the firmware framebuffer's parent device, which usually is the
> graphics hardware's physical device. Integrates the framebuffer in
> the Linux device hierarchy and lets Linux handle dependencies among
> devices. For example, the graphics hardware won't be suspended while
> the firmware device is still active.

This is a very nice benefit, I can't agree more!

Because the backing memory of the firmware framebuffer occupied
belongs to the graphics hardware itself. For PCIe device, the
backing memory is typically the dedicated VRAM of the PCIe GPU.
But there are some exceptions, for example, the gma500. But I
think this can be fixed in the future, as majority(>99.9%) PCIe
GPU has the a dedicated VRAM.


For ARM and ARM64 platform device, the backing memory of the
firmware framebuffer may located at the system RAM. It's common
that the display controller is a platform device in the embedded
world. So I think the sysfb_parent_dev() function can be extended
to be able to works for platform device in the future.

Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Sui Jingfeng]

Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> diff --git a/drivers/video/screen_info_pci.c b/drivers/video/screen_info_pci.c
> new file mode 100644
> index 0000000000000..d959a4c6ba3d5
> --- /dev/null
> +++ b/drivers/video/screen_info_pci.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/pci.h>
> +#include <linux/screen_info.h>
> +
> +static struct pci_dev *__screen_info_pci_dev(struct resource *res)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!(res->flags & IORESOURCE_MEM))
> +		return NULL;
> +
> +	for_each_pci_dev(pdev) {
> +		const struct resource *r;
> +
> +		if ((pdev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
> +			continue;
> +
> +		r = pci_find_resource(pdev, res);
> +		if (r)
> +			return pdev;
> +	}
> +
> +	return NULL;
> +}


I recommend using the pci_get_base_class() or pci_get_class() helper function at here,
for example:


static struct pci_dev *__screen_info_pci_dev(struct resource *res)
{
	struct pci_dev *pdev;

	if (!(res->flags & IORESOURCE_MEM))
		return NULL;

	while ((pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev))) {
		if (pci_find_resource(pdev, res))
			return pdev;
	}

	return NULL;
}

Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Sui Jingfeng]

Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> +
> +/**
> + * screen_info_pci_dev() - Return PCI parent device that contains screen_info's framebuffer
> + * @si: the screen_info
> + *
> + * Returns:
> + * The screen_info's parent device on success, or NULL otherwise.
> + */
> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
> +{
> +	struct resource res[SCREEN_INFO_MAX_RESOURCES];
> +	ssize_t i, numres;
> +
> +	numres = screen_info_resources(si, res, ARRAY_SIZE(res));
> +	if (numres < 0)
> +		return ERR_PTR(numres);


Please return NULL at here, otherwise we have to use the IS_ERR or IS_ERR_OR_NULL()
in the caller function to check the returned value. Meanwhile, I noticed that you
didn't actually call IS_ERR() in the sysfb_parent_dev() function (introduced by the
3/8 patch), so I think we probably should return NULL at here.

Please also consider that the comments of this function says that it return NULL on
the otherwise cases.


> +	for (i = 0; i < numres; ++i) {
> +		struct pci_dev *pdev = __screen_info_pci_dev(&res[i]);
> +
> +		if (pdev)
> +			return pdev;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(screen_info_pci_dev);

Re: [v2, 5/8] firmware/sysfb: Create firmware device only for enabled PCI devices

[Sui Jingfeng]

HI,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> Test if the firmware framebuffer's parent PCI device, if any, has
> been enabled. If not, the firmware framebuffer is most likely not
> working. Hence, do not create a device for the firmware framebuffer
> on disabled PCI devices.
>
> So far, efifb tracked the status of the PCI parent device internally
> and did not bind if it was disabled. This patch implements the
> functionality for all firmware framebuffers.


For *all* ?

I think the functionality this patch implemented is only target for the
PCIe device firmware framebuffers, the framebuffer consumed by the simplefb
driver (fbdev/simplefb.c) is also a kind of firmware framebuffer, but it is
target for the platform device only.

So, the correct description would be: "this patch implements the functionality
for the PCIe firmware framebuffers".

> v2:
> 	* rework sysfb_pci_dev_is_enabled() (Javier)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/firmware/sysfb.c | 30 +++++++++++++++++++++++++++++-
>   1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index d02945b0d8ea1..ab5cbc0326f6d 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -70,13 +70,39 @@ void sysfb_disable(void)
>   }
>   EXPORT_SYMBOL_GPL(sysfb_disable);
>   
> +#if defined(CONFIG_PCI)
> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
> +{
> +	/*
> +	 * TODO: Try to integrate this code into the PCI subsystem
> +	 */
> +	int ret;
> +	u16 command;
> +
> +	ret = pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	if (ret != PCIBIOS_SUCCESSFUL)
> +		return false;
> +	if (!(command & PCI_COMMAND_MEMORY))
> +		return false;
> +	return true;
> +}
> +#else
> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
> +{
> +	return false;
> +}
> +#endif
> +
>   static __init struct device *sysfb_parent_dev(const struct screen_info *si)
>   {
>   	struct pci_dev *pdev;
>   
>   	pdev = screen_info_pci_dev(si);
> -	if (pdev)
> +	if (pdev) {
> +		if (!sysfb_pci_dev_is_enabled(pdev))
> +			return ERR_PTR(-ENODEV);


Is it better to move the call of sysfb_pci_dev_is_enabled() out of sysfb_parent_dev() ?
Because then we don't need check the returned value by calling the IS_ERR() inthe sysfb_init() function.


>   		return &pdev->dev;
> +	}
>   
>   	return NULL;
>   }
> @@ -97,6 +123,8 @@ static __init int sysfb_init(void)
>   	sysfb_apply_efi_quirks();
>   
>   	parent = sysfb_parent_dev(si);
> +	if (IS_ERR(parent))
> +		goto unlock_mutex;

	if (!sysfb_pci_dev_is_enabled(parent))
		goto unlock_mutex;

>   
>   	/* try to create a simple-framebuffer device */
>   	compatible = sysfb_parse_mode(si, &mode);

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

[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?

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

[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.

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

Re: [v2,8/8] fbdev/efifb: Remove framebuffer relocation tracking

[Sui Jingfeng]

Hi,

Nice job, well done!

Because this the fix the problem at the root level, I appreciate this 
solution.


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> If the firmware framebuffer has been reloacted, the sysfb code
> fixes the screen_info state before it creates the framebuffer's
> platform device. Efifb will automatically receive a screen_info
> with updated values. Hence remove the tracking from efifb.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Re: [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v6.8-rc2 next-20240202]
[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/Thomas-Zimmermann/video-Add-helpers-for-decoding-screen_info/20240202-200314
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240202120140.3517-4-tzimmermann%40suse.de
patch subject: [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device
config: i386-buildonly-randconfig-002-20240203 (https://download.01.org/0day-ci/archive/20240204/202402040214.GFutmkRC-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240204/202402040214.GFutmkRC-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/202402040214.GFutmkRC-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/sysfb.c: In function 'sysfb_init':
>> drivers/firmware/sysfb.c:104:8: error: too many arguments to function 'sysfb_create_simplefb'
      pd = sysfb_create_simplefb(si, &mode, parent);
           ^~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/firmware/sysfb.c:36:0:
   include/linux/sysfb.h:105:39: note: declared here
    static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
                                          ^~~~~~~~~~~~~~~~~~~~~


vim +/sysfb_create_simplefb +104 drivers/firmware/sysfb.c

    83	
    84	static __init int sysfb_init(void)
    85	{
    86		struct screen_info *si = &screen_info;
    87		struct device *parent;
    88		struct simplefb_platform_data mode;
    89		const char *name;
    90		bool compatible;
    91		int ret = 0;
    92	
    93		mutex_lock(&disable_lock);
    94		if (disabled)
    95			goto unlock_mutex;
    96	
    97		sysfb_apply_efi_quirks();
    98	
    99		parent = sysfb_parent_dev(si);
   100	
   101		/* try to create a simple-framebuffer device */
   102		compatible = sysfb_parse_mode(si, &mode);
   103		if (compatible) {
 > 104			pd = sysfb_create_simplefb(si, &mode, parent);
   105			if (!IS_ERR(pd))
   106				goto unlock_mutex;
   107		}
   108	
   109		/* if the FB is incompatible, create a legacy framebuffer device */
   110		if (si->orig_video_isVGA == VIDEO_TYPE_EFI)
   111			name = "efi-framebuffer";
   112		else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
   113			name = "vesa-framebuffer";
   114		else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
   115			name = "vga-framebuffer";
   116		else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
   117			name = "ega-framebuffer";
   118		else
   119			name = "platform-framebuffer";
   120	
   121		pd = platform_device_alloc(name, 0);
   122		if (!pd) {
   123			ret = -ENOMEM;
   124			goto unlock_mutex;
   125		}
   126	
   127		pd->dev.parent = parent;
   128	
   129		sysfb_set_efifb_fwnode(pd);
   130	
   131		ret = platform_device_add_data(pd, si, sizeof(*si));
   132		if (ret)
   133			goto err;
   134	
   135		ret = platform_device_add(pd);
   136		if (ret)
   137			goto err;
   138	
   139		goto unlock_mutex;
   140	err:
   141		platform_device_put(pd);
   142	unlock_mutex:
   143		mutex_unlock(&disable_lock);
   144		return ret;
   145	}
   146	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-tip/drm-tip linus/master v6.8-rc2 next-20240202]
[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/Thomas-Zimmermann/video-Add-helpers-for-decoding-screen_info/20240202-200314
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240202120140.3517-3-tzimmermann%40suse.de
patch subject: [PATCH v2 2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device
config: i386-buildonly-randconfig-004-20240203 (https://download.01.org/0day-ci/archive/20240204/202402040957.xNqUV75N-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240204/202402040957.xNqUV75N-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/202402040957.xNqUV75N-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/video/screen_info_pci.c:10:6: warning: variable 'pdev' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
      10 |         if (!(res->flags & IORESOURCE_MEM))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/video/screen_info_pci.c:13:19: note: uninitialized use occurs here
      13 |         for_each_pci_dev(pdev) {
         |                          ^~~~
   include/linux/pci.h:546:80: note: expanded from macro 'for_each_pci_dev'
     546 | #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
         |                                                                                ^
   drivers/video/screen_info_pci.c:10:2: note: remove the 'if' if its condition is always true
      10 |         if (!(res->flags & IORESOURCE_MEM))
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      11 |                 return NULL;
   drivers/video/screen_info_pci.c:8:22: note: initialize the variable 'pdev' to silence this warning
       8 |         struct pci_dev *pdev;
         |                             ^
         |                              = NULL
   1 warning generated.


vim +10 drivers/video/screen_info_pci.c

     5	
     6	static struct pci_dev *__screen_info_pci_dev(struct resource *res)
     7	{
     8		struct pci_dev *pdev;
     9	
  > 10		if (!(res->flags & IORESOURCE_MEM))
    11			return NULL;
    12	
    13		for_each_pci_dev(pdev) {
    14			const struct resource *r;
    15	
    16			if ((pdev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
    17				continue;
    18	
    19			r = pci_find_resource(pdev, res);
    20			if (r)
    21				return pdev;
    22		}
    23	
    24		return NULL;
    25	}
    26	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v6.8-rc2 next-20240202]
[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/Thomas-Zimmermann/video-Add-helpers-for-decoding-screen_info/20240202-200314
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240202120140.3517-4-tzimmermann%40suse.de
patch subject: [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device
config: i386-buildonly-randconfig-003-20240203 (https://download.01.org/0day-ci/archive/20240204/202402041001.rJrT47HE-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240204/202402041001.rJrT47HE-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/202402041001.rJrT47HE-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/sysfb.c:104:41: error: too many arguments to function call, expected 2, have 3
     104 |                 pd = sysfb_create_simplefb(si, &mode, parent);
         |                      ~~~~~~~~~~~~~~~~~~~~~            ^~~~~~
   include/linux/sysfb.h:105:39: note: 'sysfb_create_simplefb' declared here
     105 | static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
         |                                       ^                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     106 |                                                             const struct simplefb_platform_data *mode)
         |                                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +104 drivers/firmware/sysfb.c

    83	
    84	static __init int sysfb_init(void)
    85	{
    86		struct screen_info *si = &screen_info;
    87		struct device *parent;
    88		struct simplefb_platform_data mode;
    89		const char *name;
    90		bool compatible;
    91		int ret = 0;
    92	
    93		mutex_lock(&disable_lock);
    94		if (disabled)
    95			goto unlock_mutex;
    96	
    97		sysfb_apply_efi_quirks();
    98	
    99		parent = sysfb_parent_dev(si);
   100	
   101		/* try to create a simple-framebuffer device */
   102		compatible = sysfb_parse_mode(si, &mode);
   103		if (compatible) {
 > 104			pd = sysfb_create_simplefb(si, &mode, parent);
   105			if (!IS_ERR(pd))
   106				goto unlock_mutex;
   107		}
   108	
   109		/* if the FB is incompatible, create a legacy framebuffer device */
   110		if (si->orig_video_isVGA == VIDEO_TYPE_EFI)
   111			name = "efi-framebuffer";
   112		else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
   113			name = "vesa-framebuffer";
   114		else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
   115			name = "vga-framebuffer";
   116		else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
   117			name = "ega-framebuffer";
   118		else
   119			name = "platform-framebuffer";
   120	
   121		pd = platform_device_alloc(name, 0);
   122		if (!pd) {
   123			ret = -ENOMEM;
   124			goto unlock_mutex;
   125		}
   126	
   127		pd->dev.parent = parent;
   128	
   129		sysfb_set_efifb_fwnode(pd);
   130	
   131		ret = platform_device_add_data(pd, si, sizeof(*si));
   132		if (ret)
   133			goto err;
   134	
   135		ret = platform_device_add(pd);
   136		if (ret)
   137			goto err;
   138	
   139		goto unlock_mutex;
   140	err:
   141		platform_device_put(pd);
   142	unlock_mutex:
   143		mutex_unlock(&disable_lock);
   144		return ret;
   145	}
   146	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1894 bytes --]

Hi

Am 02.02.24 um 17:31 schrieb Sui Jingfeng:
> Hi,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> diff --git a/drivers/video/screen_info_pci.c 
>> b/drivers/video/screen_info_pci.c
>> new file mode 100644
>> index 0000000000000..d959a4c6ba3d5
>> --- /dev/null
>> +++ b/drivers/video/screen_info_pci.c
>> @@ -0,0 +1,52 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/pci.h>
>> +#include <linux/screen_info.h>
>> +
>> +static struct pci_dev *__screen_info_pci_dev(struct resource *res)
>> +{
>> +    struct pci_dev *pdev;
>> +
>> +    if (!(res->flags & IORESOURCE_MEM))
>> +        return NULL;
>> +
>> +    for_each_pci_dev(pdev) {
>> +        const struct resource *r;
>> +
>> +        if ((pdev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
>> +            continue;
>> +
>> +        r = pci_find_resource(pdev, res);
>> +        if (r)
>> +            return pdev;
>> +    }
>> +
>> +    return NULL;
>> +}
> 
> 
> I recommend using the pci_get_base_class() or pci_get_class() helper 
> function at here,
> for example:

Good idea, I think I'll do that. Thanks!

Best regards
Thomas

> 
> 
> static struct pci_dev *__screen_info_pci_dev(struct resource *res)
> {
>      struct pci_dev *pdev;
> 
>      if (!(res->flags & IORESOURCE_MEM))
>          return NULL;
> 
>      while ((pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev))) {
>          if (pci_find_resource(pdev, res))
>              return pdev;
>      }
> 
>      return NULL;
> }
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1939 bytes --]

Hi

Am 02.02.24 um 18:03 schrieb Sui Jingfeng:
> Hi,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> +
>> +/**
>> + * screen_info_pci_dev() - Return PCI parent device that contains 
>> screen_info's framebuffer
>> + * @si: the screen_info
>> + *
>> + * Returns:
>> + * The screen_info's parent device on success, or NULL otherwise.
>> + */
>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>> +{
>> +    struct resource res[SCREEN_INFO_MAX_RESOURCES];
>> +    ssize_t i, numres;
>> +
>> +    numres = screen_info_resources(si, res, ARRAY_SIZE(res));
>> +    if (numres < 0)
>> +        return ERR_PTR(numres);
> 
> 
> Please return NULL at here, otherwise we have to use the IS_ERR or 
> IS_ERR_OR_NULL()
> in the caller function to check the returned value. Meanwhile, I noticed 
> that you
> didn't actually call IS_ERR() in the sysfb_parent_dev() function 
> (introduced by the
> 3/8 patch), so I think we probably should return NULL at here.
> 
> Please also consider that the comments of this function says that it 
> return NULL on
> the otherwise cases.

Right. The idea is to return NULL is there is no parent device. The 
errno pointer is for actual runtime problems. The doc comment is still 
incorrect and I think I need to review the callers of this function. 
Thanks for pointing this out.

Best regards
Thomas

> 
> 
>> +    for (i = 0; i < numres; ++i) {
>> +        struct pci_dev *pdev = __screen_info_pci_dev(&res[i]);
>> +
>> +        if (pdev)
>> +            return pdev;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +EXPORT_SYMBOL(screen_info_pci_dev);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [v2,3/8] firmware/sysfb: Set firmware-framebuffer parent device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1865 bytes --]

Hi

Am 02.02.24 um 16:23 schrieb Sui Jingfeng:
> Hi,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> Set the firmware framebuffer's parent device, which usually is the
>> graphics hardware's physical device. Integrates the framebuffer in
>> the Linux device hierarchy and lets Linux handle dependencies among
>> devices. For example, the graphics hardware won't be suspended while
>> the firmware device is still active.
> 
> This is a very nice benefit, I can't agree more!
> 
> Because the backing memory of the firmware framebuffer occupied
> belongs to the graphics hardware itself. For PCIe device, the
> backing memory is typically the dedicated VRAM of the PCIe GPU.
> But there are some exceptions, for example, the gma500. But I
> think this can be fixed in the future, as majority(>99.9%) PCIe
> GPU has the a dedicated VRAM.
> 
> 
> For ARM and ARM64 platform device, the backing memory of the
> firmware framebuffer may located at the system RAM. It's common
> that the display controller is a platform device in the embedded
> world. So I think the sysfb_parent_dev() function can be extended
> to be able to works for platform device in the future.

The current approach has been taken from efifb. It would already not 
work reliably with gma500 or ARM SoCs. So there's no immediate loss of 
functionality AFAICT. But with the patchset now have a correct device 
hierarchy and PM for simpledrm, vesafb et al.

In the long term, I want to employ some of the logic in vgaarb that 
detects the firmware default device. That needs additional work, though.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [v2, 5/8] firmware/sysfb: Create firmware device only for enabled PCI devices

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3582 bytes --]



Am 02.02.24 um 18:50 schrieb Sui Jingfeng:
> HI,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> Test if the firmware framebuffer's parent PCI device, if any, has
>> been enabled. If not, the firmware framebuffer is most likely not
>> working. Hence, do not create a device for the firmware framebuffer
>> on disabled PCI devices.
>>
>> So far, efifb tracked the status of the PCI parent device internally
>> and did not bind if it was disabled. This patch implements the
>> functionality for all firmware framebuffers.
> 
> 
> For *all* ?
> 
> I think the functionality this patch implemented is only target for the
> PCIe device firmware framebuffers, the framebuffer consumed by the simplefb
> driver (fbdev/simplefb.c) is also a kind of firmware framebuffer, but it is
> target for the platform device only.
> 
> So, the correct description would be: "this patch implements the 
> functionality
> for the PCIe firmware framebuffers".

Fair enough.

> 
>> v2:
>>     * rework sysfb_pci_dev_is_enabled() (Javier)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>   drivers/firmware/sysfb.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
>> index d02945b0d8ea1..ab5cbc0326f6d 100644
>> --- a/drivers/firmware/sysfb.c
>> +++ b/drivers/firmware/sysfb.c
>> @@ -70,13 +70,39 @@ void sysfb_disable(void)
>>   }
>>   EXPORT_SYMBOL_GPL(sysfb_disable);
>> +#if defined(CONFIG_PCI)
>> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
>> +{
>> +    /*
>> +     * TODO: Try to integrate this code into the PCI subsystem
>> +     */
>> +    int ret;
>> +    u16 command;
>> +
>> +    ret = pci_read_config_word(pdev, PCI_COMMAND, &command);
>> +    if (ret != PCIBIOS_SUCCESSFUL)
>> +        return false;
>> +    if (!(command & PCI_COMMAND_MEMORY))
>> +        return false;
>> +    return true;
>> +}
>> +#else
>> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
>> +{
>> +    return false;
>> +}
>> +#endif
>> +
>>   static __init struct device *sysfb_parent_dev(const struct 
>> screen_info *si)
>>   {
>>       struct pci_dev *pdev;
>>       pdev = screen_info_pci_dev(si);
>> -    if (pdev)
>> +    if (pdev) {
>> +        if (!sysfb_pci_dev_is_enabled(pdev))
>> +            return ERR_PTR(-ENODEV);
> 
> 
> Is it better to move the call of sysfb_pci_dev_is_enabled() out of 
> sysfb_parent_dev() ?
> Because then we don't need check the returned value by calling the 
> IS_ERR() inthe sysfb_init() function.
> 
> 
>>           return &pdev->dev;
>> +    }
>>       return NULL;
>>   }
>> @@ -97,6 +123,8 @@ static __init int sysfb_init(void)
>>       sysfb_apply_efi_quirks();
>>       parent = sysfb_parent_dev(si);
>> +    if (IS_ERR(parent))
>> +        goto unlock_mutex;
> 
>      if (!sysfb_pci_dev_is_enabled(parent))
>          goto unlock_mutex;
> 
>>       /* try to create a simple-framebuffer device */
>>       compatible = sysfb_parse_mode(si, &mode);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Sui Jingfeng]

Hi,

On 2024/2/5 16:17, Thomas Zimmermann wrote:
> Hi
>
> Am 02.02.24 um 18:03 schrieb Sui Jingfeng:
>> Hi,
>>
>>
>> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>>> +
>>> +/**
>>> + * screen_info_pci_dev() - Return PCI parent device that contains 
>>> screen_info's framebuffer
>>> + * @si: the screen_info
>>> + *
>>> + * Returns:
>>> + * The screen_info's parent device on success, or NULL otherwise.
>>> + */
>>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>>> +{
>>> +    struct resource res[SCREEN_INFO_MAX_RESOURCES];
>>> +    ssize_t i, numres;
>>> +
>>> +    numres = screen_info_resources(si, res, ARRAY_SIZE(res));
>>> +    if (numres < 0)
>>> +        return ERR_PTR(numres);
>>
>>
>> Please return NULL at here, otherwise we have to use the IS_ERR or 
>> IS_ERR_OR_NULL()
>> in the caller function to check the returned value. Meanwhile, I 
>> noticed that you
>> didn't actually call IS_ERR() in the sysfb_parent_dev() function 
>> (introduced by the
>> 3/8 patch), so I think we probably should return NULL at here.
>>
>> Please also consider that the comments of this function says that it 
>> return NULL on
>> the otherwise cases.
>
> Right. The idea is to return NULL is there is no parent device. 


return NULL is more easier and clear, it stand for "None" or "don't exist".
There is another reason that I want to tell you:

Some systems which don't have a good UEFI firmware support for uncommon GPUs.
the word "uncommon" means "not very popular GPU" or "extremely new GPU" or
"just refer to the GPUs that UEFI firmware don't know(recognize) about"

On such cases, there is no firmware framebuffer support. I means it is possible
that screen_info_resources() return -EINVAL because of not support yet. Then,
the screen_info_pci_dev(si) returns ERR_PTR(-EINVAL) and sysfb_pci_dev_is_enabled()
will take this error code as a pointer and de-reference it, cause the following
problem:

And even the x86-64 motherboard will not likely support all GPU(for example the one
with a old UEFI BIOS). And for an example, The intel Xe is the "extremely new GPU".


[    5.031966] CPU 4 Unable to handle kernel paging request at virtual 
address 000000000000081a, era == 900000000329b448, ra == 900000000329b440
[    5.044587] Oops[#1]:
[    5.046837] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc1+ #7
[    5.053062] Hardware name: Loongson 
Loongson-3A6000-HV-7A2000-XA61200/Loongson-3A6000-HV-7A2000-XA61200, 
BIOS Loongson-UDK2018-V4.0.05636-stable202311 12/
[    5.066803] pc 900000000329b448 ra 900000000329b440 tp 
90000001000d0000 sp 90000001000d3d40
[    5.075100] a0 ffffffffffffffea a1 90000001000d3c38 a2 
0000000000000003 a3 9000000003867ce8
[    5.083398] a4 9000000003867ce0 a5 90000001000d3a80 a6 
0000000000000001 a7 0000000000000001
[    5.091695] t0 ac81f55e34713962 t1 ac81f55e34713962 t2 
0000000000000000 t3 0000000000000001
[    5.099992] t4 0000000000000004 t5 0000000000000000 t6 
0000000000000030 t7 0000000000000000
[    5.108290] t8 00000000000070b1 u0 0000000000000000 s9 
0000000000000008 s0 9000000003d58b48
[    5.116587] s1 9000000003c0b4a8 s2 9000000003787000 s3 
9000000003778000 s4 90000000032c0578
[    5.124884] s5 ffffffffffffffea s6 90000000032c0560 s7 
90000000032df900 s8 ffffffffccccc000
[    5.133182]    ra: 900000000329b440 sysfb_init+0x80/0x1f0
[    5.138545]   ERA: 900000000329b448 sysfb_init+0x88/0x1f0
[    5.143905]  CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
[    5.150048]  PRMD: 00000004 (PPLV0 +PIE -PWE)
[    5.154373]  EUEN: 00000000 (-FPE -SXE -ASXE -BTE)
[    5.159131]  ECFG: 00071c1c (LIE=2-4,10-12 VS=7)
[    5.163717] ESTAT: 00010000 [PIL] (IS= ECode=1 EsubCode=0)
[    5.169164]  BADV: 000000000000081a
[    5.172623]  PRID: 0014d000 (Loongson-64bit, Loongson-3A6000-HV)
[    5.178587] Modules linked in:
[    5.181614] Process swapper/0 (pid: 1, threadinfo=(____ptrval____), 
task=(____ptrval____))
[    5.189827] Stack : 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
[    5.197782]         0000000000000000 ac81f55e34713962 
90000000032c0000 9000000003778000
[    5.205736]         90000000032c0578 0000000000000000 
900000000329b3c0 9000000003c54000
[    5.213691]         90000001000d3db8 9000000002260154 
0000000000000006 0000000000000000
[    5.221645]         0000000000000000 0000000000000000 
0000000000000000 0000000000000000
[    5.229599]         0000000000000000 0000000000000000 
0000000000000000 ac81f55e34713962
[    5.237553]         90000000037468f8 90000000037468f8 
90000000032c0578 90000000036a7658
[    5.245508]         0000000000000006 9000000100041e00 
0000000000000a55 9000000003260ff4
[    5.251549] ata3: SATA link down (SStatus 0 SControl 300)
[    5.253463]         0000000000000000 90000000032600b0 
0000000000000000 0000000000000000
[    5.266777]         0000000000000000 0000000000000000 
0000000000000000 0000000000000000
[    5.274731]         ...
[    5.277154] Call Trace:
[    5.277155] [<900000000329b448>] sysfb_init+0x88/0x1f0
[    5.284678] [<9000000002260154>] do_one_initcall+0x78/0x1cc
[    5.290213] [<9000000003260ff4>] kernel_init_freeable+0x228/0x298
[    5.296267] [<900000000324d104>] kernel_init+0x20/0x110
[    5.301455] [<90000000022611e8>] ret_from_kernel_thread+0xc/0xa4
[    5.307421]
[    5.308892] Code: 561667fe  0015009c  40007080 <2408308c> 29403860  
02c2e09b  0040818c  6400180c  1a007d45
[    5.318579]
[    5.320053] ---[ end trace 0000000000000000 ]---
[    5.324640] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b
[    5.332247] Kernel relocated by 0x2040000
[    5.336226]  .text @ 0x9000000002240000
[    5.340031]  .data @ 0x90000000032f0000
[    5.343835]  .bss  @ 0x9000000003c3f200
[    5.347640] ---[ end Kernel panic - not syncing: Attempted to kill 
init! exitcode=0x0000000b ]---


> Best regards
> Thomas
>

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1022 bytes --]

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
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7303 bytes --]

Hi

Am 05.02.24 um 11:05 schrieb Sui Jingfeng:
> Hi,
> 
> On 2024/2/5 16:17, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 02.02.24 um 18:03 schrieb Sui Jingfeng:
>>> Hi,
>>>
>>>
>>> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>>>> +
>>>> +/**
>>>> + * screen_info_pci_dev() - Return PCI parent device that contains 
>>>> screen_info's framebuffer
>>>> + * @si: the screen_info
>>>> + *
>>>> + * Returns:
>>>> + * The screen_info's parent device on success, or NULL otherwise.
>>>> + */
>>>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>>>> +{
>>>> +    struct resource res[SCREEN_INFO_MAX_RESOURCES];
>>>> +    ssize_t i, numres;
>>>> +
>>>> +    numres = screen_info_resources(si, res, ARRAY_SIZE(res));
>>>> +    if (numres < 0)
>>>> +        return ERR_PTR(numres);
>>>
>>>
>>> Please return NULL at here, otherwise we have to use the IS_ERR or 
>>> IS_ERR_OR_NULL()
>>> in the caller function to check the returned value. Meanwhile, I 
>>> noticed that you
>>> didn't actually call IS_ERR() in the sysfb_parent_dev() function 
>>> (introduced by the
>>> 3/8 patch), so I think we probably should return NULL at here.
>>>
>>> Please also consider that the comments of this function says that it 
>>> return NULL on
>>> the otherwise cases.
>>
>> Right. The idea is to return NULL is there is no parent device. 
> 
> 
> return NULL is more easier and clear, it stand for "None" or "don't exist".
> There is another reason that I want to tell you:
> 
> Some systems which don't have a good UEFI firmware support for uncommon 
> GPUs.
> the word "uncommon" means "not very popular GPU" or "extremely new GPU" or
> "just refer to the GPUs that UEFI firmware don't know(recognize) about"
> 
> On such cases, there is no firmware framebuffer support. I means it is 
> possible
> that screen_info_resources() return -EINVAL because of not support yet. 
> Then,
> the screen_info_pci_dev(si) returns ERR_PTR(-EINVAL) and 
> sysfb_pci_dev_is_enabled()
> will take this error code as a pointer and de-reference it, cause the 
> following
> problem:

Right, that's part of the bug you already pointed out. As I said, I need 
to review the callers of screen_info_pci_dev() and fix them.

But returning an errno pointer is useful in cases where a possible 
parent device would have been detected, but something did not work out. 
For example, screen_info_resources() does not support all VIDEO_TYPE_ 
constants. In such a case, it's better to tell the caller about the 
problem than to silently return NULL.

Best regards
Thomas

> 
> And even the x86-64 motherboard will not likely support all GPU(for 
> example the one
> with a old UEFI BIOS). And for an example, The intel Xe is the 
> "extremely new GPU".
> 
> 
> [    5.031966] CPU 4 Unable to handle kernel paging request at virtual 
> address 000000000000081a, era == 900000000329b448, ra == 900000000329b440
> [    5.044587] Oops[#1]:
> [    5.046837] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc1+ #7
> [    5.053062] Hardware name: Loongson 
> Loongson-3A6000-HV-7A2000-XA61200/Loongson-3A6000-HV-7A2000-XA61200, 
> BIOS Loongson-UDK2018-V4.0.05636-stable202311 12/
> [    5.066803] pc 900000000329b448 ra 900000000329b440 tp 
> 90000001000d0000 sp 90000001000d3d40
> [    5.075100] a0 ffffffffffffffea a1 90000001000d3c38 a2 
> 0000000000000003 a3 9000000003867ce8
> [    5.083398] a4 9000000003867ce0 a5 90000001000d3a80 a6 
> 0000000000000001 a7 0000000000000001
> [    5.091695] t0 ac81f55e34713962 t1 ac81f55e34713962 t2 
> 0000000000000000 t3 0000000000000001
> [    5.099992] t4 0000000000000004 t5 0000000000000000 t6 
> 0000000000000030 t7 0000000000000000
> [    5.108290] t8 00000000000070b1 u0 0000000000000000 s9 
> 0000000000000008 s0 9000000003d58b48
> [    5.116587] s1 9000000003c0b4a8 s2 9000000003787000 s3 
> 9000000003778000 s4 90000000032c0578
> [    5.124884] s5 ffffffffffffffea s6 90000000032c0560 s7 
> 90000000032df900 s8 ffffffffccccc000
> [    5.133182]    ra: 900000000329b440 sysfb_init+0x80/0x1f0
> [    5.138545]   ERA: 900000000329b448 sysfb_init+0x88/0x1f0
> [    5.143905]  CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
> [    5.150048]  PRMD: 00000004 (PPLV0 +PIE -PWE)
> [    5.154373]  EUEN: 00000000 (-FPE -SXE -ASXE -BTE)
> [    5.159131]  ECFG: 00071c1c (LIE=2-4,10-12 VS=7)
> [    5.163717] ESTAT: 00010000 [PIL] (IS= ECode=1 EsubCode=0)
> [    5.169164]  BADV: 000000000000081a
> [    5.172623]  PRID: 0014d000 (Loongson-64bit, Loongson-3A6000-HV)
> [    5.178587] Modules linked in:
> [    5.181614] Process swapper/0 (pid: 1, threadinfo=(____ptrval____), 
> task=(____ptrval____))
> [    5.189827] Stack : 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
> [    5.197782]         0000000000000000 ac81f55e34713962 
> 90000000032c0000 9000000003778000
> [    5.205736]         90000000032c0578 0000000000000000 
> 900000000329b3c0 9000000003c54000
> [    5.213691]         90000001000d3db8 9000000002260154 
> 0000000000000006 0000000000000000
> [    5.221645]         0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
> [    5.229599]         0000000000000000 0000000000000000 
> 0000000000000000 ac81f55e34713962
> [    5.237553]         90000000037468f8 90000000037468f8 
> 90000000032c0578 90000000036a7658
> [    5.245508]         0000000000000006 9000000100041e00 
> 0000000000000a55 9000000003260ff4
> [    5.251549] ata3: SATA link down (SStatus 0 SControl 300)
> [    5.253463]         0000000000000000 90000000032600b0 
> 0000000000000000 0000000000000000
> [    5.266777]         0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
> [    5.274731]         ...
> [    5.277154] Call Trace:
> [    5.277155] [<900000000329b448>] sysfb_init+0x88/0x1f0
> [    5.284678] [<9000000002260154>] do_one_initcall+0x78/0x1cc
> [    5.290213] [<9000000003260ff4>] kernel_init_freeable+0x228/0x298
> [    5.296267] [<900000000324d104>] kernel_init+0x20/0x110
> [    5.301455] [<90000000022611e8>] ret_from_kernel_thread+0xc/0xa4
> [    5.307421]
> [    5.308892] Code: 561667fe  0015009c  40007080 <2408308c> 29403860 
> 02c2e09b  0040818c  6400180c  1a007d45
> [    5.318579]
> [    5.320053] ---[ end trace 0000000000000000 ]---
> [    5.324640] Kernel panic - not syncing: Attempted to kill init! 
> exitcode=0x0000000b
> [    5.332247] Kernel relocated by 0x2040000
> [    5.336226]  .text @ 0x9000000002240000
> [    5.340031]  .data @ 0x90000000032f0000
> [    5.343835]  .bss  @ 0x9000000003c3f200
> [    5.347640] ---[ end Kernel panic - not syncing: Attempted to kill 
> init! exitcode=0x0000000b ]---
> 
> 
>> Best regards
>> Thomas
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Thomas Zimmermann]

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;
>> +}
>> +

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [v2,3/8] firmware/sysfb: Set firmware-framebuffer parent device

[Sui Jingfeng]

Hi,


On 2024/2/5 16:24, Thomas Zimmermann wrote:
> Hi
>
> Am 02.02.24 um 16:23 schrieb Sui Jingfeng:
>> Hi,
>>
>>
>> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>>> Set the firmware framebuffer's parent device, which usually is the
>>> graphics hardware's physical device. Integrates the framebuffer in
>>> the Linux device hierarchy and lets Linux handle dependencies among
>>> devices. For example, the graphics hardware won't be suspended while
>>> the firmware device is still active.
>>
>> This is a very nice benefit, I can't agree more!
>>
>> Because the backing memory of the firmware framebuffer occupied
>> belongs to the graphics hardware itself. For PCIe device, the
>> backing memory is typically the dedicated VRAM of the PCIe GPU.
>> But there are some exceptions, for example, the gma500. But I
>> think this can be fixed in the future, as majority(>99.9%) PCIe
>> GPU has the a dedicated VRAM.
>>
>>
>> For ARM and ARM64 platform device, the backing memory of the
>> firmware framebuffer may located at the system RAM. It's common
>> that the display controller is a platform device in the embedded
>> world. So I think the sysfb_parent_dev() function can be extended
>> to be able to works for platform device in the future.
>
> The current approach has been taken from efifb. It would already not 
> work reliably with gma500 or ARM SoCs. So there's no immediate loss of 
> functionality AFAICT. But with the patchset now have a correct device 
> hierarchy and PM for simpledrm, vesafb et al.
>
> In the long term, I want to employ some of the logic in vgaarb that 
> detects the firmware default device. That needs additional work, though.
>

Good ideas, try to be impressive.
I probably could help to test if I'm online.


> Best regards
> Thomas
>
>>
>