[PATCH 0/2] Add monochrome framebuffer support to simple-framebuffer

[PATCH 0/2] Add monochrome framebuffer support to simple-framebuffer

[Sebastian Raase]

This patch series adds support for monochrome framebuffers (1 bpp, packed-pixel)
to simple-framebuffer.

Monochrome framebuffers require far less video memory and bandwidth than
16bpp or 32bpp framebuffers, making them useful for small devices or FPGA
implementations.

Sebastian Raase (2):
  fbdev: simplefb: Add mono framebuffer format
  dt-bindings: display: simple-framebuffer: Document mono format

 .../devicetree/bindings/display/simple-framebuffer.yaml         | 2 ++
 drivers/video/fbdev/simplefb.c                                  | 2 ++
 include/linux/platform_data/simplefb.h                          | 1 +
 3 files changed, 5 insertions(+)

-- 
2.47.3

[PATCH 1/2] fbdev: simplefb: Add mono framebuffer format

[Sebastian Raase]

Add a new simple-framebuffer format for monochrome 1 bit-per-pixel
framebuffers. Monochrome framebuffers are not true color, so their
visual is set to FB_VISUAL_MONO10 (set pixels are bright).

Signed-off-by: Sebastian Raase <linux@sraa.de>
---
 drivers/video/fbdev/simplefb.c         | 2 ++
 include/linux/platform_data/simplefb.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 60e5dcec201f..4662101885ec 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -583,6 +583,8 @@ static int simplefb_probe(struct platform_device *pdev)
 	info->fix.smem_start = mem->start;
 	info->fix.smem_len = resource_size(mem);
 	info->fix.line_length = params.stride;
+	if (params.format->bits_per_pixel == 1)
+		info->fix.visual = FB_VISUAL_MONO10;
 
 	info->var = simplefb_var;
 	info->var.xres = params.width;
diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
index 4f94d52ac99f..9b6d822145c9 100644
--- a/include/linux/platform_data/simplefb.h
+++ b/include/linux/platform_data/simplefb.h
@@ -15,6 +15,7 @@
 /* format array, use it to initialize a "struct simplefb_format" array */
 #define SIMPLEFB_FORMATS \
 { \
+	{ "mono", 1, {0, 1}, {0, 1}, {0, 1}, {0, 0}, DRM_FORMAT_R1 }, \
 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0}, DRM_FORMAT_RGB565 }, \
 	{ "r5g5b5a1", 16, {11, 5}, {6, 5}, {1, 5}, {0, 1}, DRM_FORMAT_RGBA5551 }, \
 	{ "x1r5g5b5", 16, {10, 5}, {5, 5}, {0, 5}, {0, 0}, DRM_FORMAT_XRGB1555 }, \
-- 
2.47.3

Re: [PATCH 1/2] fbdev: simplefb: Add mono framebuffer format

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fbdev: simplefb: adding DRM_FORMAT_R1 to SIMPLEFB_FORMATS breaks simpledrm
- [Medium] fbdev: simplefb: mono bit order in cfb helpers mismatches DRM_FORMAT_R1
--

commit 89b64a8b00831d236b9621c5f7f6d3729ed70512
Author: Sebastian Raase <linux@sraa.de>

fbdev: simplefb: Add mono framebuffer format

This commit adds a 1 bit-per-pixel monochrome ("mono") format entry mapped
to DRM_FORMAT_R1 in SIMPLEFB_FORMATS and configures simplefb to use the
FB_VISUAL_MONO10 visual when probing 1-bpp framebuffers.

> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 60e5dcec201ff..4662101885ec5 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -583,6 +583,8 @@ static int simplefb_probe(struct platform_device *pdev)
>  	info->fix.smem_start = mem->start;
>  	info->fix.smem_len = resource_size(mem);
>  	info->fix.line_length = params.stride;
> +	if (params.format->bits_per_pixel == 1)
> +		info->fix.visual = FB_VISUAL_MONO10;

[Severity: Medium]
Does the bit ordering used by the cfb drawing helpers match DRM_FORMAT_R1 on
little-endian systems?

