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