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 <[email protected]> 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 <[email protected]> > > 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/[email protected]?part=1
