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

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

[Thomas Zimmermann]

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);
+		if (hrtimer_try_to_cancel(&vtimer->timer) == -1) {
+			scoped_guard(spinlock_irqsave, &vtimer->interval_lock)
+				vtimer->interval = 0;
+			return -EDEADLK;
+		}
 	}
 
 	drm_calc_timestamping_constants(crtc, &crtc->mode);
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/vblank: deadlock in drm_vblank_enable() retry when outer locks are held
--

commit 0a0f76a052c65f5a2ede08f67964f40f4ca7f5fc
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/vblank: Restart enabling vblank timer on possible deadlock

This commit updates drm_crtc_vblank_start_timer() to return -EDEADLK when
hrtimer_try_to_cancel() indicates the vblank hrtimer callback is currently
running, and updates drm_vblank_enable() to drop and reacquire
vblank_time_lock before retrying.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e423..5a4c11e57484c 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;

[Severity: High]
Can this retry loop still deadlock when outer locks are held across
drm_vblank_enable()?

When drm_crtc_vblank_start_timer() returns -EDEADLK because
drm_vblank_timer_function() is currently executing on another CPU,
drm_vblank_enable() only releases dev->vblank_time_lock while continuing to
hold dev->vbl_lock and any caller-held locks.

However, the running timer callback drm_vblank_timer_function() calls into
drm_handle_vblank(), which acquires dev->event_lock before
dev->vblank_time_lock, and can also call vblank_disable_fn(), which acquires
dev->vbl_lock:

drivers/gpu/drm/drm_vblank.c:drm_handle_vblank() {
	...
	spin_lock_irqsave(&dev->event_lock, irqflags);
	spin_lock(&dev->vblank_time_lock);
	...
	if (disable_irq)
		vblank_disable_fn(&vblank->disable_timer);
}

In addition, vkms_crtc_handle_vblank_timeout() acquires vkms_output->lock
before calling drm_crtc_handle_vblank():

drivers/gpu/drm/vkms/vkms_crtc.c:vkms_crtc_handle_vblank_timeout() {
	...
	spin_lock(&output->lock);
	ret = drm_crtc_handle_vblank(crtc);
	...
}

Callers that enable vblank during atomic flush or work scheduling hold these
outer locks before calling drm_crtc_vblank_get() -> drm_vblank_get() ->
drm_vblank_enable(). For example, drm_crtc_vblank_atomic_flush() holds
dev->event_lock:

drivers/gpu/drm/drm_vblank_helper.c:drm_crtc_vblank_atomic_flush() {
	...
	spin_lock_irq(&dev->event_lock);
	...
	if (event) {
		if (drm_crtc_vblank_get(crtc) == 0)
			drm_crtc_arm_vblank_event(crtc, event);
	...
	spin_unlock_irq(&dev->event_lock);
}

Similarly, vkms_crtc_atomic_begin() and vkms_crtc_atomic_flush() hold both
vkms_output->lock and dev->event_lock across drm_crtc_vblank_get(), and
drm_vblank_get() always holds dev->vbl_lock across drm_vblank_enable().

If drm_vblank_timer_function() already read a non-zero vtimer->interval before
a prior disable cleared it, and is now waiting on vkms_output->lock,
dev->event_lock, or dev->vbl_lock, won't CPU A spin forever in the retry loop
in drm_vblank_enable() while holding the very locks that the timer callback on
CPU B needs to finish?

Also, because vtimer->timer is set up with HRTIMER_MODE_REL rather than
HRTIMER_MODE_REL_HARD, on CONFIG_PREEMPT_RT the timer callback runs in softirq
context. If a task calling drm_vblank_enable() preempts the timer callback on
the same CPU and spins with cpu_relax() while holding spinlocks, can the
preempted timer callback ever complete?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102143919.1672293-1-tzimmermann@suse.de?part=1

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

[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?

> +		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

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

[Thomas Zimmermann]

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?


>
>> +		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.

Setting vtimer->interval to zero will signal any non-canceled timer to 
cancel itself immediately. [1]  Either hrtimer_try_to_cancel() is 
successful or the next run of the timer function will cacnel itself.  We 
do this because we cannot cancel the timer directl with hrtimer_cancel() 
due to the possible deadlock.

[1] 
https://elixir.bootlin.com/linux/v7.3/source/drivers/gpu/drm/drm_vblank.c#L2191

Best regards
Thomas


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

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)

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

[Ville Syrjälä]

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

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

[Thomas Zimmermann]

Hi

Am 02.11.26 um 18:15 schrieb Ville Syrjälä:
[...]
> But what I'm thinking is:

I've ran a lengthy session to explore various solutions.

> - 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

Having an old vblank timer still running while enable_vblank sets up a 
new one can mess up the hrtimer's internal ordering. It's full of subtle 
bugs, according to my LLM.


> - 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

I'll soon send out a series implementing this idea.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)

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

[Ville Syrjälä]

On Tue, Nov 03, 2026 at 04:55:34PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.11.26 um 18:15 schrieb Ville Syrjälä:
> [...]
> > But what I'm thinking is:
> 
> I've ran a lengthy session to explore various solutions.
> 
> > - 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
> 
> Having an old vblank timer still running while enable_vblank sets up a 
> new one can mess up the hrtimer's internal ordering. It's full of subtle 
> bugs, according to my LLM.

The hrtimer code looks like it's designed to handle this. It
first removes the old queued timer and then inserts it back
with the new expire time. And there's also locks in there.

> 
> 
> > - 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
> 
> I'll soon send out a series implementing this idea.
> 
> Best regards
> Thomas
> 
> >
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)
> 

-- 
Ville Syrjälä
Intel