[Intel-gfx] [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Dhinakaran Pandiyan]

On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> As stated in CRC_CTL spec, after PSR entry state CRC will not be
> calculated anymore what is not a problem as IGT tests do some screen
> change and then request the pipe CRC right after the change so PSR
> will go to idle state and only will entry again after at least 6
> idles frames.
> 
> But for PSR2 it is more problematic as any change to the screen could
> trigger a selective/partial update causing the CRC value not to be
> calculated over the full frame.
> 
> So here it disables PSR2 and keep it disabled while user is
> requesting pipe CRC.
> 
> BSpec: 7536
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 17fe942eaafa..609e9c5bd453 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -520,6 +520,7 @@ struct i915_psr {
>  	bool sink_not_reliable;
>  	bool irq_aux_error;
>  	u16 su_x_granularity;
> +	bool pipe_crc_enabled;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3398b28c053b..40ce7a600585 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state,
>  			    u32 *out_value);
>  bool intel_psr_enabled(struct intel_dp *intel_dp);
> +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> *dev_priv, enum pipe pipe, bool prepare);
>  
>  /* intel_quirks.c */
>  void intel_init_quirks(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a8554dc4f196..5d8772399f60 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc
> *crtc, const char *source_name,
>  	return -EINVAL;
>  }
>  
> +static inline void intel_crtc_crc_prepare_or_finish(struct drm_crtc
> *crtc, bool prepare)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> prepare);
> +}
> +
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  	if (ret != 0)
>  		goto out;
>  
> +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> INTEL_PIPE_CRC_SOURCE_NONE);
> +
>  	pipe_crc->source = source;
>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 08967836b48e..9c93138988aa 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	if (dev_priv->psr.pipe_crc_enabled)
> +		return false;
> +

Disabling PSR instead of switching to PSR1 is safer considering the
past bug reports with PSR1.
 
>  	return true;
>  }
>  
> @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> *intel_dp)
>  
>  	return ret;
>  }
> +
> +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> *dev_priv, enum pipe pipe, bool prepare)
> +{
> +	bool fastset = false;
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +
> +	if (dev_priv->psr.pipe == pipe) {
> +		dev_priv->psr.pipe_crc_enabled = prepare;

.crc_enabled seems like it belongs in crtc_state rather than in the
global atomic state.

Looks like we could rename and re-purpose crtc_state.ips_force_disable
for this. I don't see that flag being used for anything other working
around CRC issues.


> +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> +	}
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +	if (fastset)
> +		intel_psr_fastset_force(dev_priv);
> +}