On 9/14/26 13:00, Thomas Zimmermann wrote: > Am 11.09.26 um 09:39 schrieb Thorsten Leemhuis: >> On 9/4/26 08:19, Thomas Zimmermann wrote: >>> Am 03.09.26 um 16:07 schrieb Thorsten Leemhuis: >>>> On 7/3/26 09:26, Thomas Zimmermann wrote: >>>>> Am 02.07.26 um 20:23 schrieb Roman Ilin: >>>>>> Apologies, I accidentally fired off send-email before saving my final >>>>>> changelog notes. Please ignore the changelog and notes in the >>>>>> original >>>>>> v3 email. >>>>> We want to know when this happens. And user space will only be able to >>>>> trigger this once, so there's no risk of spamming the kernel log. >>>>>> Hmmm, just wondering: what's the status here? >>>> Roman Ilin, did you loose interest? Or was this continued somewhere or >>>> maybe even resolved and I just missed that? >>>> >>>> Thomas: if Roman left this behind, could you maybe handle this, [...] >>> AFAICT it was almost there except for the error messaging. The v4 then >>> never happened. I can finish the patch if Roman is no longer around. >> No answer from Roman since then, so yes, that would be great! Tia! > > I've now sent out an update with minor changes.
Many thx again. But the thing is: it still s >>>>> So this is not really a problem. drm_WARN_ON_ONCE was ok for that. You >>>>> can also use a regular DRM print macro. But instead of drm_dbg_kms() >>>>> should use drm_err_once(). But please also output linedur_ns and >>>>> framedur_ns in the error. We want to know which of them is incorrect. >>>>> You can also add more information to the error message. See [1] for >>>>> the >>>>> mode-formating macros. >>>>> >>>>> [1] https://elixir.bootlin.com/linux/v7.1.2/source/include/drm/ >>>>> drm_modes.h#L422 >>>>> >>>>> >>>>>> - Updated drm_calc_timestamping_constants with the goto error >>>>>> fallback >>>>>> to clear the stale state. >>>>>> >>>>>> Also, an automated review bot pointed out an AB-BA deadlock in >>>>>> drm_crtc_vblank_start_timer(). But I am leaving this out of the >>>>>> patch to >>>>>> keep the fixes orthogonal. >>>>>> >>>>>> Sorry for the noise. >>>>>> >>>>>>> On Jul 2, 2026, at 21:10, Roman Ilin <[email protected]> wrote: >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> Fixes: 74afeb812850 ("drm/vblank: Add vblank timer") >>>>>>> Suggested-by: Thomas Zimmermann <[email protected]> >>>>>>> Signed-off-by: Roman Ilin <[email protected]> >>>>>>> --- >>>>>>> Changes in v3: >>>>>>> >>>>>>> - Changed the WARN_ON_ONCE to drm_err_once >>>>>>> >>>>>>> Notes: >>>>>>> >>>>>>> >>>>>>> >>>>>>> drivers/gpu/drm/drm_vblank.c | 71 ++++++++++++++++++++++ >>>>>>> +------------- >>>>>>> include/drm/drm_vblank.h | 4 +- >>>>>>> 2 files changed, 48 insertions(+), 27 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/ >>>>>>> drm_vblank.c >>>>>>> index f90fb2d13..629b9fcc7 100644 >>>>>>> --- a/drivers/gpu/drm/drm_vblank.c >>>>>>> +++ b/drivers/gpu/drm/drm_vblank.c >>>>>>> @@ -631,42 +631,51 @@ 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(dev, "crtc %u: Can't calculate constants, dotclock = >>>>>>> %d!\n", >>>>> Please turn this into drm_err_once() because this call can actually be >>>>> triggered repeatedly from userspace. >>>>> >>>>> Best regards >>>>> Thomas >>>>> >>>>> >>>>>>> + 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_dbg_kms(dev, "crtc %u: Can't calculate constants, mode clock >>>>>>> too small!\n", >>>>>>> + crtc->base.id); >>>>>>> + goto error; >>>>>>> } >>>>>>> >>>>>>> vblank->linedur_ns = linedur_ns; >>>>>>> @@ -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 2fcef9c0f..d99772dfa 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); >>>>>>> -- >>>>>>> 2.54.0 >>>>>>> >
