On Mon, 2018-10-08 at 17:49 -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:30 -0700, Souza, Jose wrote:
> > 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_trans
> > > > coder));
> > > > + 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
> Yeah, but doesn't work consistently on the panel that I have, writing
> to the sink DP_SET_POWER dpcd is needed. I guess, we could add a
> panel
> specific quirk to work around it.
The DP_SET_POWER writes that HW does while PSR is enabled in HW should
also fail, that is really strange that only the psr_exit() fails.
>
> -DK
>
> > 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
