Re: [Intel-gfx] [PATCH 2/2] drm/i915/drrs: Disable DRRS when needed in fastsets

[Anshuman Gupta <anshuman.gupta@intel.com>]

On 2020-08-20 at 22:53:53 +0530, José Roberto de Souza wrote:
> Changes in the configuration could cause PSR to be compatible and
> enabled so driver must also be able to disable DRRS when doing
> fastsets.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/209
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/173
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/209
> Cc: Srinivas K <srinivasx.k@intel.com>
> Cc: Hariom Pandey <hariom.pandey@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
overall patch looks goood to me,
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c |  2 +-
>  drivers/gpu/drm/i915/display/intel_dp.c  | 84 +++++++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_dp.h  |  2 +
>  3 files changed, 71 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index de5b216561d8..ff05a852417c 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4012,7 +4012,7 @@ static void intel_ddi_update_pipe_dp(struct intel_atomic_state *state,
>  
>  	intel_psr_update(intel_dp, crtc_state, conn_state);
>  	intel_dp_set_infoframes(encoder, true, crtc_state, conn_state);
> -	intel_edp_drrs_enable(intel_dp, crtc_state);
> +	intel_edp_drrs_update(intel_dp, crtc_state);
>  
>  	intel_panel_update_backlight(state, encoder, crtc_state, conn_state);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 3bf50b1ae983..de2c9851395d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2576,9 +2576,9 @@ intel_dp_compute_hdr_metadata_infoframe_sdp(struct intel_dp *intel_dp,
>  }
>  
>  static void
> -intel_dp_drrs_compute_config(struct intel_dp *intel_dp,
> -			     struct intel_crtc_state *pipe_config,
> -			     int output_bpp, bool constant_n)
> +intel_dp_compute_drrs(struct intel_dp *intel_dp,
> +		      struct intel_crtc_state *pipe_config,
> +		      int output_bpp, bool constant_n)
intel_dp_drrs_compute_config looks a better name which u have introduced in patch 1 of this series.
any reason to change the function name in second patch ?
>  {
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> @@ -2586,8 +2586,8 @@ intel_dp_drrs_compute_config(struct intel_dp *intel_dp,
>  	/*
>  	 * DRRS and PSR can't be enable together, so giving preference to PSR
>  	 * as it allows more power-savings by complete shutting down display,
> -	 * so to guarantee this, intel_dp_drrs_compute_config() must be called
> -	 * after intel_psr_compute_config().
> +	 * so to guarantee this, intel_dp_compute_drrs() must be called after
> +	 * intel_psr_compute_config().
>  	 */
>  	if (pipe_config->has_psr)
>  		return;
> @@ -2688,8 +2688,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  		intel_dp_set_clock(encoder, pipe_config);
>  
>  	intel_psr_compute_config(intel_dp, pipe_config);
> -	intel_dp_drrs_compute_config(intel_dp, pipe_config, output_bpp,
> -				     constant_n);
> +	intel_dp_compute_drrs(intel_dp, pipe_config, output_bpp, constant_n);
>  	intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state);
>  	intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state);
>  
> @@ -7736,6 +7735,15 @@ static void intel_dp_set_drrs_state(struct drm_i915_private *dev_priv,
>  		    refresh_rate);
>  }
>  
> +static void
> +intel_edp_drrs_enable_locked(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	dev_priv->drrs.busy_frontbuffer_bits = 0;
> +	dev_priv->drrs.dp = intel_dp;
> +}
> +
>  /**
>   * intel_edp_drrs_enable - init drrs struct if supported
>   * @intel_dp: DP struct
> @@ -7752,19 +7760,34 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp,
>  		return;
>  
>  	mutex_lock(&dev_priv->drrs.mutex);
> +
>  	if (dev_priv->drrs.dp) {
> -		drm_dbg_kms(&dev_priv->drm, "DRRS already enabled\n");
> +		drm_warn(&dev_priv->drm, "DRRS already enabled\n");
>  		goto unlock;
>  	}
>  
> -	dev_priv->drrs.busy_frontbuffer_bits = 0;
> -
> -	dev_priv->drrs.dp = intel_dp;
> +	intel_edp_drrs_enable_locked(intel_dp);
>  
>  unlock:
>  	mutex_unlock(&dev_priv->drrs.mutex);
>  }
>  
> +static void
> +intel_edp_drrs_disable_locked(struct intel_dp *intel_dp,
> +			      const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
> +		int refresh;
> +
> +		refresh = drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode);
> +		intel_dp_set_drrs_state(dev_priv, crtc_state, refresh);
> +	}
> +
> +	dev_priv->drrs.dp = NULL;
> +}
> +
>  /**
>   * intel_edp_drrs_disable - Disable DRRS
>   * @intel_dp: DP struct
> @@ -7785,16 +7808,45 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp,
>  		return;
>  	}
>  
> -	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
> -		intel_dp_set_drrs_state(dev_priv, old_crtc_state,
> -			drm_mode_vrefresh(intel_dp->attached_connector->panel.fixed_mode));
> -
> -	dev_priv->drrs.dp = NULL;
> +	intel_edp_drrs_disable_locked(intel_dp, old_crtc_state);
>  	mutex_unlock(&dev_priv->drrs.mutex);
>  
>  	cancel_delayed_work_sync(&dev_priv->drrs.work);
>  }
>  
> +/**
> + * intel_edp_drrs_update - Update DRRS state
> + * @intel_dp: Intel DP
> + * @crtc_state: new CRTC state
> + *
> + * This function will update DRRS states, disabling or enabling DRRS when
> + * executing fastsets. For full modeset, intel_edp_drrs_disable() and
> + * intel_edp_drrs_enable() should be called instead.
> + */
> +void
> +intel_edp_drrs_update(struct intel_dp *intel_dp,
> +		      const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	if (dev_priv->drrs.type != SEAMLESS_DRRS_SUPPORT)
> +		return;
> +
> +	mutex_lock(&dev_priv->drrs.mutex);
> +
> +	/* New state matches current one? */
> +	if (crtc_state->has_drrs == !!dev_priv->drrs.dp)
> +		goto unlock;
> +
> +	if (crtc_state->has_drrs)
> +		intel_edp_drrs_enable_locked(intel_dp);
> +	else
> +		intel_edp_drrs_disable_locked(intel_dp, crtc_state);
> +
> +unlock:
> +	mutex_unlock(&dev_priv->drrs.mutex);
> +}
> +
>  static void intel_edp_drrs_downclock_work(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index b901ab850cbd..057b2c152cbd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -81,6 +81,8 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp,
>  			   const struct intel_crtc_state *crtc_state);
>  void intel_edp_drrs_disable(struct intel_dp *intel_dp,
>  			    const struct intel_crtc_state *crtc_state);
> +void intel_edp_drrs_update(struct intel_dp *intel_dp,
> +			   const struct intel_crtc_state *crtc_state);
>  void intel_edp_drrs_invalidate(struct drm_i915_private *dev_priv,
>  			       unsigned int frontbuffer_bits);
>  void intel_edp_drrs_flush(struct drm_i915_private *dev_priv,
> -- 
> 2.28.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx