[Intel-gfx] [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen

[Souza, Jose]

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 at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  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)