Re: [PATCH RFC 14/25] drm: Implement VRR rate limiting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm: integer overflow and implicit sign conversion in VRR vsync pacing
- [Medium] drm: missing initialization of VRR accumulator cur_err on rate transitions
- [Medium] drm: fixed VRR target rates bypass sink's max_vfreq capability check
- [Low] drm: redundant check_mul_overflow and unused limit_d in VRR limits validation
--

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

    drm: Implement VRR rate limiting

    Currently, VRR is allowed to use the entire range advertised by the
    sink's EDID. Userspace may find it desirable, however, to limit this
    range, or set a fixed target rate entirely. The latter is mainly useful
    for multimedia playback applications, where judder can be avoided by
    matching the content's frame rate precisely.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 33d0c94916430..7dbd678cf0612 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
[ ... ]
> @@ -1046,6 +1047,39 @@ 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;
> +	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;
> +		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]
Is the limit_d variable ever used after being assigned in this overflow check?

Also, doesn't hdmi_calculate_vtotal() duplicate the exact same
mode->crtc_htotal * rate_n check immediately below, making this check
redundant?

> +
> +	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);

[Severity: Medium]
When userspace changes the fixed VRR target rate or switches from dynamic
to fixed VRR, does this state transition leave the old cur_err
accumulator value intact?

If the new denom is significantly smaller than the old one, could a
disproportionately large carried-over cur_err take thousands of frames
to drain, causing a persistent frame rate artifact (stuck at
base_vtotal + 1) for several seconds?

> +	if (ret)
> +		return ret;
> +
> +	if (vrr_state->base_vtotal < mode->crtc_vtotal)
> +		return -EINVAL;
> +
> +	return 0;
> +}
[ ... ]
> @@ -1112,25 +1151,64 @@ static int hdmi_validate_vrr(struct drm_connector *connector,
>  
>  	vrr_state->vic = drm_match_cea_mode(mode);
>  
> -	if (cmp_fraction(min_vfreq_n, min_vfreq_d, max_vfreq_n, max_vfreq_d) > 0) {
> -		drm_dbg_kms(dev, "Target max (%u/%u) > target min (%u/%u)\n",
> -			    max_vfreq_n, max_vfreq_d, min_vfreq_n, min_vfreq_d);
> -		return -EINVAL;
> -	}
> -
> -	ret = hdmi_calculate_vtotal(mode, max_vfreq_n, max_vfreq_d,
> -				    &vrr_state->base_vtotal, NULL, NULL);
> +	ret = hdmi_validate_vrr_limits(new_crtc_state, min_vfreq_n);
>  	if (ret) {
> -		drm_dbg_kms(dev, "Couldn't calculate base_vtotal: %pe\n", ERR_PTR(ret));
> +		drm_dbg_kms(dev, "Invalid VRR fixed target rate: %pe\n", ERR_PTR(ret));
>  		return ret;
>  	}
>  
> -	/*
> -	 * Even at its fastest, it can't go faster than the mode, so
> -	 * clamp to avoid imprecisely rounded mode_refresh values we
> -	 * can't do anything about from ruining our day.
> -	 */
> -	vrr_state->base_vtotal = max(vrr_state->base_vtotal, mode->crtc_vtotal);
> +	if (!is_fixed) {

[Severity: Medium]
If a userspace DRM client requests a fixed VRR target rate, does wrapping
the capability checks in if (!is_fixed) cause the driver to silently
bypass validating against the monitor's max_vfreq?

If the monitor's EDID advertises a max_vfreq strictly lower than the
mode's refresh rate, couldn't userspace improperly exceed the sink's
hardware capabilities?

> +		vrr_state->dynamic = true;
> +
> +		if (vrr_state->vrr_min_n) {
> +			if (!vrr_state->vrr_min_d)
> +				return -EINVAL;
[ ... ]
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 4ffb3b0b78780..c46de778df309 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 * 2 >= vrr->denom) {

[Severity: High]
Since vrr->cur_err is an int and vrr->denom is a u32, can this
cause an implicit sign conversion bug?

When userspace sets a fixed VRR rate where per_frame_err < denom / 2,
cur_err becomes negative after vrr->cur_err -= vrr->denom executes.

Because vrr->denom is a u32, won't C integer promotion rules implicitly
cast the negative int to a massive u32, causing this check to be
unconditionally true every frame thereafter?

> +			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;
> +	} else {
>  		vrr->cur_vtotal = vrr->max_vtotal;
> +	}
[ ... ]
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index df37f4ebb58ed..00e9df02f3d5e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -71,6 +71,9 @@ struct drm_crtc_vrr_state {
>  	u32 vrr_min_d;
>  	u32 vrr_max_n;
>  	u32 vrr_max_d;
> +	u32 denom;
> +	u32 per_frame_err;
> +	int cur_err;

[Severity: High]
Does declaring cur_err as an int while denom is a u32 cause the
signed/unsigned comparison flaw in drm_crtc_helper_vrr_vsync()?

>  	u16 base_vtotal;
>  	bool dynamic;
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=14