Re: [PATCH v2 05/11] drm/i915/vrr: Enable/Disable CMRR based on enable/disable preconditions

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

On 6/16/2026 8:12 PM, Mitul Golani wrote:
> Enable or disable CMRR based on it is being computed and set of
> preconditions. Should be enabled when dueing adaptive mode as well
> as conditions which sets cmrr.enable gets set similarly to disable case.
> Also separate out register writes for both enable and disable case.
> 

The intention of this patch is unclear to me.

> Remove VRR_CTL_CMRR_ENABLE write in the current path, add the same
> when actually CMRR is enabled.
> 
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_vrr.c | 81 ++++++++++++++++++++++--
>   1 file changed, 76 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 5678c3a86796..c979950d32cb 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -34,6 +34,43 @@
>   #define DCB_CORRECTION_AGGRESSIVENESS	1000 /* ms × 100; 10 ms */
>   #define DCB_BLANK_TARGET		50
>   
> +#define is_enabling(feature, old_crtc_state, new_crtc_state) \
> +	((!(old_crtc_state)->feature || intel_crtc_needs_modeset(new_crtc_state)) && \
> +	 (new_crtc_state)->feature)
> +#define is_disabling(feature, old_crtc_state, new_crtc_state) \
> +	((old_crtc_state)->feature && \
> +	 (!(new_crtc_state)->feature || intel_crtc_needs_modeset(new_crtc_state)))
> +
> +static bool intel_crtc_cmrr_enabling(struct intel_atomic_state *state,
> +				     struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +	const struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +
> +	if (!new_crtc_state->hw.active)
> +		return false;
> +
> +	return is_enabling(cmrr.enable, old_crtc_state, new_crtc_state) ||
> +		(new_crtc_state->cmrr.enable);
> +}
> +
> +static bool intel_crtc_cmrr_disabling(struct intel_atomic_state *state,
> +				      struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +	const struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +
> +	if (!new_crtc_state->hw.active)
> +		return false;
> +
> +	return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state) ||
> +			(old_crtc_state->cmrr.enable);
> +}
> +
>   bool intel_vrr_is_capable(struct intel_connector *connector)
>   {
>   	struct intel_display *display = to_intel_display(connector);
> @@ -831,6 +868,34 @@ static void intel_vrr_set_vrr_timings(const struct intel_crtc_state *crtc_state)
>   		       intel_vrr_hw_flipline(crtc_state) - 1);
>   }
>   
> +static void
> +intel_vrr_enable_cmrr(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +
> +	intel_de_write(display, TRANS_CMRR_M_HI(display, cpu_transcoder),
> +		       upper_32_bits(crtc_state->cmrr.cmrr_m));
> +	intel_de_write(display, TRANS_CMRR_M_LO(display, cpu_transcoder),
> +		       lower_32_bits(crtc_state->cmrr.cmrr_m));
> +	intel_de_write(display, TRANS_CMRR_N_HI(display, cpu_transcoder),
> +		       upper_32_bits(crtc_state->cmrr.cmrr_n));
> +	intel_de_write(display, TRANS_CMRR_N_LO(display, cpu_transcoder),
> +		       lower_32_bits(crtc_state->cmrr.cmrr_n));
> +}
> +
> +static void
> +intel_vrr_disable_cmrr(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +
> +	intel_de_write(display, TRANS_CMRR_M_HI(display, cpu_transcoder), 0);
> +	intel_de_write(display, TRANS_CMRR_M_LO(display, cpu_transcoder), 0);
> +	intel_de_write(display, TRANS_CMRR_N_HI(display, cpu_transcoder), 0);
> +	intel_de_write(display, TRANS_CMRR_N_LO(display, cpu_transcoder), 0);
> +}
> +
>   static void
>   intel_vrr_enable_dc_balancing(const struct intel_crtc_state *crtc_state)
>   {
> @@ -934,8 +999,6 @@ static void intel_vrr_tg_enable(const struct intel_crtc_state *crtc_state,
>   	 * even VRR_CTL_CMRR_ENABLE is armed by TRANS_CMRR_N_HI
>   	 * when enabling CMRR (but not when disabling CMRR?).
>   	 */
> -	if (cmrr_enable)
> -		vrr_ctl |= VRR_CTL_CMRR_ENABLE;
>   
>   	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), vrr_ctl);
>   
> @@ -962,10 +1025,15 @@ static void intel_vrr_tg_disable(const struct intel_crtc_state *old_crtc_state)
>   void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>   {
>   	struct intel_display *display = to_intel_display(crtc_state);
> +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   
>   	if (!crtc_state->vrr.enable)
>   		return;
>   
> +	if (intel_crtc_cmrr_disabling(state, crtc))
> +		intel_vrr_disable_cmrr(crtc_state);
> +
>   	intel_vrr_set_vrr_timings(crtc_state);
>   	intel_vrr_enable_dc_balancing(crtc_state);
>   
> @@ -976,6 +1044,8 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>   void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>   {
>   	struct intel_display *display = to_intel_display(old_crtc_state);
> +	struct intel_atomic_state *state = to_intel_atomic_state(old_crtc_state->uapi.state);
> +	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>   
>   	if (!old_crtc_state->vrr.enable)
>   		return;
> @@ -984,6 +1054,10 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>   		intel_vrr_tg_disable(old_crtc_state);
>   
>   	intel_vrr_disable_dc_balancing(old_crtc_state);
> +
> +	if (intel_crtc_cmrr_enabling(state, crtc))
> +		intel_vrr_enable_cmrr(old_crtc_state);
> +
>   	intel_vrr_set_fixed_rr_timings(old_crtc_state, old_crtc_state->cpu_transcoder);
>   }
>   
> @@ -1066,9 +1140,6 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>   	trans_vrr_ctl = intel_de_read(display,
>   				      TRANS_VRR_CTL(display, cpu_transcoder));
>   
> -	if (HAS_CMRR(display))
> -		crtc_state->cmrr.enable = (trans_vrr_ctl & VRR_CTL_CMRR_ENABLE);
> -
>   	if (crtc_state->cmrr.enable) {
>   		crtc_state->cmrr.cmrr_n =
>   			intel_de_read64_2x32(display, TRANS_CMRR_N_LO(display, cpu_transcoder));