On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote: > On Fri, 2018-10-05 at 16:35 -0700, 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. > That's a good point, the short pulse handler does handle retraining. > > > 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 | 39 ++++++++++++++++++++++++++++ > > ---- > > 2 files changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 794a8a03c7e6..efbebe1c2ba3 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -638,6 +638,7 @@ struct i915_psr { > > u8 sink_sync_latency; > > ktime_t last_entry_attempt; > > ktime_t last_exit; > > + u32 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 cd9a60d1efa1..74090fffea23 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -159,10 +159,16 @@ 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)); > > + > > + spin_lock(&dev_priv->irq_lock); > > + dev_priv->psr.irq_aux_error |= > > BIT(cpu_transcoder); > > + spin_unlock(&dev_priv->irq_lock); > > + > > + schedule_work(&dev_priv->psr.work); > > + } > > > > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) { > > dev_priv->psr.last_entry_attempt = time_ns; > > @@ -891,11 +897,36 @@ 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; > > + u32 irq_aux_error; > > + > > + spin_lock_irq(&dev_priv->irq_lock); > > + irq_aux_error = psr->irq_aux_error; > > + psr->irq_aux_error = 0; > > + spin_unlock_irq(&dev_priv->irq_lock); > > + > > + /* right now PSR is only enabled in eDP */ > > + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP)); > > + > > + mutex_lock(&psr->lock); > > + > > + intel_psr_disable_locked(psr->dp); > > + /* let's make sure that sink is awaken */ > > + drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER, > > DP_SET_POWER_D0); > > We should be making sure the sink exits PSR after psr_invalidate() -> > psr_exit() too? Which means, we have to figure out a cleaner way to > handle all of this. I am not sure, at this point, what a cleaner > solution will like. However, I'd like PSR disable from invalidate, > PSR > disable from a modeset and PSR disable due to an error share code and > behavior. All of them should basically be > 1) Disable PSR in PSR_CTL > 2) Wait for idle in PSR_STATUS > 3) Write to sink DP_SET_POWER We don't need to wait PSR to be disabled for invalidate(), hardware will do the exit sequence including write to DP_SET_POWER, also in this case we want activate PSR as soon as all frontbuffer changes was commited and PSR is inactive. > > > > > + > > + mutex_unlock(&dev_priv->psr.lock); > > +} > > + > > static void intel_psr_work(struct work_struct *work) > > { > > struct drm_i915_private *dev_priv = > > container_of(work, typeof(*dev_priv), psr.work); > > > > + if (READ_ONCE(dev_priv->psr.irq_aux_error)) > > + intel_psr_handle_irq(dev_priv); > > + > > mutex_lock(&dev_priv->psr.lock); > > > > if (!dev_priv->psr.enabled) _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx