[PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[Thomas Zimmermann]

Coreboot implements framebuffer support via simple-framebuffer. Provide a
dedicated DRM driver. Keep the simple-framebuffer code for now.

For each firmware's provided framebuffer, we prefer a dedicated DRM driver
tailored towards the platform's feature set. The coreboot framebuffer
device currently creates a simple-framebuffer device for the provided
framebuffer aperture. But simple-framebuffer is for DeviceTree nodes; not
for coreboot. The simple-framebuffer infrastructure should be phased out
for non-DT use cases. Coreboot is one of the final users of the code
(besides n64).

Patches 1 to 5 start by fixing problems in the coreboot framebuffer
implementation. There is a possible dangling pointer, the memory is
marked as busy, the device hierarchy is incorrect, and a few minor things.

Patches 6 to 9 prepare the coreboot support for use by external drivers.
Specifically, structures for the entries os the coreboot payload table
have to be exported.

Patches 10 to 12 add corebootdrm, a DRM driver for the new
coreboot-framebuffer platform device. Corebootdrm follows the pattern
established by similar drivers. It also uses the same sysfb helpers. It
is therefore fairly small. With patch 11, it has feature parity with
simpledrm on the old simple-framebuffer. Patch 12 adds support for panel-
orientation flags that coreboot makes available.

Tested on an HP Chromebook with MrChromebox 4.16. Runs with Weston and
fbcon. Xorg requires an additional patch available at [1].

v2:
- keep design of coreboot framebuffer code intact (Julius)
- fix bugs in the coreboot framebuffer code
- rewrite corebootdrm as platform device
- support panel orientation

[1] https://gitlab.freedesktop.org/tzimmermann/xserver/-/commit/0b326aad28549762ed2b0e2bedf8f8a42f1f6b3b

Thomas Zimmermann (12):
  firmware: google: framebuffer: Do not unregister platform device
  firmware: google: framebuffer: Do not mark framebuffer as busy
  firmware: google: framebuffer: Init memory resource with helper macro
  firmware: google: framebuffer: Tie platform device to PCI hardware
  firmware: google: framebuffer: Fix dependencies
  firmware: google: Init coreboot bus with subsys_initcall()
  firmware: google: Clean up include statements in coreboot_table.h
  firmware: google: Export coreboot table entries
  firmware: google: Pack structures for coreboot table entries
  drm/sysfb: Generalize pixel-format matching
  drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers
  drm/corebotdrm: Support panel orientation

 MAINTAINERS                                   |   1 +
 drivers/firmware/google/Kconfig               |   5 +-
 drivers/firmware/google/cbmem.c               |   1 +
 drivers/firmware/google/coreboot_table.c      |  13 +-
 drivers/firmware/google/coreboot_table.h      |  59 +--
 .../firmware/google/framebuffer-coreboot.c    | 122 +++--
 drivers/firmware/google/memconsole-coreboot.c |   1 +
 drivers/firmware/google/vpd.c                 |   1 +
 drivers/gpu/drm/sysfb/Kconfig                 |  16 +
 drivers/gpu/drm/sysfb/Makefile                |   1 +
 drivers/gpu/drm/sysfb/corebootdrm.c           | 434 ++++++++++++++++++
 drivers/gpu/drm/sysfb/drm_sysfb.c             |  24 +
 drivers/gpu/drm/sysfb/drm_sysfb_helper.h      |   8 +-
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c |  30 --
 drivers/gpu/drm/sysfb/efidrm.c                |   8 +-
 drivers/gpu/drm/sysfb/vesadrm.c               |   8 +-
 include/linux/coreboot.h                      |  80 ++++
 17 files changed, 689 insertions(+), 123 deletions(-)
 create mode 100644 drivers/gpu/drm/sysfb/corebootdrm.c
 create mode 100644 include/linux/coreboot.h


base-commit: d5b0278e4bea2fb27de6d12b7f865a74072af677
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
prerequisite-patch-id: 719d09751d38f5da743beed6266585ee063e1e29
-- 
2.52.0

[PATCH v2 01/12] firmware: google: framebuffer: Do not unregister platform device

[Thomas Zimmermann]

The native driver takes over the framebuffer aperture by removing the
system- framebuffer platform device. Afterwards the pointer in drvdata
is dangling. Remove the entire logic around drvdata and let the kernel's
aperture helpers handle this. The platform device depends on the native
hardware device instead of the coreboot device anyway.

When commit 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer
driver") added the coreboot framebuffer code, the kernel did not support
device-based aperture management. Instead native driviers only removed
the conflicting fbdev device. At that point, unregistering the framebuffer
device most likely worked correctly. It was definitely broken after
commit d9702b2a2171 ("fbdev/simplefb: Do not use struct
fb_info.apertures"). So take this commit for the Fixes tag. Earlier
releases might work depending on the native hardware driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d9702b2a2171 ("fbdev/simplefb: Do not use struct fb_info.apertures")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Hans de Goede <hansg@kernel.org>
Cc: linux-fbdev@vger.kernel.org
Cc: <stable@vger.kernel.org> # v6.3+
---
 drivers/firmware/google/framebuffer-coreboot.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index c68c9f56370f..4e9177105992 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -81,19 +81,10 @@ static int framebuffer_probe(struct coreboot_device *dev)
 						 sizeof(pdata));
 	if (IS_ERR(pdev))
 		pr_warn("coreboot: could not register framebuffer\n");
-	else
-		dev_set_drvdata(&dev->dev, pdev);
 
 	return PTR_ERR_OR_ZERO(pdev);
 }
 
