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

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

[Thomas Zimmermann]

From: Roman Ilin <me@romanilin.is>

When a CRTC's display mode carries a too small pixel clock,
drm_calc_timestamping_constants() computes a frame duration that
exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
interval, after which vblank events are no longer delivered. Pending
page flips never complete and the display appears frozen.

This could be triggered on virtio-gpu guests that have dynamic resolution
enabled: when the SPICE agent or the X server resizes the output, it
submits a mode whose pixel clock is off by a factor of 1000, e.g.:

    clock = 406 kHz, htotal = 3152, vtotal = 2148

    framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)

16675852216 does not fit into an int and wraps to roughly -504000000.
ns_to_ktime() then yields a negative interval and the timer stops working.

Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
Use vblank timer"). That commit merely made virtio-gpu use the vblank
timer and thereby exposed the pre-existing problem in the timer setup
added by commit 74afeb812850 ("drm/vblank: Add vblank timer").

To fix this, modify drm_calc_timestamping_constants() to use u64 for
calculations, check for INT_MAX overflows, and return an error code.
drm_crtc_vblank_start_timer() will then propagate the error, enabling
the driver to fall back to immediate vblank events. Valid modes are
unaffected, and the timer self-heals on the next mode with a sane clock.

v4:
- avoid log spamming with drm_warn_once()

Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Roman Ilin <me@romanilin.is>
---
I took Roman's patch from [1] and replaced the error messages with
warn_once, as suggestde in my review.

[1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-me@romanilin.is/
---
 drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
 include/drm/drm_vblank.h     |  4 +-
 2 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f90fb2d13e42..72b543bca9e3 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
  * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
  * CRTC's true scanout timing, so they take things like panel scaling or
  * other adjustments into account.
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
  */
-void drm_calc_timestamping_constants(struct drm_crtc *crtc,
-				     const struct drm_display_mode *mode)
+int drm_calc_timestamping_constants(struct drm_crtc *crtc,
+				    const struct drm_display_mode *mode)
 {
 	struct drm_device *dev = crtc->dev;
 	unsigned int pipe = drm_crtc_index(crtc);
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
-	int linedur_ns = 0, framedur_ns = 0;
+	u64 linedur_ns, framedur_ns;
 	int dotclock = mode->crtc_clock;
+	unsigned int frame_size;
 
 	if (!drm_dev_has_vblank(dev))
-		return;
+		return 0;
 
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
-		return;
+		return -EINVAL;
 
-	/* Valid dotclock? */
-	if (dotclock > 0) {
-		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
+	if (dotclock <= 0) {
+		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
+			     crtc->base.id, dotclock);
+		goto error;
+	}
 
-		/*
-		 * Convert scanline length in pixels and video
-		 * dot clock to line duration and frame duration
-		 * in nanoseconds:
-		 */
-		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
-		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
+	frame_size = (unsigned int)mode->crtc_htotal * (unsigned int)mode->crtc_vtotal;
 
-		/*
-		 * Fields of interlaced scanout modes are only half a frame duration.
-		 */
-		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-			framedur_ns /= 2;
-	} else {
-		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
-			crtc->base.id);
+	/*
+	 * Convert scanline length in pixels and video dot clock to line duration
+	 * and frame duration in nanoseconds.
+	 */
+	linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
+	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
+
+	/*
+	 * Fields of interlaced scanout modes are only half a frame duration.
+	 */
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		framedur_ns /= 2;
+
+	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
+		drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock too small!\n",
+			     crtc->base.id);
+		goto error;
 	}
 
-	vblank->linedur_ns  = linedur_ns;
+	vblank->linedur_ns = linedur_ns;
 	vblank->framedur_ns = framedur_ns;
 	drm_mode_copy(&vblank->hwmode, mode);
 
@@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 		     crtc->base.id, mode->crtc_htotal,
 		     mode->crtc_vtotal, mode->crtc_vdisplay);
 	drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
-		     crtc->base.id, dotclock, framedur_ns, linedur_ns);
+		     crtc->base.id, dotclock,
+		     vblank->framedur_ns, vblank->linedur_ns);
+
+	return 0;
+
+error:
+	vblank->linedur_ns = 0;
+	vblank->framedur_ns = 0;
+	drm_mode_copy(&vblank->hwmode, mode);
+	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_calc_timestamping_constants);
 
@@ -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);
 	}
 
-	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);
 	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 2fcef9c0f5b1..d99772dfa034 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
 u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
 void drm_crtc_vblank_restore(struct drm_crtc *crtc);
 
-void drm_calc_timestamping_constants(struct drm_crtc *crtc,
-				     const struct drm_display_mode *mode);
+int drm_calc_timestamping_constants(struct drm_crtc *crtc,
+				    const struct drm_display_mode *mode);
 wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
 void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
 				   u32 max_vblank_count);

base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] AB-BA deadlock between synchronous hrtimer cancellation in drm_crtc_vblank_start_timer() and drm_handle_vblank() taking vblank_time_lock.
--

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

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

This commit modifies drm_calc_timestamping_constants() to check for INT_MAX
overflows and returns an error instead of proceeding with invalid frame
durations. It also updates drm_crtc_vblank_start_timer() to propagate this
error, allowing the driver 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..72b543bca9e31 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: High]
This is a pre-existing issue, but can this busy-wait loop cause a hard
deadlock?

When CPU 1 enables vblanks, it holds the vblank_time_lock while executing this
synchronous cancellation loop:

CPU 1:
drm_vblank_enable() {
    spin_lock(&dev->vblank_time_lock);
    __enable_vblank(dev, pipe);
    ...
    drm_crtc_vblank_start_timer(crtc);
    while (hrtimer_active(&vtimer->timer))
        hrtimer_try_to_cancel(&vtimer->timer);

If the timer has just fired concurrently, CPU 2 executes the callback which
attempts to acquire the same lock:

CPU 2:
drm_vblank_timer_function() {
    ...
    drm_handle_vblank(dev, pipe);
    spin_lock(&dev->vblank_time_lock);

This creates an AB-BA deadlock where CPU 1 spins forever waiting for the
callback to finish on CPU 2, while CPU 2 spins forever waiting for CPU 1
to release vblank_time_lock. Is it safe to synchronously cancel the timer
here while holding a lock that the timer's callback also takes?

>  	}
>  
> -	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);
>  	vtimer->interval = ns_to_ktime(vblank->framedur_ns);

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

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

[Thomas Zimmermann]

Ping. Please review!

Am 14.09.26 um 12:57 schrieb Thomas Zimmermann:
> From: Roman Ilin <me@romanilin.is>
>
> When a CRTC's display mode carries a too small pixel clock,
> drm_calc_timestamping_constants() computes a frame duration that
> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
> interval, after which vblank events are no longer delivered. Pending
> page flips never complete and the display appears frozen.
>
> This could be triggered on virtio-gpu guests that have dynamic resolution
> enabled: when the SPICE agent or the X server resizes the output, it
> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
>
>      clock = 406 kHz, htotal = 3152, vtotal = 2148
>
>      framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
>
> 16675852216 does not fit into an int and wraps to roughly -504000000.
> ns_to_ktime() then yields a negative interval and the timer stops working.
>
> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
> Use vblank timer"). That commit merely made virtio-gpu use the vblank
> timer and thereby exposed the pre-existing problem in the timer setup
> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
>
> To fix this, modify drm_calc_timestamping_constants() to use u64 for
> calculations, check for INT_MAX overflows, and return an error code.
> drm_crtc_vblank_start_timer() will then propagate the error, enabling
> the driver to fall back to immediate vblank events. Valid modes are
> unaffected, and the timer self-heals on the next mode with a sane clock.
>
> v4:
> - avoid log spamming with drm_warn_once()
>
> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Roman Ilin <me@romanilin.is>
> ---
> I took Roman's patch from [1] and replaced the error messages with
> warn_once, as suggestde in my review.
>
> [1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-me@romanilin.is/
> ---
>   drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
>   include/drm/drm_vblank.h     |  4 +-
>   2 files changed, 49 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e42..72b543bca9e3 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>    * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
>    * CRTC's true scanout timing, so they take things like panel scaling or
>    * other adjustments into account.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
>    */
> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> -				     const struct drm_display_mode *mode)
> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
> +				    const struct drm_display_mode *mode)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	unsigned int pipe = drm_crtc_index(crtc);
>   	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> -	int linedur_ns = 0, framedur_ns = 0;
> +	u64 linedur_ns, framedur_ns;
>   	int dotclock = mode->crtc_clock;
> +	unsigned int frame_size;
>   
>   	if (!drm_dev_has_vblank(dev))
> -		return;
> +		return 0;
>   
>   	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> -		return;
> +		return -EINVAL;
>   
> -	/* Valid dotclock? */
> -	if (dotclock > 0) {
> -		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
> +	if (dotclock <= 0) {
> +		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
> +			     crtc->base.id, dotclock);
> +		goto error;
> +	}
>   
> -		/*
> -		 * Convert scanline length in pixels and video
> -		 * dot clock to line duration and frame duration
> -		 * in nanoseconds:
> -		 */
> -		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
> -		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
> +	frame_size = (unsigned int)mode->crtc_htotal * (unsigned int)mode->crtc_vtotal;
>   
> -		/*
> -		 * Fields of interlaced scanout modes are only half a frame duration.
> -		 */
> -		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -			framedur_ns /= 2;
> -	} else {
> -		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
> -			crtc->base.id);
> +	/*
> +	 * Convert scanline length in pixels and video dot clock to line duration
> +	 * and frame duration in nanoseconds.
> +	 */
> +	linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
> +
> +	/*
> +	 * Fields of interlaced scanout modes are only half a frame duration.
> +	 */
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		framedur_ns /= 2;
> +
> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
> +		drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock too small!\n",
> +			     crtc->base.id);
> +		goto error;
>   	}
>   
> -	vblank->linedur_ns  = linedur_ns;
> +	vblank->linedur_ns = linedur_ns;
>   	vblank->framedur_ns = framedur_ns;
>   	drm_mode_copy(&vblank->hwmode, mode);
>   
> @@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>   		     crtc->base.id, mode->crtc_htotal,
>   		     mode->crtc_vtotal, mode->crtc_vdisplay);
>   	drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
> -		     crtc->base.id, dotclock, framedur_ns, linedur_ns);
> +		     crtc->base.id, dotclock,
> +		     vblank->framedur_ns, vblank->linedur_ns);
> +
> +	return 0;
> +
> +error:
> +	vblank->linedur_ns = 0;
> +	vblank->framedur_ns = 0;
> +	drm_mode_copy(&vblank->hwmode, mode);
> +	return -EINVAL;
>   }
>   EXPORT_SYMBOL(drm_calc_timestamping_constants);
>   
> @@ -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);
>   	}
>   
> -	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);
>   	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 2fcef9c0f5b1..d99772dfa034 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
>   u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>   void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>   
> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> -				     const struct drm_display_mode *mode);
> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
> +				    const struct drm_display_mode *mode);
>   wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
>   void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>   				   u32 max_vblank_count);
>
> base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
> prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
> prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
> prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
> prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
> prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
> prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
> prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
> prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
> prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
> prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
> prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
> prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
> prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
> prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
> prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
> prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
> prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
> prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
> prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
> prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a

