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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Thursday, 12 November 2026 06:35:44 Central European Standard Time Vidith Madhu wrote:
> 
> On Thu, 5 Nov 2026, Nicolas Frattaroli wrote:
> 
> > 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.
> > 
> > With the new VRR target frame rate properties, the necessary
> > infrastructure is in place to allow implementing this.
> > 
> > Add the necessary logic to hdmi_validate_vrr, as well as the VRR
> > helpers. Since not every target rate can be reached precisely, the
> > per-frame error is calculated, and dithered one row at a time across
> > subsequent frames. This means some frames are one vtotal larger than
> > others, so that averaged over the frames, the desired target rate is
> > achieved. Such dithering is explicitly permitted by HDMI, and is still
> > considered a constant target frame rate.
> > 
> > Co-developed-by: Derek Foreman <derek.foreman@collabora.com>
> > Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 134 +++++++++++++++++++++---
> >  drivers/gpu/drm/drm_crtc_helper.c               |  34 +++++-
> >  include/drm/drm_crtc.h                          |   3 +
> >  include/drm/drm_crtc_helper.h                   |   2 +
> >  4 files changed, 156 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > index ce680eaa5047..846f7a72fddc 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > @@ -5,6 +5,7 @@
> >  
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_connector.h>
> > +#include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_edid.h>
> >  #include <drm/drm_modes.h>
> >  #include <drm/drm_print.h>
> > @@ -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;
> > +
> > +	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;
> > +
> > +	return 0;
> > +}
> > +
> I believe in the previous revision we discussed factoring out the 
> protocol-agnostic validation of the CRTC frame limit ranges properties
> (e.g. checking against min/max_vfreq, checking for a degenerate range)
> to the main CRTC atomic check path. Is that still the intention?

I think the validate limit function could be moved to the CRTC helpers.
I did try to move the whole hdmi_validate_vrr logic to CRTC, but that
doesn't work because a lot of it requires the connector.

I'll move the limit validation to the protocol-agnostic CRTC helpers then
and have the passed-in params be whatever the CRTC doesn't know of itself,
so that it's at least reusable for non-HDMI-state-helper drivers.

> 
> >  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;
> >  
> [snip]