Re: [Intel-gfx] [PATCH v2 6/6] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[Souza, Jose]

On Fri, 2018-10-19 at 14:02 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-10-10 at 17:41 -0700, José Roberto de Souza wrote:
> > When a PSR error happens sink also update the link status values
> > while source do not retrain link as required in the PSR exit
> > sequence.
> > So in the short pulse handling it was returning earlier and doing a
> > full detection and attempting to retrain but it fails because for
> > most sinks the main link is disabled while PSR is active, before
> > even
> > check PSR errors.
> > 
> > Just call intel_psr_short_pulse() before
> > intel_dp_needs_link_retrain() is also not the right fix as
> > intel_dp_needs_link_retrain() would return true and trigger a full
> > detection and trying to retrain link while PSR HW is also doing
> > that.
> > 
> > Check for PSR active is also not safe as it could be inactive due a
> > frontbuffer invalidate and still doing the PSR exit sequence.
> > 
> > Cc: Dhinakaran Pandiyan 
> > Signed-off-by: José Roberto de Souza 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  |  7 +++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 15 +++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 1826d491efd7..2a21a9e29eb2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4396,6 +4396,13 @@ intel_dp_needs_link_retrain(struct intel_dp
> > *intel_dp)
> > if (!intel_dp->link_trained)
> > return false;
> >  
> > +   /*
> > +* While PSR is enabled, HW will control main-link and retrain
> > when
> > +* exiting PSR
> > +*/
> > +   if (intel_psr_enabled(intel_dp))
> 
> You need a check for !link_standby. The link status is expected to be
> good if we are not turning the link fully off.

Even with link on while PSR is active, sink could lost sync with main
link.

> 
> -DK
> 
> > +   return false;
> > +
> > if (!intel_dp_get_link_status(intel_dp, link_status))
> > return false;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 3dea7a1bda7f..147a116cfc71 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1958,6 +1958,7 @@ void intel_psr_irq_handler(struct
> > drm_i915_private *dev_priv, u32 psr_iir);
> >  void intel_psr_short_pulse(struct intel_dp *intel_dp);
> >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > *new_crtc_state,
> > u32 *out_value);
> > +bool intel_psr_enabled(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 ad09130cb4ad..e4429f2f3034 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -1138,3 +1138,18 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp)
> >  exit:
> > mutex_unlock(>lock);
> >  }
> > +
> > +bool intel_psr_enabled(struct intel_dp *intel_dp)
> > +{
> > +   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +   bool ret;
> > +
> > +   if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> > +   return false;
> > +
> > +   mutex_lock(_priv->psr.lock);
> > +   ret = (dev_priv->psr.dp == intel_dp && dev_priv->psr.enabled);
> > +   mutex_unlock(_priv->psr.lock);
> > +
> > +   return ret;
> > +}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 6/6] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[Dhinakaran Pandiyan]

On Wed, 2018-10-10 at 17:41 -0700, José Roberto de Souza wrote:
> When a PSR error happens sink also update the link status values
> while source do not retrain link as required in the PSR exit
> sequence.
> So in the short pulse handling it was returning earlier and doing a
> full detection and attempting to retrain but it fails because for
> most sinks the main link is disabled while PSR is active, before even
> check PSR errors.
> 
> Just call intel_psr_short_pulse() before
> intel_dp_needs_link_retrain() is also not the right fix as
> intel_dp_needs_link_retrain() would return true and trigger a full
> detection and trying to retrain link while PSR HW is also doing that.
> 
> Check for PSR active is also not safe as it could be inactive due a
> frontbuffer invalidate and still doing the PSR exit sequence.
> 
> Cc: Dhinakaran Pandiyan 
> Signed-off-by: José Roberto de Souza 
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  7 +++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 15 +++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 1826d491efd7..2a21a9e29eb2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4396,6 +4396,13 @@ intel_dp_needs_link_retrain(struct intel_dp
> *intel_dp)
>   if (!intel_dp->link_trained)
>   return false;
>  
> + /*
> +  * While PSR is enabled, HW will control main-link and retrain
> when
> +  * exiting PSR
> +  */
> + if (intel_psr_enabled(intel_dp))

You need a check for !link_standby. The link status is expected to be
good if we are not turning the link fully off.

-DK

> + return false;
> +
>   if (!intel_dp_get_link_status(intel_dp, link_status))
>   return false;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3dea7a1bda7f..147a116cfc71 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1958,6 +1958,7 @@ void intel_psr_irq_handler(struct
> drm_i915_private *dev_priv, u32 psr_iir);
>  void intel_psr_short_pulse(struct intel_dp *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state,
>   u32 *out_value);
> +bool intel_psr_enabled(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 ad09130cb4ad..e4429f2f3034 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1138,3 +1138,18 @@ void intel_psr_short_pulse(struct intel_dp
> *intel_dp)
>  exit:
>   mutex_unlock(>lock);
>  }
> +
> +bool intel_psr_enabled(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> + bool ret;
> +
> + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> + return false;
> +
> + mutex_lock(_priv->psr.lock);
> + ret = (dev_priv->psr.dp == intel_dp && dev_priv->psr.enabled);
> + mutex_unlock(_priv->psr.lock);
> +
> + return ret;
> +}

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

[Intel-gfx] [PATCH v2 6/6] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[José Roberto de Souza]

When a PSR error happens sink also update the link status values
while source do not retrain link as required in the PSR exit
sequence.
So in the short pulse handling it was returning earlier and doing a
full detection and attempting to retrain but it fails because for
most sinks the main link is disabled while PSR is active, before even
check PSR errors.

Just call intel_psr_short_pulse() before
intel_dp_needs_link_retrain() is also not the right fix as
intel_dp_needs_link_retrain() would return true and trigger a full
detection and trying to retrain link while PSR HW is also doing that.

Check for PSR active is also not safe as it could be inactive due a
frontbuffer invalidate and still doing the PSR exit sequence.

Cc: Dhinakaran Pandiyan 
Signed-off-by: José Roberto de Souza 
---
 drivers/gpu/drm/i915/intel_dp.c  |  7 +++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 15 +++
 3 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1826d491efd7..2a21a9e29eb2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4396,6 +4396,13 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
if (!intel_dp->link_trained)
return false;
 
+   /*
+* While PSR is enabled, HW will control main-link and retrain when
+* exiting PSR
+*/
+   if (intel_psr_enabled(intel_dp))
+   return false;
+
if (!intel_dp_get_link_status(intel_dp, link_status))
return false;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3dea7a1bda7f..147a116cfc71 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1958,6 +1958,7 @@ void intel_psr_irq_handler(struct drm_i915_private 
*dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
 int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
u32 *out_value);
+bool intel_psr_enabled(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 ad09130cb4ad..e4429f2f3034 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1138,3 +1138,18 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
 exit:
mutex_unlock(>lock);
 }
+
+bool intel_psr_enabled(struct intel_dp *intel_dp)
+{
+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+   bool ret;
+
+   if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
+   return false;
+
+   mutex_lock(_priv->psr.lock);
+   ret = (dev_priv->psr.dp == intel_dp && dev_priv->psr.enabled);
+   mutex_unlock(_priv->psr.lock);
+
+   return ret;
+}
-- 
2.19.1

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