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 <[email protected]> > 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 <[email protected]> > > 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
