Re: [PATCH v3 4/7] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2018-05-22 at 16:58 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-05-17 at 15:21 -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.
> > 
> > Here taking the safest approach and disabling PSR(at least until
> > the next modeset), to avoid multiple rendering issues due to
> > bad pannels.
> > 
> > v3:
> > disabling PSR instead of exiting on error
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 62 +++++++++++++++++++++++++---
> > --
> > --
> >  3 files changed, 52 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index b86da48fd38e..fa2851d4fb36 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4479,6 +4479,8 @@ intel_dp_short_pulse(struct intel_dp
> > *intel_dp)
> >  	if (intel_dp_needs_link_retrain(intel_dp))
> >  		return false;
> >  
> > +	intel_psr_short_pulse(intel_dp);
> > +
> >  	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 4508be628450..892da65358e9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1921,6 +1921,7 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  			      struct intel_crtc_state
> > *crtc_state);
> >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> > debug);
> >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > psr_iir);
> > +void intel_psr_short_pulse(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 d88799482875..60797c8f9f0e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -741,6 +741,23 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp)
> >  	psr_aux_io_power_put(intel_dp);
> >  }
> >  
> > +static void psr_disable(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);
> > +
> > +	if (!dev_priv->psr.enabled)
> > +		return;
> > +
> > +	dev_priv->psr.disable_source(intel_dp);
> > +
> > +	/* Disable PSR on Sink */
> > +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> > +	dev_priv->psr.enabled = NULL;
> > +	cancel_delayed_work_sync(&dev_priv->psr.work);
> > +}
> > +
> >  /**
> >   * intel_psr_disable - Disable PSR
> >   * @intel_dp: Intel DP
> > @@ -762,20 +779,8 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  		return;
> >  
> >  	mutex_lock(&dev_priv->psr.lock);
> > -	if (!dev_priv->psr.enabled) {
> > -		mutex_unlock(&dev_priv->psr.lock);
> > -		return;
> > -	}
> > -
> > -	dev_priv->psr.disable_source(intel_dp);
> > -
> > -	/* Disable PSR on Sink */
> > -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> > -
> > -	dev_priv->psr.enabled = NULL;
> > +	psr_disable(intel_dp);
> >  	mutex_unlock(&dev_priv->psr.lock);
> > -
> > -	cancel_delayed_work_sync(&dev_priv->psr.work);
> >  }
> >  
> >  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > @@ -1014,3 +1019,34 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> >  	dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
> >  
> >  }
> > +
> > +void intel_psr_short_pulse(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;
> 
> 	CAN_PSR(dev_priv) should take care of this.

CAN_PSR is better and I will use that, but to remove the lock and 'if
(psr->enabled != intel_dp)' we would also need to check
i915_modparams.enable_psr. Even although we could end up doing the aux
transactions bellow and PSR is disabled(because of one of the errors
bellow), what do you think it is still worthy do it lockless?

> 
> > +
> > +	mutex_lock(&psr->lock);
> 
> Do we really need to acquire the mutex here? How about
> > +
> > +	if (psr->enabled != intel_dp)
> 
> not doing this check?
> 
> > +		goto exit;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val)
> > != 1) {
> > +		DRM_ERROR("PSR_STATUS dpcd read failed\n");
> > +		goto exit;
> > +	}
> > +
> > +	if ((val & DP_PSR_SINK_STATE_MASK) ==
> > DP_PSR_SINK_INTERNAL_ERROR) {
> > +		DRM_DEBUG_KMS("PSR sink internal error, disabling
> > PSR\n");
> > +		psr_disable(intel_dp);
> 
> And calling intel_psr_disable() here?
> 
> By doing this, we can minimize the time for which the lock is held.
> 
> > +	}
> > +
> > +	/* TODO: handle other PSR/PSR2 errors */
> > +exit:
> > +	mutex_unlock(&psr->lock);
> > +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx