Re: [PATCH v6 04/15] drm/rockchip: vop2: Balance state on atomic_enable() error paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/rockchip: vop2: plane left enabled in vop2_plane_atomic_update()
- [High] drm/rockchip: vop2: unhandled vblank event in vop2_crtc_atomic_flush()
--

commit ad40a8e305ba6e768ae3478a34b5ddce63b79357
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Balance state on atomic_enable() error paths

This commit cleans up clocks, vop2->enable_count, and PM runtime state on
vop2_crtc_atomic_enable() failure paths and introduces a per-video-port
enabled flag so atomic commit callbacks skip hardware programming and
teardown when a CRTC failed to enable.

Link: https://lore.kernel.org/all/20260623202043.8BFE21F000E9@smtp.kernel.org/

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index bc14ca9d598dc..8d5613531acb9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -1197,13 +1203,21 @@ static void vop2_plane_atomic_disable(struct drm_plane *plane,
>  	struct drm_plane_state *old_pstate = NULL;
>  	struct vop2_win *win = to_vop2_win(plane);
>  	struct vop2 *vop2 = win->vop2;
> +	struct vop2_video_port *vp;
>  
>  	drm_dbg(vop2->drm, "%s disable\n", win->data->name);
>  
>  	if (state)
>  		old_pstate = drm_atomic_get_old_plane_state(state, plane);
> -	if (old_pstate && !old_pstate->crtc)
> -		return;
> +
> +	if (old_pstate) {
> +		if (!old_pstate->crtc)
> +			return;
> +
> +		vp = to_vop2_video_port(old_pstate->crtc);
> +		if (!vp->enabled)
> +			return;
> +	}
>  
>  	vop2_win_disable(win);
>  	vop2_win_write(win, VOP2_WIN_YUV_CLIP, 0);
> @@ -1298,6 +1312,9 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  	if (WARN_ON(!crtc))
>  		return;
>  
> +	if (!vp->enabled)
> +		return;
> +
>  	if (!pstate->visible) {
>  		vop2_plane_atomic_disable(plane, state);
>  		return;

[Severity: High]
What happens if an active plane is migrated from an enabled CRTC to another
CRTC whose vop2_crtc_atomic_enable() failed?

When a plane moves between CRTCs, drm_atomic_helper_commit_planes() sees
new_plane_state->crtc != NULL and plane_crtc_active(new_plane_state) == true,
so it calls vop2_plane_atomic_update() rather than
vop2_plane_atomic_disable(). Meanwhile, on the source CRTC,
vop2_crtc_atomic_begin() -> setup_overlay() only iterates over planes in the
source CRTC's new plane_mask and does not disable the migrated window.

In vop2_plane_atomic_update(), vp is derived from the destination CRTC
(pstate->crtc). Returning early when !vp->enabled skips
vop2_plane_atomic_disable(plane, state) and leaves VOP2_WIN_ENABLE set on the
source CRTC while drm_atomic_helper_cleanup_planes() unmaps and releases
old_plane_state->fb at the end of the commit tail, allowing the hardware
window to continue DMA scanout from the old framebuffer.

Since vop2_plane_atomic_disable() already checks old_pstate->crtc and
whether the old video port is enabled before touching hardware registers,
should vop2_plane_atomic_update() call vop2_plane_atomic_disable(plane, state)
when !vp->enabled, just as it does when !pstate->visible?

[ ... ]

> @@ -2034,13 +2066,19 @@ static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
>  	struct vop2_video_port *vp = to_vop2_video_port(crtc);
>  	struct vop2 *vop2 = vp->vop2;
>  
> -	/* In case of modeset, gamma lut update already happened in atomic enable */
> -	if (!drm_atomic_crtc_needs_modeset(crtc_state) && crtc_state->color_mgmt_changed)
> -		vop2_crtc_atomic_try_set_gamma_locked(vop2, vp, crtc, crtc_state);
> +	if (vp->enabled) {
> +		/*
> +		 * In case of modeset, gamma lut update already happened
> +		 * in atomic enable.
> +		 */
> +		if (!drm_atomic_crtc_needs_modeset(crtc_state) &&
> +		    crtc_state->color_mgmt_changed)
> +			vop2_crtc_atomic_try_set_gamma_locked(vop2, vp, crtc, crtc_state);
>  
> -	vop2_post_config(crtc, false, crtc_state, old_crtc_state);
> +		vop2_post_config(crtc, false, crtc_state, old_crtc_state);
>  
> -	vop2_cfg_done(vp);
> +		vop2_cfg_done(vp);
> +	}
>  
>  	spin_lock_irq(&crtc->dev->event_lock);

[Severity: High]
As reported by Sashiko on v2 [1], how is crtc->state->event handled in
vop2_crtc_atomic_flush() when vop2_crtc_atomic_enable() fails and leaves
vp->enabled as false?

When vop2_crtc_atomic_enable() aborts on any error path, it returns before
calling drm_crtc_vblank_on(crtc). Later in the same commit tail,
drm_atomic_helper_commit_planes() still calls vop2_crtc_atomic_flush(), which
runs:

	spin_lock_irq(&crtc->dev->event_lock);

	if (crtc->state->event) {
		WARN_ON(drm_crtc_vblank_get(crtc));
		vp->event = crtc->state->event;
		crtc->state->event = NULL;
	}

	spin_unlock_irq(&crtc->dev->event_lock);

Because drm_crtc_vblank_on(crtc) was never called, drm_crtc_vblank_get(crtc)
fails with -EINVAL and triggers the WARN_ON(). The event is then moved into
vp->event, but because the video port is not running and vop2_cfg_done(vp)
was skipped, the frame start interrupt never fires to complete vp->event, and
vop2_crtc_atomic_disable() only checks crtc->state->event while skipping
teardown when !vp->enabled.

Does this leave commit->flip_done unsignaled (stalling subsequent commits on
this CRTC in drm_atomic_helper_wait_for_dependencies()) and leak vp->event if
another commit flushes before the CRTC is enabled, or cause an unbalanced
drm_crtc_vblank_put() in the interrupt handler if a later modeset succeeds?
When !vp->enabled, should vop2_crtc_atomic_flush() send crtc->state->event
immediately via drm_crtc_send_vblank_event() instead of queuing it in
vp->event?

[1] https://lore.kernel.org/r/20260706194935.AC7601F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-dw-hdmi-qp-yuv-v6-0-1bb63ef5766e@collabora.com?part=4