Re: [PATCH v3] drm/vblank: Don't arm vblank timer with invalid frame duration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] drm_crtc_vblank_start_timer() deadlocks when synchronously spinning on hrtimer cancellation if the timer callback is concurrently running.
--

commit ea5cbfbf25c3ea02070a1ec1d83634d5505e475b
Author: Roman Ilin <me@romanilin.is>

drm/vblank: Don't arm vblank timer with invalid frame duration

This commit fixes a bug where an extremely small pixel clock results in a
computed frame duration that wraps to a negative value, causing the vblank
hrtimer to fail and stall pending page flips. It updates the calculation to
use u64 and adds overflow checks, returning an error to fall back to
immediate vblank events.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e423..629b9fcc7a52d 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c

[ ... ]

> @@ -2221,6 +2239,7 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>  	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
>  	unsigned long flags;
> +	int ret;
>  
>  	if (!vtimer->crtc) {
>  		/*
> @@ -2239,7 +2258,9 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
>  			hrtimer_try_to_cancel(&vtimer->timer);
>  	}

[Severity: Critical]
This isn't a bug introduced by this patch, but does this busy-wait loop
cause a deadlock if the timer callback is running concurrently?

When drm_vblank_enable() is called, it takes dev->vblank_time_lock before
calling down to __enable_vblank(), which eventually calls
drm_crtc_vblank_start_timer().

If the timer fires on another CPU at this exact moment, this loop will spin
indefinitely waiting for the callback to finish. However, the timer callback
executes drm_handle_vblank(), which tries to acquire dev->vblank_time_lock:

drm_vblank_timer_function() -> drm_handle_vblank() {
    ...
    spin_lock(&dev->vblank_time_lock);
}

Could this result in a deadlock between the spinning thread holding
vblank_time_lock and the timer callback trying to acquire it?

> -	drm_calc_timestamping_constants(crtc, &crtc->mode);
> +	ret = drm_calc_timestamping_constants(crtc, &crtc->mode);
> +	if (ret)
> +		return ret;
>  
>  	spin_lock_irqsave(&vtimer->interval_lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702181027.98526-1-me@romanilin.is?part=1