Re: [PATCH v3 03/14] drm/rockchip: vop2: Balance state on atomic_enable() error paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unhandled VBLANK event on `atomic_enable` error paths causes userspace to hang indefinitely and triggers a kernel warning.
--

--- Patch [3]: [PATCH v3 03/14] drm/rockchip: vop2: Balance state on atomic_enable() error paths ---
commit 852e494693bdb0b841a64b02cd2bceb48e307231
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

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

This commit attempts to fix resource leaks in the error paths of
vop2_crtc_atomic_enable() by properly decrementing enable_count and
unpreparing clocks. It also tracks the per-VP bring-up state to skip 
teardown in atomic_disable() and hardware programming in atomic_flush() 
when the enable phase does not complete successfully.

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 e11dc8c7f78ba..271c3cba6fb49 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -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]
This is a pre-existing issue, but does this code leave userspace hanging on
atomic_enable error paths?

Since this patch uses vp->enabled to skip hardware programming on failure,
what happens to the pending vblank event? If vop2_crtc_atomic_enable() fails,
it returns before calling drm_crtc_vblank_on().

However, immediately following the spin_lock_irq() in vop2_crtc_atomic_flush(),
the code unconditionally attempts to arm the event:

	if (crtc->state->event) {
		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
		drm_crtc_arm_vblank_event(crtc, crtc->state->event);
		crtc->state->event = NULL;
	}

Because vblank wasn't enabled during the failed bring-up,
drm_crtc_vblank_get() fails and triggers the WARN_ON. The event is armed but
never signaled because the disabled hardware generates no interrupts.

Should the driver consume the event directly here (e.g., using
drm_crtc_send_vblank_event) if vp->enabled is false to prevent the compositor
from blocking indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-dw-hdmi-qp-yuv-v3-0-a4a982a9f2e7@collabora.com?part=3