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

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

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