Hi
Am 06.02.26 um 06:14 schrieb Tzung-Bi Shih:
On Tue, Feb 03, 2026 at 02:52:30PM +0100, Thomas Zimmermann wrote:
Add corebootdrm, 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.
v3:
- comment on _HAS_LFB semantics (Tzung-Bi)
- fix typo in commit description (Tzung-Bi)
- comment on simple-framebuffer being obsolete for coreboot
v2:
- reimplement as platform driver
- limit resources and mappings to known framebuffer memory; no
page alignment
- create corebootdrm device from coreboot framebuffer code
Changelog should be after "---" otherwise it becomes part of commit message.
I see. In DRM land, we usually keep the change log in the commit
message. I'll change it for the coreboot patches, but I'd rather keep it
for the DRM patches. I can split off the coreboot changes for this patch
into its own.
+static int corebootdrm_probe(struct platform_device *pdev)
+{
[...]
+ 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;
[...]
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index 5746b99a070d..b51665165f9f 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>
Move it before types.h? 's' vs. 't'.
+/*
+ * True if the coreboot-provided data is large enough to hold information
+ * on the linear framebuffer. False otherwise.
+ */
+#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
+ ((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
+
To make sure I understand, do you mean:
- The __fb->size is possibly less than sizeof(struct lb_framebuffer).
LB_FRAMEBUFFER_HAS_LFB() is for checking the following fields
(e.g. fb->x_resolution) are available?
Yes.
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;
};
- If answer of the previous question is yes, the next question: does
LB_FRAMEBUFFER_HAS_LFB() needs to check up to `reserved_mask_size`?
As in the patch, it only accesses up to `blue_mask_size`.
Well, it's a correctness thing. The reserved_mask fields have been part
of the structure since the first version in commit b700254aa5 ("Add
coreboot framebuffer support to libpayload"). I'd that expect that the
framebuffer entry is bogus if it does not contain these fields. If you
really want to leave them out, we can do that of course.
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)
