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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 7/13/2026 7:27 PM, Golani, Mitulkumar Ajitkumar wrote:
> 
> 
>> -----Original Message-----
>> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
>> Sent: 06 July 2026 17:29
>> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>;
>> intel-gfx@lists.freedesktop.org
>> Cc: intel-xe@lists.freedesktop.org; Nautiyal, Ankit K
>> <ankit.k.nautiyal@intel.com>
>> Subject: Re: [PATCH v2 02/11] drm/i915/vrr: compute CMRR fractional timings
>> generically
>>
>>
>>
>> On 6/16/2026 8:12 PM, Mitul Golani wrote:
>>> Replace the disabled,
>>
>> I guess it is still disabled at this point, so the commit message is a bit
>> misleading.
>>
>> eDP-only,
>>
>> Should be a separate patch.
> 
> Since this patch rewrites the CMRR compute path as a transcoder-agnostic helper, keeping the old eDP-only gate did not make sense; if this is acceptable, I’ll call this out explicitly in the commit message.
> 

I guess this would work because the whole CMRR path currently is dead 
code anyway. Making it explicit in the commit message should be fine.
Same goes for the disabling part.

>>
>> 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
>>
>> typo
>>
>>    if CMRR is required to be enabled.
> 
> I will update with next revision.
> 
> Thanks
> 
>>>
>>> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@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;
>>
>> I am not sure why this is needed at all, since you can just use the numerator
>> and denominator to derive if it is a video mode within compute_config.
> 
> Agree, Same opinion  from Jani too. I will update in next revision.
> 
>>
>>>    	} 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
>>
>> Not accurate, because numerator/divider together provides the refresh rate.
>> If anything the numerator is in mHz as commented in patch 1.
>>
>>> +	 * 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;
>>
>> Integer division loses precision. It can be used in the comparison below but
>> incorrect to use it in actual M and N value calculations done later.
> 
> Good catch, I agree, this will loose precision later. I will fix this in next revision.
> 
>>
>>> +	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;
>>> +	}
>>
>> more intuitive to use names like rr_multiplier, rr_divider. Also the names are
>> reversed now.
> 
> No worries, In-intention to create better readability of this code, I will update in next revision, although these names were picked based on existing algo jargons from Bspec.
> 
>>
>>>
>>> -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;
>>
>> where do we set this flag now?
>>
>>> +	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;
>>
>> Please align the calculation as per BSpec-68925, calculate the cmrr votal here
>> and retain the cmrr_get_vtotal semantics.
>>
>>> +	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 -
>