| Message ID | 20180418224311.16577-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote: > eDP spec states that sink device will do a short pulse in HPD > line when there is a PSR/PSR2 error that needs to be handled by > source, this is handling the first and most simples error: > DP_PSR_SINK_INTERNAL_ERROR. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > > Changes from v1: > - printing a debug message when sink assert a error > > drivers/gpu/drm/i915/intel_dp.c | 2 ++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_psr.c | 48 +++++++++++++++++++++++++++++--- > 3 files changed, 47 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 62f82c4298ac..701963a192ee 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) > if (intel_dp_needs_link_retrain(intel_dp)) > return false; > > + intel_psr_hpd_short_pulse_handle(intel_dp); intel_psr_short_pulse() should be sufficient. > + > if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { > DRM_DEBUG_KMS("Link Training Compliance Test requested\n"); > /* Send a Hotplug Uevent to userspace to start modeset */ > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 5bd2263407b2..b79e15ecd052 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, > unsigned frontbuffer_bits); > void intel_psr_compute_config(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state); > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp); > > /* intel_runtime_pm.c */ > int intel_power_domains_init(struct drm_i915_private *); > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 934498505356..4cb27faab707 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -996,6 +996,15 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, > mutex_unlock(&dev_priv->psr.lock); > } > > +static void intel_psr_schedule_activate_work(struct drm_i915_private *dev_priv) > +{ > + if (dev_priv->psr.active || dev_priv->psr.busy_frontbuffer_bits > + || work_busy(&dev_priv->psr.work.work)) > + return; > + > + schedule_delayed_work(&dev_priv->psr.work, msecs_to_jiffies(100)); > +} > + > /** > * intel_psr_flush - Flush PSR > * @dev_priv: i915 device > @@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > } > } > > - if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) > - if (!work_busy(&dev_priv->psr.work.work)) > - schedule_delayed_work(&dev_priv->psr.work, > - msecs_to_jiffies(100)); > + intel_psr_schedule_activate_work(dev_priv); > + > mutex_unlock(&dev_priv->psr.lock); > } > > @@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > dev_priv->psr.exit = hsw_psr_exit; > } > } > + > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct i915_psr *psr = &dev_priv->psr; > + uint8_t val; > + > + if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp)) > + return; > + > + mutex_lock(&psr->lock); > + > + if (psr->enabled != intel_dp) > + goto not_enabled; > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) { > + DRM_DEBUG_KMS("PSR_STATUS read failed\n"); Since we can't handle the irq without reading the dpcd and the irq is potentially an error condition, I think this should be DRM_ERROR. > + goto dpcd_error; > + } > + > + if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) { > + DRM_DEBUG_KMS("PSR sink internal error, exiting PSR\n"); > + intel_psr_exit(dev_priv); Shouldn't this be disabling PSR? Exit will allow for re-activation immediately. An unknown error in the sink IMO should disable PSR for good. > + } > + > + /* TODO: handle other PSR/PSR2 errors */ > +dpcd_error: > + intel_psr_schedule_activate_work(dev_priv); Why? There is no intel_psr_exit() before the goto. > +not_enabled: > + mutex_unlock(&psr->lock); > +}
On Thu, 2018-04-26 at 15:29 -0700, Dhinakaran Pandiyan wrote: > > > On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote: > > eDP spec states that sink device will do a short pulse in HPD > > line when there is a PSR/PSR2 error that needs to be handled by > > source, this is handling the first and most simples error: > > DP_PSR_SINK_INTERNAL_ERROR. > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > > > Changes from v1: > > - printing a debug message when sink assert a error > > > > drivers/gpu/drm/i915/intel_dp.c | 2 ++ > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_psr.c | 48 > > +++++++++++++++++++++++++++++--- > > 3 files changed, 47 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 62f82c4298ac..701963a192ee 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp > > *intel_dp) > > if (intel_dp_needs_link_retrain(intel_dp)) > > return false; > > > > + intel_psr_hpd_short_pulse_handle(intel_dp); > > intel_psr_short_pulse() should be sufficient. done > > > + > > if (intel_dp->compliance.test_type == > > DP_TEST_LINK_TRAINING) { > > DRM_DEBUG_KMS("Link Training Compliance Test > > requested\n"); > > /* Send a Hotplug Uevent to userspace to start > > modeset */ > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 5bd2263407b2..b79e15ecd052 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct > > drm_i915_private *dev_priv, > > unsigned frontbuffer_bits); > > void intel_psr_compute_config(struct intel_dp *intel_dp, > > struct intel_crtc_state > > *crtc_state); > > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp); > > > > /* intel_runtime_pm.c */ > > int intel_power_domains_init(struct drm_i915_private *); > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 934498505356..4cb27faab707 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -996,6 +996,15 @@ void intel_psr_invalidate(struct > > drm_i915_private *dev_priv, > > mutex_unlock(&dev_priv->psr.lock); > > } > > > > +static void intel_psr_schedule_activate_work(struct > > drm_i915_private *dev_priv) > > +{ > > + if (dev_priv->psr.active || dev_priv- > > >psr.busy_frontbuffer_bits > > + || work_busy(&dev_priv->psr.work.work)) > > + return; > > + > > + schedule_delayed_work(&dev_priv->psr.work, > > msecs_to_jiffies(100)); > > +} > > + > > /** > > * intel_psr_flush - Flush PSR > > * @dev_priv: i915 device > > @@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private > > *dev_priv, > > } > > } > > > > - if (!dev_priv->psr.active && !dev_priv- > > >psr.busy_frontbuffer_bits) > > - if (!work_busy(&dev_priv->psr.work.work)) > > - schedule_delayed_work(&dev_priv->psr.work, > > - msecs_to_jiffies(100 > > )); > > + intel_psr_schedule_activate_work(dev_priv); > > + > > mutex_unlock(&dev_priv->psr.lock); > > } > > > > @@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private > > *dev_priv) > > dev_priv->psr.exit = hsw_psr_exit; > > } > > } > > + > > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp) > > +{ > > + struct intel_digital_port *intel_dig_port = > > dp_to_dig_port(intel_dp); > > + struct drm_device *dev = intel_dig_port->base.base.dev; > > + struct drm_i915_private *dev_priv = to_i915(dev); > > + struct i915_psr *psr = &dev_priv->psr; > > + uint8_t val; > > + > > + if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp)) > > + return; > > + > > + mutex_lock(&psr->lock); > > + > > + if (psr->enabled != intel_dp) > > + goto not_enabled; > > + > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) > > != 1) { > > + DRM_DEBUG_KMS("PSR_STATUS read failed\n"); > > Since we can't handle the irq without reading the dpcd and the irq is > potentially an error condition, I think this should be DRM_ERROR. Done > > > > + goto dpcd_error; > > + } > > + > > + if ((val & DP_PSR_SINK_STATE_MASK) == > > DP_PSR_SINK_INTERNAL_ERROR) { > > + DRM_DEBUG_KMS("PSR sink internal error, exiting > > PSR\n"); > > + intel_psr_exit(dev_priv); > > Shouldn't this be disabling PSR? Exit will allow for re-activation > immediately. An unknown error in the sink IMO should disable PSR for > good. I was not able to reproduce any of the PSR errors even once, I guess only exit and enter PSR should be enough to sink fix the error but I guess you are right, better be safe and avoid any rendering problems. > > > + } > > + > > + /* TODO: handle other PSR/PSR2 errors */ > > +dpcd_error: > > + intel_psr_schedule_activate_work(dev_priv); > > Why? There is no intel_psr_exit() before the goto. yep, it can be removed. It was a left over from the series with the patches to exit psr on aux ch transfer. Also now disabling PSR it will not be need. > > > > +not_enabled: > > + mutex_unlock(&psr->lock); > > +} > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 62f82c4298ac..701963a192ee 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) if (intel_dp_needs_link_retrain(intel_dp)) return false; + intel_psr_hpd_short_pulse_handle(intel_dp); + if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { DRM_DEBUG_KMS("Link Training Compliance Test requested\n"); /* Send a Hotplug Uevent to userspace to start modeset */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 5bd2263407b2..b79e15ecd052 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, unsigned frontbuffer_bits); void intel_psr_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state); +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp); /* intel_runtime_pm.c */ int intel_power_domains_init(struct drm_i915_private *); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 934498505356..4cb27faab707 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -996,6 +996,15 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, mutex_unlock(&dev_priv->psr.lock); } +static void intel_psr_schedule_activate_work(struct drm_i915_private *dev_priv) +{ + if (dev_priv->psr.active || dev_priv->psr.busy_frontbuffer_bits + || work_busy(&dev_priv->psr.work.work)) + return; + + schedule_delayed_work(&dev_priv->psr.work, msecs_to_jiffies(100)); +} + /** * intel_psr_flush - Flush PSR * @dev_priv: i915 device @@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, } } - if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) - if (!work_busy(&dev_priv->psr.work.work)) - schedule_delayed_work(&dev_priv->psr.work, - msecs_to_jiffies(100)); + intel_psr_schedule_activate_work(dev_priv); + mutex_unlock(&dev_priv->psr.lock); } @@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private *dev_priv) dev_priv->psr.exit = hsw_psr_exit; } } + +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = to_i915(dev); + struct i915_psr *psr = &dev_priv->psr; + uint8_t val; + + if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp)) + return; + + mutex_lock(&psr->lock); + + if (psr->enabled != intel_dp) + goto not_enabled; + + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) { + DRM_DEBUG_KMS("PSR_STATUS read failed\n"); + goto dpcd_error; + } + + if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) { + DRM_DEBUG_KMS("PSR sink internal error, exiting PSR\n"); + intel_psr_exit(dev_priv); + } + + /* TODO: handle other PSR/PSR2 errors */ +dpcd_error: + intel_psr_schedule_activate_work(dev_priv); +not_enabled: + 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. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> --- Changes from v1: - printing a debug message when sink assert a error drivers/gpu/drm/i915/intel_dp.c | 2 ++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 48 +++++++++++++++++++++++++++++--- 3 files changed, 47 insertions(+), 4 deletions(-)