Re: [PATCH] drm/vblank: Restart enabling vblank timer on possible deadlock

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

On Mon, Nov 02, 2026 at 03:38:52PM +0100, Thomas Zimmermann wrote:
> Return -EDEADLK when trying to cancel a running hrtimer as part of
> enable_vblank(). Reacquire vblank_time_lock and try again.
> 
> The vblank helper drm_vblank_disable_and_save() and the vblank IRQ
> handler hold the spin lock vblank_time_lock at some point. This
> can result in a deadlock if vblank timer's disable code in
> drm_crtc_vblank_cancel_timer() calls hrtimer_cancel() directly: the
> helper wais for the timer function to complete and the timer function
> waits for the helper to release vblank.time_lock.
> 
> drm_crtc_vblank_cancel_timer() works around this problem by calling
> hrtimer_try_to_cancel() instead. For re-enabling the vblank timer,
> drm_crtc_vblank_start_timer() must wait for the vblank timer to stop.
> It runs into the same deadlock problem while waiting for the timer
> function to complete.
> 
> The solution to this problem is to release and reacquire the spin
> lock vblank_time_lock if hrtimer_try_to_cancel() signals a running
> timer function. Releasing the lock allows for the timer function to
> make progress.
> 
> Signal the possible deadlock with -EDEADLK and perform the necessary
> steps. No other driver uses -EDEADLK in their enable_vblank, so the
> change should not interfere.
> 
> Tested with bochs on qemu.
> 
> Closes: https://lore.kernel.org/dri-devel/20260914111129.300691F00898@smtp.kernel.org/
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Louis Chauvet <louis.chauvet@bootlin.com>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.19+
> ---
> I also tried to move hrtimer_cancel() out of the critical section. The
> result was fairly fragile. So returning an error and re-acquiring the
> lock was the easier solution.
> ---
>  drivers/gpu/drm/drm_vblank.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 5162250e8813..135efde86298 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1182,10 +1182,12 @@ static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
>  static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
> -	int ret = 0;
> +	int ret;
>  
>  	assert_spin_locked(&dev->vbl_lock);
>  
> +retry:
> +	ret = 0;
>  	spin_lock(&dev->vblank_time_lock);
>  
>  	if (!vblank->enabled) {
> @@ -1199,7 +1201,11 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
>  		ret = __enable_vblank(dev, pipe);
>  		drm_dbg_core(dev, "enabling vblank on crtc %u, ret: %d\n",
>  			     pipe, ret);
> -		if (ret) {
> +		if (ret == -EDEADLK) {
> +			spin_unlock(&dev->vblank_time_lock);
> +			cpu_relax();
> +			goto retry;
> +		} else if (ret) {
>  			atomic_dec(&vblank->refcount);
>  		} else {
>  			drm_update_vblank_count(dev, pipe, 0);
> @@ -2232,11 +2238,14 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
>  			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>  	} else {
>  		/*
> -		 * Timer should not be active. If it is, wait for the
> -		 * previous cancel operations to finish.
> +		 * Timer should not be running. If it is, signal cancellation
> +		 * again and restart to release locks.
>  		 */
> -		while (hrtimer_active(&vtimer->timer))
> -			hrtimer_try_to_cancel(&vtimer->timer);

Why do you even have to cancel the timer here? Just let it run?

> +		if (hrtimer_try_to_cancel(&vtimer->timer) == -1) {
> +			scoped_guard(spinlock_irqsave, &vtimer->interval_lock)
> +				vtimer->interval = 0;

Does this interval_lock serve some actual purpose? Looks like it's just
protecting a single variable, and there are no read-modify-writes
happening to it that I can see.

> +			return -EDEADLK;
> +		}
>  	}
>  
>  	drm_calc_timestamping_constants(crtc, &crtc->mode);
> -- 
> 2.56.0

-- 
Ville Syrjälä
Intel