Re: [PATCH v2 4/7] drm/i915/display: Remove locking from intel_vblank_evade critical section

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 04, 2025 at 09:36:28AM +0100, Maarten Lankhorst wrote:
> finish_wait() may take a lock, which means that it can take any amount
> of time. On PREEMPT-RT we should not be taking any lock after disabling
> preemption, so ensure that the completion is done before disabling
> interrupts.
> 
> This also has the benefit of making vblank evasion more deterministic,
> by performing the final vblank check after all locking is done.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 2b106ffa3f5f5..3628d2a1b8f38 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
>  		evade->min -= vblank_delay;
>  }
>  
> +static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade, int *scanline)

The name is confusing. But having a function
would be nice since we need two checks I guess.
scanline_is_safe() or something?

Also type should be bool, and inline looks pointless.

> +{
> +	*scanline = intel_get_crtc_scanline(evade->crtc);
> +
> +	return *scanline < evade->min || *scanline > evade->max;
> +}
> +
>  /* must be called with vblank interrupt already enabled! */
>  int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  {
> @@ -715,23 +722,22 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  	struct intel_display *display = to_intel_display(crtc);
>  	long timeout = msecs_to_jiffies_timeout(1);
>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> -	DEFINE_WAIT(wait);
>  	int scanline;
>  
>  	if (evade->min <= 0 || evade->max <= 0)
>  		return 0;
>  
> -	for (;;) {
> -		/*
> -		 * prepare_to_wait() has a memory barrier, which guarantees
> -		 * other CPUs can see the task state update by the time we
> -		 * read the scanline.
> -		 */
> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +	while (!vblank_evadable(evade, &scanline)) {
> +		local_irq_enable();
>  
> -		scanline = intel_get_crtc_scanline(crtc);
> -		if (scanline < evade->min || scanline > evade->max)
> -			break;
> +		DEFINE_WAIT(wait);
> +		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
> +			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +			timeout = schedule_timeout(timeout);
> +		}
> +		finish_wait(wq, &wait);
> +
> +		local_irq_disable();
>  
>  		if (!timeout) {

This looks to introduce the classic "didn't check the
condition after timeout" race.

I guess what you're going for is something like this (done
through a somewhat less intrusive reordering of the current
code):

for (;;) {
	if (scanline_is_safe(evade, &scanline))
		break;

	if (!timeout) {
		drm_dbg_kms(display->drm,
			    "Potential atomic update failure on pipe %c\n",
			    pipe_name(crtc->pipe));
		break;
	}

	local_irq_enable();

	prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);

	if (!scanline_is_safe(evade, &scanline))
		timeout = schedule_timeout(timeout);

	finish_wait(wq, &wait);

	local_irq_disable();
}

And then maybe the whole prepare+wait+finish thing there
could be a simple wait_event_timeout(). That would make
that inner thing a loop though. We might not want that
just because jiffies is so coarse and we don't really
want to wait multiple times there.

>  			drm_dbg_kms(display->drm,
> @@ -740,15 +746,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  			break;
>  		}
>  
> -		local_irq_enable();
> -
> -		timeout = schedule_timeout(timeout);
> -
> -		local_irq_disable();
>  	}
>  
> -	finish_wait(wq, &wait);
> -
>  	/*
>  	 * On VLV/CHV DSI the scanline counter would appear to
>  	 * increment approx. 1/3 of a scanline before start of vblank.
> -- 
> 2.51.0

-- 
Ville Syrjälä
Intel