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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/vblank: inverted check_mul_overflow() in drm_crtc_vblank_mode_fixup()
- [High] drm/vblank: drm_crtc_vblank_start_timer() overwrites fixed-up framedur_ns
- [High] drm/atomic-helper: premature drm_crtc_vblank_mode_fixup() in mode_fixup()
- [Low] drm/vblank: uncleaned email reply, revert notes, and missing author sign-off
--

commit a8f16692f361f62f9c1ca7a34a1c2c7c62ba5c70
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/vblank: Don't arm vblank timer with invalid frame duration

This commit introduces drm_crtc_vblank_mode_fixup() and calls it from
mode_fixup() in drm_atomic_helper.c to validate and adjust display mode
dotclocks that would otherwise overflow the frame duration calculation in
drm_calc_timestamping_constants().

>     drm/vblank: Don't arm vblank timer with invalid frame duration
>     
>     Am 30.10.26 um 15:25 schrieb Thomas Zimmermann:
>     [...]
>     > How about we add a dedicated mode_fixup helper for vblank validation?
>     > It would run after new CRTC's mode_fixup callback. [1] There we could
>     > try to adapt the CRTC's adjusted_mode.  Since we know the bug exactly,
>     > we can try to detect it, fix it, and document it.
>     >
>     
>     The attached patch implements this idea. It does work in the sense that
>     it doesn't immediately break my testing setup.
>     
>     Thorsten (or anyone else) could you please test if this patch also
>     resolve the bug with SPICE?
>     
>     >>
>     >
>     
>     --
>     --
>     Thomas Zimmermann
>     Graphics Driver Developer
>     SUSE Software Solutions Germany GmbH
>     Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
>     GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)
>     
>     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, introduce drm_crtc_vblank_mode_fixup() to detect and adjust
>     the bogus pixel clock. The helper runs unconditionally at the beginning
>     of the mode-fixup code. Drivers still have the opportunity to detect bogus
>     adjustments in later mode-fixup steps.
>     
>     v5:
>     - fix up the broken pixel clock beforehand (Ville)
>     v4:
>     - avoid log spamming with drm_warn_once()
>     
>     Fixes: 74afeb812850 ("drm/vblank: Add vblank timer")
>     Signed-off-by: Roman Ilin <me@romanilin.is>
>     
>     mode_fixup
>     
>     Revert "drm/vblank: Don't arm vblank timer with invalid frame duration"
>     
>     This reverts commit f3ca78682a4f9607444a3dafbe12ab6acdb1fa2f.

[Severity: Low]
This isn't a bug in the code, but the commit message includes an uncleaned
email reply header and signature block at the top, trailing git squash and
revert scratch notes at the bottom, and is missing a Signed-off-by tag from
the commit author. Should the commit message be cleaned up and include the
author's sign-off?

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a5..6a0f04a72292b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -432,6 +432,17 @@ mode_fixup(struct drm_atomic_commit *state)
>  			continue;
>  
>  		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
> +
> +		if (!new_crtc_state->enable)
> +			continue;
> +
> +		ret = drm_crtc_vblank_mode_fixup(crtc, &new_crtc_state->mode,
> +						 &new_crtc_state->adjusted_mode);
> +		if (!ret) {
> +			drm_dbg_atomic(crtc->dev, "[CRTC:%d:%s] vblank fixup failed\n",
> +				       crtc->base.id, crtc->name);
> +			return -EINVAL;
> +		}
>  	}

[Severity: High]
Could running drm_crtc_vblank_mode_fixup() at the beginning of mode_fixup()
before bridge, encoder, and CRTC callbacks cause regressions for callers that
populate or recompute crtc_clock later in the atomic check?

For example, in-kernel callers and KUnit tests such as
vc4_mock_atomic_add_output() in drivers/gpu/drm/vc4/tests/vc4_mock_output.c
set up a mode with DRM_SIMPLE_MODE() where crtc_clock is 0:

drivers/gpu/drm/vc4/tests/vc4_mock_output.c:vc4_mock_atomic_add_output() {
	...
	ret = drm_atomic_set_mode_for_crtc(crtc_state, &default_mode);
	if (ret)
		return ret;
	...
}

Similarly, vmw_du_crtc_atomic_check() in drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
populates adjusted_mode.crtc_clock during crtc->atomic_check(), which runs in
drm_atomic_helper_check_planes() after mode_fixup():

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:vmw_du_crtc_atomic_check() {
	...
	if (new_state->mode.crtc_clock == 0)
		new_state->adjusted_mode.crtc_clock = new_state->mode.clock;
	...
}

Because adjusted_mode.crtc_clock is still 0 at the start of mode_fixup(),
drm_vblank_validate_dotclock() will return -EINVAL and fail the check before
those callbacks run.

