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

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



>>>>> 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@xxxxxxxxxxxx> 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@xxxxxxx>
>>>>>>> Signed-off-by: Roman Ilin <me@xxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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
>>>>>>>
>