Re: [PATCH v6 6/9] drm/i915/vrr: Compute CMRR fractional timings generically

On 7/22/2026 10:09 AM, Mitul Golani wrote:
> Rework the fractional-CMRR computation into a generic,
> transcoder-agnostic helper driven by an explicit per-CRTC debugfs
> target, replacing the previous disabled, eDP-only code path. Compute
> CMRR_M and CMRR_N timings based on the video mode requirement. Note the
> CMRR enable path is wired up separately; this patch only lays down the
> generic computation. Remove computing mode_flags, I915_MODE_FLAG_VRR
> as we are moving CMRR to fixed refresh rate path. Also return early
> from CMRR computation if existing computation is well sufficient.
> 
> --v2:
> - Derive video_mode locally instead of caching it in persistent
>    struct intel_crtc state (Jani, Chaitanya)
> - Fix numerator unit in comment: milli-Hz, not kHz (Chaitanya)
> - Fix "reqirement" typo and clarify CMRR is not yet enabled in the
>    commit message (Chaitanya)
> - Fix precision issue while computing M/N ration (Chaitanya)

typo: ratio

> - Multiplier_m and n naming update to increase readability. (Chaitanya)
> - Compute vtotal as it is required to deither as per algo

typo: dither

> implementation. (Chaitanya)
> - Replace misleading adjusted_pixel_rate to dividend which is somewhat
> relatable.
> 
> --v3:
> - Mention rationale for not to compute I915_MODE_FLAG_VRR. (Chaitanya)
> - Remove redundant return statement from last line of cmrr compute.
> (Chaitanya)
> - Correct cmrr_n calculation. (Chaitanya)
> - Add early return condition is current computaion is well sufficient

typo: computation


> to drive without CMRR. (Chaitanya)
> - Take round up for requested_refresh_rate. (Validation)
> - Correct integer overflow
> 
> --v4:
> - Update cmrr_n calculation to aviod aggressive dithering

typo: avoid

> - Use div64_u64 to truncate value to floor value, as DIV_ROUND_UP_ULL
> rounding to ceil value.
> - Add TODO comment at cmrr_n calculation
> - Add condition for custom refresh rate request
> - Update CMRR computational comment block in copute config

typo: compute

> 
> Signed-off-by: Mitul Golani <[email protected]>
> ---
>   drivers/gpu/drm/i915/display/intel_vrr.c | 138 ++++++++++++-----------
>   1 file changed, 72 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 84b1ec7f1616..e8c6516fd36a 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -27,9 +27,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.
> @@ -198,68 +195,79 @@ static bool intel_vrr_cmrr_possible(const struct intel_crtc_state *crtc_state)
>   	return (HAS_CMRR(display) && intel_vrr_always_use_vrr_tg(display));
>   }
>   
> -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)
>   {
> -	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +	u64 dividend;
> +	int requested_refresh_rate, current_refresh_rate;
> +	int rr_multiplier = 1, rr_divider = 1;
> +	bool video_mode;
>   
> -	/* Avoid CMRR for now till we have VRR with fixed timings working */
> -	if (!intel_vrr_cmrr_possible(crtc_state) || 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;
> +	if (!intel_vrr_cmrr_possible(crtc_state))
> +		return;
>   
> -	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> +	/* No CMRR ratio configured through debugfs */
> +	if (!crtc->force_cmrr.numerator)
> +		return;
>   
> -	if (video_mode_required) {
> -		multiplier_m = 1001;
> -		multiplier_n = 1000;
> +	/*
> +	 * The numerator encodes the requested refresh rate in milli-Hz, so the
> +	 * requested refresh rate in Hz is numerator / 1000. It must match the
> +	 * refresh rate of the current mode.
> +	 */
> +	requested_refresh_rate = DIV_ROUND_UP_ULL(crtc->force_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;
>   	}
>   
> -	crtc_state->vrr.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->vrr.cmrr.cmrr_n);
> -	adjusted_pixel_rate = mul_u32_u32(adjusted_mode->crtc_clock * 1000, multiplier_m);
> -	crtc_state->vrr.cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->vrr.cmrr.cmrr_n);
> -
> -	return vtotal;
> -}
> -
> -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.
> +	 * A 1:1 ratio (denominator == 1000) means no video timing is required
> +	 * Any other ratio (e.g. 1000/1001) requires the video timing.
>   	 */
> -	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;
> +	video_mode = crtc->force_cmrr.denominator != 1000;
> +	if (video_mode) {
> +		rr_multiplier = 1000;
> +		rr_divider = 1001;
> +	} else if (crtc->force_cmrr.numerator == current_refresh_rate * 1000) {

As discussed this should be a comparision between num/den and actual 
refresh rate derived from the mode line. Moreover, this check should be 
done for both video and non video mode branches.

> +		drm_dbg_kms(display->drm,
> +			    "[CRTC:%d:%s] Requested mode refresh rate %d Hz can be driven without CMRR\n",
> +				crtc->base.base.id, crtc->base.name,
> +				requested_refresh_rate);
> +		return;
> +	}
>   
> -	crtc_state->vrr.cmrr.enable = true;
> -	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> +	/*
> +	 * Let pixel_clock_hz = adjusted_mode->crtc_clock * 1000.
> +	 *
> +	 * cmrr_n = (numerator_mHz x htotal x rr_multiplier) / 1000
> +	 * cmrr_m = (pixel_clock_hz x rr_divider) % cmrr_n
> +	 *
> +	 * where numerator_mHz is the requested refresh rate in milli-Hz
> +	 * and rr_multiplier/rr_divider = 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.
> +	 *
> +	 * TODO: Using the actual desired rate for cmrr_n in video
> +	 * mode produces more aggressive vtotal dithering than
> +	 * expected; revisit once the Bspec algorithm is clarified.
> +	 */
> +	crtc_state->vrr.cmrr.cmrr_n =
> +		div64_u64((u64)crtc->force_cmrr.numerator *
> +			   adjusted_mode->crtc_htotal * rr_multiplier, 1000);
> +	dividend = (u64)adjusted_mode->crtc_clock * rr_divider * 1000;
> +	adjusted_mode->crtc_vtotal = div64_u64_rem(dividend,
> +						   crtc_state->vrr.cmrr.cmrr_n,
> +						   &crtc_state->vrr.cmrr.cmrr_m);
>   }
>   
>   static
> @@ -435,8 +443,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;
>   
> @@ -470,12 +476,17 @@ 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 {
> +		/*
> +		 * 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);
>   		intel_vrr_compute_fixed_rr_timings(crtc_state);
> +	}
>   
>   	if (HAS_AS_SDP(display)) {
>   		crtc_state->vrr.vsync_start =
> @@ -1142,11 +1153,6 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>   
>   	intel_vrr_get_dc_balance_config(crtc_state);
>   
> -	/*
> -	 * #TODO: For Both VRR and CMRR the flag I915_MODE_FLAG_VRR is set for mode_flags.
> -	 * Since CMRR is currently disabled, set this flag for VRR for now.
> -	 * Need to keep this in mind while re-enabling CMRR.
> -	 */
>   	if (crtc_state->vrr.enable)
>   		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>