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

[Dhinakaran Pandiyan]

On Wed, 2018-06-13 at 20:02 +, Souza, Jose wrote:
> On Wed, 2018-06-13 at 13:17 -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Tue, 2018-06-05 at 22:45 +, Souza, Jose wrote:
> > > 
> > > 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 
> > > > > Cc: Rodrigo Vivi 
> > > > > Signed-off-by: José Roberto de Souza 
> > > > > ---
> > > > >  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)
> > nit: How about __psr_disable()? Might be worth checking other files
> > what the right convention is.
> It varies from file to file but inside of intel_psr.c we are not
> adding
>  "__" so better keep that, right?
Sure, either way is okay with me.



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Souza, Jose]

On Wed, 2018-06-13 at 13:17 -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-05 at 22:45 +, Souza, Jose wrote:
> > 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 
> > > > Cc: Rodrigo Vivi 
> > > > Signed-off-by: José Roberto de Souza 
> > > > ---
> > > >  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)
> 
> nit: How about __psr_disable()? Might be worth checking other files
> what the right convention is.

It varies from file to file but inside of intel_psr.c we are not adding
 "__" so better keep that, right?

> 
> > > > +{+ 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(_dp->aux, DP_PSR_EN_CFG, 0);
> > > > +   dev_priv->psr.enabled = NULL;
> > > > +   cancel_delayed_work_sync(_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(_priv->psr.lock);
> > > > -   if (!dev_priv->psr.enabled) {
> > > > -   mutex_unlock(_priv->psr.lock);
> > > > -   return;
> > > > -   }
> > > > -
> > > > -   dev_priv->psr.disable_source(intel_dp);
> > > > -
> > > > -   /* Disable PSR on Sink */
> > > > -   drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, 0);
> > > > -
> > > > -   dev_priv->psr.enabled = NULL;
> > > > +   psr_disable(intel_dp);
> > > > mutex_unlock(_priv->psr.lock);
> > > > -
> > > > -   cancel_delayed_work_sync(_priv->psr.work);
> > > >  }
> > > >  
> > > >  static bool psr_wait_for_idle(struct drm_i915_private
> > > > *dev_priv)
> > > > @@ -1014,3 +1019,34 

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

[Dhinakaran Pandiyan]

On Tue, 2018-06-05 at 22:45 +, Souza, Jose wrote:
> 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 
> > > Cc: Rodrigo Vivi 
> > > Signed-off-by: José Roberto de Souza 
> > > ---
> > >  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)
nit: How about __psr_disable()? Might be worth checking other files
what the right convention is.

> > > +{+   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(_dp->aux, DP_PSR_EN_CFG, 0);
> > > + dev_priv->psr.enabled = NULL;
> > > + cancel_delayed_work_sync(_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(_priv->psr.lock);
> > > - if (!dev_priv->psr.enabled) {
> > > - mutex_unlock(_priv->psr.lock);
> > > - return;
> > > - }
> > > -
> > > - dev_priv->psr.disable_source(intel_dp);
> > > -
> > > - /* Disable PSR on Sink */
> > > - drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, 0);
> > > -
> > > - dev_priv->psr.enabled = NULL;
> > > + psr_disable(intel_dp);
> > >   mutex_unlock(_priv->psr.lock);
> > > -
> > > - cancel_delayed_work_sync(_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 = _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 

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

[Souza, Jose]

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 
> > Cc: Rodrigo Vivi 
> > Signed-off-by: José Roberto de Souza 
> > ---
> >  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(_dp->aux, DP_PSR_EN_CFG, 0);
> > +   dev_priv->psr.enabled = NULL;
> > +   cancel_delayed_work_sync(_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(_priv->psr.lock);
> > -   if (!dev_priv->psr.enabled) {
> > -   mutex_unlock(_priv->psr.lock);
> > -   return;
> > -   }
> > -
> > -   dev_priv->psr.disable_source(intel_dp);
> > -
> > -   /* Disable PSR on Sink */
> > -   drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, 0);
> > -
> > -   dev_priv->psr.enabled = NULL;
> > +   psr_disable(intel_dp);
> > mutex_unlock(_priv->psr.lock);
> > -
> > -   cancel_delayed_work_sync(_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 = _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(>lock);
> 
> Do we really 

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

[Dhinakaran Pandiyan]

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 
> Cc: Rodrigo Vivi 
> Signed-off-by: José Roberto de Souza 
> ---
>  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(_dp->aux, DP_PSR_EN_CFG, 0);
> + dev_priv->psr.enabled = NULL;
> + cancel_delayed_work_sync(_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(_priv->psr.lock);
> - if (!dev_priv->psr.enabled) {
> - mutex_unlock(_priv->psr.lock);
> - return;
> - }
> -
> - dev_priv->psr.disable_source(intel_dp);
> -
> - /* Disable PSR on Sink */
> - drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, 0);
> -
> - dev_priv->psr.enabled = NULL;
> + psr_disable(intel_dp);
>   mutex_unlock(_priv->psr.lock);
> -
> - cancel_delayed_work_sync(_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 = _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.

> +
> + mutex_lock(>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(_dp->aux, DP_PSR_STATUS, )
> != 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-gfx] [PATCH v3 4/7] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[José Roberto de Souza]

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 
Cc: Rodrigo Vivi 
Signed-off-by: José Roberto de Souza 
---
 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(_dp->aux, DP_PSR_EN_CFG, 0);
+   dev_priv->psr.enabled = NULL;
+   cancel_delayed_work_sync(_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(_priv->psr.lock);
-   if (!dev_priv->psr.enabled) {
-   mutex_unlock(_priv->psr.lock);
-   return;
-   }
-
-   dev_priv->psr.disable_source(intel_dp);
-
-   /* Disable PSR on Sink */
-   drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, 0);
-
-   dev_priv->psr.enabled = NULL;
+   psr_disable(intel_dp);
mutex_unlock(_priv->psr.lock);
-
-   cancel_delayed_work_sync(_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 = _priv->psr;
+   uint8_t val;
+
+   if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
+   return;
+
+   mutex_lock(>lock);
+
+   if (psr->enabled != intel_dp)
+   goto exit;
+
+   if (drm_dp_dpcd_readb(_dp->aux, DP_PSR_STATUS, ) != 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);
+   }
+
+   /* TODO: handle other PSR/PSR2 errors */
+exit:
+   mutex_unlock(>lock);
+}
-- 
2.17.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx