Re: [PATCH v2 11/27] drm: Implement VRR rate limiting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm: NULL pointer dereference on old_conn_state->crtc in hdmi_validate_vrr()
- [High] drm: zero-denominator WARN_ON() in cmp_fraction() from hdmi_validate_vrr()
- [Medium] drm: signed 32-bit overflow in cur_err in drm_crtc_helper_vrr_vsync()
- [Medium] drm: fixed-rate VRR validation bugs in hdmi_validate_vrr_limits()
- [Medium] drm: lost vrr_state updates due to in-place mutation in drm_crtc_helper_vrr_vsync()
- [Low] drm: redundant overflow check and unused limit_d in hdmi_validate_vrr_limits()
--

commit 41d8f0b98056d34444a098f24f445d7ca3d40b1e
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm: Implement VRR rate limiting

This commit implements VRR target frame rate limiting in the HDMI state
and CRTC VRR helpers, allowing userspace to restrict the VRR refresh rate
range or configure a fixed target frame rate with per-frame error dithering.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 497af307a690c..1e576dfbd856d 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -1051,15 +1052,60 @@ static int hdmi_calculate_vtotal(const struct drm_display_mode *mode,
>  	return 0;
>  }
>  
> +static int hdmi_validate_vrr_limits(struct drm_crtc_state *crtc_state, u16 vrr_min)
> +{
> +	struct drm_crtc_vrr_state *vrr_state = &crtc_state->vrr_state;
> +	const struct drm_display_mode *mode = &crtc_state->mode;
> +	u32 limit_d;
> +	u8 round;
> +	int ret;
> +
> +	/* No limiter, don't need to validate, but reset the error */
> +	if (!drm_crtc_helper_vrr_is_fixed_rate(vrr_state)) {
> +		vrr_state->per_frame_err = 0;
> +		vrr_state->cur_err = 0;
> +		return 0;
> +	}
> +
> +	/* Don't allow rates equal to or below the minimum VRR rate / 1.001 */
> +	if (cmp_fraction(vrr_state->vrr_min_n, vrr_state->vrr_min_d,
> +			 vrr_min * 1000, 1001) < 0)
> +		return -EINVAL;
> +
> +	if (check_mul_overflow(mode->crtc_htotal, vrr_state->vrr_min_n, &limit_d))
> +		return -ERANGE;

[Severity: Low]
As reported by Sashiko on RFC [1], this isn't a bug, but is this
check_mul_overflow() into the unused limit_d variable redundant since
hdmi_calculate_vtotal() immediately checks mode->crtc_htotal * rate_n as
its first operation?

> +
> +	ret = hdmi_calculate_vtotal(&crtc_state->mode, vrr_state->vrr_min_n,
> +				    vrr_state->vrr_min_d, &vrr_state->base_vtotal,
> +				    &vrr_state->denom, &vrr_state->per_frame_err);
> +	if (ret)
> +		return ret;
> +
> +	/* Allow rounding up for the vtotal check below */
> +	if (vrr_state->per_frame_err >= vrr_state->denom / 2)
> +		round = 1;
> +	else
> +		round = 0;
> +
> +	if (vrr_state->base_vtotal + round < mode->crtc_vtotal)
> +		return -EINVAL;

[Severity: Medium]
As reported by Sashiko on RFC [1], can hdmi_validate_vrr_limits() and
hdmi_validate_vrr() allow invalid hardware timings in fixed-rate mode?

First, when per_frame_err >= denom / 2, round is 1, so the check above
accepts base_vtotal == mode->crtc_vtotal - 1 without clamping base_vtotal
to mode->crtc_vtotal. When drm_crtc_helper_vrr_vsync() runs on frames
where cur_err < denom / 2 (including the initial frame where cur_err is 0),
it sets cur_vtotal = base_vtotal, programming a vtotal one line smaller
than mode->crtc_vtotal.

Second, if hdmi_calculate_vtotal() produces base_vtotal == U16_MAX with
per_frame_err >= denom / 2, drm_crtc_helper_vrr_vsync() will compute
vrr->cur_vtotal = vrr->base_vtotal + 1, wrapping the u16 cur_vtotal to 0.

Third, in hdmi_validate_vrr(), the check against max_vfreq_n is placed
inside the if (!is_fixed) block, so a fixed target rate exceeding
info->hdmi.vrr_cap.vrr_max (when vrr_max < mode_refresh) is not rejected.

> +
> +	return 0;
> +}
> +
>  static int hdmi_validate_vrr(struct drm_connector *connector,
>  			     struct drm_atomic_commit *state)
>  {
>  	struct drm_connector_state *new_conn_state =
>  		drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_connector_state *old_conn_state =
> +		drm_atomic_get_old_connector_state(state, connector);
>  	struct drm_crtc_state *new_crtc_state =
>  		drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
>  	struct drm_crtc_vrr_state *vrr_state =
>  		&new_crtc_state->vrr_state;
> +	struct drm_crtc_vrr_state *old_vrr_state;
> +	struct drm_crtc_state *old_crtc_state = NULL;
>  	struct drm_display_info *info = &connector->display_info;
>  	const struct drm_display_mode *mode = &new_crtc_state->mode;
>  	struct drm_device *dev = connector->dev;
> @@ -1068,10 +1114,14 @@ static int hdmi_validate_vrr(struct drm_connector *connector,
>  	u32 min_vfreq_d = 1;
>  	u32 max_vfreq_n = info->hdmi.vrr_cap.vrr_max;
>  	u32 max_vfreq_d = 1;
> +	bool is_fixed;
>  
>  	if (!new_crtc_state->vrr_enabled)
>  		return 0;
>  
> +	if (old_conn_state)
> +		old_crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);

[Severity: High]
Can this dereference a NULL pointer when enabling VRR on a connector that
was previously disabled or unassigned?

When a connector in the atomic commit is newly routed to a CRTC with VRR
enabled, old_conn_state is non-NULL, but old_conn_state->crtc is NULL.
Passing old_conn_state->crtc to drm_atomic_get_old_crtc_state() invokes
drm_crtc_index(NULL), which immediately dereferences crtc->index:

include/drm/drm_atomic.h:drm_atomic_get_old_crtc_state() {
	return state->crtcs[drm_crtc_index(crtc)].old_state;
}

Also, if a connector is moved directly from one CRTC to another,
old_conn_state->crtc points to the previous CRTC rather than
new_conn_state->crtc from which new_crtc_state->vrr_state was duplicated.
Should this look up the old CRTC state via new_conn_state->crtc instead?

> +
>  	if (!info->hdmi.vrr_cap.supported)
>  		return -EOPNOTSUPP;
>  
> @@ -1087,6 +1137,21 @@ static int hdmi_validate_vrr(struct drm_connector *connector,
>  	if (!max_vfreq_n || max_vfreq_n > mode_refresh)
>  		max_vfreq_n = mode_refresh;
>  
> +	is_fixed = drm_crtc_helper_vrr_is_fixed_rate(vrr_state);
> +	if (is_fixed && info->hdmi.vrr_cap.cinema_vrr)
> +		min_vfreq_n = 24;
> +
> +	/* If the VRR target changed, make sure to reset the error accumulator */
> +	if (old_crtc_state) {
> +		old_vrr_state = &old_crtc_state->vrr_state;
> +		if (drm_crtc_helper_vrr_is_fixed_rate(old_vrr_state) != is_fixed ||
> +		    cmp_fraction(vrr_state->vrr_min_n, vrr_state->vrr_min_d,
> +				 old_vrr_state->vrr_min_n, old_vrr_state->vrr_min_d) ||
> +		    cmp_fraction(vrr_state->vrr_max_n, vrr_state->vrr_max_d,
> +				 old_vrr_state->vrr_max_n, old_vrr_state->vrr_max_d))
> +			vrr_state->cur_err = 0;
> +	}

[Severity: High]
Will this trigger the WARN_ON(!b || !d) in cmp_fraction() when VRR is
enabled without a fixed target rate?

When neither old_vrr_state nor vrr_state is a fixed rate (for example,
normal unconstrained VRR where vrr_min_d and vrr_max_d are 0 in both
states, or when transitioning between unconstrained VRR and a bounded
range), drm_crtc_helper_vrr_is_fixed_rate(old_vrr_state) != is_fixed is
false. Evaluation then falls through to cmp_fraction() with zero
denominators:

drivers/gpu/drm/display/drm_hdmi_state_helper.c:cmp_fraction() {
	u64 a_d = mul_u32_u32(a, d);
	u64 b_c = mul_u32_u32(b, c);

	if (WARN_ON(!b || !d))
		return -EDOM;
	...
}

> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 4ffb3b0b78780..dd696f0c50169 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c

[ ... ]

> @@ -889,10 +908,21 @@ bool drm_crtc_helper_vrr_vsync(struct drm_crtc_state *crtc_state)
>  	struct drm_crtc_vrr_state *vrr = &crtc_state->vrr_state;
>  	u16 old_vtotal = vrr->cur_vtotal;
>  
> -	if (!crtc_state->vrr_enabled)
> +	if (!crtc_state->vrr_enabled) {
>  		vrr->cur_vtotal = 0;
> -	else
> +		vrr->cur_err = 0;
> +	} else if (drm_crtc_helper_vrr_is_fixed_rate(vrr)) {
> +		if (vrr->cur_err >= (int) (vrr->denom / 2)) {
> +			vrr->cur_err -= vrr->denom;
> +			vrr->cur_vtotal = vrr->base_vtotal + 1;
> +		} else {
> +			vrr->cur_vtotal = vrr->base_vtotal;
> +		}
> +
> +		vrr->cur_err += vrr->per_frame_err;

[Severity: Medium]
Can vrr->cur_err overflow a signed 32-bit int here?

In struct drm_crtc_vrr_state, denom and per_frame_err are u32, while
cur_err is a signed 32-bit int. hdmi_calculate_vtotal() allows denom
(mode->crtc_htotal * rate_n) to be any u32 up to U32_MAX, and
per_frame_err can be up to denom - 1.

When vrr->cur_err < (int)(vrr->denom / 2), vrr->denom is not subtracted
before adding vrr->per_frame_err, so the sum can reach roughly
1.5 * denom - 2. Whenever denom exceeds 1,431,655,765 (for example,
crtc_htotal = 2200 and vrr_min_n = 1,000,000), vrr->cur_err +=
vrr->per_frame_err overflows INT_MAX and wraps negative.

[Severity: Medium]
Also, can in-place updates to vrr->cur_err, vrr->cur_vtotal, and
vrr->dynamic in drm_crtc_helper_vrr_vsync() be lost across atomic commits?

During atomic commits, drm_atomic_get_crtc_state() duplicates crtc->state
into new_crtc_state under crtc->mutex while the vblank interrupt handler
can concurrently call drm_crtc_helper_vrr_vsync(crtc->state) and mutate
the old crtc->state->vrr_state in place. When drm_atomic_helper_swap_state()
later installs new_crtc_state into crtc->state, any vrr_state updates
performed by drm_crtc_helper_vrr_vsync() after atomic_duplicate_state()
are overwritten by the stale copy in new_crtc_state.

[1] https://lore.kernel.org/r/20260921-vrr-limiter-uapi-v1-14-2fcd7d011646@collabora.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-vrr-limiter-uapi-v2-0-55f4215be2c2@collabora.com?part=11