Re: [PATCH 1/2] drm/sysfb: Use iosys_map_memset() to clear buffer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The atomic_disable callback uses the new plane state to determine the screen area to clear, which may be zeroed out and fail to clear the screen.
--

commit cdbf0de82b5bf761696ff2cc3c658a3d090a02c8
Author: Chen-Yu Tsai <wenst@chromium.org>

drm/sysfb: Use iosys_map_memset() to clear buffer

This patch updates the sysfb atomic disable routine to use
iosys_map_memset() with an offset, rather than directly extracting the
virtual address for memory clearing.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index d2de29caf89eb..99b13af4e391c 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> @@ -375,13 +375,11 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(dev);
> -	struct iosys_map dst = sysfb->fb_addr;
>  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
>  	unsigned int dst_pitch = sysfb->fb_pitch;
>  	const struct drm_format_info *dst_format = sysfb->fb_format;
>  	struct drm_rect dst_clip;
> -	unsigned long lines, linepixels, i;
> +	unsigned long lines, linepixels, i, offset;
>  	int idx;
>  
>  	drm_rect_init(&dst_clip,
>  		      plane_state->src_x >> 16, plane_state->src_y >> 16,
>  		      plane_state->src_w >> 16, plane_state->src_h >> 16);
>  
>  	lines = drm_rect_height(&dst_clip);
>  	linepixels = drm_rect_width(&dst_clip);

[Severity: High]
This isn't a bug introduced by this patch, but does using the new plane state
to calculate the clear area result in a failure to clear the screen?

Because drm_sysfb_plane_helper_atomic_disable() fetches the new plane state
above, it uses the state properties after the plane is disabled. If a
userspace display server zeroes out SRC_W and SRC_H when setting FB_ID = 0
to disable the plane, both lines and linepixels will evaluate to 0 here.

Could this cause the clearing loop to be completely skipped, leaving stale
framebuffer data visible on the screen?

Should this code use drm_atomic_get_old_plane_state() instead to ensure it
always correctly clears the area previously occupied by the plane?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903060651.3724163-1-wenst@chromium.org?part=1