-- 
--
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 v4] drm/vblank: Don't arm vblank timer with invalid frame duration

[Dmitry Osipenko]

On 10/28/26 10:29, Thomas Zimmermann wrote:
> 
> Ping. Please review!
> 
> Am 14.09.26 um 12:57 schrieb Thomas Zimmermann:
>> From: Roman Ilin <me@romanilin.is>
>>
>> When a CRTC's display mode carries a too small pixel clock,
>> drm_calc_timestamping_constants() computes a frame duration that
>> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
>> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
>> interval, after which vblank events are no longer delivered. Pending
>> page flips never complete and the display appears frozen.
>>
>> This could be triggered on virtio-gpu guests that have dynamic resolution
>> enabled: when the SPICE agent or the X server resizes the output, it
>> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
>>
>>      clock = 406 kHz, htotal = 3152, vtotal = 2148
>>
>>      framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
>>
>> 16675852216 does not fit into an int and wraps to roughly -504000000.
>> ns_to_ktime() then yields a negative interval and the timer stops
>> working.
>>
>> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
>> Use vblank timer"). That commit merely made virtio-gpu use the vblank
>> timer and thereby exposed the pre-existing problem in the timer setup
>> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
>>
>> To fix this, modify drm_calc_timestamping_constants() to use u64 for
>> calculations, check for INT_MAX overflows, and return an error code.
>> drm_crtc_vblank_start_timer() will then propagate the error, enabling
>> the driver to fall back to immediate vblank events. Valid modes are
>> unaffected, and the timer self-heals on the next mode with a sane clock.
>>
>> v4:
>> - avoid log spamming with drm_warn_once()
>>
>> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Roman Ilin <me@romanilin.is>
>> ---
>> I took Roman's patch from [1] and replaced the error messages with
>> warn_once, as suggestde in my review.
>>
>> [1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-
>> me@romanilin.is/
>> ---
>>   drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
>>   include/drm/drm_vblank.h     |  4 +-
>>   2 files changed, 49 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index f90fb2d13e42..72b543bca9e3 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>>    * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
>>    * CRTC's true scanout timing, so they take things like panel
>> scaling or
>>    * other adjustments into account.
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise.
>>    */
>> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> -                     const struct drm_display_mode *mode)
>> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> +                    const struct drm_display_mode *mode)
>>   {
>>       struct drm_device *dev = crtc->dev;
>>       unsigned int pipe = drm_crtc_index(crtc);
>>       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>> -    int linedur_ns = 0, framedur_ns = 0;
>> +    u64 linedur_ns, framedur_ns;
>>       int dotclock = mode->crtc_clock;
>> +    unsigned int frame_size;
>>         if (!drm_dev_has_vblank(dev))
>> -        return;
>> +        return 0;
>>         if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>> -        return;
>> +        return -EINVAL;
>>   -    /* Valid dotclock? */
>> -    if (dotclock > 0) {
>> -        int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>> +    if (dotclock <= 0) {
>> +        drm_err_once(dev, "crtc %u: Can't calculate constants,
>> dotclock = %d!\n",
>> +                 crtc->base.id, dotclock);
>> +        goto error;
>> +    }
>>   -        /*
>> -         * Convert scanline length in pixels and video
>> -         * dot clock to line duration and frame duration
>> -         * in nanoseconds:
>> -         */
>> -        linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000,
>> dotclock);
>> -        framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
>> +    frame_size = (unsigned int)mode->crtc_htotal * (unsigned
>> int)mode->crtc_vtotal;
>>   -        /*
>> -         * Fields of interlaced scanout modes are only half a frame
>> duration.
>> -         */
>> -        if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> -            framedur_ns /= 2;
>> -    } else {
>> -        drm_err(dev, "crtc %u: Can't calculate constants, dotclock =
>> 0!\n",
>> -            crtc->base.id);
>> +    /*
>> +     * Convert scanline length in pixels and video dot clock to line
>> duration
>> +     * and frame duration in nanoseconds.
>> +     */
>> +    linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
>> +    framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
>> +
>> +    /*
>> +     * Fields of interlaced scanout modes are only half a frame
>> duration.
>> +     */
>> +    if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> +        framedur_ns /= 2;
>> +
>> +    if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
>> +        drm_err_once(dev, "crtc %u: Can't calculate constants, mode
>> clock too small!\n",
>> +                 crtc->base.id);
>> +        goto error;
>>       }
>>   -    vblank->linedur_ns  = linedur_ns;
>> +    vblank->linedur_ns = linedur_ns;
>>       vblank->framedur_ns = framedur_ns;
>>       drm_mode_copy(&vblank->hwmode, mode);
>>   @@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct
>> drm_crtc *crtc,
>>                crtc->base.id, mode->crtc_htotal,
>>                mode->crtc_vtotal, mode->crtc_vdisplay);
>>       drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
>> -             crtc->base.id, dotclock, framedur_ns, linedur_ns);
>> +             crtc->base.id, dotclock,
>> +             vblank->framedur_ns, vblank->linedur_ns);
>> +
>> +    return 0;
>> +
>> +error:
>> +    vblank->linedur_ns = 0;
>> +    vblank->framedur_ns = 0;
>> +    drm_mode_copy(&vblank->hwmode, mode);
>> +    return -EINVAL;
>>   }
>>   EXPORT_SYMBOL(drm_calc_timestamping_constants);
>>   @@ -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);
>>       }
>>   -    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);
>>       vtimer->interval = ns_to_ktime(vblank->framedur_ns);
>> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
>> index 2fcef9c0f5b1..d99772dfa034 100644
>> --- a/include/drm/drm_vblank.h
>> +++ b/include/drm/drm_vblank.h
>> @@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
>>   u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>>   void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>>   -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> -                     const struct drm_display_mode *mode);
>> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> +                    const struct drm_display_mode *mode);
>>   wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
>>   void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>>                      u32 max_vblank_count);
>>
>> base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
>> prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
>> prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
>> prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
>> prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
>> prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
>> prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
>> prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
>> prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
>> prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
>> prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
>> prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
>> prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
>> prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
>> prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
>> prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
>> prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
>> prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
>> prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
>> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
>> prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
>> prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
>> prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
> 

The patch looks correct, though will be better to have a smaller diff 
that doesn't require LLM for a review.


-void drm_calc_timestamping_constants(struct drm_crtc *crtc,
-				     const struct drm_display_mode *mode)
+int drm_calc_timestamping_constants(struct drm_crtc *crtc,
+				    const struct drm_display_mode *mode)
  {
  	struct drm_device *dev = crtc->dev;
  	unsigned int pipe = drm_crtc_index(crtc);
  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
-	int linedur_ns = 0, framedur_ns = 0;
+	u64 linedur_ns = 0, framedur_ns = 0;
  	int dotclock = mode->crtc_clock;

  	if (!drm_dev_has_vblank(dev))
-		return;
+		return 0;

  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
-		return;
+		return -EINVAL;

  	/* Valid dotclock? */
  	if (dotclock > 0) {
-		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
+		u64 frame_size = (u64)mode->crtc_htotal * mode->crtc_vtotal;

  		/*
  		 * Convert scanline length in pixels and video
  		 * dot clock to line duration and frame duration
  		 * in nanoseconds:
  		 */
  		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
-		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
+		framedur_ns = div_u64(frame_size * 1000000, dotclock);

  		/*
  		 * Fields of interlaced scanout modes are only half a frame duration.
  		 */
  		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
  			framedur_ns /= 2;
+
+		if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
+			drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock 
too small!\n",
+				     crtc->base.id);
+			linedur_ns = 0;
+			framedur_ns = 0;
+			dotclock = 0; /* reuse the existing error-logging branch below */
+		}
  	} else {
-		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
-			crtc->base.id);
+		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
+			     crtc->base.id, dotclock);
  	}

-- 
Best regards,
Dmitry

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

[Ville Syrjälä]

