Re: [PATCH v2 02/11] drm/i915/vrr: compute CMRR fractional timings generically

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 16 Jun 2026, Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> wrote:
> Replace the disabled, eDP-only, fractional-CMRR code
> with a generic, transcoder-agnostic computation driven by an
> explicit per-CRTC target. Compute CMRR_M and CMRR_N timings
> based on video mode reqirement if CMRR is required to be enabled.
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |   2 +
>  drivers/gpu/drm/i915/display/intel_vrr.c      | 121 +++++++++---------
>  2 files changed, 63 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 897a1ffd7b79..39e11362630c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1548,6 +1548,8 @@ struct intel_crtc {
>  	struct {
>  		u32 numerator;
>  		u32 denominator;
> +		/* Derived during atomic check: 1000/1001 video timing required */
> +		bool video_mode;

compute config must not change permanent state, only crtc state.

Moreover, the video_mode here is just a funcion of denominator, so why
do we cache it when it can be computed on the fly?

>  	} cmrr;
>  
>  	int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 41118883b845..e36c0cab096a 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -26,9 +26,6 @@
>  #include "skl_prefill.h"
>  #include "skl_watermark.h"
>  
> -#define FIXED_POINT_PRECISION		100
> -#define CMRR_PRECISION_TOLERANCE	10
> -
>  /*
>   * Tunable parameters for DC Balance correction.
>   * These are captured based on experimentations.
> @@ -186,69 +183,69 @@ int intel_vrr_vmax_vblank_start(const struct intel_crtc_state *crtc_state)
>  	return intel_vrr_vmax_vtotal(crtc_state) - crtc_state->vrr.guardband;
>  }
>  
> -static bool
> -is_cmrr_frac_required(struct intel_crtc_state *crtc_state)
> +static void
> +intel_vrr_cmrr_compute_config(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
> -	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> -
> -	/* Avoid CMRR for now till we have VRR with fixed timings working */
> -	if (!HAS_CMRR(display) || true)
> -		return false;
> -
> -	actual_refresh_k =
> -		drm_mode_vrefresh(adjusted_mode) * FIXED_POINT_PRECISION;
> -	pixel_clock_per_line =
> -		adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
> -	calculated_refresh_k =
> -		pixel_clock_per_line * FIXED_POINT_PRECISION / adjusted_mode->crtc_vtotal;
> -
> -	if ((actual_refresh_k - calculated_refresh_k) < CMRR_PRECISION_TOLERANCE)
> -		return false;
> -
> -	return true;
> -}
> -
> -static unsigned int
> -cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool video_mode_required)
> -{
> -	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
>  	u64 adjusted_pixel_rate;
> -	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +	int requested_refresh_rate, current_refresh_rate;
> +	int multiplier_m = 1, multiplier_n = 1;
>  
> -	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> +	if (!HAS_CMRR(display))
> +		return;
>  
> -	if (video_mode_required) {
> -		multiplier_m = 1001;
> -		multiplier_n = 1000;
> -	}
> +	/* No CMRR ratio configured through debugfs */
> +	if (!crtc->cmrr.numerator)
> +		return;
>  
> -	crtc_state->cmrr.cmrr_n = mul_u32_u32(desired_refresh_rate * adjusted_mode->crtc_htotal,
> -					      multiplier_n);
> -	vtotal = DIV_ROUND_UP_ULL(mul_u32_u32(adjusted_mode->crtc_clock * 1000, multiplier_n),
> -				  crtc_state->cmrr.cmrr_n);
> -	adjusted_pixel_rate = mul_u32_u32(adjusted_mode->crtc_clock * 1000, multiplier_m);
> -	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n);
> +	/*
> +	 * The numerator encodes the requested refresh rate in kHz, so the
> +	 * requested refresh rate in Hz is numerator / 1000. It must match the
> +	 * refresh rate of the current mode.
> +	 */
> +	requested_refresh_rate = crtc->cmrr.numerator / 1000;
> +	current_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> +
> +	if (requested_refresh_rate != current_refresh_rate) {
> +		drm_dbg_kms(display->drm,
> +			    "[CRTC:%d:%s] CMRR requested refresh rate %d Hz does not match current mode refresh rate %d Hz\n",
> +				crtc->base.base.id, crtc->base.name,
> +				requested_refresh_rate, current_refresh_rate);
> +		return;
> +	}
>  
> -	return vtotal;
> -}
> +	/*
> +	 * A 1:1 ratio (denominator == 1000) means no video timing is required
> +	 * Any other ratio (e.g. 1000/1001) requires the video timing.
> +	 */
> +	crtc->cmrr.video_mode = crtc->cmrr.denominator != 1000;
> +	if (crtc->cmrr.video_mode) {
> +		multiplier_m = 1000;
> +		multiplier_n = 1001;
> +	}
>  
> -static
> -void intel_vrr_compute_cmrr_timings(struct intel_crtc_state *crtc_state)
> -{
>  	/*
> -	 * TODO: Compute precise target refresh rate to determine
> -	 * if video_mode_required should be true. Currently set to
> -	 * false due to uncertainty about the precise target
> -	 * refresh Rate.
> +	 * Let pixel_clock_hz = adjusted_mode->crtc_clock * 1000.
> +	 *
> +	 * cmrr_n = requested_refresh_rate x htotal x multiplier_m
> +	 * cmrr_m = (pixel_clock_hz x scale_m) % cmrr_n
> +	 *
> +	 * where multiplier_m/multiplier_n = 1000/1001 when the
> +	 * video timing is required, else 1/1. The integer vtotal
> +	 * term is tracked in SW (it is the programmed mode vtotal)
> +	 * while the fractional part represented by cmrr_m/cmrr_n
> +	 * is tracked in HW.
>  	 */
> -	crtc_state->vrr.vmax = cmrr_get_vtotal(crtc_state, false);
> -	crtc_state->vrr.vmin = crtc_state->vrr.vmax;
> -	crtc_state->vrr.flipline = crtc_state->vrr.vmin;
>  
> -	crtc_state->cmrr.enable = true;
> -	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> +	crtc_state->cmrr.cmrr_n =
> +		mul_u32_u32(requested_refresh_rate * adjusted_mode->crtc_htotal,
> +			    multiplier_m);
> +	adjusted_pixel_rate = mul_u32_u32(adjusted_mode->crtc_clock, 1000) * multiplier_n;
> +	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n);
> +
> +	return;
>  }
>  
>  static
> @@ -424,8 +421,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_connector *connector =
>  		to_intel_connector(conn_state->connector);
> -	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	bool is_edp = intel_dp_is_edp(intel_dp);
>  	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  	int vmin, vmax;
>  
> @@ -459,13 +454,19 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  		vmax = vmin;
>  	}
>  
> -	if (crtc_state->uapi.vrr_enabled && vmin < vmax)
> +	if (crtc_state->uapi.vrr_enabled && vmin < vmax) {
>  		intel_vrr_compute_vrr_timings(crtc_state, vmin, vmax);
> -	else if (is_cmrr_frac_required(crtc_state) && is_edp)
> -		intel_vrr_compute_cmrr_timings(crtc_state);
> -	else
> +	} else {
>  		intel_vrr_compute_fixed_rr_timings(crtc_state);
>  
> +		/*
> +		 * CMRR is a fixed average Vtotal mode and is only computed on
> +		 * the fixed refresh rate path. It is generic across transcoders
> +		 * and gated on platform support and a valid debugfs ratio.
> +		 */
> +		intel_vrr_cmrr_compute_config(crtc_state);
> +	}
> +
>  	if (HAS_AS_SDP(display)) {
>  		crtc_state->vrr.vsync_start =
>  			(crtc_state->hw.adjusted_mode.crtc_vtotal -

-- 
Jani Nikula, Intel