Re: [PATCH v8 04/27] drm/intel/display: Convert vblank event handling to 2-stage arming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Vblank events and workers execute one frame too early if vblank evasion occurs, causing premature page flip completions and potential use-after-free.
--

commit 4b140829cb62a6227f3079e278066d8cfb9ea1b4
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/intel/display: Convert vblank event handling to 2-stage arming

This converts the vblank functions to be called with interrupts disabled,
even on PREEMPT_RT kernels.

> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 365267c79e54f..6b71810803139 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -527,6 +527,10 @@ static void intel_crtc_vblank_work_init(struct intel_crtc_state *crtc_state)
>  
>  	drm_vblank_work_init(&crtc_state->vblank_work, &crtc->base,
>  			     intel_crtc_vblank_work);
> +
> +	drm_vblank_work_schedule_disabled(&crtc_state->vblank_work,
> +					  drm_crtc_accurate_vblank_count(&crtc->base) + 1);
> +
>  	/*
>  	 * Interrupt latency is critical for getting the vblank
>  	 * work executed as early as possible during the vblank.

[ ... ]

> @@ -607,6 +626,8 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
>  
>  	if (intel_crtc_needs_vblank_work(new_crtc_state))
>  		intel_crtc_vblank_work_init(new_crtc_state);
> +	else
> +		intel_crtc_arm_vblank_event(new_crtc_state);
>  
>  	if (state->base.legacy_cursor_update) {
>  		struct intel_plane *plane;

[ ... ]

> @@ -754,29 +758,10 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
>  	 * event outside of the critical section - the spinlock might spin for a
>  	 * while ... */
>  	if (intel_crtc_needs_vblank_work(new_crtc_state)) {
> -		drm_vblank_work_schedule(&new_crtc_state->vblank_work,
> -					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> -					 false);
> -	} else {
> -		intel_crtc_arm_vblank_event(new_crtc_state);
> -	}
> -
> -	if (state->base.legacy_cursor_update) {
> -		struct intel_plane *plane;
> -		struct intel_plane_state *old_plane_state;
> -		int i;
> -
> -		for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
> -			if (old_plane_state->hw.crtc == &crtc->base &&
> -			    old_plane_state->unpin_work.vblank) {
> -				drm_vblank_work_schedule(&old_plane_state->unpin_work,
> -							 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> -							 false);
> -
> -				/* Remove plane from atomic state, cleanup/free is done from vblank worker. */
> -				memset(&state->base.planes[i], 0, sizeof(state->base.planes[i]));
> -			}
> -		}
> +		drm_vblank_work_enable(&new_crtc_state->vblank_work);
> +	} else if (new_crtc_state->uapi.event) {
> +		drm_crtc_arm_prepared_vblank_event(new_crtc_state->uapi.event);
> +		new_crtc_state->uapi.event = NULL;
>  	}

[Severity: High]
If intel_vblank_evade() spins and the hardware vblank counter increments,
will the target sequence number calculated earlier in
intel_pipe_update_start() become stale?

Since intel_crtc_vblank_work_init() and intel_crtc_arm_vblank_event() now
run before evasion, the target sequence is calculated as N + 1. If evasion
occurs, the counter increments to N + 1, and the hardware will latch the
new state at N + 2.

When drm_vblank_work_enable() or drm_crtc_arm_prepared_vblank_event() are
called here, they are still targeted for N + 1. Could this result in the
event firing immediately and signaling completion one frame before the
hardware actually latches the new display state, potentially leading to
premature freeing of the old framebuffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615201846.307297-1-dev@lankhorst.se?part=4