(cc Ville)
Hi,
thanks for addressing the issue.
Am 14.06.26 um 00:44 schrieb Roman Ilin:
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.:
'off by' as in it should be in Hz rather than kHz.
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").
Reject a non-positive frame duration in drm_crtc_vblank_start_timer() and
return an error. enable_vblank then fails and the driver falls back to
sending the vblank event immediately, as it did before the vblank timer
was introduced. Valid modes are unaffected, and the timer self-heals on
the next mode that has a sane clock.
Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
Cc: [email protected]
Signed-off-by: Roman Ilin <[email protected]>
---
Notes:
Based on v7.1-rc7. Tested on 6.19 and 7.1-rc7.
Open questions:
This relies on the int overflow producing a negative value. The deeper
issue is that drm_calc_timestamping_constants() truncates framedur_ns to
int. Would you prefer to widen framedur_ns to s64, or to bound the
interval here (e.g. reject framedur_ns above one second) so that any
bogus interval is rejected regardless of sign?
Generally speaking, I think we should not accept such a bugos mode in
the first place. But this would require changes to the mode-setting code
that are too invasive for a bug fix.
So, if anything, we should try to detect the problem in
drm_calc_timestamping_constants(). Let's make the helper return errno
codes instead of failing silently. Within the helper, let's do the
following changes:
- declare frame_size an unsigned int
- declare linedur_ns and framedur_ns of type u64
This should avoid possible overflows in the code. And before assigning
linedur_ns and framedur_ns to the vblank fields, test them against INT_MAX.
Maybe at [1]. Using drm_WARN_ON_ONCE is likely a good idea for future
debugging.
if (drm_WARN_ON_ONCE(dev, linedur_ns > INT_MAX) || drm_WARN_ON_ONCE(dev,
framedur_ns > INT_MAX))
return -EINVAL
[1]
https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/drm_vblank.c#L662
Should virtio-gpu additionally sanitize the user-supplied clock in its
atomic_check (similar to vmwgfx for the clock==0 case) so the
vblank-timer throttling is preserved for these resizes, instead of
falling back to immediate events?
drivers/gpu/drm/drm_vblank.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f78bf37f1..557cd0bc8 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2235,7 +2235,13 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
drm_calc_timestamping_constants(crtc, &crtc->mode);
+ /*
+ * Return an error so the driver falls back to sending vblank events
+ * when a small mode clock yields a frame duration exceeding INT_MAX.
+ */
+ if (vblank->framedur_ns <= 0)
+ return -EINVAL;
Here, you would just forward the error upwards in the call stack.
Best regards
Thomas
+
spin_lock_irqsave(&vtimer->interval_lock, flags);
vtimer->interval = ns_to_ktime(vblank->framedur_ns);
spin_unlock_irqrestore(&vtimer->interval_lock, flags);
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
