Re: [PATCH v6 2/6] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `fmtcnv_state` buffer reservation in `ssd133x_primary_plane_atomic_check` causes a `GFP_KERNEL` allocation in atomic context.
--

commit 58733b43936278b0113cbbd363c279ece2464617
Author: Amit Barzilai <amit.barzilai22@gmail.com>

drm/ssd130x: Change SSD133X color format to RGB565 from RGB332

SSD133X screens were driven at 8bpp RGB332 despite supporting 16bpp RGB565.
Switch the SSD133X data path to RGB565.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index feec4933e3e6b..cbf77a74c4fa0 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
[ ... ]
> @@ -978,17 +990,18 @@ static int ssd133x_fb_blit_rect(struct drm_framebuffer *fb,
>  				struct drm_format_conv_state *fmtcnv_state)
>  {
>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> -	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB332);
> +	const struct drm_format_info *fi;
>  	unsigned int dst_pitch;
>  	struct iosys_map dst;
>  
> +	fi = drm_format_info(DRM_FORMAT_RGB565);
>  	if (!fi)
>  		return -EINVAL;
>  
>  	dst_pitch = drm_format_info_min_pitch(fi, 0, drm_rect_width(rect));
>  
>  	iosys_map_set_vaddr(&dst, data_array);
> -	drm_fb_xrgb8888_to_rgb332(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
> +	drm_fb_xrgb8888_to_rgb565be(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);

[Severity: High]
This is a pre-existing issue, but does this code cause a GFP_KERNEL allocation
in atomic context?

When the screen is updated, ssd133x_primary_plane_atomic_update() (which runs
in atomic context) calls ssd133x_fb_blit_rect() which in turn calls
drm_fb_xrgb8888_to_rgb565be().

This internally calls __drm_fb_xfrm(), which attempts a GFP_KERNEL allocation
if the conversion buffer was not previously reserved:

drivers/gpu/drm/drm_format_helper.c:__drm_fb_xfrm() {
	if (!vaddr_cached_hint) {
		stmp = drm_format_conv_state_reserve(state, sbuf_len, GFP_KERNEL);
		if (!stmp)
			return -ENOMEM;
	}
}

Because ssd133x_primary_plane_atomic_check() omits the
drm_format_conv_state_reserve() call (unlike its ssd130x and ssd132x variants):

drivers/gpu/drm/solomon/ssd130x.c:ssd133x_primary_plane_atomic_check() {
	...
	else if (!plane_state->visible)
		return 0;

	return 0;
}

Will this cause the allocation to be unconditionally executed in atomic context
on the first frame update?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914085931.66857-1-amit.barzilai22@gmail.com?part=2