On Mon, Sep 14, 2026 at 12:57:26PM +0200, Thomas Zimmermann wrote:
> From: Roman Ilin <me@romanilin.is>
> 
> When a CRTC's display mode carries a too small pixel clock,
> drm_calc_timestamping_constants() computes a frame duration that
> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
> interval, after which vblank events are no longer delivered. Pending
> page flips never complete and the display appears frozen.
> 
> This could be triggered on virtio-gpu guests that have dynamic resolution
> enabled: when the SPICE agent or the X server resizes the output, it
> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
> 
>     clock = 406 kHz, htotal = 3152, vtotal = 2148

How about actually "fixing" the clock in the driver?

> 
>     framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
> 
> 16675852216 does not fit into an int and wraps to roughly -504000000.
> ns_to_ktime() then yields a negative interval and the timer stops working.
> 
> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
> Use vblank timer"). That commit merely made virtio-gpu use the vblank
> timer and thereby exposed the pre-existing problem in the timer setup
> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
> 
> To fix this, modify drm_calc_timestamping_constants() to use u64 for
> calculations, check for INT_MAX overflows, and return an error code.
> drm_crtc_vblank_start_timer() will then propagate the error, enabling
> the driver to fall back to immediate vblank events. Valid modes are
> unaffected, and the timer self-heals on the next mode with a sane clock.
> 
> v4:
> - avoid log spamming with drm_warn_once()
> 
> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Roman Ilin <me@romanilin.is>
> ---
> I took Roman's patch from [1] and replaced the error messages with
> warn_once, as suggestde in my review.
> 
> [1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-me@romanilin.is/
> ---
>  drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
>  include/drm/drm_vblank.h     |  4 +-
>  2 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e42..72b543bca9e3 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>   * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
>   * CRTC's true scanout timing, so they take things like panel scaling or
>   * other adjustments into account.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
>   */
> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> -				     const struct drm_display_mode *mode)
> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
> +				    const struct drm_display_mode *mode)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> -	int linedur_ns = 0, framedur_ns = 0;
> +	u64 linedur_ns, framedur_ns;
>  	int dotclock = mode->crtc_clock;
> +	unsigned int frame_size;
>  
>  	if (!drm_dev_has_vblank(dev))
> -		return;
> +		return 0;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> -		return;
> +		return -EINVAL;
>  
> -	/* Valid dotclock? */
> -	if (dotclock > 0) {
> -		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
> +	if (dotclock <= 0) {
> +		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
> +			     crtc->base.id, dotclock);
> +		goto error;
> +	}
>  
> -		/*
> -		 * Convert scanline length in pixels and video
> -		 * dot clock to line duration and frame duration
> -		 * in nanoseconds:
> -		 */
> -		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
> -		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
> +	frame_size = (unsigned int)mode->crtc_htotal * (unsigned int)mode->crtc_vtotal;
>  
> -		/*
> -		 * Fields of interlaced scanout modes are only half a frame duration.
> -		 */
> -		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -			framedur_ns /= 2;
> -	} else {
> -		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
> -			crtc->base.id);
> +	/*
> +	 * Convert scanline length in pixels and video dot clock to line duration
> +	 * and frame duration in nanoseconds.
> +	 */
> +	linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
> +
> +	/*
> +	 * Fields of interlaced scanout modes are only half a frame duration.
> +	 */
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		framedur_ns /= 2;
> +
> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
> +		drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock too small!\n",
> +			     crtc->base.id);
> +		goto error;
>  	}
>  
> -	vblank->linedur_ns  = linedur_ns;
> +	vblank->linedur_ns = linedur_ns;
>  	vblank->framedur_ns = framedur_ns;
>  	drm_mode_copy(&vblank->hwmode, mode);
>  
> @@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  		     crtc->base.id, mode->crtc_htotal,
>  		     mode->crtc_vtotal, mode->crtc_vdisplay);
>  	drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
> -		     crtc->base.id, dotclock, framedur_ns, linedur_ns);
> +		     crtc->base.id, dotclock,
> +		     vblank->framedur_ns, vblank->linedur_ns);
> +
> +	return 0;
> +
> +error:
> +	vblank->linedur_ns = 0;
> +	vblank->framedur_ns = 0;
> +	drm_mode_copy(&vblank->hwmode, mode);
> +	return -EINVAL;
>  }
>  EXPORT_SYMBOL(drm_calc_timestamping_constants);
>  
> @@ -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);
>  	}
>  
> -	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);
>  	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 2fcef9c0f5b1..d99772dfa034 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
>  u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>  void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>  
> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> -				     const struct drm_display_mode *mode);
> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
> +				    const struct drm_display_mode *mode);
>  wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
>  void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>  				   u32 max_vblank_count);
> 
> base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
> prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
> prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
> prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
> prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
> prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
> prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
> prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
> prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
> prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
> prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
> prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
> prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
> prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
> prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
> prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
> prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
> prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
> prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
> prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
> prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
> -- 
> 2.55.0

-- 
Ville Syrjälä
Intel

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

[Thomas Zimmermann]

Hi

Am 28.10.26 um 15:54 schrieb Ville Syrjälä:
> On Mon, Sep 14, 2026 at 12:57:26PM +0200, Thomas Zimmermann wrote:
>> From: Roman Ilin <me@romanilin.is>
>>
>> When a CRTC's display mode carries a too small pixel clock,
>> drm_calc_timestamping_constants() computes a frame duration that
>> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
>> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
>> interval, after which vblank events are no longer delivered. Pending
>> page flips never complete and the display appears frozen.
>>
>> This could be triggered on virtio-gpu guests that have dynamic resolution
>> enabled: when the SPICE agent or the X server resizes the output, it
>> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
>>
>>      clock = 406 kHz, htotal = 3152, vtotal = 2148
> How about actually "fixing" the clock in the driver?

It's not a broken driver, but user space. There is a bug report open for 
spice to replace the incorrect units with correct ones. [1] The fix here 
is supposed to resolve the kernel problem this triggers.

[1] https://gitlab.freedesktop.org/spice/linux/vd_agent/-/work_items/52

Best regards
Thomas


>
>>      framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
>>
>> 16675852216 does not fit into an int and wraps to roughly -504000000.
>> ns_to_ktime() then yields a negative interval and the timer stops working.
>>
>> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
>> Use vblank timer"). That commit merely made virtio-gpu use the vblank
>> timer and thereby exposed the pre-existing problem in the timer setup
>> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
>>
>> To fix this, modify drm_calc_timestamping_constants() to use u64 for
>> calculations, check for INT_MAX overflows, and return an error code.
>> drm_crtc_vblank_start_timer() will then propagate the error, enabling
>> the driver to fall back to immediate vblank events. Valid modes are
>> unaffected, and the timer self-heals on the next mode with a sane clock.
>>
>> v4:
>> - avoid log spamming with drm_warn_once()
>>
>> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Roman Ilin <me@romanilin.is>
>> ---
>> I took Roman's patch from [1] and replaced the error messages with
>> warn_once, as suggestde in my review.
>>
>> [1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-me@romanilin.is/
>> ---
>>   drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
>>   include/drm/drm_vblank.h     |  4 +-
>>   2 files changed, 49 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index f90fb2d13e42..72b543bca9e3 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>>    * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
>>    * CRTC's true scanout timing, so they take things like panel scaling or
>>    * other adjustments into account.
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise.
>>    */
>> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> -				     const struct drm_display_mode *mode)
>> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> +				    const struct drm_display_mode *mode)
>>   {
>>   	struct drm_device *dev = crtc->dev;
>>   	unsigned int pipe = drm_crtc_index(crtc);
>>   	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>> -	int linedur_ns = 0, framedur_ns = 0;
>> +	u64 linedur_ns, framedur_ns;
>>   	int dotclock = mode->crtc_clock;
>> +	unsigned int frame_size;
>>   
>>   	if (!drm_dev_has_vblank(dev))
>> -		return;
>> +		return 0;
>>   
>>   	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>> -		return;
>> +		return -EINVAL;
>>   
>> -	/* Valid dotclock? */
>> -	if (dotclock > 0) {
>> -		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>> +	if (dotclock <= 0) {
>> +		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
>> +			     crtc->base.id, dotclock);
>> +		goto error;
>> +	}
>>   
>> -		/*
>> -		 * Convert scanline length in pixels and video
>> -		 * dot clock to line duration and frame duration
>> -		 * in nanoseconds:
>> -		 */
>> -		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
>> -		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
>> +	frame_size = (unsigned int)mode->crtc_htotal * (unsigned int)mode->crtc_vtotal;
>>   
>> -		/*
>> -		 * Fields of interlaced scanout modes are only half a frame duration.
>> -		 */
>> -		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> -			framedur_ns /= 2;
>> -	} else {
>> -		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
>> -			crtc->base.id);
>> +	/*
>> +	 * Convert scanline length in pixels and video dot clock to line duration
>> +	 * and frame duration in nanoseconds.
>> +	 */
>> +	linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
>> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
>> +
>> +	/*
>> +	 * Fields of interlaced scanout modes are only half a frame duration.
>> +	 */
>> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> +		framedur_ns /= 2;
>> +
>> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
>> +		drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock too small!\n",
>> +			     crtc->base.id);
>> +		goto error;
>>   	}
>>   
>> -	vblank->linedur_ns  = linedur_ns;
>> +	vblank->linedur_ns = linedur_ns;
>>   	vblank->framedur_ns = framedur_ns;
>>   	drm_mode_copy(&vblank->hwmode, mode);
>>   
>> @@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>>   		     crtc->base.id, mode->crtc_htotal,
>>   		     mode->crtc_vtotal, mode->crtc_vdisplay);
>>   	drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
>> -		     crtc->base.id, dotclock, framedur_ns, linedur_ns);
>> +		     crtc->base.id, dotclock,
>> +		     vblank->framedur_ns, vblank->linedur_ns);
>> +
>> +	return 0;
>> +
>> +error:
>> +	vblank->linedur_ns = 0;
>> +	vblank->framedur_ns = 0;
>> +	drm_mode_copy(&vblank->hwmode, mode);
>> +	return -EINVAL;
>>   }
>>   EXPORT_SYMBOL(drm_calc_timestamping_constants);
>>   
>> @@ -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);
>>   	}
>>   
>> -	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);
>>   	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
>> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
>> index 2fcef9c0f5b1..d99772dfa034 100644
>> --- a/include/drm/drm_vblank.h
>> +++ b/include/drm/drm_vblank.h
>> @@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
>>   u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>>   void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>>   
>> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> -				     const struct drm_display_mode *mode);
>> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
>> +				    const struct drm_display_mode *mode);
>>   wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
>>   void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>>   				   u32 max_vblank_count);
>>
>> base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
>> prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
>> prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
>> prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
>> prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
>> prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
>> prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
>> prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
>> prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
>> prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
>> prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
>> prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
>> prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
>> prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
>> prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
>> prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
>> prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
>> prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
>> prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
>> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
>> prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
>> prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
>> prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
>> -- 
>> 2.55.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 v4] drm/vblank: Don't arm vblank timer with invalid frame duration

