| Message ID | 20180614203433.5602-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 14, 2018 at 01:34:30PM -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. > > v4: > Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse > > 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 | 60 ++++++++++++++++++++++++++------ > 3 files changed, 52 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 67875b00c8df..19585523e4ce 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -720,6 +720,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) what about intel_psr_disable_unlocked()? > +{ > + 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); assert it is locked here... > + > + 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; > +} > + > /** > * intel_psr_disable - Disable PSR > * @intel_dp: Intel DP > @@ -741,17 +758,7 @@ 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); > } > > @@ -992,3 +999,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 (!CAN_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); > +} > -- > 2.17.1 >
On Thu, 2018-06-14 at 14:09 -0700, Rodrigo Vivi wrote: > On Thu, Jun 14, 2018 at 01:34:30PM -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. > > > > v4: > > Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse > > > > 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 | 60 ++++++++++++++++++++++++++ > > ------ > > 3 files changed, 52 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 67875b00c8df..19585523e4ce 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -720,6 +720,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) > what about intel_psr_disable_unlocked()? I was going to suggest this, definitely sounds better. With Rodrigo's suggestions included, Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
On Thu, 2018-06-14 at 14:09 -0700, Rodrigo Vivi wrote: > On Thu, Jun 14, 2018 at 01:34:30PM -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. > > > > v4: > > Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse > > > > 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 | 60 ++++++++++++++++++++++++++ > > ------ > > 3 files changed, 52 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 67875b00c8df..19585523e4ce 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -720,6 +720,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) > > what about intel_psr_disable_unlocked()? unlocked? shouldn't be locked? I'm okay in adding the suffix but it will be different than the other functions in this file. > > > +{ > > + 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); > > assert it is locked here... Done > > > + > > + 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; > > +} > > + > > /** > > * intel_psr_disable - Disable PSR > > * @intel_dp: Intel DP > > @@ -741,17 +758,7 @@ 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); > > } > > > > @@ -992,3 +999,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 (!CAN_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); > > +} > > -- > > 2.17.1 > >
On Thu, Jun 14, 2018 at 04:46:48PM -0700, Souza, Jose wrote: > On Thu, 2018-06-14 at 14:09 -0700, Rodrigo Vivi wrote: > > On Thu, Jun 14, 2018 at 01:34:30PM -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. > > > > > > v4: > > > Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse > > > > > > 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 | 60 ++++++++++++++++++++++++++ > > > ------ > > > 3 files changed, 52 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 67875b00c8df..19585523e4ce 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -720,6 +720,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) > > > > what about intel_psr_disable_unlocked()? > > unlocked? shouldn't be locked? dam... either way seems ambiguous... maybe just __intel_psr_disable() ? > I'm okay in adding the suffix but it will be different than the other > functions in this file. without "intel_" you are with prefix different than the other functions in this file anyways... > > > > > > +{ > > > + 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); > > > > assert it is locked here... > > Done > > > > > > + > > > + 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; > > > +} > > > + > > > /** > > > * intel_psr_disable - Disable PSR > > > * @intel_dp: Intel DP > > > @@ -741,17 +758,7 @@ 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); > > > } > > > > > > @@ -992,3 +999,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 (!CAN_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); > > > +} > > > -- > > > 2.17.1 > > >
On Thu, 2018-06-14 at 16:59 -0700, Rodrigo Vivi wrote: > On Thu, Jun 14, 2018 at 04:46:48PM -0700, Souza, Jose wrote: > > On Thu, 2018-06-14 at 14:09 -0700, Rodrigo Vivi wrote: > > > On Thu, Jun 14, 2018 at 01:34:30PM -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. > > > > > > > > v4: > > > > Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse > > > > > > > > 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 | 60 > > > > ++++++++++++++++++++++++++ > > > > ------ > > > > 3 files changed, 52 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index 67875b00c8df..19585523e4ce 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -720,6 +720,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) > > > > > > what about intel_psr_disable_unlocked()? > > > > unlocked? shouldn't be locked? > > dam... either way seems ambiguous... > > maybe just __intel_psr_disable() ? > > > I'm okay in adding the suffix but it will be different than the > > other > > functions in this file. > > without "intel_" you are with prefix different than the other > functions > in this file anyways... we have some functions without "intel_", like: psr_aux_domain, psr_aux_io_power_get/put, psr_wait_for_idle... > > > > > > > > > > +{ > > > > + 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); > > > > > > assert it is locked here... > > > > Done > > > > > > > > > + > > > > + 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; > > > > +} > > > > + > > > > /** > > > > * intel_psr_disable - Disable PSR > > > > * @intel_dp: Intel DP > > > > @@ -741,17 +758,7 @@ 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); > > > > } > > > > > > > > @@ -992,3 +999,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 (!CAN_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); > > > > +} > > > > -- > > > > 2.17.1 > > > >
On Thu, Jun 14, 2018 at 05:11:53PM -0700, Souza, Jose wrote: > On Thu, 2018-06-14 at 16:59 -0700, Rodrigo Vivi wrote: > > On Thu, Jun 14, 2018 at 04:46:48PM -0700, Souza, Jose wrote: > > > On Thu, 2018-06-14 at 14:09 -0700, Rodrigo Vivi wrote: > > > > On Thu, Jun 14, 2018 at 01:34:30PM -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. > > > > > > > > > > v4: > > > > > Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse > > > > > > > > > > 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 | 60 > > > > > ++++++++++++++++++++++++++ > > > > > ------ > > > > > 3 files changed, 52 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index 67875b00c8df..19585523e4ce 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > > @@ -720,6 +720,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) > > > > > > > > what about intel_psr_disable_unlocked()? > > > > > > unlocked? shouldn't be locked? > > > > dam... either way seems ambiguous... > > > > maybe just __intel_psr_disable() ? > > > > > I'm okay in adding the suffix but it will be different than the > > > other > > > functions in this file. > > > > without "intel_" you are with prefix different than the other > > functions > > in this file anyways... > > we have some functions without "intel_", like: psr_aux_domain, > psr_aux_io_power_get/put, psr_wait_for_idle... oh! :( well... I'd prefer if all had intel_ and we don't have 2 functions one intel_psr_disable and one psr_disable. > > > > > > > > > > > > > > > +{ > > > > > + 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); > > > > > > > > assert it is locked here... > > > > > > Done > > > > > > > > > > > > + > > > > > + 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; > > > > > +} > > > > > + > > > > > /** > > > > > * intel_psr_disable - Disable PSR > > > > > * @intel_dp: Intel DP > > > > > @@ -741,17 +758,7 @@ 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); > > > > > } > > > > > > > > > > @@ -992,3 +999,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 (!CAN_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); > > > > > +} > > > > > -- > > > > > 2.17.1 > > > > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 67875b00c8df..19585523e4ce 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4474,6 +4474,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 8840108749a5..bb6ffdb282fd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1926,6 +1926,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 bc6d54f677dc..af5fcfd98a53 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -720,6 +720,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; +} + /** * intel_psr_disable - Disable PSR * @intel_dp: Intel DP @@ -741,17 +758,7 @@ 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); } @@ -992,3 +999,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 (!CAN_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. v4: Using CAN_PSR instead of HAS_PSR in intel_psr_short_pulse 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 | 60 ++++++++++++++++++++++++++------ 3 files changed, 52 insertions(+), 11 deletions(-)