In include/uapi/drm/drm_fourcc.h, DRM_FORMAT_R1 is defined with MSB-first
pixel ordering ([7:0] R0:R1:R2:R3:R4:R5:R6:R7, where the leftmost pixel R0 is
in bit 7).

However, in drivers/video/fbdev/core/fb_draw.h, fb_reverse_init() and the
cfb drawing helpers (cfb_imageblit(), cfb_fillrect(), cfb_copyarea()) place
pixel 0 in the LSB on little-endian architectures and in the MSB on
big-endian architectures unless CONFIG_FB_CFB_REV_PIXELS_IN_BYTE is enabled
and FB_NONSTD_REV_PIX_IN_B is set in info->var.nonstd:

drivers/video/fbdev/core/fb_draw.h:fb_reverse_init() {
	struct fb_reverse reverse;
#ifdef __LITTLE_ENDIAN
	reverse.byte = fb_be_math(info) != 0;
#else
	reverse.byte = fb_be_math(info) == 0;
#endif
#ifdef FB_REV_PIXELS_IN_BYTE
	reverse.pixel = info->var.bits_per_pixel < BITS_PER_BYTE
		&& (info->var.nonstd & FB_NONSTD_REV_PIX_IN_B);
#else
	reverse.pixel = false;
#endif
	return reverse;
}

Since simplefb leaves info->var.nonstd at 0 and does not select
FB_CFB_REV_PIXELS_IN_BYTE, won't the 8 horizontal pixels within each byte be
bit-reversed relative to DRM_FORMAT_R1 (and relative to big-endian simplefb)
on little-endian machines?