[Ville Syrjälä]

On Thu, Oct 29, 2026 at 12:35:52PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.10.26 um 15:54 schrieb Ville Syrjälä:
> > On Mon, Sep 14, 2026 at 12:57:26PM +0200, Thomas Zimmermann wrote:
> >> From: Roman Ilin <me@romanilin.is>
> >>
> >> When a CRTC's display mode carries a too small pixel clock,
> >> drm_calc_timestamping_constants() computes a frame duration that
> >> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
> >> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
> >> interval, after which vblank events are no longer delivered. Pending
> >> page flips never complete and the display appears frozen.
> >>
> >> This could be triggered on virtio-gpu guests that have dynamic resolution
> >> enabled: when the SPICE agent or the X server resizes the output, it
> >> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
> >>
> >>      clock = 406 kHz, htotal = 3152, vtotal = 2148
> > How about actually "fixing" the clock in the driver?
> 
> It's not a broken driver, but user space. There is a bug report open for 
> spice to replace the incorrect units with correct ones. [1] The fix here 
> is supposed to resolve the kernel problem this triggers.

Life becomes much simpler if you don't let garbage userspace input
past some initial sanity checks. So technically the driver should
just reject these modes. But in this case since you need to keep
the broken userspace working the driver should just fix it up,
and do so early so you don't have to deal with it anywhere else.

> 
> [1] https://gitlab.freedesktop.org/spice/linux/vd_agent/-/work_items/52
> 
> Best regards
> Thomas
> 
> 
> >
> >>      framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
> >>
> >> 16675852216 does not fit into an int and wraps to roughly -504000000.
> >> ns_to_ktime() then yields a negative interval and the timer stops working.
> >>
> >> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
> >> Use vblank timer"). That commit merely made virtio-gpu use the vblank
> >> timer and thereby exposed the pre-existing problem in the timer setup
> >> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
> >>
> >> To fix this, modify drm_calc_timestamping_constants() to use u64 for
> >> calculations, check for INT_MAX overflows, and return an error code.
> >> drm_crtc_vblank_start_timer() will then propagate the error, enabling
> >> the driver to fall back to immediate vblank events. Valid modes are
> >> unaffected, and the timer self-heals on the next mode with a sane clock.
> >>
> >> v4:
> >> - avoid log spamming with drm_warn_once()
> >>
> >> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Roman Ilin <me@romanilin.is>
> >> ---
> >> I took Roman's patch from [1] and replaced the error messages with
> >> warn_once, as suggestde in my review.
> >>
> >> [1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-me@romanilin.is/
> >> ---
> >>   drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
> >>   include/drm/drm_vblank.h     |  4 +-
> >>   2 files changed, 49 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> >> index f90fb2d13e42..72b543bca9e3 100644
> >> --- a/drivers/gpu/drm/drm_vblank.c
> >> +++ b/drivers/gpu/drm/drm_vblank.c
> >> @@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
> >>    * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
> >>    * CRTC's true scanout timing, so they take things like panel scaling or
> >>    * other adjustments into account.
> >> + *
> >> + * Returns:
> >> + * 0 on success, or a negative errno code otherwise.
> >>    */
> >> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> >> -				     const struct drm_display_mode *mode)
> >> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
> >> +				    const struct drm_display_mode *mode)
> >>   {
> >>   	struct drm_device *dev = crtc->dev;
> >>   	unsigned int pipe = drm_crtc_index(crtc);
> >>   	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> >> -	int linedur_ns = 0, framedur_ns = 0;
> >> +	u64 linedur_ns, framedur_ns;
> >>   	int dotclock = mode->crtc_clock;
> >> +	unsigned int frame_size;
> >>   
> >>   	if (!drm_dev_has_vblank(dev))
> >> -		return;
> >> +		return 0;
> >>   
> >>   	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> >> -		return;
> >> +		return -EINVAL;
> >>   
> >> -	/* Valid dotclock? */
> >> -	if (dotclock > 0) {
> >> -		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
> >> +	if (dotclock <= 0) {
> >> +		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
> >> +			     crtc->base.id, dotclock);
> >> +		goto error;
> >> +	}
> >>   
> >> -		/*
> >> -		 * Convert scanline length in pixels and video
> >> -		 * dot clock to line duration and frame duration
> >> -		 * in nanoseconds:
> >> -		 */
> >> -		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
> >> -		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
> >> +	frame_size = (unsigned int)mode->crtc_htotal * (unsigned int)mode->crtc_vtotal;
> >>   
> >> -		/*
> >> -		 * Fields of interlaced scanout modes are only half a frame duration.
> >> -		 */
> >> -		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> -			framedur_ns /= 2;
> >> -	} else {
> >> -		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
> >> -			crtc->base.id);
> >> +	/*
> >> +	 * Convert scanline length in pixels and video dot clock to line duration
> >> +	 * and frame duration in nanoseconds.
> >> +	 */
> >> +	linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
> >> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
> >> +
> >> +	/*
> >> +	 * Fields of interlaced scanout modes are only half a frame duration.
> >> +	 */
> >> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> +		framedur_ns /= 2;
> >> +
> >> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
> >> +		drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock too small!\n",
> >> +			     crtc->base.id);
> >> +		goto error;
> >>   	}
> >>   
> >> -	vblank->linedur_ns  = linedur_ns;
> >> +	vblank->linedur_ns = linedur_ns;
> >>   	vblank->framedur_ns = framedur_ns;
> >>   	drm_mode_copy(&vblank->hwmode, mode);
> >>   
> >> @@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> >>   		     crtc->base.id, mode->crtc_htotal,
> >>   		     mode->crtc_vtotal, mode->crtc_vdisplay);
> >>   	drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
> >> -		     crtc->base.id, dotclock, framedur_ns, linedur_ns);
> >> +		     crtc->base.id, dotclock,
> >> +		     vblank->framedur_ns, vblank->linedur_ns);
> >> +
> >> +	return 0;
> >> +
> >> +error:
> >> +	vblank->linedur_ns = 0;
> >> +	vblank->framedur_ns = 0;
> >> +	drm_mode_copy(&vblank->hwmode, mode);
> >> +	return -EINVAL;
> >>   }
> >>   EXPORT_SYMBOL(drm_calc_timestamping_constants);
> >>   
> >> @@ -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);
> >>   	}
> >>   
> >> -	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);
> >>   	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
> >> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> >> index 2fcef9c0f5b1..d99772dfa034 100644
> >> --- a/include/drm/drm_vblank.h
> >> +++ b/include/drm/drm_vblank.h
> >> @@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >>   u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> >>   void drm_crtc_vblank_restore(struct drm_crtc *crtc);
> >>   
> >> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> >> -				     const struct drm_display_mode *mode);
> >> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
> >> +				    const struct drm_display_mode *mode);
> >>   wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
> >>   void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> >>   				   u32 max_vblank_count);
> >>
> >> base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
> >> prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
> >> prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
> >> prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
> >> prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
> >> prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
> >> prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
> >> prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
> >> prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
> >> prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
> >> prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
> >> prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
> >> prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
> >> prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
> >> prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
> >> prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
> >> prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
> >> prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
> >> prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
> >> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> >> prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
> >> prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
> >> prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
> >> -- 
> >> 2.55.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)
> 

-- 
Ville Syrjälä
Intel

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

[Thomas Zimmermann]

Hi

Am 30.10.26 um 13:07 schrieb Ville Syrjälä:
> On Thu, Oct 29, 2026 at 12:35:52PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 28.10.26 um 15:54 schrieb Ville Syrjälä:
>>> On Mon, Sep 14, 2026 at 12:57:26PM +0200, Thomas Zimmermann wrote:
>>>> From: Roman Ilin <me@romanilin.is>
>>>>
>>>> When a CRTC's display mode carries a too small pixel clock,
>>>> drm_calc_timestamping_constants() computes a frame duration that
>>>> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
>>>> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
>>>> interval, after which vblank events are no longer delivered. Pending
>>>> page flips never complete and the display appears frozen.
>>>>
>>>> This could be triggered on virtio-gpu guests that have dynamic resolution
>>>> enabled: when the SPICE agent or the X server resizes the output, it
>>>> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
>>>>
>>>>       clock = 406 kHz, htotal = 3152, vtotal = 2148
>>> How about actually "fixing" the clock in the driver?
>> It's not a broken driver, but user space. There is a bug report open for
>> spice to replace the incorrect units with correct ones. [1] The fix here
>> is supposed to resolve the kernel problem this triggers.
> Life becomes much simpler if you don't let garbage userspace input
> past some initial sanity checks. So technically the driver should
> just reject these modes. But in this case since you need to keep
> the broken userspace working the driver should just fix it up,
> and do so early so you don't have to deal with it anywhere else.

