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

[Thorsten Leemhuis <regressions@leemhuis.info>]

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.
>>
>> The actual changes in v3 are:
>>
>> - Changed the WARN_ON_ONCE to drm_dbg_kms (to avoid user-triggerable
>>    panics).
> 
> 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, as it
fixes a regressions that iirc is caused by a change of yours? Or is it
fine for some reason to ignore this, even if the userland problem that
triggers this is still unfixed afaics? Side note: I by chance saw that
the latter a few days ago got a new comment from someone that ran into it.
https://gitlab.freedesktop.org/spice/linux/vd_agent/-/work_items/52

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 <me@romanilin.is> 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 <tzimmermann@suse.de>
>>> Signed-off-by: Roman Ilin <me@romanilin.is>
>>> ---
>>> 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
>>>
>