Re: [Intel-gfx] [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen
On Wed, 2018-10-24 at 16:22 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-10-24 at 15:08 -0700, Dhinakaran Pandiyan wrote:
> > On Sat, 2018-10-20 at 00:12 +, Souza, Jose wrote:
> > > On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> > > > On Wed, 2018-10-10 at 17:41 -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.
> > > > >
> > > > > Cc: Dhinakaran Pandiyan
> > > > > Signed-off-by: José Roberto de Souza
> > > > > ---
> > > > > 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 3017ef037fed..e8ba00dd2c51 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 70d4e26e17b5..ad09130cb4ad 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
> > > > > ));
> > > >
> > > > Downgrade this to debug since the error is handled in the
> > > > driver?
> > >
> > > I think is better keep as DRM_WARN so it is shown in regular
> > > kernel
> > > logs this way if a user opens a bug complaning why PSR is
> > > disabled
> > > we
> > > can check that is because of PSR aux error.
> > >
> > > > > +
> > > > > + spin_lock(_priv->irq_lock);
> >
> > This lock isn't needed either. How about setting a bool only if the
> > transcoder is eDP and then scheduling a disable.
> >
> > > > > + dev_priv->psr.irq_aux_error |=
> > > > > BIT(cpu_transcoder);
> > > >
> > > > Just ignore the non eDP bits, I don't think we want to do
> > > > anything
> > > > with
> > > > the information that some other bit was set.
> > > >
> > > > > + spin_unlock(_priv->irq_lock);
> > > > > +
> > > > > + schedule_work(_priv->psr.work);
> > > > > + }
> > > > >
> > > > > if (psr_iir &
> > > > > EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > > > dev_priv->psr.last_entry_attempt =
> > > > > time_ns;
> > > > > @@ -893,11 +899,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 = _priv->psr;
> > > > > + u32 irq_aux_error;
> > > > > +
> > > > > + spin_lock_irq(_priv->irq_lock);
> > > > > + irq_aux_error = psr->irq_aux_error;
> > > > > + psr->irq_aux_error = 0;
> > > >
> > > > A subsequent modeset will enable PSR again. I don't expect a
> > > > modeset
> > > > to
> > > > to be able to fix an AUX wake up failure, so might as well
> > > > disable
> > > > it
> > > > for good.
> > >
> > > Add another field to do that or set sink_support=false? I
Re: [Intel-gfx] [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen
On Wed, 2018-10-24 at 15:08 -0700, Dhinakaran Pandiyan wrote:
> On Sat, 2018-10-20 at 00:12 +, Souza, Jose wrote:
> > On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> > > On Wed, 2018-10-10 at 17:41 -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.
> > > >
> > > > Cc: Dhinakaran Pandiyan
> > > > Signed-off-by: José Roberto de Souza
> > > > ---
> > > > 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 3017ef037fed..e8ba00dd2c51 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 70d4e26e17b5..ad09130cb4ad 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
> > > > ));
> > >
> > > Downgrade this to debug since the error is handled in the
> > > driver?
> >
> > I think is better keep as DRM_WARN so it is shown in regular kernel
> > logs this way if a user opens a bug complaning why PSR is disabled
> > we
> > can check that is because of PSR aux error.
> >
> > >
> > > > +
> > > > + spin_lock(_priv->irq_lock);
>
> This lock isn't needed either. How about setting a bool only if the
> transcoder is eDP and then scheduling a disable.
>
> > > > + dev_priv->psr.irq_aux_error |=
> > > > BIT(cpu_transcoder);
> > >
> > > Just ignore the non eDP bits, I don't think we want to do
> > > anything
> > > with
> > > the information that some other bit was set.
> > >
> > > > + spin_unlock(_priv->irq_lock);
> > > > +
> > > > + schedule_work(_priv->psr.work);
> > > > + }
> > > >
> > > > if (psr_iir &
> > > > EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > > dev_priv->psr.last_entry_attempt =
> > > > time_ns;
> > > > @@ -893,11 +899,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 = _priv->psr;
> > > > + u32 irq_aux_error;
> > > > +
> > > > + spin_lock_irq(_priv->irq_lock);
> > > > + irq_aux_error = psr->irq_aux_error;
> > > > + psr->irq_aux_error = 0;
> > >
> > > A subsequent modeset will enable PSR again. I don't expect a
> > > modeset
> > > to
> > > to be able to fix an AUX wake up failure, so might as well
> > > disable
> > > it
> > > for good.
> >
> > Add another field to do that or set sink_support=false? I guess PSR
> > short pulses errors should also disable it good too?
>
> Reusing sink_support will get confusing, particularly because it is
> exposed in debugfs.
>
> >
> > >
> > > > + spin_unlock_irq(_priv->irq_lock);
> > > > +
> > > > + /* right now PSR is only enabled in eDP */
> > >
> > >
Re: [Intel-gfx] [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen
On Sat, 2018-10-20 at 00:12 +, Souza, Jose wrote:
> On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> > On Wed, 2018-10-10 at 17:41 -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.
> > >
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > 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 3017ef037fed..e8ba00dd2c51 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 70d4e26e17b5..ad09130cb4ad 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));
> >
> > Downgrade this to debug since the error is handled in the driver?
>
> I think is better keep as DRM_WARN so it is shown in regular kernel
> logs this way if a user opens a bug complaning why PSR is disabled we
> can check that is because of PSR aux error.
>
> >
> > > +
> > > + spin_lock(_priv->irq_lock);
This lock isn't needed either. How about setting a bool only if the
transcoder is eDP and then scheduling a disable.
> > > + dev_priv->psr.irq_aux_error |=
> > > BIT(cpu_transcoder);
> >
> > Just ignore the non eDP bits, I don't think we want to do anything
> > with
> > the information that some other bit was set.
> >
> > > + spin_unlock(_priv->irq_lock);
> > > +
> > > + schedule_work(_priv->psr.work);
> > > + }
> > >
> > > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > dev_priv->psr.last_entry_attempt = time_ns;
> > > @@ -893,11 +899,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 = _priv->psr;
> > > + u32 irq_aux_error;
> > > +
> > > + spin_lock_irq(_priv->irq_lock);
> > > + irq_aux_error = psr->irq_aux_error;
> > > + psr->irq_aux_error = 0;
> >
> > A subsequent modeset will enable PSR again. I don't expect a
> > modeset
> > to
> > to be able to fix an AUX wake up failure, so might as well disable
> > it
> > for good.
>
> Add another field to do that or set sink_support=false? I guess PSR
> short pulses errors should also disable it good too?
Reusing sink_support will get confusing, particularly because it is
exposed in debugfs.
>
> >
> > > + spin_unlock_irq(_priv->irq_lock);
> > > +
> > > + /* right now PSR is only enabled in eDP */
> >
> > "right now" implies that PSR could be enabled for non eDP ports,
> > but
> > that's not the case.
> >
> >
> > > + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> >
> > This should go away if you ignore non-EDP bits, and a stack trace
> > isn't
> > particularly useful anyway.
>
> Okay I will remove this handlings for other transcoders.
>
> >
> > > +
> > > + mutex_lock(>lock);
> >
> > Is this sufficient? Don't we have to serialize against ongoing
> > modesets
> > like we do for debugfs enable/disable. The disable sequence in
> > bspec
> > calls out a running
Re: [Intel-gfx] [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen
On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-10-10 at 17:41 -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.
> >
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: José Roberto de Souza
> > ---
> > 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 3017ef037fed..e8ba00dd2c51 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 70d4e26e17b5..ad09130cb4ad 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));
> Downgrade this to debug since the error is handled in the driver?
I think is better keep as DRM_WARN so it is shown in regular kernel
logs this way if a user opens a bug complaning why PSR is disabled we
can check that is because of PSR aux error.
>
> > +
> > + spin_lock(_priv->irq_lock);
> > + dev_priv->psr.irq_aux_error |=
> > BIT(cpu_transcoder);
> Just ignore the non eDP bits, I don't think we want to do anything
> with
> the information that some other bit was set.
>
> > + spin_unlock(_priv->irq_lock);
> > +
> > + schedule_work(_priv->psr.work);
> > + }
> >
> > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > dev_priv->psr.last_entry_attempt = time_ns;
> > @@ -893,11 +899,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 = _priv->psr;
> > + u32 irq_aux_error;
> > +
> > + spin_lock_irq(_priv->irq_lock);
> > + irq_aux_error = psr->irq_aux_error;
> > + psr->irq_aux_error = 0;
> A subsequent modeset will enable PSR again. I don't expect a modeset
> to
> to be able to fix an AUX wake up failure, so might as well disable it
> for good.
Add another field to do that or set sink_support=false? I guess PSR
short pulses errors should also disable it good too?
>
> > + spin_unlock_irq(_priv->irq_lock);
> > +
> > + /* right now PSR is only enabled in eDP */
> "right now" implies that PSR could be enabled for non eDP ports, but
> that's not the case.
>
>
> > + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> This should go away if you ignore non-EDP bits, and a stack trace
> isn't
> particularly useful anyway.
Okay I will remove this handlings for other transcoders.
>
> > +
> > + mutex_lock(>lock);
> Is this sufficient? Don't we have to serialize against ongoing
> modesets
> like we do for debugfs enable/disable. The disable sequence in bspec
> calls out a running pipe and port as pre-requisites.
HW will only send a aux transaction when exiting PSR, in this cases
pipe will always be running:
- exiting because of changes in the screen
- exiting because pipe will be disabled
- exiting because of PSR error
>
> Ccing Ville and Maarten to get their opinion on this.
>
> > +
> > + intel_psr_disable_locked(psr->dp);
> > + /* let's make sure that sink is awaken */
> > + drm_dp_dpcd_writeb(>dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
> Given that the hardware initiated AUX
Re: [Intel-gfx] [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen
On Wed, 2018-10-10 at 17:41 -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.
>
> Cc: Dhinakaran Pandiyan
> Signed-off-by: José Roberto de Souza
> ---
> 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 3017ef037fed..e8ba00dd2c51 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 70d4e26e17b5..ad09130cb4ad 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));
Downgrade this to debug since the error is handled in the driver?
> +
> + spin_lock(_priv->irq_lock);
> + dev_priv->psr.irq_aux_error |=
> BIT(cpu_transcoder);
Just ignore the non eDP bits, I don't think we want to do anything with
the information that some other bit was set.
> + spin_unlock(_priv->irq_lock);
> +
> + schedule_work(_priv->psr.work);
> + }
>
> if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> dev_priv->psr.last_entry_attempt = time_ns;
> @@ -893,11 +899,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 = _priv->psr;
> + u32 irq_aux_error;
> +
> + spin_lock_irq(_priv->irq_lock);
> + irq_aux_error = psr->irq_aux_error;
> + psr->irq_aux_error = 0;
A subsequent modeset will enable PSR again. I don't expect a modeset to
to be able to fix an AUX wake up failure, so might as well disable it
for good.
> + spin_unlock_irq(_priv->irq_lock);
> +
> + /* right now PSR is only enabled in eDP */
"right now" implies that PSR could be enabled for non eDP ports, but
that's not the case.
> + WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
This should go away if you ignore non-EDP bits, and a stack trace isn't
particularly useful anyway.
> +
> + mutex_lock(>lock);
Is this sufficient? Don't we have to serialize against ongoing modesets
like we do for debugfs enable/disable. The disable sequence in bspec
calls out a running pipe and port as pre-requisites.
Ccing Ville and Maarten to get their opinion on this.
> +
> + intel_psr_disable_locked(psr->dp);
> + /* let's make sure that sink is awaken */
> + drm_dp_dpcd_writeb(>dp->aux, DP_SET_POWER,
> DP_SET_POWER_D0);
Given that the hardware initiated AUX write failed, I would recommend
reading back the sink PSR status to make sure disable worked.
> +
> + mutex_unlock(_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);
If psr_work() was already executing and past this check,
schedule_work() in intel_psr_irq_handler will return a failure and
disable PSR would now depend on getting an invalidate and flush
operation. We should disable PSR without any dependency on flush or
invalidate.
> +
> mutex_lock(_priv->psr.lock);
>
> if (!dev_priv->psr.enabled)
___