Not that I disagree, with the statement in general. But doing this here 
seems like whack-a-mole if we leave it to individual drivers. And the 
userspace is already dis-functional, no need to keep supporting it, I think.

And how would drivers know that the values are garbage before processing 
them? Even if a driver could program them to hardware, the vblank 
helpers could still not process them correctly. Thus leaving the 
userspace dis-functional.

I see that we might want a common test function 
drm_vblank_check_display_mode() and put it in some central place in the 
atomic_check pipeline. At least we'd separate the testing from the 
calculation.

Best regards
Thomas



>
>> [1] https://gitlab.freedesktop.org/spice/linux/vd_agent/-/work_items/52
>>
>> Best regards
>> Thomas
>>
>>
>>>>       framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
>>>>
>>>> 16675852216 does not fit into an int and wraps to roughly -504000000.
>>>> ns_to_ktime() then yields a negative interval and the timer stops working.
>>>>
>>>> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
>>>> Use vblank timer"). That commit merely made virtio-gpu use the vblank
>>>> timer and thereby exposed the pre-existing problem in the timer setup
>>>> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
>>>>
>>>> To fix this, modify drm_calc_timestamping_constants() to use u64 for
>>>> calculations, check for INT_MAX overflows, and return an error code.
>>>> drm_crtc_vblank_start_timer() will then propagate the error, enabling
>>>> the driver to fall back to immediate vblank events. Valid modes are
>>>> unaffected, and the timer self-heals on the next mode with a sane clock.
>>>>
>>>> v4:
>>>> - avoid log spamming with drm_warn_once()
>>>>
>>>> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
>>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Roman Ilin <me@romanilin.is>
>>>> ---
>>>> I took Roman's patch from [1] and replaced the error messages with
>>>> warn_once, as suggestde in my review.
>>>>
>>>> [1] https://lore.kernel.org/dri-devel/20260702181027.98526-1-me@romanilin.is/
>>>> ---
>>>>    drivers/gpu/drm/drm_vblank.c | 73 +++++++++++++++++++++++-------------
>>>>    include/drm/drm_vblank.h     |  4 +-
>>>>    2 files changed, 49 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>>>> index f90fb2d13e42..72b543bca9e3 100644
>>>> --- a/drivers/gpu/drm/drm_vblank.c
>>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>>> @@ -631,45 +631,54 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>>>>     * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from
>>>>     * CRTC's true scanout timing, so they take things like panel scaling or
>>>>     * other adjustments into account.
>>>> + *
>>>> + * Returns:
>>>> + * 0 on success, or a negative errno code otherwise.
>>>>     */
>>>> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>>>> -				     const struct drm_display_mode *mode)
>>>> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
>>>> +				    const struct drm_display_mode *mode)
>>>>    {
>>>>    	struct drm_device *dev = crtc->dev;
>>>>    	unsigned int pipe = drm_crtc_index(crtc);
>>>>    	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>>>> -	int linedur_ns = 0, framedur_ns = 0;
>>>> +	u64 linedur_ns, framedur_ns;
>>>>    	int dotclock = mode->crtc_clock;
>>>> +	unsigned int frame_size;
>>>>    
>>>>    	if (!drm_dev_has_vblank(dev))
>>>> -		return;
>>>> +		return 0;
>>>>    
>>>>    	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>>>> -		return;
>>>> +		return -EINVAL;
>>>>    
>>>> -	/* Valid dotclock? */
>>>> -	if (dotclock > 0) {
>>>> -		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>>>> +	if (dotclock <= 0) {
>>>> +		drm_err_once(dev, "crtc %u: Can't calculate constants, dotclock = %d!\n",
>>>> +			     crtc->base.id, dotclock);
>>>> +		goto error;
>>>> +	}
>>>>    
>>>> -		/*
>>>> -		 * Convert scanline length in pixels and video
>>>> -		 * dot clock to line duration and frame duration
>>>> -		 * in nanoseconds:
>>>> -		 */
>>>> -		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
>>>> -		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
>>>> +	frame_size = (unsigned int)mode->crtc_htotal * (unsigned int)mode->crtc_vtotal;
>>>>    
>>>> -		/*
>>>> -		 * Fields of interlaced scanout modes are only half a frame duration.
>>>> -		 */
>>>> -		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>>> -			framedur_ns /= 2;
>>>> -	} else {
>>>> -		drm_err(dev, "crtc %u: Can't calculate constants, dotclock = 0!\n",
>>>> -			crtc->base.id);
>>>> +	/*
>>>> +	 * Convert scanline length in pixels and video dot clock to line duration
>>>> +	 * and frame duration in nanoseconds.
>>>> +	 */
>>>> +	linedur_ns  = div_u64((u64)mode->crtc_htotal * 1000000, dotclock);
>>>> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
>>>> +
>>>> +	/*
>>>> +	 * Fields of interlaced scanout modes are only half a frame duration.
>>>> +	 */
>>>> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>>> +		framedur_ns /= 2;
>>>> +
>>>> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX) {
>>>> +		drm_err_once(dev, "crtc %u: Can't calculate constants, mode clock too small!\n",
>>>> +			     crtc->base.id);
>>>> +		goto error;
>>>>    	}
>>>>    
>>>> -	vblank->linedur_ns  = linedur_ns;
>>>> +	vblank->linedur_ns = linedur_ns;
>>>>    	vblank->framedur_ns = framedur_ns;
>>>>    	drm_mode_copy(&vblank->hwmode, mode);
>>>>    
>>>> @@ -678,7 +687,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>>>>    		     crtc->base.id, mode->crtc_htotal,
>>>>    		     mode->crtc_vtotal, mode->crtc_vdisplay);
>>>>    	drm_dbg_core(dev, "crtc %u: clock %d kHz framedur %d linedur %d\n",
>>>> -		     crtc->base.id, dotclock, framedur_ns, linedur_ns);
>>>> +		     crtc->base.id, dotclock,
>>>> +		     vblank->framedur_ns, vblank->linedur_ns);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +error:
>>>> +	vblank->linedur_ns = 0;
>>>> +	vblank->framedur_ns = 0;
>>>> +	drm_mode_copy(&vblank->hwmode, mode);
>>>> +	return -EINVAL;
>>>>    }
>>>>    EXPORT_SYMBOL(drm_calc_timestamping_constants);
>>>>    
>>>> @@ -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);
>>>>    	}
>>>>    
>>>> -	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);
>>>>    	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
>>>> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
>>>> index 2fcef9c0f5b1..d99772dfa034 100644
>>>> --- a/include/drm/drm_vblank.h
>>>> +++ b/include/drm/drm_vblank.h
>>>> @@ -311,8 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
>>>>    u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>>>>    void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>>>>    
>>>> -void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>>>> -				     const struct drm_display_mode *mode);
>>>> +int drm_calc_timestamping_constants(struct drm_crtc *crtc,
>>>> +				    const struct drm_display_mode *mode);
>>>>    wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
>>>>    void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>>>>    				   u32 max_vblank_count);
>>>>
>>>> base-commit: 06576c2b43750d990297b8ff1989b94ac72e855f
>>>> prerequisite-patch-id: 6729bb10d5e338678e78ccda2dd24f232a13cd22
>>>> prerequisite-patch-id: 9d94d20bd45967493b4dc339b58082e1c8da0f4f
>>>> prerequisite-patch-id: 6ac8d22df34db9e7d467b1ab95ad30f94841d5db
>>>> prerequisite-patch-id: f548dbde3f944ca3415c808be628719acebe898a
>>>> prerequisite-patch-id: 2ae91ee8e2bbcd33b8325be046e7214185aba6aa
>>>> prerequisite-patch-id: f97bbde08a9996d7c62ca3e3ebfcd387e50bc5c3
>>>> prerequisite-patch-id: ecc3c825277bc9978c6979455517249a074c0cb1
>>>> prerequisite-patch-id: 0449925cca36664b566f614bfb7d573a11f0575f
>>>> prerequisite-patch-id: 9fb2c2787244e9621847af09bea03e575e6276c7
>>>> prerequisite-patch-id: eb4384d581805e1178d22e7df45f134223c38305
>>>> prerequisite-patch-id: 1a7a529e4b2864abe8071aa44598d28ce72483f4
>>>> prerequisite-patch-id: 844a85f4c2fe2f72209b82d46614004c3cfe4123
>>>> prerequisite-patch-id: 13f7cb22114c648d35f8c9fa74e9165656ce31eb
>>>> prerequisite-patch-id: 20ee46bd6fdd91e39f71d22f52ef6682a4e41512
>>>> prerequisite-patch-id: 1d26c0ff91796ff1e62d3831d00bee18723788ef
>>>> prerequisite-patch-id: 007f6ea0d09bef54b689bae0eb5abeb497bdfaad
>>>> prerequisite-patch-id: 2d89c1507c909520adf9bfb1a137d458a819ae8f
>>>> prerequisite-patch-id: d90dc4134468870ab8ccbd71547c9552dff74fbd
>>>> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
>>>> prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
>>>> prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
>>>> prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
>>>> -- 
>>>> 2.55.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)
>>

-- 
--
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 v4] drm/vblank: Don't arm vblank timer with invalid frame duration

[Ville Syrjälä]