-static void framebuffer_remove(struct coreboot_device *dev)
-{
-	struct platform_device *pdev = dev_get_drvdata(&dev->dev);
-
-	platform_device_unregister(pdev);
-}
-
 static const struct coreboot_device_id framebuffer_ids[] = {
 	{ .tag = CB_TAG_FRAMEBUFFER },
 	{ /* sentinel */ }
@@ -102,7 +93,6 @@ MODULE_DEVICE_TABLE(coreboot, framebuffer_ids);
 
 static struct coreboot_driver framebuffer_driver = {
 	.probe = framebuffer_probe,
-	.remove = framebuffer_remove,
 	.drv = {
 		.name = "framebuffer",
 	},
-- 
2.52.0

[PATCH v2 02/12] firmware: google: framebuffer: Do not mark framebuffer as busy

[Thomas Zimmermann]

Remove the flag IORESOURCE_BUSY flag from coreboot's framebuffer
resource. It prevents simpledrm from successfully requesting the
range for its own use; resulting in errors such as

[    2.775430] simple-framebuffer simple-framebuffer.0: [drm] could not acquire memory region [mem 0x80000000-0x80407fff flags 0x80000200]

As with other uses of simple-framebuffer, the simple-framebuffer
device should only declare it's I/O resources, but not actively use
them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer driver")
Cc: Samuel Holland <samuel@sholland.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: chrome-platform@lists.linux.dev
Cc: <stable@vger.kernel.org> # v4.18+
---
 drivers/firmware/google/framebuffer-coreboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 4e9177105992..f44183476ed7 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -67,7 +67,7 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		return -ENODEV;
 
 	memset(&res, 0, sizeof(res));
-	res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+	res.flags = IORESOURCE_MEM;
 	res.name = "Coreboot Framebuffer";
 	res.start = fb->physical_address;
 	length = PAGE_ALIGN(fb->y_resolution * fb->bytes_per_line);
-- 
2.52.0

[PATCH v2 03/12] firmware: google: framebuffer: Init memory resource with helper macro

[Thomas Zimmermann]

Initialize framebuffer memory resource with DEFINE_RES_MEM() instead
of open-coding the setup.

While at it, drop the resource name to make the kernel use the device
name of the simple-framebuffer device for the resource. The latter
includes a device number. While the meaning of the resource name is
somewhat fuzzy and varies across entries in /proc/iomem, showing the
device name seems more helpful than showing a fixed name.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/firmware/google/framebuffer-coreboot.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index f44183476ed7..767515a30a52 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -26,7 +26,6 @@ static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
 static int framebuffer_probe(struct coreboot_device *dev)
 {
 	int i;
-	u32 length;
 	struct lb_framebuffer *fb = &dev->framebuffer;
 	struct platform_device *pdev;
 	struct resource res;
@@ -53,6 +52,11 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (!fb->physical_address)
 		return -ENODEV;
 
+	res = DEFINE_RES_MEM(fb->physical_address,
+			     PAGE_ALIGN(fb->y_resolution * fb->bytes_per_line));
+	if (res.end <= res.start)
+		return -EINVAL;
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&
@@ -66,15 +70,6 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (!pdata.format)
 		return -ENODEV;
 
-	memset(&res, 0, sizeof(res));
-	res.flags = IORESOURCE_MEM;
-	res.name = "Coreboot Framebuffer";
-	res.start = fb->physical_address;
-	length = PAGE_ALIGN(fb->y_resolution * fb->bytes_per_line);
-	res.end = res.start + length - 1;
-	if (res.end <= res.start)
-		return -EINVAL;
-
 	pdev = platform_device_register_resndata(&dev->dev,
 						 "simple-framebuffer", 0,
 						 &res, 1, &pdata,
-- 
2.52.0

[PATCH v2 04/12] firmware: google: framebuffer: Tie platform device to PCI hardware

[Thomas Zimmermann]

Use the PCI device as parent of the system-framebuffer device instead
of the coreboot device. Prevents SIGBUS or SIGSEG after hot-unplug of
the PCI device while the framebuffer is active.

The simple-framebuffer device depends on the PCI hardware, so this
device needs to be its parent. The current coreboot parent is no
longer needed after the system-framebuffer evice has been created.

On systems without PCI or if no PCI parent device could be found,
the platform device hangs on the platform bus directly.

The fix here is similar to code in sysfb, which contained that same
bug.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 .../firmware/google/framebuffer-coreboot.c    | 85 +++++++++++++++++--
 1 file changed, 80 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 767515a30a52..cdb10f5de6ad 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/sysfb.h>
@@ -21,14 +22,71 @@
 
 #define CB_TAG_FRAMEBUFFER 0x12
 
+#if defined(CONFIG_PCI)
+static bool framebuffer_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;
+}
+
+static struct pci_dev *framebuffer_parent_pci_dev(struct resource *res)
+{
+	struct pci_dev *pdev = NULL;
+	const struct resource *r = NULL;
+
+	while (!r && (pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev)))
+		r = pci_find_resource(pdev, res);
+
+	if (!r || !pdev)
+		return NULL; /* not found; not an error */
+
+	if (!framebuffer_pci_dev_is_enabled(pdev)) {
+		pci_dev_put(pdev);
+		return ERR_PTR(-ENODEV);
+	}
+
+	return pdev;
+}
+#else
+static struct pci_dev *framebuffer_parent_pci_dev(struct resource *res)
+{
+	return NULL;
+}
+#endif
+
+static struct device *framebuffer_parent_dev(struct resource *res)
+{
+	struct pci_dev *pdev;
+
+	pdev = framebuffer_parent_pci_dev(res);
+	if (IS_ERR(pdev))
+		return ERR_CAST(pdev);
+	else if (pdev)
+		return &pdev->dev;
+
+	return NULL;
+}
+
 static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
 
 static int framebuffer_probe(struct coreboot_device *dev)
 {
 	int i;
 	struct lb_framebuffer *fb = &dev->framebuffer;
+	struct device *parent;
 	struct platform_device *pdev;
 	struct resource res;
+	int ret;
 	struct simplefb_platform_data pdata = {
 		.width = fb->x_resolution,
 		.height = fb->y_resolution,
@@ -57,6 +115,10 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (res.end <= res.start)
 		return -EINVAL;
 
+	parent = framebuffer_parent_dev(&res);
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&
@@ -67,17 +129,30 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		    fb->blue_mask_size     == formats[i].blue.length)
 			pdata.format = formats[i].name;
 	}
-	if (!pdata.format)
-		return -ENODEV;
+	if (!pdata.format) {
+		ret = -ENODEV;
+		goto err_put_device_parent;
+	}
 
-	pdev = platform_device_register_resndata(&dev->dev,
+	pdev = platform_device_register_resndata(parent,
 						 "simple-framebuffer", 0,
 						 &res, 1, &pdata,
 						 sizeof(pdata));
-	if (IS_ERR(pdev))
+	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
 		pr_warn("coreboot: could not register framebuffer\n");
+		goto err_put_device_parent;
+	}
+
+	if (parent)
+		put_device(parent);
+
+	return 0;
 
-	return PTR_ERR_OR_ZERO(pdev);
+err_put_device_parent:
+	if (parent)
+		put_device(parent);
+	return ret;
 }
 
 static const struct coreboot_device_id framebuffer_ids[] = {
-- 
2.52.0

[PATCH v2 05/12] firmware: google: framebuffer: Fix dependencies

[Thomas Zimmermann]

The framebuffer on the coreboot bus represents an entry in the
coreboot payload table; not the actual device. [1] Hence it must
not depend on any other driver setting.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://lore.kernel.org/dri-devel/CAODwPW9_ym3E4za3yoUAs0+1sQfaKTDOau4Oh9Zm8+2uvYVgFQ@mail.gmail.com/ # [1]
---
 drivers/firmware/google/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 41b78f5cb735..3ab3e089328b 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -59,11 +59,11 @@ config GOOGLE_MEMCONSOLE_X86_LEGACY
 
 config GOOGLE_FRAMEBUFFER_COREBOOT
 	tristate "Coreboot Framebuffer"
-	depends on FB_SIMPLE || DRM_SIMPLEDRM
 	depends on GOOGLE_COREBOOT_TABLE
 	help
 	  This option enables the kernel to search for a framebuffer in
-	  the coreboot table.  If found, it is registered with simplefb.
+	  the coreboot table.  If found, it is registered with a platform
+	  device of type simple-framebuffer.
 
 config GOOGLE_MEMCONSOLE_COREBOOT
 	tristate "Firmware Memory Console"
-- 
2.52.0

[PATCH v2 06/12] firmware: google: Init coreboot bus with subsys_initcall()

[Thomas Zimmermann]

Using module_init()/device_initcall() is too late to initialize
the coreboot bus, as there might already be drivers that depend
on it.

So far this hasn't been a problem, as coreboot controls all device
creation. Initializing the coreboot bus earlier in subsys_initcall()
will allow for external coreboot drivers to register themselves
with device_initcall(). Prepares coreboot to support additional
coreboot drivers from other subsystems.

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

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 882db32e51be..26d93781e64a 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -251,7 +251,7 @@ static void __exit coreboot_table_driver_exit(void)
 	bus_unregister(&coreboot_bus_type);
 }
 
-module_init(coreboot_table_driver_init);
+subsys_initcall(coreboot_table_driver_init);
 module_exit(coreboot_table_driver_exit);
 
 MODULE_AUTHOR("Google, Inc.");
-- 
2.52.0

[PATCH v2 07/12] firmware: google: Clean up include statements in coreboot_table.h

[Thomas Zimmermann]

Include <linux/mod_devicetable.h> from source files and only forward-
declare struct coreboot_device_id in coreboot_table.h.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/google/cbmem.c                | 1 +
 drivers/firmware/google/coreboot_table.c       | 1 +
 drivers/firmware/google/coreboot_table.h       | 3 ++-
 drivers/firmware/google/framebuffer-coreboot.c | 1 +
 drivers/firmware/google/memconsole-coreboot.c  | 1 +
 drivers/firmware/google/vpd.c                  | 1 +
 6 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
index 54c3b8b05e5d..3397bacdfdbe 100644
--- a/drivers/firmware/google/cbmem.c
+++ b/drivers/firmware/google/cbmem.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/kobject.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 26d93781e64a..a031d6fe6bc5 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index bb6f0f7299b4..17e9e5c3f6e1 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -13,7 +13,8 @@
 #define __COREBOOT_TABLE_H
 
 #include <linux/device.h>
-#include <linux/mod_devicetable.h>
+
+struct coreboot_device_id;
 
 /* Coreboot table header structure */
 struct coreboot_table_header {
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index cdb10f5de6ad..703dd4f5d930 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_data/simplefb.h>
diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index c5f08617aa8d..4aa9b1cad3c3 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 
 #include "memconsole.h"
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index 339a3f74b247..8d7f123f96f4 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/kobject.h>
 #include <linux/list.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
-- 
2.52.0

[PATCH v2 08/12] firmware: google: Export coreboot table entries

[Thomas Zimmermann]

Move types for coreboot table entries to <linux/coreboot.h>. Allows
drivers in other subsystems to use these structures.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 MAINTAINERS                                   |  1 +
 drivers/firmware/google/coreboot_table.c      | 10 +++
 drivers/firmware/google/coreboot_table.h      | 60 +----------------
 .../firmware/google/framebuffer-coreboot.c    |  2 -
 include/linux/coreboot.h                      | 66 +++++++++++++++++++
 5 files changed, 78 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/coreboot.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0f244cf0b5c5..820d31fb8986 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10737,6 +10737,7 @@ L:	chrome-platform@lists.linux.dev
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git
 F:	drivers/firmware/google/
+F:	include/linux/coreboot.h
 
 GOOGLE TENSOR SoC SUPPORT
 M:	Peter Griffin <peter.griffin@linaro.org>
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index a031d6fe6bc5..c769631ea15d 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -22,6 +22,16 @@
 
 #include "coreboot_table.h"
 
+/* Coreboot table header structure */
+struct coreboot_table_header {
+	char signature[4];
+	u32 header_bytes;
+	u32 header_checksum;
+	u32 table_bytes;
+	u32 table_checksum;
+	u32 table_entries;
+};
+
 #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
 #define CB_DRV(d) container_of_const(d, struct coreboot_driver, drv)
 
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 17e9e5c3f6e1..616ca3903e5c 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -12,67 +12,9 @@
 #ifndef __COREBOOT_TABLE_H
 #define __COREBOOT_TABLE_H
 
+#include <linux/coreboot.h>
 #include <linux/device.h>
 
-struct coreboot_device_id;
-
-/* Coreboot table header structure */
-struct coreboot_table_header {
-	char signature[4];
-	u32 header_bytes;
-	u32 header_checksum;
-	u32 table_bytes;
-	u32 table_checksum;
-	u32 table_entries;
-};
-
-/* List of coreboot entry structures that is used */
-/* Generic */
-struct coreboot_table_entry {
-	u32 tag;
-	u32 size;
-};
-
-/* Points to a CBMEM entry */
-struct lb_cbmem_ref {
-	u32 tag;
-	u32 size;
-
-	u64 cbmem_addr;
-};
-
-#define LB_TAG_CBMEM_ENTRY 0x31
-
-/* Corresponds to LB_TAG_CBMEM_ENTRY */
-struct lb_cbmem_entry {
-	u32 tag;
-	u32 size;
-
-	u64 address;
-	u32 entry_size;
-	u32 id;
-};
-
-/* Describes framebuffer setup by coreboot */
-struct lb_framebuffer {
-	u32 tag;
-	u32 size;
-
-	u64 physical_address;
-	u32 x_resolution;
-	u32 y_resolution;
-	u32 bytes_per_line;
-	u8  bits_per_pixel;
-	u8  red_mask_pos;
-	u8  red_mask_size;
-	u8  green_mask_pos;
-	u8  green_mask_size;
-	u8  blue_mask_pos;
-	u8  blue_mask_size;
-	u8  reserved_mask_pos;
-	u8  reserved_mask_size;
-};
-
 /* A device, additionally with information from coreboot. */
 struct coreboot_device {
 	struct device dev;
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 703dd4f5d930..2d468d055872 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -21,8 +21,6 @@
 
 #include "coreboot_table.h"
 
-#define CB_TAG_FRAMEBUFFER 0x12
-
 #if defined(CONFIG_PCI)
 static bool framebuffer_pci_dev_is_enabled(struct pci_dev *pdev)
 {
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
new file mode 100644
index 000000000000..48705b439c6e
--- /dev/null
+++ b/include/linux/coreboot.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * coreboot.h
+ *
+ * Coreboot device and driver interfaces.
+ *
+ * Copyright 2014 Gerd Hoffmann <kraxel@redhat.com>
+ * Copyright 2017 Google Inc.
+ * Copyright 2017 Samuel Holland <samuel@sholland.org>
+ */
+
+#ifndef _LINUX_COREBOOT_H
+#define _LINUX_COREBOOT_H
+
+#include <linux/types.h>
+
+/* List of coreboot entry structures that is used */
+
+#define CB_TAG_FRAMEBUFFER 0x12
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+/* Generic */
+struct coreboot_table_entry {
+	u32 tag;
+	u32 size;
+};
+
+/* Points to a CBMEM entry */
+struct lb_cbmem_ref {
+	u32 tag;
+	u32 size;
+
+	u64 cbmem_addr;
+};
+
+/* Corresponds to LB_TAG_CBMEM_ENTRY */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
+/* Describes framebuffer setup by coreboot */
+struct lb_framebuffer {
+	u32 tag;
+	u32 size;
+
+	u64 physical_address;
+	u32 x_resolution;
+	u32 y_resolution;
+	u32 bytes_per_line;
+	u8  bits_per_pixel;
+	u8  red_mask_pos;
+	u8  red_mask_size;
+	u8  green_mask_pos;
+	u8  green_mask_size;
+	u8  blue_mask_pos;
+	u8  blue_mask_size;
+	u8  reserved_mask_pos;
+	u8  reserved_mask_size;
+};
+
+#endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

[PATCH v2 09/12] firmware: google: Pack structures for coreboot table entries

[Thomas Zimmermann]

Pack the fields in the coreboot table entries. These entries are part of
the coreboot ABI, so they don't follow regular calling conventions. For
example, fields of type u64 are aligned to boundaries of 4 byte instead
of 8. [1]

So far this has not been a problem. In the future, padding bytes should
be added where explicit alignment is required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://github.com/coreboot/coreboot/blob/main/payloads/libpayload/include/coreboot_tables.h#L96 # [1]
---
 drivers/firmware/google/coreboot_table.c | 2 +-
 include/linux/coreboot.h                 | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index c769631ea15d..e21b013b19e7 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -30,7 +30,7 @@ struct coreboot_table_header {
 	u32 table_bytes;
 	u32 table_checksum;
 	u32 table_entries;
-};
+} __packed;
 
 #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
 #define CB_DRV(d) container_of_const(d, struct coreboot_driver, drv)
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index 48705b439c6e..514c95f9d0e3 100644
--- a/include/linux/coreboot.h
+++ b/include/linux/coreboot.h
@@ -12,6 +12,7 @@
 #ifndef _LINUX_COREBOOT_H
 #define _LINUX_COREBOOT_H
 
+#include <linux/compiler_attributes.h>
 #include <linux/types.h>
 
 /* List of coreboot entry structures that is used */
@@ -23,7 +24,7 @@
 struct coreboot_table_entry {
 	u32 tag;
 	u32 size;
-};
+} __packed;
 
 /* Points to a CBMEM entry */
 struct lb_cbmem_ref {
@@ -31,7 +32,7 @@ struct lb_cbmem_ref {
 	u32 size;
 
 	u64 cbmem_addr;
-};
+} __packed;
 
 /* Corresponds to LB_TAG_CBMEM_ENTRY */
 struct lb_cbmem_entry {
@@ -41,7 +42,7 @@ struct lb_cbmem_entry {
 	u64 address;
 	u32 entry_size;
 	u32 id;
-};
+} __packed;
 
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
@@ -61,6 +62,6 @@ struct lb_framebuffer {
 	u8  blue_mask_size;
 	u8  reserved_mask_pos;
 	u8  reserved_mask_size;
-};
+} __packed;
 
 #endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

[PATCH v2 10/12] drm/sysfb: Generalize pixel-format matching

[Thomas Zimmermann]

Provide drm_sysfb_get_format(), a helper that finds a specific DRM
format from a list of pixel formats. The new function builds upon
drm_sysfb_get_format_si(), which finds the DRM format from a given
instance of struct screen_info. Now get the screen_info's pixel format
in the caller. Allows for matching pixel formats in drivers without
screen_info.

Convert the callers in efidrm and vesadrm to the new interface.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/sysfb/drm_sysfb.c             | 24 +++++++++++++++
 drivers/gpu/drm/sysfb/drm_sysfb_helper.h      |  8 ++---
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 30 -------------------
 drivers/gpu/drm/sysfb/efidrm.c                |  8 ++++-
 drivers/gpu/drm/sysfb/vesadrm.c               |  8 ++++-
 5 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb.c b/drivers/gpu/drm/sysfb/drm_sysfb.c
index 308f82153b15..fbfb37d0fae1 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb.c
@@ -31,5 +31,29 @@ int drm_sysfb_get_validated_int0(struct drm_device *dev, const char *name,
 }
 EXPORT_SYMBOL(drm_sysfb_get_validated_int0);
 
+const struct drm_format_info *drm_sysfb_get_format(struct drm_device *dev,
+						   const struct drm_sysfb_format *formats,
+						   size_t nformats,
+						   const struct pixel_format *pixel)
+{
+	const struct drm_format_info *format = NULL;
+	size_t i;
+
+	for (i = 0; i < nformats; ++i) {
+		const struct drm_sysfb_format *f = &formats[i];
+
+		if (pixel_format_equal(pixel, &f->pixel)) {
+			format = drm_format_info(f->fourcc);
+			break;
+		}
+	}
+
+	if (!format)
+		drm_warn(dev, "No compatible color format found\n");
+
+	return format;
+}
+EXPORT_SYMBOL(drm_sysfb_get_format);
+
 MODULE_DESCRIPTION("Helpers for DRM sysfb drivers");
 MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
index de96bfe7562c..b14df5b54bc9 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
@@ -36,6 +36,10 @@ int drm_sysfb_get_validated_int(struct drm_device *dev, const char *name,
 				u64 value, u32 max);
 int drm_sysfb_get_validated_int0(struct drm_device *dev, const char *name,
 				 u64 value, u32 max);
+const struct drm_format_info *drm_sysfb_get_format(struct drm_device *dev,
+						   const struct drm_sysfb_format *formats,
+						   size_t nformats,
+						   const struct pixel_format *pixel);
 
 #if defined(CONFIG_SCREEN_INFO)
 int drm_sysfb_get_width_si(struct drm_device *dev, const struct screen_info *si);
@@ -48,10 +52,6 @@ int drm_sysfb_get_stride_si(struct drm_device *dev, const struct screen_info *si
 			    unsigned int width, unsigned int height, u64 size);
 u64 drm_sysfb_get_visible_size_si(struct drm_device *dev, const struct screen_info *si,
 				  unsigned int height, unsigned int stride, u64 size);
-const struct drm_format_info *drm_sysfb_get_format_si(struct drm_device *dev,
-						      const struct drm_sysfb_format *formats,
-						      size_t nformats,
-						      const struct screen_info *si);
 #endif
 
 /*
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
index 885864168c54..749290196c6a 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
@@ -72,33 +72,3 @@ u64 drm_sysfb_get_visible_size_si(struct drm_device *dev, const struct screen_in
 	return drm_sysfb_get_validated_size0(dev, "visible size", vsize, size);
 }
 EXPORT_SYMBOL(drm_sysfb_get_visible_size_si);
-
-const struct drm_format_info *drm_sysfb_get_format_si(struct drm_device *dev,
-						      const struct drm_sysfb_format *formats,
-						      size_t nformats,
-						      const struct screen_info *si)
-{
-	const struct drm_format_info *format = NULL;
-	struct pixel_format pixel;
-	size_t i;
-	int ret;
-
-	ret = screen_info_pixel_format(si, &pixel);
-	if (ret)
-		return NULL;
-
-	for (i = 0; i < nformats; ++i) {
-		const struct drm_sysfb_format *f = &formats[i];
-
-		if (pixel_format_equal(&pixel, &f->pixel)) {
-			format = drm_format_info(f->fourcc);
-			break;
-		}
-	}
-
-	if (!format)
-		drm_warn(dev, "No compatible color format found\n");
-
-	return format;
-}
-EXPORT_SYMBOL(drm_sysfb_get_format_si);
diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
index 1b683d55d6ea..ee525f330441 100644
--- a/drivers/gpu/drm/sysfb/efidrm.c
+++ b/drivers/gpu/drm/sysfb/efidrm.c
@@ -45,8 +45,14 @@ static const struct drm_format_info *efidrm_get_format_si(struct drm_device *dev
 		{ PIXEL_FORMAT_XBGR8888, DRM_FORMAT_XBGR8888, },
 		{ PIXEL_FORMAT_XRGB2101010, DRM_FORMAT_XRGB2101010, },
 	};
+	struct pixel_format pixel;
+	int ret;
+
+	ret = screen_info_pixel_format(si, &pixel);
+	if (ret)
+		return NULL;
 
-	return drm_sysfb_get_format_si(dev, formats, ARRAY_SIZE(formats), si);
+	return drm_sysfb_get_format(dev, formats, ARRAY_SIZE(formats), &pixel);
 }
 
 static u64 efidrm_get_mem_flags(struct drm_device *dev, resource_size_t start,
diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
index 7b7b5ba26317..b8c16ae3faad 100644
--- a/drivers/gpu/drm/sysfb/vesadrm.c
+++ b/drivers/gpu/drm/sysfb/vesadrm.c
@@ -49,8 +49,14 @@ static const struct drm_format_info *vesadrm_get_format_si(struct drm_device *de
 		{ PIXEL_FORMAT_XBGR8888, DRM_FORMAT_XBGR8888, },
 		{ PIXEL_FORMAT_C8, DRM_FORMAT_C8, },
 	};
+	struct pixel_format pixel;
+	int ret;
+
+	ret = screen_info_pixel_format(si, &pixel);
+	if (ret)
+		return NULL;
 
-	return drm_sysfb_get_format_si(dev, formats, ARRAY_SIZE(formats), si);
+	return drm_sysfb_get_format(dev, formats, ARRAY_SIZE(formats), &pixel);
 }
 
 /*
-- 
2.52.0

[PATCH v2 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Thomas Zimmermann]

Add corebotdrm, a DRM driver for coreboot framebuffers. The driver
supports a pre-initialized framebuffer with various packed RGB formats.
The driver code is fairly small and uses the same logic as the other
sysfb drivers. Most of the implementation comes from existing sysfb
helpers.

Until now, coreboot relied on simpledrm or simplefb for boot-up graphics
output. Initialize the platform device for corebootdrm in the same place
in framebuffer_probe(). With a later commit, the simple-framebuffer should
be removed.

v2:
- reimplement as platform driver
- limit resources and mappings to known framebuffer memory; no
  page alignment
- create corebootdrm device from coreboot framebuffer code

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/firmware/google/Kconfig               |   3 +-
 .../firmware/google/framebuffer-coreboot.c    |  17 +-
 drivers/gpu/drm/sysfb/Kconfig                 |  16 +
 drivers/gpu/drm/sysfb/Makefile                |   1 +
 drivers/gpu/drm/sysfb/corebootdrm.c           | 412 ++++++++++++++++++
 include/linux/coreboot.h                      |   4 +
 6 files changed, 449 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/sysfb/corebootdrm.c

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 3ab3e089328b..b78c644fa253 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -63,7 +63,8 @@ config GOOGLE_FRAMEBUFFER_COREBOOT
 	help
 	  This option enables the kernel to search for a framebuffer in
 	  the coreboot table.  If found, it is registered with a platform
-	  device of type simple-framebuffer.
+	  device of type coreboot-framebuffer. Using the old device of
+	  type simple-framebuffer is deprecated.
 
 config GOOGLE_MEMCONSOLE_COREBOOT
 	tristate "Firmware Memory Console"
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 2d468d055872..0992288eda28 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -76,22 +76,23 @@ static struct device *framebuffer_parent_dev(struct resource *res)
 	return NULL;
 }
 
-static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
-
 static int framebuffer_probe(struct coreboot_device *dev)
 {
-	int i;
 	struct lb_framebuffer *fb = &dev->framebuffer;
 	struct device *parent;
 	struct platform_device *pdev;
 	struct resource res;
 	int ret;
+#if !IS_ENABLED(CONFIG_DRM_COREBOOTDRM)
 	struct simplefb_platform_data pdata = {
 		.width = fb->x_resolution,
 		.height = fb->y_resolution,
 		.stride = fb->bytes_per_line,
 		.format = NULL,
 	};
+	int i;
+	static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
+#endif
 
 	/*
 	 * On coreboot systems, the advertised LB_TAG_FRAMEBUFFER entry
@@ -118,6 +119,15 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (IS_ERR(parent))
 		return PTR_ERR(parent);
 
+#if IS_ENABLED(CONFIG_DRM_COREBOOTDRM)
+	pdev = platform_device_register_resndata(parent, "coreboot-framebuffer", 0,
+						 &res, 1, fb, fb->size);
+	if (IS_ERR(pdev)) {
+		pr_warn("coreboot: could not register framebuffer\n");
+		ret = PTR_ERR(pdev);
+		goto err_put_device_parent;
+	}
+#else
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&
@@ -142,6 +152,7 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		pr_warn("coreboot: could not register framebuffer\n");
 		goto err_put_device_parent;
 	}
+#endif
 
 	if (parent)
 		put_device(parent);
diff --git a/drivers/gpu/drm/sysfb/Kconfig b/drivers/gpu/drm/sysfb/Kconfig
index 9c9884c7efc6..2559ead6cf1f 100644
--- a/drivers/gpu/drm/sysfb/Kconfig
+++ b/drivers/gpu/drm/sysfb/Kconfig
@@ -7,6 +7,22 @@ config DRM_SYSFB_HELPER
 	tristate
 	depends on DRM
 
+config DRM_COREBOOTDRM
+	tristate "Coreboot framebuffer driver"
+	depends on DRM && MMU
+	depends on GOOGLE_FRAMEBUFFER_COREBOOT
+	select APERTURE_HELPERS
+	select DRM_CLIENT_SELECTION
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	select DRM_SYSFB_HELPER
+	help
+	  DRM driver for coreboot-provided framebuffers.
+
+	  This driver assumes that the display hardware has been initialized
+	  by coreboot firmware before the kernel boots. Scanout buffer, size,
+	  and display format must be provided via coreboot framebuffer device.
+
 config DRM_EFIDRM
 	tristate "EFI framebuffer driver"
 	depends on DRM && MMU && EFI && (!SYSFB_SIMPLEFB || COMPILE_TEST)
diff --git a/drivers/gpu/drm/sysfb/Makefile b/drivers/gpu/drm/sysfb/Makefile
index a156c496413d..85c9087ab03d 100644
--- a/drivers/gpu/drm/sysfb/Makefile
+++ b/drivers/gpu/drm/sysfb/Makefile
@@ -6,6 +6,7 @@ drm_sysfb_helper-y := \
 drm_sysfb_helper-$(CONFIG_SCREEN_INFO) += drm_sysfb_screen_info.o
 obj-$(CONFIG_DRM_SYSFB_HELPER)	+= drm_sysfb_helper.o
 
+obj-$(CONFIG_DRM_COREBOOTDRM)	+= corebootdrm.o
 obj-$(CONFIG_DRM_EFIDRM)	+= efidrm.o
 obj-$(CONFIG_DRM_OFDRM)		+= ofdrm.o
 obj-$(CONFIG_DRM_SIMPLEDRM)	+= simpledrm.o
diff --git a/drivers/gpu/drm/sysfb/corebootdrm.c b/drivers/gpu/drm/sysfb/corebootdrm.c
new file mode 100644
index 000000000000..745318580a5d
--- /dev/null
+++ b/drivers/gpu/drm/sysfb/corebootdrm.c
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/aperture.h>
+#include <linux/coreboot.h>
+#include <linux/minmax.h>
+#include <linux/platform_device.h>
+
+#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_device.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fbdev_shmem.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+
+#include "drm_sysfb_helper.h"
+
+#define DRIVER_NAME	"corebootdrm"
+#define DRIVER_DESC	"DRM driver for Coreboot framebuffers"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+static const struct drm_format_info *
+corebootdrm_get_format_fb(struct drm_device *dev, const struct lb_framebuffer *fb)
+{
+	static const struct drm_sysfb_format formats[] = {
+		{ PIXEL_FORMAT_XRGB1555, DRM_FORMAT_XRGB1555, },
+		{ PIXEL_FORMAT_RGB565, DRM_FORMAT_RGB565, },
+		{ PIXEL_FORMAT_RGB888, DRM_FORMAT_RGB888, },
+		{ PIXEL_FORMAT_XRGB8888, DRM_FORMAT_XRGB8888, },
+		{ PIXEL_FORMAT_XBGR8888, DRM_FORMAT_XBGR8888, },
+		{ PIXEL_FORMAT_XRGB2101010, DRM_FORMAT_XRGB2101010, },
+	};
+	const struct pixel_format pixel = {
+		.bits_per_pixel = fb->bits_per_pixel,
+		.indexed  = false,
+		.alpha = {
+			.offset = 0,
+			.length = 0,
+		},
+		.red = {
+			.offset = fb->red_mask_pos,
+			.length = fb->red_mask_size,
+		},
+		.green = {
+			.offset = fb->green_mask_pos,
+			.length = fb->green_mask_size,
+		},
+		.blue = {
+			.offset = fb->blue_mask_pos,
+			.length = fb->blue_mask_size,
+		},
+	};
+
+	return drm_sysfb_get_format(dev, formats, ARRAY_SIZE(formats), &pixel);
+}
+
+static int corebootdrm_get_width_fb(struct drm_device *dev, const struct lb_framebuffer *fb)
+{
+	return drm_sysfb_get_validated_int0(dev, "width", fb->x_resolution, INT_MAX);
+}
+
+static int corebootdrm_get_height_fb(struct drm_device *dev, const struct lb_framebuffer *fb)
+{
+	return drm_sysfb_get_validated_int0(dev, "height", fb->y_resolution, INT_MAX);
+}
+
+static int corebootdrm_get_pitch_fb(struct drm_device *dev, const struct drm_format_info *format,
+				    unsigned int width, const struct lb_framebuffer *fb)
+{
+	u64 bytes_per_line = fb->bytes_per_line;
+
+	if (!bytes_per_line)
+		bytes_per_line = drm_format_info_min_pitch(format, 0, width);
+
+	return drm_sysfb_get_validated_int0(dev, "pitch", bytes_per_line, INT_MAX);
+}
+
+static resource_size_t corebootdrm_get_size_fb(struct drm_device *dev, unsigned int height,
+					       unsigned int pitch,
+					       const struct lb_framebuffer *fb)
+{
+	resource_size_t size;
+
+	if (check_mul_overflow(height, pitch, &size))
+		return 0;
+
+	return size;
+}
+
+static phys_addr_t corebootdrm_get_address_fb(struct drm_device *dev, resource_size_t size,
+					      const struct lb_framebuffer *fb)
+{
+	if (size > PHYS_ADDR_MAX)
+		return 0;
+	if (!fb->physical_address)
+		return 0;
+	if (fb->physical_address > (PHYS_ADDR_MAX - size))
+		return 0;
+
+	return fb->physical_address;
+}
+
+/*
+ * Simple Framebuffer device
+ */
+
+struct corebootdrm_device {
+	struct drm_sysfb_device sysfb;
+
+	/* modesetting */
+	u32 formats[DRM_SYSFB_PLANE_NFORMATS(1)];
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+};
+
+/*
+ * Modesetting
+ */
+
+static const u64 corebootdrm_primary_plane_format_modifiers[] = {
+	DRM_SYSFB_PLANE_FORMAT_MODIFIERS,
+};
+
+static const struct drm_plane_helper_funcs corebootdrm_primary_plane_helper_funcs = {
+	DRM_SYSFB_PLANE_HELPER_FUNCS,
+};
+
+static const struct drm_plane_funcs corebootdrm_primary_plane_funcs = {
+	DRM_SYSFB_PLANE_FUNCS,
+	.destroy = drm_plane_cleanup,
+};
+
+static const struct drm_crtc_helper_funcs corebootdrm_crtc_helper_funcs = {
+	DRM_SYSFB_CRTC_HELPER_FUNCS,
+};
+
+static const struct drm_crtc_funcs corebootdrm_crtc_funcs = {
+	DRM_SYSFB_CRTC_FUNCS,
+	.destroy = drm_crtc_cleanup,
+};
+
+static const struct drm_encoder_funcs corebootdrm_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static const struct drm_connector_helper_funcs corebootdrm_connector_helper_funcs = {
+	DRM_SYSFB_CONNECTOR_HELPER_FUNCS,
+};
+
+static const struct drm_connector_funcs corebootdrm_connector_funcs = {
+	DRM_SYSFB_CONNECTOR_FUNCS,
+	.destroy = drm_connector_cleanup,
+};
+
+static const struct drm_mode_config_funcs corebootdrm_mode_config_funcs = {
+	DRM_SYSFB_MODE_CONFIG_FUNCS,
+};
+
+static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev)
+{
+	struct drm_sysfb_device *sysfb = &cdev->sysfb;
+	struct drm_device *dev = &sysfb->dev;
+	const struct drm_format_info *format = sysfb->fb_format;
+	unsigned int width = sysfb->fb_mode.hdisplay;
+	unsigned int height = sysfb->fb_mode.vdisplay;
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	size_t nformats;
+	int ret;
+
+	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ret;
+
+	dev->mode_config.min_width = width;
+	dev->mode_config.max_width = max_t(unsigned int, width, DRM_SHADOW_PLANE_MAX_WIDTH);
+	dev->mode_config.min_height = height;
+	dev->mode_config.max_height = max_t(unsigned int, height, DRM_SHADOW_PLANE_MAX_HEIGHT);
+	dev->mode_config.funcs = &corebootdrm_mode_config_funcs;
+	dev->mode_config.preferred_depth = format->depth;
+
+	/* Primary plane */
+
+	nformats = drm_sysfb_build_fourcc_list(dev, &format->format, 1,
+					       cdev->formats, ARRAY_SIZE(cdev->formats));
+
+	primary_plane = &cdev->primary_plane;
+	ret = drm_universal_plane_init(dev, primary_plane, 0, &corebootdrm_primary_plane_funcs,
+				       cdev->formats, nformats,
+				       corebootdrm_primary_plane_format_modifiers,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+	drm_plane_helper_add(primary_plane, &corebootdrm_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	/* CRTC */
+
+	crtc = &cdev->crtc;
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+					&corebootdrm_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+	drm_crtc_helper_add(crtc, &corebootdrm_crtc_helper_funcs);
+
+	/* Encoder */
+
+	encoder = &cdev->encoder;
+	ret = drm_encoder_init(dev, encoder, &corebootdrm_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	/* Connector */
+
+	connector = &cdev->connector;
+	ret = drm_connector_init(dev, connector, &corebootdrm_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret)
+		return ret;
+	drm_connector_helper_add(connector, &corebootdrm_connector_helper_funcs);
+	drm_connector_set_panel_orientation_with_quirk(connector,
+						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
+						       width, height);
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * DRM driver
+ */
+
+DEFINE_DRM_GEM_FOPS(corebootdrm_fops);
+
+static struct drm_driver corebootdrm_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_FBDEV_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &corebootdrm_fops,
+};
+
+/*
+ * Coreboot driver
+ */
+
+static int corebootdrm_probe(struct platform_device *pdev)
+{
+	const struct lb_framebuffer *fb = dev_get_platdata(&pdev->dev);
+	struct corebootdrm_device *cdev;
+	struct drm_sysfb_device *sysfb;
+	struct drm_device *dev;
+	const struct drm_format_info *format;
+	int width, height, pitch;
+	resource_size_t size;
+	phys_addr_t address;
+	struct resource *res, *mem = NULL;
+	struct resource aperture;
+	void __iomem *screen_base;
+	int ret;
+
+	cdev = devm_drm_dev_alloc(&pdev->dev, &corebootdrm_drm_driver,
+				  struct corebootdrm_device, sysfb.dev);
+	if (IS_ERR(cdev))
+		return PTR_ERR(cdev);
+	platform_set_drvdata(pdev, cdev);
+
+	sysfb = &cdev->sysfb;
+	dev = &sysfb->dev;
+
+	if (!fb) {
+		drm_err(dev, "coreboot framebuffer not found\n");
+		return -EINVAL;
+	} else if (!LB_FRAMEBUFFER_HAS_LFB(fb)) {
+		drm_err(dev, "coreboot framebuffer entry too small\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Hardware settings
+	 */
+
+	format = corebootdrm_get_format_fb(dev, fb);
+	if (!format)
+		return -EINVAL;
+	width = corebootdrm_get_width_fb(dev, fb);
+	if (width < 0)
+		return width;
+	height = corebootdrm_get_height_fb(dev, fb);
+	if (height < 0)
+		return height;
+	pitch = corebootdrm_get_pitch_fb(dev, format, width, fb);
+	if (pitch < 0)
+		return pitch;
+	size = corebootdrm_get_size_fb(dev, height, pitch, fb);
+	if (!size)
+		return -EINVAL;
+	address = corebootdrm_get_address_fb(dev, size, fb);
+	if (!address)
+		return -EINVAL;
+
+	sysfb->fb_mode = drm_sysfb_mode(width, height, 0, 0);
+	sysfb->fb_format = format;
+	sysfb->fb_pitch = pitch;
+
+	drm_dbg(dev, "display mode={" DRM_MODE_FMT "}\n", DRM_MODE_ARG(&sysfb->fb_mode));
+	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, pitch=%d byte\n",
+		&format->format, width, height, pitch);
+
+	/*
+	 * Memory management
+	 */
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		drm_err(dev, "memory resource not found\n");
+		return -EINVAL;
+	}
+
+	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
+				      dev->driver->name);
+	if (!mem) {
+		drm_warn(dev, "could not acquire memory resource at %pr\n", res);
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about. Use
+		 * the memory resource as-is and try to map that instead.
+		 */
+		mem = res;
+	}
+
+	drm_dbg(dev, "using memory resource at %pr\n", mem);
+
+	aperture = DEFINE_RES_MEM(address, size);
+	if (!resource_contains(mem, &aperture)) {
+		drm_err(dev, "framebuffer aperture at invalid memory range %pr\n", &aperture);
+		return -EINVAL;
+	}
+
+	ret = devm_aperture_acquire_for_platform_device(pdev, address, size);
+	if (ret) {
+		drm_err(dev, "could not acquire framebuffer aperture: %d\n", ret);
+		return ret;
+	}
+
+	screen_base = devm_ioremap_wc(&pdev->dev, address, size);
+	if (!screen_base)
+		return -ENOMEM;
+
+	iosys_map_set_vaddr_iomem(&sysfb->fb_addr, screen_base);
+
+	/*
+	 * DRM mode setting and registration
+	 */
+
+	ret = corebootdrm_mode_config_init(cdev);
+	if (ret)
+		return ret;
+
+	drm_mode_config_reset(dev);
+
+	ret = drm_dev_register(dev, 0);
+	if (ret)
+		return ret;
+
+	drm_client_setup(dev, sysfb->fb_format);
+
+	return 0;
+}
+
+static void corebootdrm_remove(struct platform_device *pdev)
+{
+	struct corebootdrm_device *cdev = platform_get_drvdata(pdev);
+	struct drm_device *dev = &cdev->sysfb.dev;
+
+	drm_dev_unplug(dev);
+}
+
+static struct platform_driver corebootdrm_platform_driver = {
+	.driver = {
+		.name = "coreboot-framebuffer",
+	},
+	.probe = corebootdrm_probe,
+	.remove = corebootdrm_remove,
+};
+
+module_platform_driver(corebootdrm_platform_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index 514c95f9d0e3..e81ee5746e2b 100644
--- a/include/linux/coreboot.h
+++ b/include/linux/coreboot.h
@@ -14,6 +14,7 @@
 
 #include <linux/compiler_attributes.h>
 #include <linux/types.h>
+#include <linux/stddef.h>
 
 /* List of coreboot entry structures that is used */
 
@@ -64,4 +65,7 @@ struct lb_framebuffer {
 	u8  reserved_mask_size;
 } __packed;
 
+#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
+	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
+
 #endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

[PATCH v2 12/12] drm/corebotdrm: Support panel orientation

[Thomas Zimmermann]

Add fields and constants for coreboot framebuffer orientation. Set
corebootdrm's DRM connector state from the values. Not all firmware
provides orientation, so make it optional. Systems without continue
to use unknown orientation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/sysfb/corebootdrm.c | 30 +++++++++++++++++++++++++----
 include/linux/coreboot.h            |  9 +++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/corebootdrm.c b/drivers/gpu/drm/sysfb/corebootdrm.c
index 745318580a5d..5dc6f3c76f7b 100644
--- a/drivers/gpu/drm/sysfb/corebootdrm.c
+++ b/drivers/gpu/drm/sysfb/corebootdrm.c
@@ -110,6 +110,26 @@ static phys_addr_t corebootdrm_get_address_fb(struct drm_device *dev, resource_s
 	return fb->physical_address;
 }
 
+static enum drm_panel_orientation corebootdrm_get_orientation_fb(struct drm_device *dev,
+								 const struct lb_framebuffer *fb)
+{
+	if (!LB_FRAMEBUFFER_HAS_ORIENTATION(fb))
+		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+
+	switch (fb->orientation) {
+	case LB_FRAMEBUFFER_ORIENTATION_NORMAL:
+		return DRM_MODE_PANEL_ORIENTATION_NORMAL;
+	case LB_FRAMEBUFFER_ORIENTATION_BOTTOM_UP:
+		return DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+	case LB_FRAMEBUFFER_ORIENTATION_LEFT_UP:
+		return DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+	case LB_FRAMEBUFFER_ORIENTATION_RIGHT_UP:
+		return DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+	}
+
+	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+}
+
 /*
  * Simple Framebuffer device
  */
@@ -168,7 +188,8 @@ static const struct drm_mode_config_funcs corebootdrm_mode_config_funcs = {
 	DRM_SYSFB_MODE_CONFIG_FUNCS,
 };
 
-static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev)
+static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev,
+					enum drm_panel_orientation orientation)
 {
 	struct drm_sysfb_device *sysfb = &cdev->sysfb;
 	struct drm_device *dev = &sysfb->dev;
@@ -234,8 +255,7 @@ static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev)
 	if (ret)
 		return ret;
 	drm_connector_helper_add(connector, &corebootdrm_connector_helper_funcs);
-	drm_connector_set_panel_orientation_with_quirk(connector,
-						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
+	drm_connector_set_panel_orientation_with_quirk(connector, orientation,
 						       width, height);
 
 	ret = drm_connector_attach_encoder(connector, encoder);
@@ -276,6 +296,7 @@ static int corebootdrm_probe(struct platform_device *pdev)
 	int width, height, pitch;
 	resource_size_t size;
 	phys_addr_t address;
+	enum drm_panel_orientation orientation;
 	struct resource *res, *mem = NULL;
 	struct resource aperture;
 	void __iomem *screen_base;
@@ -320,6 +341,7 @@ static int corebootdrm_probe(struct platform_device *pdev)
 	address = corebootdrm_get_address_fb(dev, size, fb);
 	if (!address)
 		return -EINVAL;
+	orientation = corebootdrm_get_orientation_fb(dev, fb);
 
 	sysfb->fb_mode = drm_sysfb_mode(width, height, 0, 0);
 	sysfb->fb_format = format;
@@ -375,7 +397,7 @@ static int corebootdrm_probe(struct platform_device *pdev)
 	 * DRM mode setting and registration
 	 */
 
-	ret = corebootdrm_mode_config_init(cdev);
+	ret = corebootdrm_mode_config_init(cdev, orientation);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index e81ee5746e2b..d1de94f49cd3 100644
--- a/include/linux/coreboot.h
+++ b/include/linux/coreboot.h
@@ -45,6 +45,11 @@ struct lb_cbmem_entry {
 	u32 id;
 } __packed;
 
+#define LB_FRAMEBUFFER_ORIENTATION_NORMAL	0
+#define LB_FRAMEBUFFER_ORIENTATION_BOTTOM_UP	1
+#define LB_FRAMEBUFFER_ORIENTATION_LEFT_UP	2
+#define LB_FRAMEBUFFER_ORIENTATION_RIGHT_UP	3
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -63,9 +68,13 @@ struct lb_framebuffer {
 	u8  blue_mask_size;
 	u8  reserved_mask_pos;
 	u8  reserved_mask_size;
+	u8  orientation;
 } __packed;
 
 #define LB_FRAMEBUFFER_HAS_LFB(__fb) \
 	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
 
+#define LB_FRAMEBUFFER_HAS_ORIENTATION(__fb) \
+	((__fb)->size >= offsetofend(struct lb_framebuffer, orientation))
+
 #endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

Re: [PATCH v2 05/12] firmware: google: framebuffer: Fix dependencies

[Julius Werner]

Not really sure I understand this part? The code in coreboot_table.c
creates the device that represents the entry in the table. The code in
framebuffer-coreboot.c (which this Kconfig guards) contains the code
that creates the device which will be the actual framebuffer. Doesn't
it make sense for that part to depend on a framebuffer backend being
available?

Re: [PATCH v2 09/12] firmware: google: Pack structures for coreboot table entries

[Julius Werner]

Is this necessary? The coreboot table entries are 4 byte aligned, so
only the uint64_t fields are a problem. In coreboot we solve this by
not packing the structure and instead using something like this:

typedef __aligned(4) uint64_t cb_uint64_t;

I think the same solution could work in the kernel? Packing the
structure makes all alignment assumptions go away which can cause some
architectures to generate really bad code, so I've learned to try to
avoid it where possible.

Re: [PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[Julius Werner]

Whole series looks good to me in general, so you can call it

Acked-by: Julius Werner <jwerner@chromium.org>

But I'm only really familiar with coreboot and don't have the
expertise for all these DRM interfaces, so I hope someone else can do
a detailed review.

Re: [PATCH v2 05/12] firmware: google: framebuffer: Fix dependencies

[Thomas Zimmermann]

Hi

Am 15.01.26 um 21:54 schrieb Julius Werner:
> Not really sure I understand this part? The code in coreboot_table.c
> creates the device that represents the entry in the table. The code in
> framebuffer-coreboot.c (which this Kconfig guards) contains the code
> that creates the device which will be the actual framebuffer. Doesn't
> it make sense for that part to depend on a framebuffer backend being
> available?

Corebootdrm already depends on the CONFIG_GOOGLE_FRAMEBUFFER, which is 
standard behavior for all kernel drivers (i.e., drivers depending on 
architectural options). Having dependencies in the other direction 
creates a circular dependency, which Kconfig doesn't handle. But 
generally speaking, what's the point of having that framebuffer backend 
code then? I tried to remove it, but it is supposed to remain. But it 
would not serve a purpose other than creating the platform device for 
the actual driver. I'm confused about the reason for this design.

Best regards
Thomas


-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v2 09/12] firmware: google: Pack structures for coreboot table entries

[Thomas Zimmermann]

Hi

Am 15.01.26 um 21:56 schrieb Julius Werner:
> Is this necessary? The coreboot table entries are 4 byte aligned, so
> only the uint64_t fields are a problem. In coreboot we solve this by
> not packing the structure and instead using something like this:
>
> typedef __aligned(4) uint64_t cb_uint64_t;

Well OK. It's just a safety measure anyway. From what I can tell, it 
makes no difference with the current entries.

Best regards
Thomas

>
> I think the same solution could work in the kernel? Packing the
> structure makes all alignment assumptions go away which can cause some
> architectures to generate really bad code, so I've learned to try to
> avoid it where possible.

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[Thomas Zimmermann]

Am 15.01.26 um 21:58 schrieb Julius Werner:
> Whole series looks good to me in general, so you can call it
>
> Acked-by: Julius Werner <jwerner@chromium.org>

Thanks

>
> But I'm only really familiar with coreboot and don't have the
> expertise for all these DRM interfaces, so I hope someone else can do
> a detailed review.

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:10AM +0100, Thomas Zimmermann wrote:
> Coreboot implements framebuffer support via simple-framebuffer. Provide a
> dedicated DRM driver. Keep the simple-framebuffer code for now.
> 
> For each firmware's provided framebuffer, we prefer a dedicated DRM driver
> tailored towards the platform's feature set. The coreboot framebuffer
> device currently creates a simple-framebuffer device for the provided
> framebuffer aperture. But simple-framebuffer is for DeviceTree nodes; not
> for coreboot. The simple-framebuffer infrastructure should be phased out
> for non-DT use cases. Coreboot is one of the final users of the code
> (besides n64).
> 
> Patches 1 to 5 start by fixing problems in the coreboot framebuffer
> implementation. There is a possible dangling pointer, the memory is
> marked as busy, the device hierarchy is incorrect, and a few minor things.
> 
> Patches 6 to 9 prepare the coreboot support for use by external drivers.
> Specifically, structures for the entries os the coreboot payload table
> have to be exported.
> 
> Patches 10 to 12 add corebootdrm, a DRM driver for the new
> coreboot-framebuffer platform device. Corebootdrm follows the pattern
> established by similar drivers. It also uses the same sysfb helpers. It
> is therefore fairly small. With patch 11, it has feature parity with
> simpledrm on the old simple-framebuffer. Patch 12 adds support for panel-
> orientation flags that coreboot makes available.

What would you suggest to submit the patches (e.g., which patches submit
through which tree)?  Do they have build-time dependencies?

Re: [PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[Thomas Zimmermann]

Hi

Am 16.01.26 um 10:13 schrieb Tzung-Bi Shih:
> On Thu, Jan 15, 2026 at 08:57:10AM +0100, Thomas Zimmermann wrote:
>> Coreboot implements framebuffer support via simple-framebuffer. Provide a
>> dedicated DRM driver. Keep the simple-framebuffer code for now.
>>
>> For each firmware's provided framebuffer, we prefer a dedicated DRM driver
>> tailored towards the platform's feature set. The coreboot framebuffer
>> device currently creates a simple-framebuffer device for the provided
>> framebuffer aperture. But simple-framebuffer is for DeviceTree nodes; not
>> for coreboot. The simple-framebuffer infrastructure should be phased out
>> for non-DT use cases. Coreboot is one of the final users of the code
>> (besides n64).
>>
>> Patches 1 to 5 start by fixing problems in the coreboot framebuffer
>> implementation. There is a possible dangling pointer, the memory is
>> marked as busy, the device hierarchy is incorrect, and a few minor things.
>>
>> Patches 6 to 9 prepare the coreboot support for use by external drivers.
>> Specifically, structures for the entries os the coreboot payload table
>> have to be exported.
>>
>> Patches 10 to 12 add corebootdrm, a DRM driver for the new
>> coreboot-framebuffer platform device. Corebootdrm follows the pattern
>> established by similar drivers. It also uses the same sysfb helpers. It
>> is therefore fairly small. With patch 11, it has feature parity with
>> simpledrm on the old simple-framebuffer. Patch 12 adds support for panel-
>> orientation flags that coreboot makes available.
> What would you suggest to submit the patches (e.g., which patches submit
> through which tree)?  Do they have build-time dependencies?

The patches have no dependencies besides the coreboot and DRM frameworks 
they operate in. DRM moves a lot faster than coreboot, and you likely 
don't have the latest DRM in the coreboot tree. So I'd take them via 
DRM, if possible.

Alternatively, you can also merge patches 1 to 9 via coreboot trees and 
I'll merge the DRM side in a later release cycle.

Note that there will be at least one more update to this series to 
address review comments.

Best regards
Thomas


-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v2 00/12] drm, coreboot: Add DRM coreboot driver

[Tzung-Bi Shih]

On Fri, Jan 16, 2026 at 10:19:03AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.01.26 um 10:13 schrieb Tzung-Bi Shih:
> > On Thu, Jan 15, 2026 at 08:57:10AM +0100, Thomas Zimmermann wrote:
> > > Coreboot implements framebuffer support via simple-framebuffer. Provide a
> > > dedicated DRM driver. Keep the simple-framebuffer code for now.
> > > 
> > > For each firmware's provided framebuffer, we prefer a dedicated DRM driver
> > > tailored towards the platform's feature set. The coreboot framebuffer
> > > device currently creates a simple-framebuffer device for the provided
> > > framebuffer aperture. But simple-framebuffer is for DeviceTree nodes; not
> > > for coreboot. The simple-framebuffer infrastructure should be phased out
> > > for non-DT use cases. Coreboot is one of the final users of the code
> > > (besides n64).
> > > 
> > > Patches 1 to 5 start by fixing problems in the coreboot framebuffer
> > > implementation. There is a possible dangling pointer, the memory is
> > > marked as busy, the device hierarchy is incorrect, and a few minor things.
> > > 
> > > Patches 6 to 9 prepare the coreboot support for use by external drivers.
> > > Specifically, structures for the entries os the coreboot payload table
> > > have to be exported.
> > > 
> > > Patches 10 to 12 add corebootdrm, a DRM driver for the new
> > > coreboot-framebuffer platform device. Corebootdrm follows the pattern
> > > established by similar drivers. It also uses the same sysfb helpers. It
> > > is therefore fairly small. With patch 11, it has feature parity with
> > > simpledrm on the old simple-framebuffer. Patch 12 adds support for panel-
> > > orientation flags that coreboot makes available.
> > What would you suggest to submit the patches (e.g., which patches submit
> > through which tree)?  Do they have build-time dependencies?
> 
> The patches have no dependencies besides the coreboot and DRM frameworks
> they operate in. DRM moves a lot faster than coreboot, and you likely don't
> have the latest DRM in the coreboot tree. So I'd take them via DRM, if
> possible.

Let's take this way.  I'll try to review the patches and provide my A-b tag
too if possible.

> Note that there will be at least one more update to this series to address
> review comments.

Sure, no rush.

Re: [PATCH v2 05/12] firmware: google: framebuffer: Fix dependencies

[Julius Werner]

> Corebootdrm already depends on the CONFIG_GOOGLE_FRAMEBUFFER, which is
> standard behavior for all kernel drivers (i.e., drivers depending on
> architectural options). Having dependencies in the other direction
> creates a circular dependency, which Kconfig doesn't handle.

Yes, okay, I think that makes sense. I thought `DRM_SIMPLEDRM` was
some sort of more general option that the DRM subsystem is enabled,
but I guess it was just the driver that you now replaced with
DRM_COREBOOTDRM, and I understand that you can't have a circular
dependency there.

> But
> generally speaking, what's the point of having that framebuffer backend
> code then? I tried to remove it, but it is supposed to remain. But it
> would not serve a purpose other than creating the platform device for
> the actual driver. I'm confused about the reason for this design.

I thought the point was just to have a place to put that `if
(sysfs_handles_screen_info()) return -ENODEV;`? (That and that PCI
parent device thing you're doing where I don't really understand the
code enough to know what it's for.)

If you can put that line directly in corebootdrm_probe() instead (and
bind it directly to the coreboot bus device), then that's perfectly
fine by me too, and then you can get rid of framebuffer-coreboot.c. I
just assumed that there was some reason I don't have the expertise to
understand for why you couldn't do that.

My earlier concern here was just about not putting that line into
coreboot_table_populate(), because then you would be preventing the
creation of the sysfs node for that coreboot table entry, while all
other entries that don't have associated drivers are still there (and
that seems inconsistent).

Re: [PATCH v2 01/12] firmware: google: framebuffer: Do not unregister platform device

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:11AM +0100, Thomas Zimmermann wrote:
> The native driver takes over the framebuffer aperture by removing the
> system- framebuffer platform device. Afterwards the pointer in drvdata
> is dangling. Remove the entire logic around drvdata and let the kernel's
> aperture helpers handle this. The platform device depends on the native
> hardware device instead of the coreboot device anyway.
> 
> When commit 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer
> driver") added the coreboot framebuffer code, the kernel did not support
> device-based aperture management. Instead native driviers only removed
> the conflicting fbdev device. At that point, unregistering the framebuffer
> device most likely worked correctly. It was definitely broken after
> commit d9702b2a2171 ("fbdev/simplefb: Do not use struct
> fb_info.apertures"). So take this commit for the Fixes tag. Earlier
> releases might work depending on the native hardware driver.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d9702b2a2171 ("fbdev/simplefb: Do not use struct fb_info.apertures")

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 02/12] firmware: google: framebuffer: Do not mark framebuffer as busy

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:12AM +0100, Thomas Zimmermann wrote:
> Remove the flag IORESOURCE_BUSY flag from coreboot's framebuffer
> resource. It prevents simpledrm from successfully requesting the
> range for its own use; resulting in errors such as
> 
> [    2.775430] simple-framebuffer simple-framebuffer.0: [drm] could not acquire memory region [mem 0x80000000-0x80407fff flags 0x80000200]
> 
> As with other uses of simple-framebuffer, the simple-framebuffer
> device should only declare it's I/O resources, but not actively use
> them.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer driver")

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 03/12] firmware: google: framebuffer: Init memory resource with helper macro

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:13AM +0100, Thomas Zimmermann wrote:
> Initialize framebuffer memory resource with DEFINE_RES_MEM() instead
> of open-coding the setup.
> 
> While at it, drop the resource name to make the kernel use the device
> name of the simple-framebuffer device for the resource. The latter
> includes a device number. While the meaning of the resource name is
> somewhat fuzzy and varies across entries in /proc/iomem, showing the
> device name seems more helpful than showing a fixed name.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 04/12] firmware: google: framebuffer: Tie platform device to PCI hardware

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:14AM +0100, Thomas Zimmermann wrote:
> Use the PCI device as parent of the system-framebuffer device instead
> of the coreboot device. Prevents SIGBUS or SIGSEG after hot-unplug of
> the PCI device while the framebuffer is active.
> 
> The simple-framebuffer device depends on the PCI hardware, so this
> device needs to be its parent. The current coreboot parent is no
> longer needed after the system-framebuffer evice has been created.
                                            ^ d?
