Re: [PATCH 6/9] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition and potential NULL pointer dereference due to direct access to `plane->state` and missing visibility checks in atomic callbacks.

Pre-existing issues:
- [High] Sleeping in atomic context within CRTC and plane atomic callbacks.
--

commit a10b3d5781e3b7143f432c86643ef47e8ddc3a9a
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers

Convert gm12u320 to direct primary plane, CRTC and encoder setup.
Keep shadow-plane helper state, framebuffer access helpers and
no-scaling plane-state check from simple-KMS path.

> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 4ad074337af07..b476793d0401e 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c

[ ... ]

> +static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					       struct drm_atomic_commit *commit)
>  {
>  	struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
> -	struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> +	struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
> +	struct drm_plane_state *plane_state = crtc->primary->state;

[Severity: High]
Will this cause a NULL pointer dereference if a subsequent commit disables
the plane?

Because non-blocking commits execute asynchronously, crtc->primary->state
reflects the state of the most recently queued commit, rather than the
commit currently executing. If a fast subsequent commit disables the plane,
plane_state->fb could be NULL.

Should this use drm_atomic_get_new_plane_state(commit, crtc->primary) to
get the state tied to this specific commit?

>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>  
>  	gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
>  	gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0], &rect);
>  }
>  
> -static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void gm12u320_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +						struct drm_atomic_commit *commit)
>  {
> -	struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> +	struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
>  
>  	gm12u320_stop_fb_update(gm12u320);

[Severity: High]
This isn't a bug introduced by this patch, but does gm12u320_stop_fb_update()
sleep in atomic context?

gm12u320_stop_fb_update() calls cancel_delayed_work_sync() and mutex_lock().
Since CRTC atomic disable callbacks can run in atomic context during
non-blocking commits, sleeping here is forbidden and could cause kernel
warnings or deadlocks.

>  }
>  
> -static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
> -				 struct drm_plane_state *old_state)
> +static void gm12u320_plane_helper_atomic_update(struct drm_plane *plane,
> +						struct drm_atomic_commit *commit)
>  {
> -	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(commit, plane);
> +	struct drm_plane_state *state = plane->state;

[Severity: High]
Could accessing plane->state directly here lead to a crash?

Similar to the CRTC enable function, reading plane->state directly instead of
using drm_atomic_get_new_plane_state(commit, plane) accesses the future
state.

If a subsequent commit disables the plane, state->fb will be NULL. In this
scenario, drm_atomic_helper_damage_merged() returns true for plane
disablements, leading to gm12u320_fb_mark_dirty() being called with a NULL
fb, which will oops when trying to dereference fb->dev.

This wasn't possible before because the drm_simple_kms_helper explicitly
checked if (!plane->state->visible) before calling the update hook, but
that protection is dropped in this conversion.

> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>  	struct drm_rect rect;
>  
> -	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> +	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>  		gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], &rect);

[Severity: High]
This is a pre-existing issue, but does gm12u320_fb_mark_dirty() sleep in
atomic context?

It calls mutex_lock(&gm12u320->fb_update.lock), which is unsafe to call
from a plane update callback during non-blocking commits, as these run
in atomic context.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com?part=6