On Fri, Oct 30, 2026 at 02:04:04PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 30.10.26 um 13:07 schrieb Ville Syrjälä:
> > On Thu, Oct 29, 2026 at 12:35:52PM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 28.10.26 um 15:54 schrieb Ville Syrjälä:
> >>> On Mon, Sep 14, 2026 at 12:57:26PM +0200, Thomas Zimmermann wrote:
> >>>> From: Roman Ilin <me@romanilin.is>
> >>>>
> >>>> When a CRTC's display mode carries a too small pixel clock,
> >>>> drm_calc_timestamping_constants() computes a frame duration that
> >>>> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
> >>>> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
> >>>> interval, after which vblank events are no longer delivered. Pending
> >>>> page flips never complete and the display appears frozen.
> >>>>
> >>>> This could be triggered on virtio-gpu guests that have dynamic resolution
> >>>> enabled: when the SPICE agent or the X server resizes the output, it
> >>>> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
> >>>>
> >>>>       clock = 406 kHz, htotal = 3152, vtotal = 2148
> >>> How about actually "fixing" the clock in the driver?
> >> It's not a broken driver, but user space. There is a bug report open for
> >> spice to replace the incorrect units with correct ones. [1] The fix here
> >> is supposed to resolve the kernel problem this triggers.
> > Life becomes much simpler if you don't let garbage userspace input
> > past some initial sanity checks. So technically the driver should
> > just reject these modes. But in this case since you need to keep
> > the broken userspace working the driver should just fix it up,
> > and do so early so you don't have to deal with it anywhere else.
> 
> Not that I disagree, with the statement in general. But doing this here 
> seems like whack-a-mole if we leave it to individual drivers.

I think you're just trading that whack-a-mole for a worse one
where you now have to make all kinds of low level functions
deal with the garbage input.

This particular function is never expected to fail, which is
why it returns void. And where it gets called in i915 we
can't even handle the failure, so changing the return type
to int is making the whole thing confusing.

One driver independent solution would be to have 
drm_mode_validate_basic() validate the dotclock a bit hadder,
but that will not work for this case because you need to keep
allowing these bogus modes to reach the driver.

Just the other day I was actually thinking of adding some basic
frame duration checks into drm_mode_validate_basic(), but I guess
I can't do that given this userspace bug.

> And the 
> userspace is already dis-functional, no need to keep supporting it, I think.
> 
> And how would drivers know that the values are garbage before processing 
> them? 

That's why I added the device wide mode_valid hook. Drivers can
use that to reject garbage before it reaches anything more
sensitive. I have somewhat hard time believing a mode that is
acceptable/reasonable to some real world hardware would fail 
in the vblank calculations.

Hmm, and I just realized that I haven't actually added any low limit
for the dotclock in i915. I'll go do that now. I think ~10 MHz is
probably a reasonable enough number I can use there.

> Even if a driver could program them to hardware, the vblank 
> helpers could still not process them correctly. Thus leaving the 
> userspace dis-functional.
> 
> I see that we might want a common test function 
> drm_vblank_check_display_mode() and put it in some central place in the 
> atomic_check pipeline. At least we'd separate the testing from the 
> calculation.

Yeah, something like that could work. Though I still
think it'd be better to do it from the driver's mode_valid
hook to reject the bogus stuff even earlier.

That would also filter them out from the connectors' mode
lists. But I suppose you could do the vblank check in
the connector mode filtering unconditionally without
having the driver do it. Or does the buggy userspace also
need the kernel to return bogus modes from getconnector?

-- 
Ville Syrjälä
Intel

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

[Thomas Zimmermann]

Hi

Am 30.10.26 um 14:48 schrieb Ville Syrjälä:
[...]
>> And how would drivers know that the values are garbage before processing
>> them?
> That's why I added the device wide mode_valid hook. Drivers can
> use that to reject garbage before it reaches anything more
> sensitive. I have somewhat hard time believing a mode that is
> acceptable/reasonable to some real world hardware would fail
> in the vblank calculations.

I can think of some drivers with fixed display mode, such as a panel or 
the sysfb drivers. If they use vblank timers they might run into that 
situation.  But it's really a corner case.

>
> Hmm, and I just realized that I haven't actually added any low limit
> for the dotclock in i915. I'll go do that now. I think ~10 MHz is
> probably a reasonable enough number I can use there.
>
>> Even if a driver could program them to hardware, the vblank
>> helpers could still not process them correctly. Thus leaving the
>> userspace dis-functional.
>>
>> I see that we might want a common test function
>> drm_vblank_check_display_mode() and put it in some central place in the
>> atomic_check pipeline. At least we'd separate the testing from the
>> calculation.
> Yeah, something like that could work. Though I still
> think it'd be better to do it from the driver's mode_valid
> hook to reject the bogus stuff even earlier.
>
> That would also filter them out from the connectors' mode
> lists. But I suppose you could do the vblank check in
> the connector mode filtering unconditionally without
> having the driver do it. Or does the buggy userspace also
> need the kernel to return bogus modes from getconnector?

How about we add a dedicated mode_fixup helper for vblank validation? It 
would run after new CRTC's mode_fixup callback. [1] There we could try 
to adapt the CRTC's adjusted_mode.  Since we know the bug exactly, we 
can try to detect it, fix it, and document it.

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

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 v4] drm/vblank: Don't arm vblank timer with invalid frame duration

[Thomas Zimmermann]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]



Am 30.10.26 um 15:25 schrieb Thomas Zimmermann:
[...]
> How about we add a dedicated mode_fixup helper for vblank validation? 
> It would run after new CRTC's mode_fixup callback. [1] There we could 
> try to adapt the CRTC's adjusted_mode.  Since we know the bug exactly, 
> we can try to detect it, fix it, and document it.
>

The attached patch implements this idea. It does work in the sense that 
it doesn't immediately break my testing setup.

Thorsten (or anyone else) could you please test if this patch also 
resolve the bug with SPICE?



>>
>

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


[-- Attachment #2: 0001-drm-vblank-Don-t-arm-vblank-timer-with-invalid-frame.patch --]
[-- Type: text/x-patch, Size: 6100 bytes --]

From 2a76d04bfc3630c1d6bb79b3d64ecbcadf860090 Mon Sep 17 00:00:00 2001
From: Roman Ilin <me@romanilin.is>
Date: Thu, 2 Jul 2026 21:10:27 +0300
Subject: [PATCH] drm/vblank: Don't arm vblank timer with invalid frame
 duration

When a CRTC's display mode carries a too small pixel clock,
drm_calc_timestamping_constants() computes a frame duration that
exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
interval, after which vblank events are no longer delivered. Pending
page flips never complete and the display appears frozen.

This could be triggered on virtio-gpu guests that have dynamic resolution
enabled: when the SPICE agent or the X server resizes the output, it
submits a mode whose pixel clock is off by a factor of 1000, e.g.:

    clock = 406 kHz, htotal = 3152, vtotal = 2148

    framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)

16675852216 does not fit into an int and wraps to roughly -504000000.
ns_to_ktime() then yields a negative interval and the timer stops working.

Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
Use vblank timer"). That commit merely made virtio-gpu use the vblank
timer and thereby exposed the pre-existing problem in the timer setup
added by commit 74afeb812850 ("drm/vblank: Add vblank timer").

To fix this, introduce drm_crtc_vblank_mode_fixup() to detect and adjust
the bogus pixel clock. The helper runs unconditionally at the beginning
of the mode-fixup code. Drivers still have the opportunity to detect bogus
adjustments in later mode-fixup steps.

v5:
- fix up the broken pixel clock beforehand (Ville)
v4:
- avoid log spamming with drm_warn_once()

Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
Signed-off-by: Roman Ilin <me@romanilin.is>

mode_fixup

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

This reverts commit f3ca78682a4f9607444a3dafbe12ab6acdb1fa2f.
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 +++++
 drivers/gpu/drm/drm_vblank.c        | 68 +++++++++++++++++++++++++++++
 include/drm/drm_vblank.h            |  2 +
 3 files changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9d006f98413a..6a0f04a72292 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -432,6 +432,17 @@ mode_fixup(struct drm_atomic_commit *state)
 			continue;
 
 		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
+
+		if (!new_crtc_state->enable)
+			continue;
+
+		ret = drm_crtc_vblank_mode_fixup(crtc, &new_crtc_state->mode,
+						 &new_crtc_state->adjusted_mode);
+		if (!ret) {
+			drm_dbg_atomic(crtc->dev, "[CRTC:%d:%s] vblank fixup failed\n",
+				       crtc->base.id, crtc->name);
+			return -EINVAL;
+		}
 	}
 
 	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f90fb2d13e42..6e826c3b6c42 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -620,6 +620,74 @@ wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
 
