[Intel-gfx] [PATCH v2 4/9] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Souza, Jose]

On Thu, 2018-04-26 at 15:29 -0700, Dhinakaran Pandiyan wrote:
> 
> 
> On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> > eDP spec states that sink device will do a short pulse in HPD
> > line when there is a PSR/PSR2 error that needs to be handled by
> > source, this is handling the first and most simples error:
> > DP_PSR_SINK_INTERNAL_ERROR.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > 
> > Changes from v1:
> > - printing a debug message when sink assert a error
> > 
> >  drivers/gpu/drm/i915/intel_dp.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 48
> > +++++++++++++++++++++++++++++---
> >  3 files changed, 47 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4298ac..701963a192ee 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp
> > *intel_dp)
> >  	if (intel_dp_needs_link_retrain(intel_dp))
> >  		return false;
> >  
> > +	intel_psr_hpd_short_pulse_handle(intel_dp);
> 
> 	 intel_psr_short_pulse() should be sufficient.

done

> 
> > +
> >  	if (intel_dp->compliance.test_type ==
> > DP_TEST_LINK_TRAINING) {
> >  		DRM_DEBUG_KMS("Link Training Compliance Test
> > requested\n");
> >  		/* Send a Hotplug Uevent to userspace to start
> > modeset */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 5bd2263407b2..b79e15ecd052 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct
> > drm_i915_private *dev_priv,
> >  				   unsigned frontbuffer_bits);
> >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  			      struct intel_crtc_state
> > *crtc_state);
> > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 934498505356..4cb27faab707 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -996,6 +996,15 @@ void intel_psr_invalidate(struct
> > drm_i915_private *dev_priv,
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > +static void intel_psr_schedule_activate_work(struct
> > drm_i915_private *dev_priv)
> > +{
> > +	if (dev_priv->psr.active || dev_priv-
> > >psr.busy_frontbuffer_bits
> > +	    || work_busy(&dev_priv->psr.work.work))
> > +		return;
> > +
> > +	schedule_delayed_work(&dev_priv->psr.work,
> > msecs_to_jiffies(100));
> > +}
> > +
> >  /**
> >   * intel_psr_flush - Flush PSR
> >   * @dev_priv: i915 device
> > @@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> >  		}
> >  	}
> >  
> > -	if (!dev_priv->psr.active && !dev_priv-
> > >psr.busy_frontbuffer_bits)
> > -		if (!work_busy(&dev_priv->psr.work.work))
> > -			schedule_delayed_work(&dev_priv->psr.work,
> > -					      msecs_to_jiffies(100
> > ));
> > +	intel_psr_schedule_activate_work(dev_priv);
> > +
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > @@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> >  		dev_priv->psr.exit = hsw_psr_exit;
> >  	}
> >  }
> > +
> > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct i915_psr *psr = &dev_priv->psr;
> > +	uint8_t val;
> > +
> > +	if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> > +		return;
> > +
> > +	mutex_lock(&psr->lock);
> > +
> > +	if (psr->enabled != intel_dp)
> > +		goto not_enabled;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val)
> > != 1) {
> > +		DRM_DEBUG_KMS("PSR_STATUS read failed\n");
> 
> Since we can't handle the irq without reading the dpcd and the irq is
> potentially an error condition, I think this should be DRM_ERROR.

Done

> 
> 
> > +		goto dpcd_error;
> > +	}
> > +
> > +	if ((val & DP_PSR_SINK_STATE_MASK) ==
> > DP_PSR_SINK_INTERNAL_ERROR) {
> > +		DRM_DEBUG_KMS("PSR sink internal error, exiting
> > PSR\n");
> > +		intel_psr_exit(dev_priv);
> 
> Shouldn't this be disabling PSR? Exit will allow for re-activation
> immediately. An unknown error in the sink IMO should disable PSR for
> good.


I was not able to reproduce any of the PSR errors even once, I guess
only exit and enter PSR should be enough to sink fix the error but I
guess you are right, better be safe and avoid any rendering problems.

> 
> > +	}
> > +
> > +	/* TODO: handle other PSR/PSR2 errors */
> > +dpcd_error:
> > +	intel_psr_schedule_activate_work(dev_priv);
> 
> Why? There is no intel_psr_exit() before the goto.

yep, it can be removed. It was a left over from the series with the
patches to exit psr on aux ch transfer.
Also now disabling PSR it will not be need.

> 
> 
> > +not_enabled:
> > +	mutex_unlock(&psr->lock);
> > +}
> 
>