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

[sashiko-bot@kernel.org]

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