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

[sashiko-bot@kernel.org]

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