> 
> On systems without PCI or if no PCI parent device could be found,
> the platform device hangs on the platform bus directly.
> 
> The fix here is similar to code in sysfb, which contained that same
> bug.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

With some minor comments,
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

> +	if (parent)
> +		put_device(parent);
> +
> +	return 0;

Or maybe just:

  ret = 0;

Re: [PATCH v2 05/12] firmware: google: framebuffer: Fix dependencies

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:15AM +0100, Thomas Zimmermann wrote:
> The framebuffer on the coreboot bus represents an entry in the
> coreboot payload table; not the actual device. [1] Hence it must
> not depend on any other driver setting.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://lore.kernel.org/dri-devel/CAODwPW9_ym3E4za3yoUAs0+1sQfaKTDOau4Oh9Zm8+2uvYVgFQ@mail.gmail.com/ # [1]

The driver has no build-time dependencies to neither FB_SIMPLE nor
DRM_SIMPLEDRM.

The patch makes sense to me,
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 06/12] firmware: google: Init coreboot bus with subsys_initcall()

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:16AM +0100, Thomas Zimmermann wrote:
> Using module_init()/device_initcall() is too late to initialize
> the coreboot bus, as there might already be drivers that depend
> on it.
> 
> So far this hasn't been a problem, as coreboot controls all device
> creation. Initializing the coreboot bus earlier in subsys_initcall()
> will allow for external coreboot drivers to register themselves
> with device_initcall(). Prepares coreboot to support additional
> coreboot drivers from other subsystems.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 07/12] firmware: google: Clean up include statements in coreboot_table.h

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:17AM +0100, Thomas Zimmermann wrote:
> Include <linux/mod_devicetable.h> from source files and only forward-
> declare struct coreboot_device_id in coreboot_table.h.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 08/12] firmware: google: Export coreboot table entries

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:18AM +0100, Thomas Zimmermann wrote:
> Move types for coreboot table entries to <linux/coreboot.h>. Allows
> drivers in other subsystems to use these structures.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:21AM +0100, Thomas Zimmermann wrote:
> Add corebotdrm, a DRM driver for coreboot framebuffers. The driver
           ^ oo

> diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
> index 514c95f9d0e3..e81ee5746e2b 100644
> --- a/include/linux/coreboot.h
> +++ b/include/linux/coreboot.h
> @@ -14,6 +14,7 @@
>  
>  #include <linux/compiler_attributes.h>
>  #include <linux/types.h>
> +#include <linux/stddef.h>
>  
>  /* List of coreboot entry structures that is used */
>  
> @@ -64,4 +65,7 @@ struct lb_framebuffer {
>  	u8  reserved_mask_size;
>  } __packed;
>  
> +#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
> +	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
> +

Does LFB stand for "Linear Frame Buffer"?

I supposed the LFB follows the struct lb_framebuffer in memory.  If yes, I'm
wondering does LB_FRAMEBUFFER_HAS_LFB() work?  As the struct lb_framebuffer
definition is different from [1].

`offsetofend(struct lb_framebuffer, reserved_mask_size)` is 37 in the kernel
but the tailing data (i.e., LFB in the context) might start from offset 40 in
coreboot.

[1] https://github.com/coreboot/coreboot/blob/main/payloads/libpayload/include/coreboot_tables.h#L232

Re: [PATCH v2 12/12] drm/corebotdrm: Support panel orientation

[Tzung-Bi Shih]

On Thu, Jan 15, 2026 at 08:57:22AM +0100, Thomas Zimmermann wrote:
> diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
> [...]
> @@ -63,9 +68,13 @@ struct lb_framebuffer {
>  	u8  blue_mask_size;
>  	u8  reserved_mask_pos;
>  	u8  reserved_mask_size;
> +	u8  orientation;
>  } __packed;
>  
>  #define LB_FRAMEBUFFER_HAS_LFB(__fb) \
>  	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
>  
> +#define LB_FRAMEBUFFER_HAS_ORIENTATION(__fb) \
> +	((__fb)->size >= offsetofend(struct lb_framebuffer, orientation))

