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 <[email protected]>
> ---
> 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
