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

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

Pre-existing issues:
- [High] Missing or undersized memory reservations for format conversion state in atomic check leading to atomic context violation.
- [High] Display hardware state desynchronization causing incomplete screen clears.
- [High] Uninitialized kernel memory disclosure to display panel due to silent conversion failure.
--

--- Patch [2]: [PATCH v4 2/6] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332 ---
commit 8cc727648aa06ffb4256eb0ad7ba43c6e84be6d7
Author: Amit Barzilai <[email protected]>

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 0b0fc6fe3df23..1110aa63d0666 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

[ ... ]

> @@ -909,7 +920,7 @@ static void ssd132x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
>  
>  static void ssd133x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
>  {
> -	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB332);
> +	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB565);
>  	unsigned int pitch;

[Severity: High]
This isn't a bug introduced by this patch, but does ssd133x_clear_screen() (and 
ssd132x_clear_screen()) clear the entire screen properly? 

Since the hardware update window (COL_RANGE and ROW_RANGE registers) is 
not reset before writing the full-screen zero payload, won't the hardware 
just wrap the zeroed payload within whatever stale damage rectangle was 
last configured, leaving the rest of the panel uncleared?
 
[ ... ]

> @@ -978,7 +989,7 @@ 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 = drm_format_info(DRM_FORMAT_RGB565);
>  	unsigned int dst_pitch;
>  	struct iosys_map dst;
>  
> @@ -988,7 +999,7 @@ static int ssd133x_fb_blit_rect(struct drm_framebuffer *fb,
>  	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);
>  
>  	ssd133x_update_rect(ssd130x, rect, data_array, dst_pitch);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to unconditionally write the
data_array buffer to the hardware here?

In ssd133x_crtc_atomic_check(), data_array is allocated with kmalloc_array() 
which does not zero-initialize the memory. If drm_fb_xrgb8888_to_rgb565be()
fails and silently aborts, won't ssd133x_update_rect() then push 
uninitialized kernel heap memory directly to the physical display?

[ ... ]

> @@ -1446,7 +1457,7 @@ static int ssd133x_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
>  	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  	struct ssd130x_crtc_state *ssd130x_state = to_ssd130x_crtc_state(crtc_state);
> -	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB332);
> +	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB565);
>  	unsigned int pitch;
>  	int ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this driver correctly manage memory 
reservations for format conversion to avoid atomic context violations?

Looking at ssd133x_primary_plane_atomic_check(), it completely omits a
call to drm_format_conv_state_reserve(). If the format conversion needs
intermediate buffers during the atomic update, it will dynamically provision
them using kmalloc(..., GFP_KERNEL) since the reservation is missing.

Similarly, ssd130x_primary_plane_atomic_check() and
ssd132x_primary_plane_atomic_check() seem to reserve undersized buffers 
using the destination pitch rather than the source/conversion line size 
requirement:

    buf = drm_format_conv_state_reserve(&shadow_plane_state->fmtcnv_state,
                                        pitch, GFP_KERNEL);

Won't these missing or undersized reservations cause sleeping functions to
be invoked during the non-blocking commit phase, violating atomic context
invariants?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2