+static int drm_vblank_validate_dotclock(u16 crtc_htotal, u16 crtc_vtotal, u32 flags, int dotclock)
+{
+	unsigned int frame_size;
+	u64 linedur_ns, framedur_ns;
+
+	if (dotclock <= 0)
+		return -EINVAL;
+
+	frame_size = (unsigned int)crtc_htotal * (unsigned int)crtc_vtotal;
+
+	/*
+	 * Convert scanline length in pixels and video dot clock to line duration
+	 * and frame duration in nanoseconds.
+	 */
+	linedur_ns  = div_u64((u64)crtc_htotal * 1000000, dotclock);
+	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
+
+	/*
+	 * Fields of interlaced scanout modes are only half a frame duration.
+	 */
+	if (flags & DRM_MODE_FLAG_INTERLACE)
+		framedur_ns /= 2;
+
+	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX)
+		return -ERANGE; /* dot clock too low */
+
+	return 0;
+}
+
+bool drm_crtc_vblank_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode)
+{
+	struct drm_device *dev = crtc->dev;
+	unsigned int pipe = drm_crtc_index(crtc);
+	int dotclock = adjusted_mode->crtc_clock;
+	int ret;
+
+	if (!drm_dev_has_vblank(dev))
+		return true;
+
+	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
+		return false;
+
+	ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
+					   adjusted_mode->crtc_vtotal,
+					   adjusted_mode->flags, dotclock);
+	if (ret == -ERANGE) {
+		int dotclock_1000;
+
+		/*
+		 * SPICE' vd_agent has a bug where it specifies the dot clock in kHz
+		 * instead of Hz. Setting up a vblank interrupt or timer for such a
+		 * mode overflows the calculation in drm_calc_timestamping_constants().
+		 * An errno code of ERANGE specifically points to this problem. Try
+		 * converting the dot clock to Hz and validate again.
+		 */
+		if (check_mul_overflow(dotclock, 1000, &dotclock_1000)) {
+			ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
+							   adjusted_mode->crtc_vtotal,
+							   adjusted_mode->flags, dotclock_1000);
+			if (!ret)
+				adjusted_mode->crtc_clock = dotclock_1000;
+		}
+	}
+
+	return !ret;
+}
+EXPORT_SYMBOL(drm_crtc_vblank_mode_fixup);
 
 /**
  * drm_calc_timestamping_constants - calculate vblank timestamp constants
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 2fcef9c0f5b1..a3628c477f59 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -311,6 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
 u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
 void drm_crtc_vblank_restore(struct drm_crtc *crtc);
 
+bool drm_crtc_vblank_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode);
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode);
 wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/vblank: inverted check_mul_overflow() in drm_crtc_vblank_mode_fixup()
- [High] drm/vblank: drm_crtc_vblank_start_timer() overwrites fixed-up framedur_ns
- [High] drm/atomic-helper: premature drm_crtc_vblank_mode_fixup() in mode_fixup()
- [Low] drm/vblank: uncleaned email reply, revert notes, and missing author sign-off
--

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

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

This commit introduces drm_crtc_vblank_mode_fixup() and calls it from
mode_fixup() in drm_atomic_helper.c to validate and adjust display mode
dotclocks that would otherwise overflow the frame duration calculation in
drm_calc_timestamping_constants().

>     drm/vblank: Don't arm vblank timer with invalid frame duration
>     
>     Am 30.10.26 um 15:25 schrieb Thomas Zimmermann:
>     [...]
>     > How about we add a dedicated mode_fixup helper for vblank validation?
>     > It would run after new CRTC's mode_fixup callback. [1] There we could
>     > try to adapt the CRTC's adjusted_mode.  Since we know the bug exactly,
>     > we can try to detect it, fix it, and document it.
>     >
>     
>     The attached patch implements this idea. It does work in the sense that
>     it doesn't immediately break my testing setup.
>     
>     Thorsten (or anyone else) could you please test if this patch also
>     resolve the bug with SPICE?
>     
>     >>
>     >
>     
>     --
>     --
>     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)
>     
>     When a CRTC's display mode carries a too small pixel clock,
>     drm_calc_timestamping_constants() computes a frame duration that
>     exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
>     drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
>     interval, after which vblank events are no longer delivered. Pending
>     page flips never complete and the display appears frozen.
>     
>     This could be triggered on virtio-gpu guests that have dynamic resolution
>     enabled: when the SPICE agent or the X server resizes the output, it
>     submits a mode whose pixel clock is off by a factor of 1000, e.g.:
>     
>         clock = 406 kHz, htotal = 3152, vtotal = 2148
>     
>         framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
>     
>     16675852216 does not fit into an int and wraps to roughly -504000000.
>     ns_to_ktime() then yields a negative interval and the timer stops working.
>     
>     Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
>     Use vblank timer"). That commit merely made virtio-gpu use the vblank
>     timer and thereby exposed the pre-existing problem in the timer setup
>     added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
>     
>     To fix this, introduce drm_crtc_vblank_mode_fixup() to detect and adjust
>     the bogus pixel clock. The helper runs unconditionally at the beginning
>     of the mode-fixup code. Drivers still have the opportunity to detect bogus
>     adjustments in later mode-fixup steps.
>     
>     v5:
>     - fix up the broken pixel clock beforehand (Ville)
>     v4:
>     - avoid log spamming with drm_warn_once()
>     
>     Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
>     Signed-off-by: Roman Ilin <me@romanilin.is>
>     
>     mode_fixup
>     
>     Revert "drm/vblank: Don't arm vblank timer with invalid frame duration"
>     
>     This reverts commit f3ca78682a4f9607444a3dafbe12ab6acdb1fa2f.

[Severity: Low]
This isn't a bug in the code, but the commit message includes an uncleaned
email reply header and signature block at the top, trailing git squash and
revert scratch notes at the bottom, and is missing a Signed-off-by tag from
the commit author. Should the commit message be cleaned up and include the
author's sign-off?

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a5..6a0f04a72292b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -432,6 +432,17 @@ mode_fixup(struct drm_atomic_commit *state)
>  			continue;
>  
>  		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
> +
> +		if (!new_crtc_state->enable)
> +			continue;
> +
> +		ret = drm_crtc_vblank_mode_fixup(crtc, &new_crtc_state->mode,
> +						 &new_crtc_state->adjusted_mode);
> +		if (!ret) {
> +			drm_dbg_atomic(crtc->dev, "[CRTC:%d:%s] vblank fixup failed\n",
> +				       crtc->base.id, crtc->name);
> +			return -EINVAL;
> +		}
>  	}

[Severity: High]
Could running drm_crtc_vblank_mode_fixup() at the beginning of mode_fixup()
before bridge, encoder, and CRTC callbacks cause regressions for callers that
populate or recompute crtc_clock later in the atomic check?

For example, in-kernel callers and KUnit tests such as
vc4_mock_atomic_add_output() in drivers/gpu/drm/vc4/tests/vc4_mock_output.c
set up a mode with DRM_SIMPLE_MODE() where crtc_clock is 0:

drivers/gpu/drm/vc4/tests/vc4_mock_output.c:vc4_mock_atomic_add_output() {
	...
	ret = drm_atomic_set_mode_for_crtc(crtc_state, &default_mode);
	if (ret)
		return ret;
	...
}

Similarly, vmw_du_crtc_atomic_check() in drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
populates adjusted_mode.crtc_clock during crtc->atomic_check(), which runs in
drm_atomic_helper_check_planes() after mode_fixup():

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:vmw_du_crtc_atomic_check() {
	...
	if (new_state->mode.crtc_clock == 0)
		new_state->adjusted_mode.crtc_clock = new_state->mode.clock;
	...
}

Because adjusted_mode.crtc_clock is still 0 at the start of mode_fixup(),
drm_vblank_validate_dotclock() will return -EINVAL and fail the check before
those callbacks run.

In addition, drm_crtc_vblank_mode_fixup() only adjusts
adjusted_mode->crtc_clock while leaving adjusted_mode->clock unchanged. If a
subsequent bridge, encoder, or CRTC mode_fixup() callback calls
drm_mode_set_crtcinfo(adjusted_mode, ...), won't that overwrite
adjusted_mode->crtc_clock with the unadjusted adjusted_mode->clock and leave
any driver-adjusted timings unvalidated?

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e423..6e826c3b6c425 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -620,6 +620,74 @@ wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>  
> +static int drm_vblank_validate_dotclock(u16 crtc_htotal, u16 crtc_vtotal, u32 flags, int dotclock)
> +{
> +	unsigned int frame_size;
> +	u64 linedur_ns, framedur_ns;
> +
> +	if (dotclock <= 0)
> +		return -EINVAL;
> +
> +	frame_size = (unsigned int)crtc_htotal * (unsigned int)crtc_vtotal;
> +
> +	/*
> +	 * Convert scanline length in pixels and video dot clock to line duration
> +	 * and frame duration in nanoseconds.
> +	 */
> +	linedur_ns  = div_u64((u64)crtc_htotal * 1000000, dotclock);
> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
> +
> +	/*
> +	 * Fields of interlaced scanout modes are only half a frame duration.
> +	 */
> +	if (flags & DRM_MODE_FLAG_INTERLACE)
> +		framedur_ns /= 2;
> +
> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX)
> +		return -ERANGE; /* dot clock too low */
> +
> +	return 0;
> +}
> +
> +bool drm_crtc_vblank_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode,
> +				struct drm_display_mode *adjusted_mode)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	int dotclock = adjusted_mode->crtc_clock;
> +	int ret;
> +
> +	if (!drm_dev_has_vblank(dev))
> +		return true;
> +
> +	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> +		return false;
> +
> +	ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
> +					   adjusted_mode->crtc_vtotal,
> +					   adjusted_mode->flags, dotclock);
> +	if (ret == -ERANGE) {
> +		int dotclock_1000;
> +
> +		/*
> +		 * SPICE' vd_agent has a bug where it specifies the dot clock in kHz
> +		 * instead of Hz. Setting up a vblank interrupt or timer for such a
> +		 * mode overflows the calculation in drm_calc_timestamping_constants().
> +		 * An errno code of ERANGE specifically points to this problem. Try
> +		 * converting the dot clock to Hz and validate again.
> +		 */
> +		if (check_mul_overflow(dotclock, 1000, &dotclock_1000)) {

[Severity: High]
Is the check_mul_overflow() condition inverted here?

check_mul_overflow() returns true when the multiplication overflows and false
when it succeeds without overflow. When SPICE vd_agent passes a small
dotclock such as 406, multiplying 406 by 1000 does not overflow, so
check_mul_overflow(dotclock, 1000, &dotclock_1000) returns false. As a
result, the fixup block is skipped, ret remains -ERANGE,
drm_crtc_vblank_mode_fixup() returns false, and mode_fixup() rejects the
modeset with -EINVAL.

> +			ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
> +							   adjusted_mode->crtc_vtotal,
> +							   adjusted_mode->flags, dotclock_1000);
> +			if (!ret)
> +				adjusted_mode->crtc_clock = dotclock_1000;

[Severity: High]
Even if adjusted_mode->crtc_clock is updated here, won't
drm_crtc_vblank_start_timer() later overwrite vblank->framedur_ns using the
unadjusted crtc->mode?

During an atomic commit, drm_atomic_helper_update_legacy_modeset_state() in
drivers/gpu/drm/drm_atomic_helper.c copies the unadjusted mode into
crtc->mode:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_update_legacy_modeset_state() {
	...
	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
		...
		crtc->mode = new_crtc_state->mode;
		crtc->enabled = new_crtc_state->enable;
		...
	}
}

