RE: [PATCH v3 3/8] drm/i915/vrr: Compute CMRR fractional timings generically

> -----Original Message-----
> From: Intel-gfx <[email protected]> On Behalf Of
> Golani, Mitulkumar Ajitkumar
> Sent: 16 July 2026 13:34
> To: Borah, Chaitanya Kumar <[email protected]>; intel-
> [email protected]
> Cc: [email protected]; Shankar, Uma <[email protected]>;
> Nautiyal, Ankit K <[email protected]>
> Subject: RE: [PATCH v3 3/8] drm/i915/vrr: Compute CMRR fractional timings
> generically
> 
> Thanks for the review Chaitanya,
> 
> 
> > -----Original Message-----
> > From: Borah, Chaitanya Kumar <[email protected]>
> > Sent: 15 July 2026 18:44
> > To: Golani, Mitulkumar Ajitkumar
> > <[email protected]>;
> > [email protected]
> > Cc: [email protected]; Shankar, Uma
> > <[email protected]>; Nautiyal, Ankit K
> > <[email protected]>
> > Subject: Re: [PATCH v3 3/8] drm/i915/vrr: Compute CMRR fractional
> > timings generically
> >
> >
> >
> > On 7/14/2026 4:09 PM, 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.
> > >
> >
> > mention the logic behind removing the MODE_FLAG
> 
> Applied your suggestion. Will be addressed in v4
> 
> >
> > > --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)
> > > - Multiplier_m and n naming update to increase readability.
> > > (Chaitanya)
> > > - Compute vtotal as it is required to deither as per algo
> > > implementation. (Chaitanya)
> > > - Replace misleading adjusted_pixel_rate to dividend which is
> > > somewhat relatable.
> > >
> > > Signed-off-by: Mitul Golani <[email protected]>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_vrr.c | 128 +++++++++++------------
> > >   1 file changed, 63 insertions(+), 65 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > index b36026183399..25ce56d48bb1 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.
> > > @@ -191,69 +188,72 @@ 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;
> > > +	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 (!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;
> > > +	if (!HAS_CMRR(display))
> > > +		return;
> >
> > We compute CMRR for DISPLAY_VER >= 20 (gated on HAS_CMRR), but
> looking
> > at later patches in the series, CMRR only gets enabled when
> > intel_vrr_always_use_vrr_tg() is true, which is DISPLAY_VER >= 30. On
> > <
> > 30 the CMRR_ENABLE bit gets armed via the TRANS_CMRR_N_HI write but
> is
> > then cleared by the subsequent TRANS_VRR_CTL write in
> > intel_vrr_set_transcoder_timings() (it carries neither VRR_ENABLE nor
> > CMRR_ENABLE), and intel_vrr_tg_enable() never runs to restore it since
> > vrr.enable isn't set for CMRR. So CMRR ends up computed but inactive
> > on < 30.
> >
> > This needs a closer look.
> 
> As, had discussion offline, we will enable CMRR from the platform when VRR
> timing generator is also enabled.
> 
> So, will need to add additional check intel_vrr_always_use_vrr_tg. I will
> comment this into code as well so that can be documented in-place.
> 
> So with this addition, patch #1 and #2 will also needs this check to make sure
> we have correct state programmed.
> 
> I will add this change with v4
> 
> Thanks
> 
> >
> > >
> > > -	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 = 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;
> > > -}
> > > +	/*
> > > +	 * A 1:1 ratio (denominator == 1000) means no video timing is
> > required
> > > +	 * Any other ratio (e.g. 1000/1001) requires the video timing.
> > > +	 */
> > > +	video_mode = crtc->force_cmrr.denominator != 1000;
> > > +	if (video_mode) {
> > > +		rr_multiplier = 1000;
> > > +		rr_divider = 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 rr_multiplier
> > > +	 * cmrr_m = (pixel_clock_hz x scale_m) % cmrr_n
> > > +	 *
> > > +	 * where 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.
> > >   	 */
> > > -	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->vrr.cmrr.enable = true;
> > > -	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> > > +	crtc_state->vrr.cmrr.cmrr_n =
> > > +		(mul_u32_u32(crtc->force_cmrr.numerator, adjusted_mode-
> > >crtc_htotal) *
> > > +		rr_multiplier) / 1000;
> > > +	dividend = mul_u32_u32(adjusted_mode->crtc_clock, 1000) *
> > > +rr_divider;
> >
> > By only using the numerator here you are never calculating the desired
> > refresh rate.
> 
> 
> Good point. I will address this in v4,
> 
> As we discussed, this basically we need to divide with denominator instead of
> hardcoded value, "1000"
> 
> Thanks

Hi Chaitanya,

As we discussed offline and with some hands-on experiments with vbltests, it looks like, adding denominator (1001) here, dithering more than expectation from desired refresh rate. (due to already present multiplier/divider presence)

Somehow, hardcoded 1000 is working well to achieve desired refresh rate. For this we need more clarificationon  Bspec given example.

For now I will add hardcoded 1000 and add later TODO comment, once get some more clarity, we can add a fix if it is ok. 

Thanks

> 
> 
> >
> > > +	adjusted_mode->crtc_vtotal = div64_u64_rem(dividend,
> > > +						   crtc_state->vrr.cmrr.cmrr_n,
> > > +						   &crtc_state-
> > >vrr.cmrr.cmrr_m);
> > > +
> > > +	return;
> >
> > redundant
> 
> Good point. I will address this in v4.
> 
> >
> > >   }
> > >
> > >   static
> > > @@ -429,8 +429,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;
> > >
> > > @@ -464,12 +462,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 =
> > > @@ -1136,11 +1139,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;
> > >