Re: [PATCH v11 1/2] drm/i915/vrr: Split vrr-compute-config in two phases

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

On Mon, Sep 16, 2024 at 01:24:05PM +0530, Animesh Manna wrote:
> As vrr guardband calculation is dependent on modified
> vblank start so better to compute late after all
> vblank adjustement.
> 
> v1: Initial version.
> v2: Split in a separate patch from panel-replay workaround. [Ankit]
> v3: Add a function for late vrr related computation. [Ville]
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  2 ++
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 30 +++++++++++++-------
>  drivers/gpu/drm/i915/display/intel_vrr.h     |  1 +
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index fdf244a32b24..111e61eceafc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4802,6 +4802,8 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>  	struct drm_connector *connector;
>  	int i;
>  
> +	intel_vrr_compute_config_late(crtc_state);
> +
>  	for_each_new_connector_in_state(&state->base, connector,
>  					conn_state, i) {
>  		struct intel_encoder *encoder =
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 9a51f5bac307..2de1c04bf1a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -239,18 +239,26 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			(crtc_state->hw.adjusted_mode.crtc_vtotal -
>  			 crtc_state->hw.adjusted_mode.vsync_end);
>  	}
> +}
>  
> -	/*
> -	 * For XE_LPD+, we use guardband and pipeline override
> -	 * is deprecated.
> -	 */
> -	if (DISPLAY_VER(display) >= 13) {
> -		crtc_state->vrr.guardband =
> -			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
> -	} else {
> -		crtc_state->vrr.pipeline_full =
> -			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start -
> -			    crtc_state->framestart_delay - 1);
> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	if (crtc_state->vrr.enable) {

Never use that. VRR state must be computed correctly even when
VRR is not actually enabled. That is how we can simply toggle
VRR during fastsets. 

You want to check vrr.flipline just like
intel_vrr_set_transcoder_timings() does. We should perhaps add
a helper with a decent name for that purpose. intel_vrr_possible()
perhaps?

Also flip this condition around to eliminate the nasty
whole function indentation.

> +		/*
> +		 * For XE_LPD+, we use guardband and pipeline override
> +		 * is deprecated.
> +		 */

Could drop that redundant comment while at it.

> +		if (DISPLAY_VER(display) >= 13) {
> +			crtc_state->vrr.guardband =
> +				crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
> +		} else {
> +			crtc_state->vrr.pipeline_full =
> +				min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start -
> +				crtc_state->framestart_delay - 1);
> +		}
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> index 89937858200d..3127c94e9778 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> @@ -18,6 +18,7 @@ bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh);
>  void intel_vrr_check_modeset(struct intel_atomic_state *state);
>  void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state);
> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
>  void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_send_push(const struct intel_crtc_state *crtc_state);
> -- 
> 2.29.0

-- 
Ville Syrjälä
Intel