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