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@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
