Re: [PATCH] drm/msm/dpu: count the pending frame before kicking it off

Re: [PATCH] drm/msm/dpu: count the pending frame before kicking it off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dpu: bandwidth leak due to missed frame_pending 0-transition in error path
--

commit 09d9db34ac4a7e898d02571dc93cd542a14a8979
Author: Michael Burch <me@michaelburch.net>

drm/msm/dpu: count the pending frame before kicking it off

This commit moves the increment of frame_pending in dpu_crtc_commit_kickoff()
to occur before kickoff to prevent event worker races. It also adds a rollback
decrement in the clone mode error path.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 42d0a529b4d51..c8bd9c305a59a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
[ ... ]
> @@ -1105,9 +1105,19 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  		}
>  	}
>  
> +	/* The frame-done event may be handled before the kickoff below returns. */
> +	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
> +		/* acquire bandwidth and other resources */
> +		DRM_DEBUG_ATOMIC("crtc%d first commit\n", crtc->base.id);
> +	} else {
> +		DRM_DEBUG_ATOMIC("crtc%d commit\n", crtc->base.id);
> +	}
> +
>  	if (drm_crtc_in_clone_mode(crtc->state)) {
> -		if (dpu_crtc_kickoff_clone_mode(crtc))
> +		if (dpu_crtc_kickoff_clone_mode(crtc)) {
> +			atomic_dec(&dpu_crtc->frame_pending);

[Severity: Medium]
Does this unconditional atomic_dec() bypass the 0-transition check required to
release CRTC bandwidth, causing a bandwidth leak?

If Frame N-1 is executing and Frame N is committed, dpu_crtc_commit_kickoff()
bumps frame_pending to 2. If dpu_crtc_kickoff_clone_mode() fails, it
decrements the count back.

If the hardware interrupt for Frame N-1 fires concurrently and its event worker
runs, it will decrement the count to 1, skipping the
dpu_core_perf_crtc_release_bw() call that requires a 1-to-0 transition.

The error path then blindly decrements the count to 0 with atomic_dec() and
also skips the dpu_core_perf_crtc_release_bw() call, permanently leaking the
allocated bandwidth until the CRTC is fully disabled.

>  			goto end;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013145007.13111-1-me@michaelburch.net?part=1