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 <[email protected]> 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/[email protected]?part=2
