Re: [PATCH v2 3/8] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition in atomic commit causing display hang in oneshot mode due to premature hardware start.
--

--- Patch [3]: [PATCH v2 3/8] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 24a767bb6a2c4adfefe9e0a5de7e9c8bbe7b2a64
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers

Convert MCDE to explicit plane, CRTC and encoder objects. Handle pending
page-flip events from the CRTC atomic flush path using the commit-local
CRTC state.

> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 257a6e84dd583..86edf94c52372 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
[ ... ]
> @@ -1381,44 +1398,13 @@ static void mcde_set_extsrc(struct mcde *mcde, u32 buffer_address)
> -static void mcde_display_update(struct drm_simple_display_pipe *pipe,
> -				struct drm_plane_state *old_pstate)
> +static void mcde_plane_helper_atomic_update(struct drm_plane *plane,
> +					    struct drm_atomic_commit *commit)
>  {
> -	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_device *drm = crtc->dev;
> -	struct mcde *mcde = to_mcde(drm);
> -	struct drm_pending_vblank_event *event = crtc->state->event;
> -	struct drm_plane *plane = &pipe->plane;
> -	struct drm_plane_state *pstate = plane->state;
> +	struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, plane);
> +	struct mcde *mcde = to_mcde(plane->dev);
>  	struct drm_framebuffer *fb = pstate->fb;
>  
> -	/*
> -	 * Handle any pending event first, we need to arm the vblank
> -	 * interrupt before sending any update to the display so we don't
> -	 * miss the interrupt.
> -	 */
> -	if (event) {
[ ... ]
> -		spin_unlock_irq(&crtc->dev->event_lock);
> -	}
> -
>  	/*
>  	 * We do not start sending framebuffer updates before the
>  	 * display is enabled. Update events will however be dispatched
[ ... ]
> -static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe)
> +static void mcde_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> +					  struct drm_atomic_commit *commit)
> +{
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> +	struct drm_pending_vblank_event *event = crtc_state->event;
> +
> +	/*
> +	 * Handle any pending event first, we need to arm the vblank
> +	 * interrupt before sending any update to the display so we don't
> +	 * miss the interrupt.
> +	 */
> +	if (!event)
> +		return;

[Severity: High]
Does this split introduce a race condition that could hang the pipeline in
oneshot mode?

Because the DRM core executes the plane's atomic_update callback before the
CRTC's atomic_flush callback, the hardware flow is now started before the
vblank event is armed.

If mcde_start_flow() is triggered during mcde_plane_helper_atomic_update()
(which happens just below the quoted block), it could complete the hardware
transfer and fire its interrupt before the vblank event gets armed later in
mcde_crtc_helper_atomic_flush().

The relocated comment in atomic_flush still notes this exact requirement:
"we need to arm the vblank interrupt before sending any update to the
display so we don't miss the interrupt."

If the interrupt fires early in oneshot mode, it will be missed, and the
atomic commit could wait indefinitely for an event that will never complete.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=3