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 05:48:23PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.11.26 um 16:11 schrieb Ville Syrjälä:
> > 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?
> 
> We ran hrtimer_try_to_cancel() in drm_crtc_vblank_cancel_timer() to 
> remove the vblank timer. When we later come here, we need to know if the 
> timer has meanwhile been fully canceled. Hence we need at least one call 
> to hrtimer_try_to_cancel() even if the timer is in some bogus state 
> where it should not be active. The loop here was nonsense and could 
> deadlock. In the new code, we still need that call to 
> hrtimer_try_to_cancel() to detect the possible deadlock.
> 
> I'm not confident that we can set up the hrtimer with new values while 
> it is in this half-canceled state, can we?

Shrug.

But what I'm thinking is:
- stop with the explicit cancel entirely and just rely on the
  timer cancelling itself
- try to start the timer in vblank_enable(), and if that can somehow
  fail because the timer is still running it should be fine because
  the self cancellation should anyway have been aborted
- explicitly wait for the timer to have stoppped in drm_vblank_off()
  to make sure the wrong interval can't leak from one mode to
  another

-- 
Ville Syrjälä
Intel