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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Thorsten

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

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.

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: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)