Re: [PATCH 11/35] drm/i915/vrr: Avoid prepare vrr timings for cmrr

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 24, 2025 at 08:29:56PM +0530, Ankit Nautiyal wrote:
> CMRR has a separate logic for computing vrr timings and so it
> overwrites the timings prepared for vrr.
> 
> Avoid prepare vrr timings for cmrr. This will help to separate the
> helpers for timings for vrr, cmrr and the forthcoming
> fixed_rr.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 7e69e30b2076..90fd6fe58fce 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -257,8 +257,9 @@ void intel_vrr_compute_cmrr_timings(struct intel_crtc_state *crtc_state)
>  }
>  
>  static
> -void intel_vrr_compute_vrr_timings(struct intel_crtc_state *crtc_state)
> +void intel_vrr_compute_vrr_timings(struct intel_crtc_state *crtc_state, int vmin, int vmax)
>  {
> +	intel_vrr_prepare_vrr_timings(crtc_state, vmin, vmax);
>  	crtc_state->vrr.enable = true;
>  	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>  }
> @@ -328,12 +329,12 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  	if (vmin >= vmax)
>  		return;
>  
> -	intel_vrr_prepare_vrr_timings(crtc_state, vmin, vmax);
> -
>  	if (crtc_state->uapi.vrr_enabled)
> -		intel_vrr_compute_vrr_timings(crtc_state);
> +		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
> +		intel_vrr_prepare_vrr_timings(crtc_state, vmin, vmax);

I don't understand why the caller is calculating the vrr vmin/vmax
and then passing them in to everyone. Looks to me like each of those
guys should just calculate the vmin/vmax on their own.

The
 crtc_state->vrr.flipline = crtc_state->vrr.vmin;
 crtc_state->vrr.vmin -= intel_vrr_min_flipline_offset(display);
part could stay in the caller since it's always needed regardless
of what kind of timings we use.




>  
>  	if (HAS_AS_SDP(display)) {
>  		crtc_state->vrr.vsync_start =
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel