Re: [Intel-gfx] [PATCH 1/2] drm/i915: Compute has_drrs after compute has_psr

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

On 2020-08-20 at 10:23:52 -0700, José Roberto de Souza wrote:
> 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, it should compute DRRS state after compute PSR.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 52 +++++++++++++++----------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 79c27f91f42c..3bf50b1ae983 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2575,6 +2575,34 @@ intel_dp_compute_hdr_metadata_infoframe_sdp(struct intel_dp *intel_dp,
>  		intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GAMUT_METADATA);
>  }
>  
> +static void
> +intel_dp_drrs_compute_config(struct intel_dp *intel_dp,
> +			     struct intel_crtc_state *pipe_config,
> +			     int output_bpp, bool constant_n)
> +{
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +	struct drm_i915_private *dev_priv = dp_to_i915(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().
> +	 */
> +	if (pipe_config->has_psr)
> +		return;
In cases when psr is supported but is not enabled due to any psr error, drrs can be useful.
AFAIU has_psr is false in cases psr is disabled due any PSR error, this should be fine for such case
> +
> +	if (!intel_connector->panel.downclock_mode ||
> +	    dev_priv->drrs.type != SEAMLESS_DRRS_SUPPORT)
> +		return;
> +
> +	pipe_config->has_drrs = true;
> +	intel_link_compute_m_n(output_bpp, pipe_config->lane_count,
> +			       intel_connector->panel.downclock_mode->clock,
> +			       pipe_config->port_clock, &pipe_config->dp_m2_n2,
> +			       constant_n, pipe_config->fec_enable);
> +}
> +
>  int
>  intel_dp_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config,
> @@ -2605,7 +2633,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	if (ret)
>  		return ret;
>  
> -	pipe_config->has_drrs = false;
May be u can retain it for better readability.
>  	if (!intel_dp_port_has_audio(dev_priv, port))
>  		pipe_config->has_audio = false;
>  	else if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
> @@ -2657,21 +2684,12 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  			       &pipe_config->dp_m_n,
>  			       constant_n, pipe_config->fec_enable);
>  
> -	if (intel_connector->panel.downclock_mode != NULL &&
> -		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> -			pipe_config->has_drrs = true;
> -			intel_link_compute_m_n(output_bpp,
> -					       pipe_config->lane_count,
> -					       intel_connector->panel.downclock_mode->clock,
> -					       pipe_config->port_clock,
> -					       &pipe_config->dp_m2_n2,
> -					       constant_n, pipe_config->fec_enable);
> -	}
> -
>  	if (!HAS_DDI(dev_priv))
>  		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_vsc_sdp(intel_dp, pipe_config, conn_state);
>  	intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state);
>  
> @@ -7730,16 +7748,8 @@ void intel_edp_drrs_enable(struct intel_dp *intel_dp,
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  
> -	if (!crtc_state->has_drrs) {
> -		drm_dbg_kms(&dev_priv->drm, "Panel doesn't support DRRS\n");
IMHO this print is useful.
with all of above,
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com> 
> +	if (!crtc_state->has_drrs)
>  		return;
> -	}
> -
> -	if (dev_priv->psr.enabled) {
> -		drm_dbg_kms(&dev_priv->drm,
> -			    "PSR enabled. Not enabling DRRS.\n");
> -		return;
> -	}
>  
>  	mutex_lock(&dev_priv->drrs.mutex);
>  	if (dev_priv->drrs.dp) {
> -- 
> 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