In addition, drm_crtc_vblank_mode_fixup() only adjusts
adjusted_mode->crtc_clock while leaving adjusted_mode->clock unchanged. If a
subsequent bridge, encoder, or CRTC mode_fixup() callback calls
drm_mode_set_crtcinfo(adjusted_mode, ...), won't that overwrite
adjusted_mode->crtc_clock with the unadjusted adjusted_mode->clock and leave
any driver-adjusted timings unvalidated?

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e423..6e826c3b6c425 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -620,6 +620,74 @@ wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_waitqueue);
>  
> +static int drm_vblank_validate_dotclock(u16 crtc_htotal, u16 crtc_vtotal, u32 flags, int dotclock)
> +{
> +	unsigned int frame_size;
> +	u64 linedur_ns, framedur_ns;
> +
> +	if (dotclock <= 0)
> +		return -EINVAL;
> +
> +	frame_size = (unsigned int)crtc_htotal * (unsigned int)crtc_vtotal;
> +
> +	/*
> +	 * Convert scanline length in pixels and video dot clock to line duration
> +	 * and frame duration in nanoseconds.
> +	 */
> +	linedur_ns  = div_u64((u64)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 (flags & DRM_MODE_FLAG_INTERLACE)
> +		framedur_ns /= 2;
> +
> +	if (linedur_ns > INT_MAX || framedur_ns > INT_MAX)
> +		return -ERANGE; /* dot clock too low */
> +
> +	return 0;
> +}
> +
> +bool drm_crtc_vblank_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode,
> +				struct drm_display_mode *adjusted_mode)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	int dotclock = adjusted_mode->crtc_clock;
> +	int ret;
> +
> +	if (!drm_dev_has_vblank(dev))
> +		return true;
> +
> +	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> +		return false;
> +
> +	ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
> +					   adjusted_mode->crtc_vtotal,
> +					   adjusted_mode->flags, dotclock);
> +	if (ret == -ERANGE) {
> +		int dotclock_1000;
> +
> +		/*
> +		 * SPICE' vd_agent has a bug where it specifies the dot clock in kHz
> +		 * instead of Hz. Setting up a vblank interrupt or timer for such a
> +		 * mode overflows the calculation in drm_calc_timestamping_constants().
> +		 * An errno code of ERANGE specifically points to this problem. Try
> +		 * converting the dot clock to Hz and validate again.
> +		 */
> +		if (check_mul_overflow(dotclock, 1000, &dotclock_1000)) {

[Severity: High]
Is the check_mul_overflow() condition inverted here?

check_mul_overflow() returns true when the multiplication overflows and false
when it succeeds without overflow. When SPICE vd_agent passes a small
dotclock such as 406, multiplying 406 by 1000 does not overflow, so
check_mul_overflow(dotclock, 1000, &dotclock_1000) returns false. As a
result, the fixup block is skipped, ret remains -ERANGE,
drm_crtc_vblank_mode_fixup() returns false, and mode_fixup() rejects the
modeset with -EINVAL.

> +			ret = drm_vblank_validate_dotclock(adjusted_mode->crtc_htotal,
> +							   adjusted_mode->crtc_vtotal,
> +							   adjusted_mode->flags, dotclock_1000);
> +			if (!ret)
> +				adjusted_mode->crtc_clock = dotclock_1000;

[Severity: High]
Even if adjusted_mode->crtc_clock is updated here, won't
drm_crtc_vblank_start_timer() later overwrite vblank->framedur_ns using the
unadjusted crtc->mode?

During an atomic commit, drm_atomic_helper_update_legacy_modeset_state() in
drivers/gpu/drm/drm_atomic_helper.c copies the unadjusted mode into
crtc->mode:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_update_legacy_modeset_state() {
	...
	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
		...
		crtc->mode = new_crtc_state->mode;
		crtc->enabled = new_crtc_state->enable;
		...
	}
}

Although drm_atomic_helper_calc_timestamping_constants() calculates
vblank->framedur_ns from new_crtc_state->adjusted_mode, whenever vblank is
subsequently enabled on the CRTC via
drm_crtc_vblank_get()->drm_vblank_enable()->__enable_vblank()->drm_crtc_vblank_helper_enable_vblank_timer()->drm_crtc_vblank_start_timer(),
drm_crtc_vblank_start_timer() recomputes the timestamping constants from
&crtc->mode:

drivers/gpu/drm/drm_vblank.c:drm_crtc_vblank_start_timer() {
	...
	drm_calc_timestamping_constants(crtc, &crtc->mode);

	spin_lock_irqsave(&vtimer->interval_lock, flags);
	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
	spin_unlock_irqrestore(&vtimer->interval_lock, flags);

	hrtimer_start(&vtimer->timer, vtimer->interval, HRTIMER_MODE_REL);
	...
}

Since crtc->mode still holds the unadjusted crtc_clock, won't this clobber
vblank->framedur_ns with the overflowed negative duration and arm the timer
with a negative interval anyway?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/c874d311-f7b1-4a24-ab75-b502ba1c5e58@suse.de?part=1