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

Re: [PATCH v2] 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:
- [High] drm/msm/dpu: unbalanced frame_pending decrements in clone mode
--

commit 8cc8441c63a33addc5000cc2ffb1005c0a8221b2
Author: Michael Burch <me@michaelburch.net>

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

This commit moves the frame_pending counter increment to precede the encoder
kickoff to fix an issue where a frame completing quickly could miss the
increment. It introduces dpu_crtc_count_pending_frame() and calls it before
kickoffs in both clone mode and normal mode.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 42d0a529b4d51..81282a8a5d9d1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
[ ... ]
> @@ -1062,6 +1075,8 @@ static int dpu_crtc_kickoff_clone_mode(struct drm_crtc *crtc)
>  
>  	dpu_vbif_clear_errors(dpu_kms);
>  
> +	dpu_crtc_count_pending_frame(crtc);
> +
>  	/*
>  	 * Kickoff real time encoder last as it's the encoder that
>  	 * will do the flush
>  	 */
>  	dpu_encoder_kickoff(wb_encoder);
>  	dpu_encoder_kickoff(rt_encoder);

[Severity: High]
Does this code introduce an unbalanced decrement of frame_pending?

In dpu_crtc_kickoff_clone_mode(), dpu_crtc_count_pending_frame() increments
frame_pending once per CRTC commit. However, the function subsequently kicks
off both wb_encoder and rt_encoder independently.

When they complete, both encoders independently trigger
dpu_encoder_frame_done_callback(), queueing two separate
DPU_ENCODER_FRAME_EVENT_DONE events to the CRTC:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_frame_done_callback() {
	if (!dpu_enc->frame_busy_mask[0]) {
		...
		dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_FRAME_DONE);
		if (dpu_enc->crtc)
			dpu_crtc_frame_event_cb(dpu_enc->crtc, event);
	}
}

The worker then drops the counter on each DONE event:

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_frame_event_work() {
	...
	} else if (atomic_dec_return(&dpu_crtc->frame_pending) == 0) {
		/* release bandwidth and other resources */
		trace_dpu_crtc_frame_event_done(DRMID(crtc), fevent->event);
		dpu_core_perf_crtc_release_bw(crtc);
	}
	...
}

If multiple clone-mode commits are interleaved, could the second event from an
earlier commit arrive after the next commit sets frame_pending to 1?

This would cause the worker to decrement the count to 0 and release
system memory bandwidth prematurely before the real-time encoder has actually
finished rendering the next commit. This might lead to FIFO underflows,
visual artifacts, or display hangs.

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