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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.10.26 um 08:49 schrieb Thorsten Leemhuis:
> 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 sits on the list waiting for
> review afaics:
>
> https://lore.kernel.org/all/20260914110004.211027-1-tzimmermann@suse.de/
>
> Can you maybe prod the right people to finally get this resolved? Or
> what should be do instead?

The right people are in cc already. It's just that no one reviewed. I'll 
ping them to get this resolved.

Best regards
Thomas

>
> 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
>>>>>>>>

-- 
--
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)