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@xxxxxxxxx>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> >
> > 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);
> > +}
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
