Hi Thomas! 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! Ciao, Thorsten >>> 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 >>>>> >
