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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 30, 2026 at 02:04:04PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 30.10.26 um 13:07 schrieb Ville Syrjälä:
> > On Thu, Oct 29, 2026 at 12:35:52PM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 28.10.26 um 15:54 schrieb Ville Syrjälä:
> >>> On Mon, Sep 14, 2026 at 12:57:26PM +0200, Thomas Zimmermann wrote:
> >>>> From: Roman Ilin <me@romanilin.is>
> >>>>
> >>>> 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
> >>> How about actually "fixing" the clock in the driver?
> >> It's not a broken driver, but user space. There is a bug report open for
> >> spice to replace the incorrect units with correct ones. [1] The fix here
> >> is supposed to resolve the kernel problem this triggers.
> > Life becomes much simpler if you don't let garbage userspace input
> > past some initial sanity checks. So technically the driver should
> > just reject these modes. But in this case since you need to keep
> > the broken userspace working the driver should just fix it up,
> > and do so early so you don't have to deal with it anywhere else.
> 
> Not that I disagree, with the statement in general. But doing this here 
> seems like whack-a-mole if we leave it to individual drivers.

I think you're just trading that whack-a-mole for a worse one
where you now have to make all kinds of low level functions
deal with the garbage input.

This particular function is never expected to fail, which is
why it returns void. And where it gets called in i915 we
can't even handle the failure, so changing the return type
to int is making the whole thing confusing.

One driver independent solution would be to have 
drm_mode_validate_basic() validate the dotclock a bit hadder,
but that will not work for this case because you need to keep
allowing these bogus modes to reach the driver.

Just the other day I was actually thinking of adding some basic
frame duration checks into drm_mode_validate_basic(), but I guess
I can't do that given this userspace bug.

> And the 
> userspace is already dis-functional, no need to keep supporting it, I think.
> 
> And how would drivers know that the values are garbage before processing 
> them? 

That's why I added the device wide mode_valid hook. Drivers can
use that to reject garbage before it reaches anything more
sensitive. I have somewhat hard time believing a mode that is
acceptable/reasonable to some real world hardware would fail 
in the vblank calculations.

Hmm, and I just realized that I haven't actually added any low limit
for the dotclock in i915. I'll go do that now. I think ~10 MHz is
probably a reasonable enough number I can use there.

> Even if a driver could program them to hardware, the vblank 
> helpers could still not process them correctly. Thus leaving the 
> userspace dis-functional.
> 
> I see that we might want a common test function 
> drm_vblank_check_display_mode() and put it in some central place in the 
> atomic_check pipeline. At least we'd separate the testing from the 
> calculation.

Yeah, something like that could work. Though I still
think it'd be better to do it from the driver's mode_valid
hook to reject the bogus stuff even earlier.

That would also filter them out from the connectors' mode
lists. But I suppose you could do the vblank check in
the connector mode filtering unconditionally without
having the driver do it. Or does the buggy userspace also
need the kernel to return bogus modes from getconnector?

-- 
Ville Syrjälä
Intel