Although drm_atomic_helper_calc_timestamping_constants() calculates
vblank->framedur_ns from new_crtc_state->adjusted_mode, whenever vblank is
subsequently enabled on the CRTC via
drm_crtc_vblank_get()->drm_vblank_enable()->__enable_vblank()->drm_crtc_vblank_helper_enable_vblank_timer()->drm_crtc_vblank_start_timer(),
drm_crtc_vblank_start_timer() recomputes the timestamping constants from
&crtc->mode:

drivers/gpu/drm/drm_vblank.c:drm_crtc_vblank_start_timer() {
	...
	drm_calc_timestamping_constants(crtc, &crtc->mode);

	spin_lock_irqsave(&vtimer->interval_lock, flags);
	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
	spin_unlock_irqrestore(&vtimer->interval_lock, flags);

	hrtimer_start(&vtimer->timer, vtimer->interval, HRTIMER_MODE_REL);
	...
}

Since crtc->mode still holds the unadjusted crtc_clock, won't this clobber
vblank->framedur_ns with the overflowed negative duration and arm the timer
with a negative interval anyway?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/c874d311-f7b1-4a24-ab75-b502ba1c5e58@suse.de?part=1

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

[Thorsten Leemhuis]

On 10/30/26 16:33, Thomas Zimmermann wrote:
> Am 30.10.26 um 15:25 schrieb Thomas Zimmermann:
> [...]
>> How about we add a dedicated mode_fixup helper for vblank validation?
>> It would run after new CRTC's mode_fixup callback. [1] There we could
>> try to adapt the CRTC's adjusted_mode.  Since we know the bug exactly,
>> we can try to detect it, fix it, and document it.
>>
> 
> The attached patch implements this idea. It does work in the sense that
> it doesn't immediately break my testing setup.
> 
> Thorsten (or anyone else) could you please test if this patch also
> resolve the bug with SPICE?
Sorry, can't help you here, I'm not affected; I only cared because I
keep an eye on regressions in general -- and a few weeks ago it looked
like all that was needed to resolve this one was a relative easy
adjustment and a repost of an already tested fix.

Ohh well, that turned out to not be the case. Sorry.

Given that the original reporter is MIA (iirc) I wonder if we should
just leave things here as they are until someone shows up that is
affected who can also test the patch.

Ciao, Thorsten

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

[Ville Syrjälä]

On Fri, Oct 30, 2026 at 04:33:24PM +0100, Thomas Zimmermann wrote:
> 
> 
> Am 30.10.26 um 15:25 schrieb Thomas Zimmermann:
> [...]
> > How about we add a dedicated mode_fixup helper for vblank validation? 
> > It would run after new CRTC's mode_fixup callback. [1] There we could 
> > try to adapt the CRTC's adjusted_mode.  Since we know the bug exactly, 
> > we can try to detect it, fix it, and document it.
> >
> 
> The attached patch implements this idea. It does work in the sense that 
> it doesn't immediately break my testing setup.
> 
> Thorsten (or anyone else) could you please test if this patch also 
> resolve the bug with SPICE?
> 
> 
> 
> >>
> >
> 
> -- 
> --
> 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)
> 

> From 2a76d04bfc3630c1d6bb79b3d64ecbcadf860090 Mon Sep 17 00:00:00 2001
> From: Roman Ilin <me@romanilin.is>
> Date: Thu, 2 Jul 2026 21:10:27 +0300
> Subject: [PATCH] drm/vblank: Don't arm vblank timer with invalid frame
>  duration
> 
> When a CRTC's display mode carries a too small pixel clock,
> drm_calc_timestamping_constants() computes a frame duration that
> exceeds INT_MAX. drm_vblank_crtc.framedur_ns becomes negative.
> drm_crtc_vblank_start_timer() then arms the vblank hrtimer with this
> interval, after which vblank events are no longer delivered. Pending
> page flips never complete and the display appears frozen.
> 
> This could be triggered on virtio-gpu guests that have dynamic resolution
> enabled: when the SPICE agent or the X server resizes the output, it
> submits a mode whose pixel clock is off by a factor of 1000, e.g.:
> 
>     clock = 406 kHz, htotal = 3152, vtotal = 2148
> 
>     framedur_ns = 3152 * 2148 * 1000000 / 406 = 16675852216 ns (~16.7 s)
> 
> 16675852216 does not fit into an int and wraps to roughly -504000000.
> ns_to_ktime() then yields a negative interval and the timer stops working.
> 
> Found by bisection, which pointed at commit a036f5fceedb ("drm/virtgpu:
> Use vblank timer"). That commit merely made virtio-gpu use the vblank
> timer and thereby exposed the pre-existing problem in the timer setup
> added by commit 74afeb812850 ("drm/vblank: Add vblank timer").
> 
> To fix this, introduce drm_crtc_vblank_mode_fixup() to detect and adjust
> the bogus pixel clock. The helper runs unconditionally at the beginning
> of the mode-fixup code. Drivers still have the opportunity to detect bogus
> adjustments in later mode-fixup steps.
> 
> v5:
> - fix up the broken pixel clock beforehand (Ville)
> v4:
> - avoid log spamming with drm_warn_once()
> 
> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
> Signed-off-by: Roman Ilin <me@romanilin.is>
> 
> mode_fixup
> 
> Revert "drm/vblank: Don't arm vblank timer with invalid frame duration"
> 
> This reverts commit f3ca78682a4f9607444a3dafbe12ab6acdb1fa2f.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 +++++
>  drivers/gpu/drm/drm_vblank.c        | 68 +++++++++++++++++++++++++++++
>  include/drm/drm_vblank.h            |  2 +
>  3 files changed, 81 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a..6a0f04a72292 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -432,6 +432,17 @@ mode_fixup(struct drm_atomic_commit *state)
>  			continue;
>  
>  		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
> +
> +		if (!new_crtc_state->enable)
> +			continue;
> +
> +		ret = drm_crtc_vblank_mode_fixup(crtc, &new_crtc_state->mode,
> +						 &new_crtc_state->adjusted_mode);

We don't want this hack applied to all drivers. It's a problem
for a single driver, so that one driver should handle it.

> +		if (!ret) {
> +			drm_dbg_atomic(crtc->dev, "[CRTC:%d:%s] vblank fixup failed\n",
> +				       crtc->base.id, crtc->name);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e42..6e826c3b6c42 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -620,6 +620,74 @@ wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>  
> +static int drm_vblank_validate_dotclock(u16 crtc_htotal, u16 crtc_vtotal, u32 flags, int dotclock)
> +{
> +	unsigned int frame_size;
> +	u64 linedur_ns, framedur_ns;
> +
> +	if (dotclock <= 0)
> +		return -EINVAL;
> +
> +	frame_size = (unsigned int)crtc_htotal * (unsigned int)crtc_vtotal;
> +
> +	/*
> +	 * Convert scanline length in pixels and video dot clock to line duration
> +	 * and frame duration in nanoseconds.
> +	 */
> +	linedur_ns  = div_u64((u64)crtc_htotal * 1000000, dotclock);
> +	framedur_ns = div_u64((u64)frame_size * 1000000, dotclock);
> +
> +	/*
> +	 * Fields of interlaced scanout modes are only half a frame duration.
> +	 */
> +	if (flags & DRM_MODE_FLAG_INTERLACE)
> +		framedur_ns /= 2;
> +
> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX)
> +		return -ERANGE; /* dot clock too low */
> +
> +	return 0;
> +}
> +
> +bool drm_crtc_vblank_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode,
> +				struct drm_display_mode *adjusted_mode)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	int dotclock = adjusted_mode->crtc_clock;
> +	int ret;
> +
> +	if (!drm_dev_has_vblank(dev))
> +		return true;
> +
> +	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> +		return false;
> +
> +	ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
> +					   adjusted_mode->crtc_vtotal,
> +					   adjusted_mode->flags, dotclock);
> +	if (ret == -ERANGE) {
> +		int dotclock_1000;
> +
> +		/*
> +		 * SPICE' vd_agent has a bug where it specifies the dot clock in kHz
> +		 * instead of Hz. Setting up a vblank interrupt or timer for such a
> +		 * mode overflows the calculation in drm_calc_timestamping_constants().
> +		 * An errno code of ERANGE specifically points to this problem. Try
> +		 * converting the dot clock to Hz and validate again.
> +		 */
> +		if (check_mul_overflow(dotclock, 1000, &dotclock_1000)) {
> +			ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
> +							   adjusted_mode->crtc_vtotal,
> +							   adjusted_mode->flags, dotclock_1000);
> +			if (!ret)
> +				adjusted_mode->crtc_clock = dotclock_1000;
> +		}
> +	}
> +
> +	return !ret;
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_mode_fixup);
>  
>  /**
>   * drm_calc_timestamping_constants - calculate vblank timestamp constants
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 2fcef9c0f5b1..a3628c477f59 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -311,6 +311,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc);
>  u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>  void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>  
> +bool drm_crtc_vblank_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode,
> +				struct drm_display_mode *adjusted_mode);
>  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode);
>  wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
> -- 
> 2.56.0
> 


-- 
Ville Syrjälä
Intel