Wouldn't the new field in struct lb_framebuffer break LB_FRAMEBUFFER_HAS_LFB()?
E.g., fb->size == offsetofend(struct lb_framebuffer, reserved_mask_size)
   -> LB_FRAMEBUFFER_HAS_LFB(fb) == true
   -> LB_FRAMEBUFFER_HAS_ORIENTATION(fb) == false?

Re: [PATCH v2 12/12] drm/corebotdrm: Support panel orientation

[Tzung-Bi Shih]

On Tue, Jan 27, 2026 at 07:49:46AM +0000, Tzung-Bi Shih wrote:
> On Thu, Jan 15, 2026 at 08:57:22AM +0100, Thomas Zimmermann wrote:
> > diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
> > [...]
> > @@ -63,9 +68,13 @@ struct lb_framebuffer {
> >  	u8  blue_mask_size;
> >  	u8  reserved_mask_pos;
> >  	u8  reserved_mask_size;
> > +	u8  orientation;
> >  } __packed;
> >  
> >  #define LB_FRAMEBUFFER_HAS_LFB(__fb) \
> >  	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
> >  
> > +#define LB_FRAMEBUFFER_HAS_ORIENTATION(__fb) \
> > +	((__fb)->size >= offsetofend(struct lb_framebuffer, orientation))
> 
> Wouldn't the new field in struct lb_framebuffer break LB_FRAMEBUFFER_HAS_LFB()?
> E.g., fb->size == offsetofend(struct lb_framebuffer, reserved_mask_size)
>    -> LB_FRAMEBUFFER_HAS_LFB(fb) == true
>    -> LB_FRAMEBUFFER_HAS_ORIENTATION(fb) == false?

