Re: [Intel-gfx] [PATCH 2/2] drm/i915/display: Check PSR parameter and flag only in state compute phase

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

On Tue, Sep 15, 2020 at 07:44:10PM -0700, José Roberto de Souza wrote:
> Due to the debugfs flag, has_psr2 in CRTC state could have a different
> value than psr.psr2_enabled and it was causing PSR2 subfeatures(DC3CO
> and selective fetch) to be set to not a expected state.
> 
> So here only taking in consideration the parameter and debugfs flag
> when computing PSR state, this way the CRTC state will also have
> the correct state.
> 
> intel_psr_fastset_force() was already broken as
> intel_psr_compute_config() was already only enabling PSR when
> psr_global_enabled() and all other PSR requirements are met.
> So some changes was required in this function, now it iterates over
> all connectors, if it is a eDP connector and is active force a modeset
> in the CRTC driving this connector, what will cause the new PSR state
> to be set based on the debugfs flag.
> 
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Looks sensible.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 65 ++++++++++++++----------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 4e09ae61d4aa..383b66d9f2f2 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -91,19 +91,14 @@ static bool psr_global_enabled(struct drm_i915_private *i915)
>  	}
>  }
>  
> -static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
> -			       const struct intel_crtc_state *crtc_state)
> +static bool psr2_global_enabled(struct drm_i915_private *dev_priv)
>  {
> -	/* Cannot enable DSC and PSR2 simultaneously */
> -	drm_WARN_ON(&dev_priv->drm, crtc_state->dsc.compression_enable &&
> -		    crtc_state->has_psr2);
> -
>  	switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
>  	case I915_PSR_DEBUG_DISABLE:
>  	case I915_PSR_DEBUG_FORCE_PSR1:
>  		return false;
>  	default:
> -		return crtc_state->has_psr2;
> +		return true;
>  	}
>  }
>  
> @@ -729,6 +724,11 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	if (!psr2_global_enabled(dev_priv)) {
> +		drm_dbg_kms(&dev_priv->drm, "PSR2 disabled by flag\n");
> +		return false;
> +	}
> +
>  	/*
>  	 * DSC and PSR2 cannot be enabled simultaneously. If a requested
>  	 * resolution requires DSC to be enabled, priority is given to DSC
> @@ -817,8 +817,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	if (intel_dp != dev_priv->psr.dp)
>  		return;
>  
> -	if (!psr_global_enabled(dev_priv))
> +	if (!psr_global_enabled(dev_priv)) {
> +		drm_dbg_kms(&dev_priv->drm, "PSR disabled by flag\n");
>  		return;
> +	}
> +
>  	/*
>  	 * HSW spec explicitly says PSR is tied to port A.
>  	 * BDW+ platforms have a instance of PSR registers per transcoder but
> @@ -959,7 +962,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>  
>  	drm_WARN_ON(&dev_priv->drm, dev_priv->psr.enabled);
>  
> -	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> +	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->uapi.crtc)->pipe;
>  	dev_priv->psr.dc3co_enabled = !!crtc_state->dc3co_exitline;
> @@ -1029,15 +1032,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	drm_WARN_ON(&dev_priv->drm, dev_priv->drrs.dp);
>  
>  	mutex_lock(&dev_priv->psr.lock);
> -
> -	if (!psr_global_enabled(dev_priv)) {
> -		drm_dbg_kms(&dev_priv->drm, "PSR disabled by flag\n");
> -		goto unlock;
> -	}
> -
>  	intel_psr_enable_locked(dev_priv, crtc_state, conn_state);
> -
> -unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1222,8 +1217,8 @@ void intel_psr_update(struct intel_dp *intel_dp,
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  
> -	enable = crtc_state->has_psr && psr_global_enabled(dev_priv);
> -	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
> +	enable = crtc_state->has_psr;
> +	psr2_enable = crtc_state->has_psr2;
>  
>  	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) {
>  		/* Force a PSR exit when enabling CRC to avoid CRC timeouts */
> @@ -1320,10 +1315,11 @@ static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
>  
>  static int intel_psr_fastset_force(struct drm_i915_private *dev_priv)
>  {
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_atomic_state *state;
> -	struct intel_crtc *crtc;
> +	struct drm_connector *conn;
>  	int err;
>  
>  	state = drm_atomic_state_alloc(dev);
> @@ -1334,21 +1330,34 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv)
>  	state->acquire_ctx = &ctx;
>  
>  retry:
> -	for_each_intel_crtc(dev, crtc) {
> -		struct intel_crtc_state *crtc_state =
> -			intel_atomic_get_crtc_state(state, crtc);
>  
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(conn, &conn_iter) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (conn->connector_type != DRM_MODE_CONNECTOR_eDP)
> +			continue;
> +
> +		conn_state = drm_atomic_get_connector_state(state, conn);
> +		if (IS_ERR(conn_state)) {
> +			err = PTR_ERR(conn_state);
> +			goto error;
> +		}
> +
> +		if (!conn_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
>  		if (IS_ERR(crtc_state)) {
>  			err = PTR_ERR(crtc_state);
>  			goto error;
>  		}
>  
> -		if (crtc_state->hw.active && crtc_state->has_psr) {
> -			/* Mark mode as changed to trigger a pipe->update() */
> -			crtc_state->uapi.mode_changed = true;
> -			break;
> -		}
> +		/* Mark mode as changed to trigger a pipe->update() */
> +		crtc_state->mode_changed = true;
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	err = drm_atomic_commit(state);
>  
> -- 
> 2.28.0

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx