Re: [PATCH 4/7] drm/i915: Disable PSR when a PSR aux error happen

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Nov 09, 2018 at 12:20:13PM -0800, José Roberto de Souza wrote:
> While PSR is active hardware will do aux transactions by it self to
> wakeup sink to receive a new frame when necessary. If that
> transaction is not acked by sink, hardware will trigger this
> interruption.
> 
> So let's disable PSR as it is a hint that there is problem with this
> sink.
> 
> The removed FIXME was asking to manually train the link but we don't
> need to do that as by spec sink should do a short pulse when it is
> out of sync with source, we just need to make sure it is awaken and
> the SDP header with PSR disable will trigger this condition.
> 
> v3: added workarround to fix scheduled work starvation cause by
> to frequent PSR error interruption
> v4: only setting irq_aux_error as we don't care in clear it and
> not using dev_priv->irq_lock as consequence.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 41 ++++++++++++++++++++++++++++----
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e13222518c1b..4022a317cf05 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -642,6 +642,7 @@ struct i915_psr {
>  	ktime_t last_entry_attempt;
>  	ktime_t last_exit;
>  	bool sink_not_reliable;
> +	bool irq_aux_error;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index cc738497d551..93d8538a2383 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -152,6 +152,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  	u32 transcoders = BIT(TRANSCODER_EDP);
>  	enum transcoder cpu_transcoder;
>  	ktime_t time_ns =  ktime_get();
> +	u32 mask = 0;
>  
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		transcoders |= BIT(TRANSCODER_A) |
> @@ -159,10 +160,22 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  			       BIT(TRANSCODER_C);
>  
>  	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> -		/* FIXME: Exit PSR and link train manually when this happens. */
> -		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> -			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> -				      transcoder_name(cpu_transcoder));
> +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> +			DRM_WARN("[transcoder %s] PSR aux error\n",
> +				 transcoder_name(cpu_transcoder));
> +
> +			dev_priv->psr.irq_aux_error = true;
> +
> +			/*
> +			 * If this interruption is not masked it will keep
> +			 * interrupting so fast that it prevents the scheduled
> +			 * work to run.
> +			 * Also after a PSR error, we don't want to arm PSR
> +			 * again so we don't care about unmask the interruption
> +			 * or unset irq_aux_error.
> +			 */
> +			mask |= EDP_PSR_ERROR(cpu_transcoder);
> +		}
>  
>  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
>  			dev_priv->psr.last_entry_attempt = time_ns;
> @@ -184,6 +197,13 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  			}
>  		}
>  	}
> +
> +	if (mask) {
> +		mask |= I915_READ(EDP_PSR_IMR);
> +		I915_WRITE(EDP_PSR_IMR, mask);
> +
> +		schedule_work(&dev_priv->psr.work);
> +	}
>  }
>  
>  static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> @@ -898,6 +918,16 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> +static void intel_psr_handle_irq(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_psr *psr = &dev_priv->psr;
> +
> +	intel_psr_disable_locked(psr->dp);
> +	psr->sink_not_reliable = true;
> +	/* let's make sure that sink is awaken */
> +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER, DP_SET_POWER_D0);

Hmmmm... my gut feeling and my bad memory about PSR tells this can flicker
in some panels. But at least you are disabling PSR for good after that what
is a good thing.

I wonder what other alternatives we would have here?
maybe the one suggested on FIXME? disable PSR and train link manually?

> +}
> +
>  static void intel_psr_work(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -908,6 +938,9 @@ static void intel_psr_work(struct work_struct *work)
>  	if (!dev_priv->psr.enabled)
>  		goto unlock;
>  
> +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> +		intel_psr_handle_irq(dev_priv);
> +
>  	/*
>  	 * We have to make sure PSR is ready for re-enable
>  	 * otherwise it keeps disabled until next full enable/disable cycle.
> -- 
> 2.19.1
> 
> _______________________________________________
> 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