The example is wrong, please forget it.

Again, I supposed the "LFB" is tailing data of struct lb_framebuffer.

I meant: is it possible that there is no tailing data
      -> fb->size == offsetofend(struct lb_framebuffer, orientation)
      -> LB_FRAMEBUFFER_HAS_LFB() == true falsely.

Re: [PATCH v2 05/12] firmware: google: framebuffer: Fix dependencies

[Thomas Zimmermann]

Hi

Am 17.01.26 um 02:38 schrieb Julius Werner:
>> Corebootdrm already depends on the CONFIG_GOOGLE_FRAMEBUFFER, which is
>> standard behavior for all kernel drivers (i.e., drivers depending on
>> architectural options). Having dependencies in the other direction
>> creates a circular dependency, which Kconfig doesn't handle.
> Yes, okay, I think that makes sense. I thought `DRM_SIMPLEDRM` was
> some sort of more general option that the DRM subsystem is enabled,
> but I guess it was just the driver that you now replaced with
> DRM_COREBOOTDRM, and I understand that you can't have a circular
> dependency there.
>
>> But
>> generally speaking, what's the point of having that framebuffer backend
>> code then? I tried to remove it, but it is supposed to remain. But it
>> would not serve a purpose other than creating the platform device for
>> the actual driver. I'm confused about the reason for this design.
> I thought the point was just to have a place to put that `if
> (sysfs_handles_screen_info()) return -ENODEV;`? (That and that PCI
> parent device thing you're doing where I don't really understand the
> code enough to know what it's for.)
>
> If you can put that line directly in corebootdrm_probe() instead (and
> bind it directly to the coreboot bus device), then that's perfectly
> fine by me too, and then you can get rid of framebuffer-coreboot.c. I
> just assumed that there was some reason I don't have the expertise to
> understand for why you couldn't do that.
>
> My earlier concern here was just about not putting that line into
> coreboot_table_populate(), because then you would be preventing the
> creation of the sysfs node for that coreboot table entry, while all
> other entries that don't have associated drivers are still there (and
> that seems inconsistent).

Makes sense. Then let's leave the logic as it is.

Best regards
Thomas



-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v2 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Thomas Zimmermann]

Hi

Am 27.01.26 um 08:49 schrieb Tzung-Bi Shih:
> On Thu, Jan 15, 2026 at 08:57:21AM +0100, Thomas Zimmermann wrote:
>> Add corebotdrm, a DRM driver for coreboot framebuffers. The driver
>             ^ oo
>
>> diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
>> index 514c95f9d0e3..e81ee5746e2b 100644
>> --- a/include/linux/coreboot.h
>> +++ b/include/linux/coreboot.h
>> @@ -14,6 +14,7 @@
>>   
>>   #include <linux/compiler_attributes.h>
>>   #include <linux/types.h>
>> +#include <linux/stddef.h>
>>   
>>   /* List of coreboot entry structures that is used */
>>   
>> @@ -64,4 +65,7 @@ struct lb_framebuffer {
>>   	u8  reserved_mask_size;
>>   } __packed;
>>   
>> +#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
>> +	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
>> +
> Does LFB stand for "Linear Frame Buffer"?

Yes.

>
> I supposed the LFB follows the struct lb_framebuffer in memory.  If yes, I'm
> wondering does LB_FRAMEBUFFER_HAS_LFB() work?  As the struct lb_framebuffer
> definition is different from [1].
>
> `offsetofend(struct lb_framebuffer, reserved_mask_size)` is 37 in the kernel
> but the tailing data (i.e., LFB in the context) might start from offset 40 in
> coreboot.

