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

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Jani Nikula <jani.nik...@linux.intel.com>
> Sent: 17 June 2026 17:30
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.gol...@intel.com>;
> intel-gfx@lists.freedesktop.org
> Cc: intel...@lists.freedesktop.org; Nautiyal, Ankit K
> <ankit.k.nauti...@intel.com>; Borah, Chaitanya Kumar
> <chaitanya.kumar.bo...@intel.com>
> Subject: Re: [PATCH v2 02/11] drm/i915/vrr: compute CMRR fractional
> timings generically
> 
> On Tue, 16 Jun 2026, Mitul Golani <mitulkumar.ajitkumar.gol...@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.gol...@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?

Right, no explicit requirement, can be configured on the fly. 

Thanks for review, I will make the change and float with next revision.

Regards,
Mitul

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