On Tue, 2018-11-20 at 14:47 -0800, Rodrigo Vivi wrote:
> 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?
I think that sinks will trigger a short pulse with a bad link status
set right after receive the first frame after disable PSR, unfortunally
eDP spec don't cover error cases like this.
So if sink does the short pulse and PSR is disabled we are covered
about the retrain with the current code, we only must be sure that sink
is awaken.
DK suggested to read the DPCD to check if sink is awaken but if the
DPCD write failed again(one from hardware and other from driver that
already do multiple tries) it means that sink is in a really bad state
and it will not recover without turning it off and on again.
>
> > +}
> > +
> > 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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