Newer versions of coreboot add fields to the end of struct 
cb_framebuffer. So it keeps growing in size.

This macro checks is the provided size is large enough to cover 
framebuffer information. The framebuffer info consists of all the fields 
up-to-and-including reserved_mask_size. Testing with 
LB_FRAMEBUFFER_HAS_LFB is a safety measure, as we can assume that it's 
always there.

If coreboot provides less fields, it is likely broken. If it provides 
more fields, it doesn't affect the framebuffer.

Best regards
Thomas

>
> [1] https://github.com/coreboot/coreboot/blob/main/payloads/libpayload/include/coreboot_tables.h#L232

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v2 12/12] drm/corebotdrm: Support panel orientation

[Thomas Zimmermann]

Hi

Am 27.01.26 um 09:04 schrieb Tzung-Bi Shih:
> On Tue, Jan 27, 2026 at 07:49:46AM +0000, Tzung-Bi Shih wrote:
>> On Thu, Jan 15, 2026 at 08:57:22AM +0100, Thomas Zimmermann wrote:
>>> diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
>>> [...]
>>> @@ -63,9 +68,13 @@ struct lb_framebuffer {
>>>   	u8  blue_mask_size;
>>>   	u8  reserved_mask_pos;
>>>   	u8  reserved_mask_size;
>>> +	u8  orientation;
>>>   } __packed;
>>>   
>>>   #define LB_FRAMEBUFFER_HAS_LFB(__fb) \
>>>   	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
>>>   
>>> +#define LB_FRAMEBUFFER_HAS_ORIENTATION(__fb) \
>>> +	((__fb)->size >= offsetofend(struct lb_framebuffer, orientation))
>> Wouldn't the new field in struct lb_framebuffer break LB_FRAMEBUFFER_HAS_LFB()?
>> E.g., fb->size == offsetofend(struct lb_framebuffer, reserved_mask_size)
>>     -> LB_FRAMEBUFFER_HAS_LFB(fb) == true
>>     -> LB_FRAMEBUFFER_HAS_ORIENTATION(fb) == false?
> The example is wrong, please forget it.
>
> Again, I supposed the "LFB" is tailing data of struct lb_framebuffer.
>
> I meant: is it possible that there is no tailing data
>        -> fb->size == offsetofend(struct lb_framebuffer, orientation)
>        -> LB_FRAMEBUFFER_HAS_LFB() == true falsely.

I'm not quite sure if I understand the question: _HAS_LFB test is the 
provided entry is large enough to hold framebuffer information (true 
unless the firmware is broken). _HAS_ORIENTATION tests is the provided 
entry is large enough to hold the orientation field. That depends on the 
coreboot version, as early versions might not have it.  If either macro 
is true, we can safely look at the rsp fields; otherwise the fields are 
undefined.

If provided, the individual fields still need to be validated whether 
they have meaning full values.

Best regards
Thomas

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)