>  
>  	info->var = simplefb_var;
>  	info->var.xres = params.width;
> diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
> index 4f94d52ac99fc..9b6d822145c9e 100644
> --- a/include/linux/platform_data/simplefb.h
> +++ b/include/linux/platform_data/simplefb.h
> @@ -15,6 +15,7 @@
>  /* format array, use it to initialize a "struct simplefb_format" array */
>  #define SIMPLEFB_FORMATS \
>  { \
> +	{ "mono", 1, {0, 1}, {0, 1}, {0, 1}, {0, 0}, DRM_FORMAT_R1 }, \

[Severity: High]
Can adding "mono" with DRM_FORMAT_R1 to SIMPLEFB_FORMATS break display output
when simpledrm is enabled?

SIMPLEFB_FORMATS is shared with drivers/gpu/drm/sysfb/simpledrm.c, where
simplefb_get_validated_format() matches fmt->name and looks up fmt->fourcc:

drivers/gpu/drm/sysfb/simpledrm.c:simplefb_get_validated_format() {
	...
		if (!strcmp(format_name, fmt->name)) {
			info = drm_format_info(fmt->fourcc);
			if (!info)
				return ERR_PTR(-EINVAL);
			return info;
		}
	...
}

With this entry added, simpledrm binds to "mono" simple-framebuffer devices,
acquires the aperture (evicting or blocking simplefb), and registers a
primary plane advertising DRM_FORMAT_R1 and emulated DRM_FORMAT_XRGB8888.

When in-kernel clients (such as fbdev emulation via drm_client_setup() ->
drm_fbdev_client_setup()) or userspace compositors commit an XRGB8888
framebuffer, drm_sysfb_plane_helper_begin_fb_access() calls
drm_sysfb_get_blit_func(DRM_FORMAT_R1, DRM_FORMAT_XRGB8888):

drivers/gpu/drm/sysfb/drm_sysfb_modeset.c:drm_sysfb_get_blit_func() {
	if (src_format == dst_format) {
		return drm_sysfb_memcpy;
	} else if (src_format == DRM_FORMAT_XRGB8888) {
		switch (dst_format) {
		case DRM_FORMAT_RGB565:
			return drm_fb_xrgb8888_to_rgb565;
		...
		}
	}
	return NULL;
}

Because drm_sysfb_get_blit_func() has no conversion helper for DRM_FORMAT_R1,
it returns NULL, triggering a warning and failing every XRGB8888 atomic
commit with -EINVAL.

Additionally, if a userspace client commits a native DRM_FORMAT_R1
framebuffer with a non-zero horizontal damage clip offset (clip->x1 > 0),
drm_sysfb_memcpy() calls drm_fb_memcpy():

drivers/gpu/drm/drm_format_helper.c:drm_fb_memcpy() {
	...
		unsigned int bpp_i = drm_format_info_bpp(format, i);
		unsigned int cpp_i = DIV_ROUND_UP(bpp_i, 8);
		size_t len_i = DIV_ROUND_UP(drm_rect_width(clip) * bpp_i, 8);

		src_i += clip_offset(clip, src->pitches[i], cpp_i);
	...
}

For 1-bpp DRM_FORMAT_R1, cpp_i evaluates to 1 byte per pixel, so clip_offset()
advances src_i by clip->x1 bytes instead of bits (8 times the horizontal
byte offset) while drm_sysfb_plane_helper_atomic_update() offsets dst using
dst_format->cpp[0] == 0. Won't that read past the end of the source buffer on
partial horizontal updates?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793758480.git.linux@sraa.de?part=1

[PATCH 2/2] dt-bindings: display: simple-framebuffer: Document mono format

[Sebastian Raase]

This is a 1 bit-per-pixel monochrome format, with 8 pixels/byte.

Signed-off-by: Sebastian Raase <linux@sraa.de>
---
 .../devicetree/bindings/display/simple-framebuffer.yaml         | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index 45ffdebc9d86..c627ec088434 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -94,6 +94,7 @@ properties:
   format:
     description: >
       Format of the framebuffer:
+        * `mono` - 1-bit packed pixels, 8 pixels/byte
         * `a1r5g5b5` - 16-bit pixels, d[15]=a, d[14:10]=r, d[9:5]=g, d[4:0]=b
         * `a2r10g10b10` - 32-bit pixels, d[31:30]=a, d[29:20]=r, d[19:10]=g, d[9:0]=b
         * `a8b8g8r8` - 32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r
@@ -106,6 +107,7 @@ properties:
         * `x8r8g8b8` - 32-bit pixels, d[23:16]=r, d[15:8]=g, d[7:0]=b
         * `x8b8g8r8` - 32-bit pixels, d[23:16]=b, d[15:8]=g, d[7:0]=r
     enum:
+      - mono
       - a1r5g5b5
       - a2r10g10b10
       - a8b8g8r8
-- 
2.47.3

Re: [PATCH 2/2] dt-bindings: display: simple-framebuffer: Document mono format

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/sysfb: missing DRM_FORMAT_R1 blit and clip support for mono format
- [Medium] dt-bindings: display: simple-framebuffer: underspecified mono bit order
--

commit 20af76c182c22fb42d62d508852f5a56db4c14ee
Author: Sebastian Raase <linux@sraa.de>

dt-bindings: display: simple-framebuffer: Document mono format

This commit documents the mono format in the simple-framebuffer Device Tree
binding as a 1-bit packed pixel monochrome format with 8 pixels per byte.

> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> index 45ffdebc9d86b..c627ec088434e 100644
> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> @@ -94,6 +94,7 @@ properties:
>    format:
>      description: >
>        Format of the framebuffer:
> +        * `mono` - 1-bit packed pixels, 8 pixels/byte

[Severity: Medium]
Should the binding description specify both the pixel polarity and the
intra-byte bit ordering for mono, similar to how the other formats define
their bit-to-channel mappings?

In include/linux/platform_data/simplefb.h, SIMPLEFB_FORMATS maps mono to
DRM_FORMAT_R1:

include/linux/platform_data/simplefb.h:
    #define SIMPLEFB_FORMATS \
    { \
        { "mono", 1, {0, 1}, {0, 1}, {0, 1}, {0, 0}, DRM_FORMAT_R1 }, \

In include/uapi/drm/drm_fourcc.h, DRM_FORMAT_R1 is defined with the leftmost
pixel in bit 7 (MSB) and the rightmost pixel in bit 0 (LSB).

However, in drivers/video/fbdev/simplefb.c, simplefb_probe() sets
info->fix.visual = FB_VISUAL_MONO10 and uses FB_DEFAULT_IOMEM_OPS without
setting FBINFO_BE_MATH or FB_NONSTD_REV_PIX_IN_B:

drivers/video/fbdev/simplefb.c:simplefb_probe() {
    ...
	if (params.format->bits_per_pixel == 1)
		info->fix.visual = FB_VISUAL_MONO10;
	else
		info->fix.visual = FB_VISUAL_TRUECOLOR;
    ...
}

Because fb_reverse_init() in drivers/video/fbdev/core/fb_draw.h places the
leftmost pixel at bit 0 (LSB) on little-endian CPUs and at bit 7 (MSB) on
big-endian CPUs unless reversed, does simplefb end up using a CPU-endian
dependent pixel bit order that is reversed relative to DRM_FORMAT_R1 on
little-endian systems?

>          * `a1r5g5b5` - 16-bit pixels, d[15]=a, d[14:10]=r, d[9:5]=g, d[4:0]=b
>          * `a2r10g10b10` - 32-bit pixels, d[31:30]=a, d[29:20]=r, d[19:10]=g, d[9:0]=b
>          * `a8b8g8r8` - 32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r
> @@ -106,6 +107,7 @@ properties:
>          * `x8r8g8b8` - 32-bit pixels, d[23:16]=r, d[15:8]=g, d[7:0]=b
>          * `x8b8g8r8` - 32-bit pixels, d[23:16]=b, d[15:8]=g, d[7:0]=r
>      enum:
> +      - mono

[Severity: High]
How does simpledrm handle a simple-framebuffer Device Tree node with
format = "mono" when CONFIG_DRM_SIMPLEDRM is enabled?

Both simplefb and simpledrm bind to simple-framebuffer nodes and validate the
format property against SIMPLEFB_FORMATS:

drivers/gpu/drm/sysfb/simpledrm.c:
    simpledrm_probe() ->
        simpledrm_device_create() ->
            simplefb_get_validated_format()

When simplefb_get_validated_format() accepts mono as DRM_FORMAT_R1,
simpledrm claims the framebuffer aperture and registers a DRM device whose
primary plane advertises DRM_FORMAT_R1 and emulated DRM_FORMAT_XRGB8888 via
drm_sysfb_build_fourcc_list().

During drm_client_setup(), drm_fbdev_client_setup() requests color_mode = 1,
which maps via drm_mode_legacy_fb_format(1, 1) to DRM_FORMAT_C1 rather than
DRM_FORMAT_R1, so __drm_fb_helper_find_sizes() falls back to
DRM_FORMAT_XRGB8888.

When the fbdev client or an XRGB8888 userspace compositor commits a
framebuffer, drm_sysfb_plane_helper_begin_fb_access() calls
drm_sysfb_get_blit_func(DRM_FORMAT_R1, DRM_FORMAT_XRGB8888):

drivers/gpu/drm/sysfb/drm_sysfb_modeset.c:drm_sysfb_get_blit_func() {
	if (src_format == dst_format) {
		return drm_sysfb_memcpy;
	} else if (src_format == DRM_FORMAT_XRGB8888) {
		switch (dst_format) {
		case DRM_FORMAT_RGB565:
		...
		}
	}
	return NULL;
}

Since drm_sysfb_get_blit_func() has no case for DRM_FORMAT_R1, won't
drm_sysfb_plane_helper_begin_fb_access() warn and fail every XRGB8888 atomic
commit with -EINVAL?

Additionally, if a client commits a native DRM_FORMAT_R1 framebuffer with a
partial damage clip where x1 > 0, drm_sysfb_plane_helper_atomic_update()
calculates the destination offset using drm_fb_clip_offset():

drivers/gpu/drm/sysfb/drm_sysfb_modeset.c:drm_sysfb_plane_helper_atomic_update() {
    ...
		iosys_map_incr(&dst, drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip));
		blit_to_crtc(&dst, &dst_pitch, shadow_plane_state->data, fb, &damage,
			     &shadow_plane_state->fmtcnv_state);
    ...
}

Because dst_format->cpp[0] is 0 for DRM_FORMAT_R1 while drm_fb_memcpy() uses
DIV_ROUND_UP(bpp, 8) = 1 byte per pixel for the source clip offset, wouldn't
partial damage updates also miscalculate horizontal offsets?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793758480.git.linux@sraa.de?part=2