| Message ID | 20180517222118.14246-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + > + 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); > +}
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); > > +}
On Wed, 2018-06-13 at 13:17 -0700, Dhinakaran Pandiyan wrote: > On Tue, 2018-06-05 at 22:45 +0000, 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 <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) > > 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(&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? > > That is a good point, avoiding DPCD read in case PSR is disabled is > indeed better. Cool, I will send the new version soon. Thanks
On Tue, 2018-06-05 at 22:45 +0000, 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 <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) 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(&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? That is a good point, avoiding DPCD read in case PSR is disabled is indeed better.
On Wed, 2018-06-13 at 20:02 +0000, Souza, Jose wrote: > On Wed, 2018-06-13 at 13:17 -0700, Dhinakaran Pandiyan wrote: > > > > On Tue, 2018-06-05 at 22:45 +0000, 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 <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) > > 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.
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; + + mutex_lock(&psr->lock); + + if (psr->enabled != intel_dp) + 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); + } + + /* TODO: handle other PSR/PSR2 errors */ +exit: + mutex_unlock(&psr->lock); +}
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(-)