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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 11.09.26 um 09:39 schrieb Thorsten Leemhuis:
> 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!

I've now sent out an update with minor changes.

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)