[PATCH v4 1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[PATCH v4 1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[José Roberto de Souza]

It was only used in VLV/CHV so after the removal of the PSR support
for those platforms it is not necessary any more.

Reviewed-by: 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/i915_drv.h  | 3 +--
 drivers/gpu/drm/i915/intel_psr.c | 5 ++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74dd88d8563e..d4b2f9f63806 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -627,8 +627,7 @@ struct i915_psr {
 
 	void (*enable_source)(struct intel_dp *,
 			      const struct intel_crtc_state *);
-	void (*disable_source)(struct intel_dp *,
-			       const struct intel_crtc_state *);
+	void (*disable_source)(struct intel_dp *intel_dp);
 	void (*enable_sink)(struct intel_dp *);
 	void (*activate)(struct intel_dp *);
 	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ef0f4741a95d..bc6d54f677dc 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -677,8 +677,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-static void hsw_psr_disable(struct intel_dp *intel_dp,
-			    const struct intel_crtc_state *old_crtc_state)
+static void hsw_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;
@@ -747,7 +746,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 		return;
 	}
 
-	dev_priv->psr.disable_source(intel_dp, old_crtc_state);
+	dev_priv->psr.disable_source(intel_dp);
 
 	/* Disable PSR on Sink */
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
-- 
2.17.1

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

[PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[José Roberto de Souza]

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)
+{
+	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);
+}
-- 
2.17.1

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

Re: [PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Rodrigo Vivi]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Dhinakaran Pandiyan]

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>

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

Re: [PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Souza, Jose]

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
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Rodrigo Vivi]

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
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Souza, Jose]

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
> > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/5] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Rodrigo Vivi]

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
> > > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/5] drm/i915/psr: Handle PSR RFB storage error

[José Roberto de Souza]

Sink will interrupt source when it have any problem saving or reading
the remote frame buffer.

v3:
disabling PSR instead of exiting on error

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index af5fcfd98a53..fd240e45f341 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1026,6 +1026,20 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
 		psr_disable(intel_dp);
 	}
 
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS, &val) != 1) {
+		DRM_ERROR("PSR_ERROR_STATUS dpcd read failed\n");
+		goto exit;
+	}
+
+	if (val & DP_PSR_RFB_STORAGE_ERROR) {
+		DRM_DEBUG_KMS("PSR RFB storage error, exiting PSR\n");
+		psr_disable(intel_dp);
+	}
+	if (val & (DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR | DP_PSR_LINK_CRC_ERROR))
+		DRM_ERROR("PSR_ERROR_STATUS not handled %x\n", val);
+	/* clear status register */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
+
 	/* TODO: handle other PSR/PSR2 errors */
 exit:
 	mutex_unlock(&psr->lock);
-- 
2.17.1

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

[PATCH v4 4/5] drm/i915/psr: Avoid PSR exit max time timeout

[José Roberto de Souza]

Specification requires that max time should be masked from bdw and
forward but it can be also safely enabled to hsw.
This will make PSR exits more deterministic and only when really
needed. If this was used to fix a issue in some panel than can
only self-refresh for a few seconds, that panel will interrupt
and assert one of the PSR errors handled in:
'drm/i915/psr: Handle PSR RFB storage error' and
'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink'

Also fixing style here.

Spec: 21664

v4:
patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the
static frame on the sink side' to avoid touch in 2 patches
EDP_PSR_DEBUG.

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_psr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index fd240e45f341..177cd57b1029 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 		 * on frontbuffer tracking.
 		 */
 		I915_WRITE(EDP_PSR_DEBUG,
-			   EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD |
-			   EDP_PSR_DEBUG_MASK_LPSP |
-			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
+			  EDP_PSR_DEBUG_MASK_MEMUP |
+			  EDP_PSR_DEBUG_MASK_HPD |
+			  EDP_PSR_DEBUG_MASK_LPSP |
+			  EDP_PSR_DEBUG_MASK_DISP_REG_WRITE |
+			  EDP_PSR_DEBUG_MASK_MAX_SLEEP);
 	}
 }
 
-- 
2.17.1

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

Re: [PATCH v4 4/5] drm/i915/psr: Avoid PSR exit max time timeout

[Rodrigo Vivi]

On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza wrote:
> Specification requires that max time should be masked from bdw and
> forward but it can be also safely enabled to hsw.
> This will make PSR exits more deterministic and only when really
> needed. If this was used to fix a issue in some panel than can
> only self-refresh for a few seconds, that panel will interrupt
> and assert one of the PSR errors handled in:
> 'drm/i915/psr: Handle PSR RFB storage error' and
> 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink'
> 
> Also fixing style here.

I don't believe that is a style error. So I believe only
the MAX_SLEEP one should be added following same style.

> 
> Spec: 21664
> 
> v4:
> patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the
> static frame on the sink side' to avoid touch in 2 patches
> EDP_PSR_DEBUG.

I thought this change here depended on having CRC check enabled
so it should come first and only bdw+...
Isn't this the case?

> 
> 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_psr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index fd240e45f341..177cd57b1029 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  		 * on frontbuffer tracking.
>  		 */
>  		I915_WRITE(EDP_PSR_DEBUG,
> -			   EDP_PSR_DEBUG_MASK_MEMUP |
> -			   EDP_PSR_DEBUG_MASK_HPD |
> -			   EDP_PSR_DEBUG_MASK_LPSP |
> -			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
> +			  EDP_PSR_DEBUG_MASK_MEMUP |
> +			  EDP_PSR_DEBUG_MASK_HPD |
> +			  EDP_PSR_DEBUG_MASK_LPSP |
> +			  EDP_PSR_DEBUG_MASK_DISP_REG_WRITE |
> +			  EDP_PSR_DEBUG_MASK_MAX_SLEEP);
>  	}
>  }
>  
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm/i915/psr: Avoid PSR exit max time timeout

[Dhinakaran Pandiyan]

On Thu, 2018-06-14 at 14:13 -0700, Rodrigo Vivi wrote:
> On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza
> wrote:
> > 
> > Specification requires that max time should be masked from bdw and
> > forward but it can be also safely enabled to hsw.
> > This will make PSR exits more deterministic and only when really
> > needed. If this was used to fix a issue in some panel than can
> > only self-refresh for a few seconds, that panel will interrupt
> > and assert one of the PSR errors handled in:
> > 'drm/i915/psr: Handle PSR RFB storage error' and
> > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink'
> > 
> > Also fixing style here.
> I don't believe that is a style error. So I believe only
> the MAX_SLEEP one should be added following same style.
> 
> > 
> > 
> > Spec: 21664
> > 
> > v4:
> > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the
> > static frame on the sink side' to avoid touch in 2 patches
> > EDP_PSR_DEBUG.
> I thought this change here depended on having CRC check enabled
> so it should come first and only bdw+...
> Isn't this the case?

The dependency is the other way around, sending CRC's requires timeout
to be disabled. Since disabling timeouts results in a predictable
behavior, we can do it for all platforms.


> 
> > 
> > 
> > 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_psr.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index fd240e45f341..177cd57b1029 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  		 * on frontbuffer tracking.
> >  		 */
> >  		I915_WRITE(EDP_PSR_DEBUG,
> > -			   EDP_PSR_DEBUG_MASK_MEMUP |
> > -			   EDP_PSR_DEBUG_MASK_HPD |
> > -			   EDP_PSR_DEBUG_MASK_LPSP |
> > -			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
> > +			  EDP_PSR_DEBUG_MASK_MEMUP |
> > +			  EDP_PSR_DEBUG_MASK_HPD |
> > +			  EDP_PSR_DEBUG_MASK_LPSP |
> > +			  EDP_PSR_DEBUG_MASK_DISP_REG_WRITE |
> > +			  EDP_PSR_DEBUG_MASK_MAX_SLEEP);
> >  	}
> >  }
> >  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm/i915/psr: Avoid PSR exit max time timeout

[Rodrigo Vivi]

On Thu, Jun 14, 2018 at 02:50:38PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-06-14 at 14:13 -0700, Rodrigo Vivi wrote:
> > On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza
> > wrote:
> > > 
> > > Specification requires that max time should be masked from bdw and
> > > forward but it can be also safely enabled to hsw.
> > > This will make PSR exits more deterministic and only when really
> > > needed. If this was used to fix a issue in some panel than can
> > > only self-refresh for a few seconds, that panel will interrupt
> > > and assert one of the PSR errors handled in:
> > > 'drm/i915/psr: Handle PSR RFB storage error' and
> > > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink'
> > > 
> > > Also fixing style here.
> > I don't believe that is a style error. So I believe only
> > the MAX_SLEEP one should be added following same style.
> > 
> > > 
> > > 
> > > Spec: 21664
> > > 
> > > v4:
> > > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the
> > > static frame on the sink side' to avoid touch in 2 patches
> > > EDP_PSR_DEBUG.
> > I thought this change here depended on having CRC check enabled
> > so it should come first and only bdw+...
> > Isn't this the case?
> 
> The dependency is the other way around, sending CRC's requires timeout
> to be disabled. Since disabling timeouts results in a predictable
> behavior, we can do it for all platforms.

ah, cool. It makes sense then....

> 
> 
> > 
> > > 
> > > 
> > > 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_psr.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index fd240e45f341..177cd57b1029 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  		 * on frontbuffer tracking.
> > >  		 */
> > >  		I915_WRITE(EDP_PSR_DEBUG,
> > > -			   EDP_PSR_DEBUG_MASK_MEMUP |
> > > -			   EDP_PSR_DEBUG_MASK_HPD |
> > > -			   EDP_PSR_DEBUG_MASK_LPSP |
> > > -			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
> > > +			  EDP_PSR_DEBUG_MASK_MEMUP |
> > > +			  EDP_PSR_DEBUG_MASK_HPD |
> > > +			  EDP_PSR_DEBUG_MASK_LPSP |
> > > +			  EDP_PSR_DEBUG_MASK_DISP_REG_WRITE |
> > > +			  EDP_PSR_DEBUG_MASK_MAX_SLEEP);
> > >  	}
> > >  }
> > >  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/5] drm/i915/psr: Avoid PSR exit max time timeout

[Dhinakaran Pandiyan]

On Thu, 2018-06-14 at 14:50 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-06-14 at 14:13 -0700, Rodrigo Vivi wrote:
> > 
> > On Thu, Jun 14, 2018 at 01:34:32PM -0700, José Roberto de Souza
> > wrote:
> > > 
> > > 
> > > Specification requires that max time should be masked from bdw
> > > and
> > > forward but it can be also safely enabled to hsw.
> > > This will make PSR exits more deterministic and only when really
> > > needed. If this was used to fix a issue in some panel than can
> > > only self-refresh for a few seconds, that panel will interrupt
> > > and assert one of the PSR errors handled in:
> > > 'drm/i915/psr: Handle PSR RFB storage error' and
> > > 'drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink'
> > > 
> > > Also fixing style here.
> > I don't believe that is a style error. So I believe only
> > the MAX_SLEEP one should be added following same style.
> > 
> > > 
> > > 
> > > 
> > > Spec: 21664
> > > 
> > > v4:
> > > patch moved to before 'drm/i915/psr/bdw+: Enable CRC check in the
> > > static frame on the sink side' to avoid touch in 2 patches
> > > EDP_PSR_DEBUG.
> > I thought this change here depended on having CRC check enabled
> > so it should come first and only bdw+...
> > Isn't this the case?
> The dependency is the other way around, sending CRC's requires
> timeout
> to be disabled. Since disabling timeouts results in a predictable
> behavior, we can do it for all platforms.
> 
> 
> > 
> > 
> > > 
> > > 
> > > 
> > > 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_psr.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index fd240e45f341..177cd57b1029 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -629,10 +629,11 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  		 * on frontbuffer tracking.
> > >  		 */
> > >  		I915_WRITE(EDP_PSR_DEBUG,
> > > -			   EDP_PSR_DEBUG_MASK_MEMUP |
> > > -			   EDP_PSR_DEBUG_MASK_HPD |
> > > -			   EDP_PSR_DEBUG_MASK_LPSP |
> > > -			   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
> > > +			  EDP_PSR_DEBUG_MASK_MEMUP |
> > > +			  EDP_PSR_DEBUG_MASK_HPD |
> > > +			  EDP_PSR_DEBUG_MASK_LPSP |
> > > +			  EDP_PSR_DEBUG_MASK_DISP_REG_WRITE |
> > > +			  EDP_PSR_DEBUG_MASK_MAX_SLEEP);

With indentation reverted to how it was earlier,
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

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

[PATCH v4 5/5] drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side

[José Roberto de Souza]

Sink can be configured to calculate the CRC over the static frame and
compare with the CRC calculated and transmited in the VSC SDP by
source, if there is a mismatch sink will do a short pulse in HPD
and set DP_PSR_LINK_CRC_ERROR in DP_PSR_ERROR_STATUS.

Spec: 7723

v4:
patch moved to after 'drm/i915/psr: Avoid PSR exit max time timeout'
to avoid touch in 2 patches EDP_PSR_DEBUG.

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/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 140f6a27d696..ed34ccd81c7c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4038,6 +4038,7 @@ enum {
 #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
 #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
 #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
+#define   EDP_PSR_CRC_ENABLE			(1<<10) /* BDW+ */
 #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
 #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
 #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 177cd57b1029..cf72b79caf3f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -360,6 +360,8 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 
 	if (dev_priv->psr.link_standby)
 		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
+	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >= 8)
+		dpcd_val |= DP_PSR_CRC_VERIFICATION;
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
 
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
@@ -415,6 +417,9 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	else
 		val |= EDP_PSR_TP1_TP2_SEL;
 
+	if (INTEL_GEN(dev_priv) >= 8)
+		val |= EDP_PSR_CRC_ENABLE;
+
 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
 	I915_WRITE(EDP_PSR_CTL, val);
 }
@@ -1032,16 +1037,19 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
 		goto exit;
 	}
 
-	if (val & DP_PSR_RFB_STORAGE_ERROR) {
-		DRM_DEBUG_KMS("PSR RFB storage error, exiting PSR\n");
+	if (val & (DP_PSR_RFB_STORAGE_ERROR | DP_PSR_LINK_CRC_ERROR)) {
+		if (val & DP_PSR_RFB_STORAGE_ERROR)
+			DRM_DEBUG_KMS("PSR RFB storage error, disabling PSR\n");
+		if (val & DP_PSR_LINK_CRC_ERROR)
+			DRM_DEBUG_KMS("PSR Link CRC error, disabling PSR\n");
 		psr_disable(intel_dp);
 	}
-	if (val & (DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR | DP_PSR_LINK_CRC_ERROR))
+	if (val & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
 		DRM_ERROR("PSR_ERROR_STATUS not handled %x\n", val);
 	/* clear status register */
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
 
-	/* TODO: handle other PSR/PSR2 errors */
+	/* TODO: handle PSR2 errors */
 exit:
 	mutex_unlock(&psr->lock);
 }
-- 
2.17.1

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

Re: [PATCH v4 5/5] drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side

[Rodrigo Vivi]

On Thu, Jun 14, 2018 at 01:34:33PM -0700, José Roberto de Souza wrote:
> Sink can be configured to calculate the CRC over the static frame and
> compare with the CRC calculated and transmited in the VSC SDP by
> source, if there is a mismatch sink will do a short pulse in HPD
> and set DP_PSR_LINK_CRC_ERROR in DP_PSR_ERROR_STATUS.
> 
> Spec: 7723
> 
> v4:
> patch moved to after 'drm/i915/psr: Avoid PSR exit max time timeout'
> to avoid touch in 2 patches EDP_PSR_DEBUG.
> 
> 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/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 140f6a27d696..ed34ccd81c7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4038,6 +4038,7 @@ enum {
>  #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
>  #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
>  #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
> +#define   EDP_PSR_CRC_ENABLE			(1<<10) /* BDW+ */
>  #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
>  #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
>  #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 177cd57b1029..cf72b79caf3f 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -360,6 +360,8 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  
>  	if (dev_priv->psr.link_standby)
>  		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> +	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >= 8)
> +		dpcd_val |= DP_PSR_CRC_VERIFICATION;
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
>  
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> @@ -415,6 +417,9 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	else
>  		val |= EDP_PSR_TP1_TP2_SEL;
>  
> +	if (INTEL_GEN(dev_priv) >= 8)
> +		val |= EDP_PSR_CRC_ENABLE;
> +
>  	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
>  	I915_WRITE(EDP_PSR_CTL, val);
>  }
> @@ -1032,16 +1037,19 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
>  		goto exit;
>  	}
>  
> -	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> -		DRM_DEBUG_KMS("PSR RFB storage error, exiting PSR\n");
> +	if (val & (DP_PSR_RFB_STORAGE_ERROR | DP_PSR_LINK_CRC_ERROR)) {
> +		if (val & DP_PSR_RFB_STORAGE_ERROR)

I believe we can avoid the duplication of the conditions here...
maybe with:
if (val & DP_PSR_RFB_STORAGE_ERROR)
//msg
else if (val & DP_PSR_LINK_CRC_ERROR)
//msg
else
goto clear

intel_psr_disable_locked()

> +			DRM_DEBUG_KMS("PSR RFB storage error, disabling PSR\n");
> +		if (val & DP_PSR_LINK_CRC_ERROR)
> +			DRM_DEBUG_KMS("PSR Link CRC error, disabling PSR\n");
>  		psr_disable(intel_dp);
>  	}
> -	if (val & (DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR | DP_PSR_LINK_CRC_ERROR))
> +	if (val & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
>  		DRM_ERROR("PSR_ERROR_STATUS not handled %x\n", val);

clear:
>  	/* clear status register */
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
>  
> -	/* TODO: handle other PSR/PSR2 errors */
> +	/* TODO: handle PSR2 errors */
>  exit:
>  	mutex_unlock(&psr->lock);
>  }
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/5] drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side

[Dhinakaran Pandiyan]

On Thu, 2018-06-14 at 14:19 -0700, Rodrigo Vivi wrote:
> On Thu, Jun 14, 2018 at 01:34:33PM -0700, José Roberto de Souza
> wrote:
> > 
> > Sink can be configured to calculate the CRC over the static frame
> > and
> > compare with the CRC calculated and transmited in the VSC SDP by
> > source, if there is a mismatch sink will do a short pulse in HPD
> > and set DP_PSR_LINK_CRC_ERROR in DP_PSR_ERROR_STATUS.
> > 
> > Spec: 7723
> > 
> > v4:
> > patch moved to after 'drm/i915/psr: Avoid PSR exit max time
> > timeout'
> > to avoid touch in 2 patches EDP_PSR_DEBUG.
> > 
> > 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/i915_reg.h  |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++++----
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 140f6a27d696..ed34ccd81c7c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4038,6 +4038,7 @@ enum {
> >  #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
> >  #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
> >  #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
> > +#define   EDP_PSR_CRC_ENABLE			(1<<10) /*
> > BDW+ */
> >  #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 177cd57b1029..cf72b79caf3f 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -360,6 +360,8 @@ static void hsw_psr_enable_sink(struct intel_dp
> > *intel_dp)
> >  
> >  	if (dev_priv->psr.link_standby)
> >  		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > +	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >=
> > 8)
> > +		dpcd_val |= DP_PSR_CRC_VERIFICATION;
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > dpcd_val);
> >  
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
> > @@ -415,6 +417,9 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	else
> >  		val |= EDP_PSR_TP1_TP2_SEL;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 8)
> > +		val |= EDP_PSR_CRC_ENABLE;
> > +
> >  	val |= I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> >  	I915_WRITE(EDP_PSR_CTL, val);
> >  }
> > @@ -1032,16 +1037,19 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp)
> >  		goto exit;
> >  	}
> >  
> > -	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> > -		DRM_DEBUG_KMS("PSR RFB storage error, exiting
> > PSR\n");
> > +	if (val & (DP_PSR_RFB_STORAGE_ERROR |
> > DP_PSR_LINK_CRC_ERROR)) {
> > +		if (val & DP_PSR_RFB_STORAGE_ERROR)
> I believe we can avoid the duplication of the conditions here...
> maybe with:
> if (val & DP_PSR_RFB_STORAGE_ERROR)
> //msg
> else if (val & DP_PSR_LINK_CRC_ERROR)
> //msg
> else
> goto clear
> 
> intel_psr_disable_locked()
> 
> > 
> > +			DRM_DEBUG_KMS("PSR RFB storage error,
> > disabling PSR\n");
> > +		if (val & DP_PSR_LINK_CRC_ERROR)
> > +			DRM_DEBUG_KMS("PSR Link CRC error,
> > disabling PSR\n");
> >  		psr_disable(intel_dp);
> >  	}
> > -	if (val & (DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> > DP_PSR_LINK_CRC_ERROR))
> > +	if (val & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> >  		DRM_ERROR("PSR_ERROR_STATUS not handled %x\n",
> > val);

Shouldn't PSR be disabled for this case too? Or is this going to be
done in another patch?

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

Re: [PATCH v4 5/5] drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side

[Souza, Jose]

On Thu, 2018-06-14 at 14:19 -0700, Rodrigo Vivi wrote:
> On Thu, Jun 14, 2018 at 01:34:33PM -0700, José Roberto de Souza
> wrote:
> > Sink can be configured to calculate the CRC over the static frame
> > and
> > compare with the CRC calculated and transmited in the VSC SDP by
> > source, if there is a mismatch sink will do a short pulse in HPD
> > and set DP_PSR_LINK_CRC_ERROR in DP_PSR_ERROR_STATUS.
> > 
> > Spec: 7723
> > 
> > v4:
> > patch moved to after 'drm/i915/psr: Avoid PSR exit max time
> > timeout'
> > to avoid touch in 2 patches EDP_PSR_DEBUG.
> > 
> > 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/i915_reg.h  |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++++----
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 140f6a27d696..ed34ccd81c7c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4038,6 +4038,7 @@ enum {
> >  #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
> >  #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
> >  #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
> > +#define   EDP_PSR_CRC_ENABLE			(1<<10) /*
> > BDW+ */
> >  #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 177cd57b1029..cf72b79caf3f 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -360,6 +360,8 @@ static void hsw_psr_enable_sink(struct intel_dp
> > *intel_dp)
> >  
> >  	if (dev_priv->psr.link_standby)
> >  		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > +	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >=
> > 8)
> > +		dpcd_val |= DP_PSR_CRC_VERIFICATION;
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > dpcd_val);
> >  
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
> > @@ -415,6 +417,9 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	else
> >  		val |= EDP_PSR_TP1_TP2_SEL;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 8)
> > +		val |= EDP_PSR_CRC_ENABLE;
> > +
> >  	val |= I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> >  	I915_WRITE(EDP_PSR_CTL, val);
> >  }
> > @@ -1032,16 +1037,19 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp)
> >  		goto exit;
> >  	}
> >  
> > -	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> > -		DRM_DEBUG_KMS("PSR RFB storage error, exiting
> > PSR\n");
> > +	if (val & (DP_PSR_RFB_STORAGE_ERROR |
> > DP_PSR_LINK_CRC_ERROR)) {
> > +		if (val & DP_PSR_RFB_STORAGE_ERROR)
> 
> I believe we can avoid the duplication of the conditions here...
> maybe with:
> if (val & DP_PSR_RFB_STORAGE_ERROR)
> //msg
> else if (val & DP_PSR_LINK_CRC_ERROR)
> //msg
> else
> goto clear

Well val can have all 3 errors set, so it would only print the first
debug message but I got your point, I will change to:

if (val & DP_PSR_RFB_STORAGE_ERROR)
//msg
if (val & DP_PSR_LINK_CRC_ERROR)
//msg
...

if (val)
	intel_psr_disable_locked()

as right all errors are unrecovered. This sounds good?


> 
> intel_psr_disable_locked()
> 
> > +			DRM_DEBUG_KMS("PSR RFB storage error,
> > disabling PSR\n");
> > +		if (val & DP_PSR_LINK_CRC_ERROR)
> > +			DRM_DEBUG_KMS("PSR Link CRC error,
> > disabling PSR\n");
> >  		psr_disable(intel_dp);
> >  	}
> > -	if (val & (DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> > DP_PSR_LINK_CRC_ERROR))
> > +	if (val & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> >  		DRM_ERROR("PSR_ERROR_STATUS not handled %x\n",
> > val);
> 
> clear:
> >  	/* clear status register */
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> > val);
> >  
> > -	/* TODO: handle other PSR/PSR2 errors */
> > +	/* TODO: handle PSR2 errors */
> >  exit:
> >  	mutex_unlock(&psr->lock);
> >  }
> > -- 
> > 2.17.1
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/5] drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side

[Rodrigo Vivi]

On Thu, Jun 14, 2018 at 05:02:41PM -0700, Souza, Jose wrote:
> On Thu, 2018-06-14 at 14:19 -0700, Rodrigo Vivi wrote:
> > On Thu, Jun 14, 2018 at 01:34:33PM -0700, José Roberto de Souza
> > wrote:
> > > Sink can be configured to calculate the CRC over the static frame
> > > and
> > > compare with the CRC calculated and transmited in the VSC SDP by
> > > source, if there is a mismatch sink will do a short pulse in HPD
> > > and set DP_PSR_LINK_CRC_ERROR in DP_PSR_ERROR_STATUS.
> > > 
> > > Spec: 7723
> > > 
> > > v4:
> > > patch moved to after 'drm/i915/psr: Avoid PSR exit max time
> > > timeout'
> > > to avoid touch in 2 patches EDP_PSR_DEBUG.
> > > 
> > > 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/i915_reg.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++++----
> > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 140f6a27d696..ed34ccd81c7c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4038,6 +4038,7 @@ enum {
> > >  #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
> > >  #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
> > >  #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
> > > +#define   EDP_PSR_CRC_ENABLE			(1<<10) /*
> > > BDW+ */
> > >  #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
> > >  #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
> > >  #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 177cd57b1029..cf72b79caf3f 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -360,6 +360,8 @@ static void hsw_psr_enable_sink(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	if (dev_priv->psr.link_standby)
> > >  		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > > +	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >=
> > > 8)
> > > +		dpcd_val |= DP_PSR_CRC_VERIFICATION;
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > > dpcd_val);
> > >  
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> > > @@ -415,6 +417,9 @@ static void hsw_activate_psr1(struct intel_dp
> > > *intel_dp)
> > >  	else
> > >  		val |= EDP_PSR_TP1_TP2_SEL;
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 8)
> > > +		val |= EDP_PSR_CRC_ENABLE;
> > > +
> > >  	val |= I915_READ(EDP_PSR_CTL) &
> > > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> > >  	I915_WRITE(EDP_PSR_CTL, val);
> > >  }
> > > @@ -1032,16 +1037,19 @@ void intel_psr_short_pulse(struct intel_dp
> > > *intel_dp)
> > >  		goto exit;
> > >  	}
> > >  
> > > -	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> > > -		DRM_DEBUG_KMS("PSR RFB storage error, exiting
> > > PSR\n");
> > > +	if (val & (DP_PSR_RFB_STORAGE_ERROR |
> > > DP_PSR_LINK_CRC_ERROR)) {
> > > +		if (val & DP_PSR_RFB_STORAGE_ERROR)
> > 
> > I believe we can avoid the duplication of the conditions here...
> > maybe with:
> > if (val & DP_PSR_RFB_STORAGE_ERROR)
> > //msg
> > else if (val & DP_PSR_LINK_CRC_ERROR)
> > //msg
> > else
> > goto clear
> 
> Well val can have all 3 errors set, so it would only print the first
> debug message but I got your point, I will change to:
> 
> if (val & DP_PSR_RFB_STORAGE_ERROR)
> //msg
> if (val & DP_PSR_LINK_CRC_ERROR)
> //msg
> ...
> 
> if (val)
> 	intel_psr_disable_locked()
> 
> as right all errors are unrecovered. This sounds good?

cool, better than my goto idea ;)

> 
> 
> > 
> > intel_psr_disable_locked()
> > 
> > > +			DRM_DEBUG_KMS("PSR RFB storage error,
> > > disabling PSR\n");
> > > +		if (val & DP_PSR_LINK_CRC_ERROR)
> > > +			DRM_DEBUG_KMS("PSR Link CRC error,
> > > disabling PSR\n");
> > >  		psr_disable(intel_dp);
> > >  	}
> > > -	if (val & (DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> > > DP_PSR_LINK_CRC_ERROR))
> > > +	if (val & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> > >  		DRM_ERROR("PSR_ERROR_STATUS not handled %x\n",
> > > val);
> > 
> > clear:
> > >  	/* clear status register */
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> > > val);
> > >  
> > > -	/* TODO: handle other PSR/PSR2 errors */
> > > +	/* TODO: handle PSR2 errors */
> > >  exit:
> > >  	mutex_unlock(&psr->lock);
> > >  }
> > > -- 
> > > 2.17.1
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()
URL   : https://patchwork.freedesktop.org/series/44780/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2068bdba7b19 drm/i915/psr: Remove intel_crtc_state parameter from disable()
09e7f49b31bd drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
3030895017f9 drm/i915/psr: Handle PSR RFB storage error
e76008d83e28 drm/i915/psr: Avoid PSR exit max time timeout
f29a0fb19046 drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
-:36: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#36: FILE: drivers/gpu/drm/i915/i915_reg.h:4041:
+#define   EDP_PSR_CRC_ENABLE			(1<<10) /* BDW+ */
                             			  ^

total: 0 errors, 0 warnings, 1 checks, 47 lines checked

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

✗ Fi.CI.SPARSE: warning for series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()
URL   : https://patchwork.freedesktop.org/series/44780/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/psr: Remove intel_crtc_state parameter from disable()
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3683:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3682:16: warning: expression using sizeof(void)

Commit: drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
Okay!

Commit: drm/i915/psr: Handle PSR RFB storage error
Okay!

Commit: drm/i915/psr: Avoid PSR exit max time timeout
Okay!

Commit: drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
Okay!

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

✓ Fi.CI.BAT: success for series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()
URL   : https://patchwork.freedesktop.org/series/44780/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4321 -> Patchwork_9311 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44780/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9311 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-no-display:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725)

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000, fdo#106097)

    igt@pm_rpm@basic-rte:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload-inject:
      fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-skl-6700k2:      FAIL (fdo#103191, fdo#104724) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744


== Participating hosts (41 -> 38) ==

  Additional (1): fi-byt-j1900 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4321 -> Patchwork_9311

  CI_DRM_4321: 505d7dfc3c02d3efdb74aca0dbffd86d8b761a49 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4519: 3381a56be31defb3b5c23a4fbc19ac26a000c35b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9311: f29a0fb19046d895bcbd95fa165985af71e3b0b2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f29a0fb19046 drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
e76008d83e28 drm/i915/psr: Avoid PSR exit max time timeout
3030895017f9 drm/i915/psr: Handle PSR RFB storage error
09e7f49b31bd drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
2068bdba7b19 drm/i915/psr: Remove intel_crtc_state parameter from disable()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9311/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/5] drm/i915/psr: Remove intel_crtc_state parameter from disable()
URL   : https://patchwork.freedesktop.org/series/44780/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4321_full -> Patchwork_9311_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9311_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-glk:          PASS -> FAIL (fdo#105347)

    igt@drv_suspend@shrink:
      shard-hsw:          PASS -> INCOMPLETE (fdo#103540)
      shard-glk:          PASS -> FAIL (fdo#106886)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355)

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#103060)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#103822, fdo#104724) +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render:
      shard-snb:          PASS -> FAIL (fdo#103167, fdo#104724)

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-hsw:          PASS -> FAIL (fdo#103925, fdo#104724)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@drv_selftest@live_hangcheck:
      shard-glk:          DMESG-FAIL (fdo#106560) -> PASS

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          FAIL (fdo#105703) -> PASS

    igt@kms_flip@2x-plain-flip-ts-check-interruptible:
      shard-hsw:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip@flip-vs-blocking-wf-vblank:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@wf_vblank-ts-check:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4321 -> Patchwork_9311

  CI_DRM_4321: 505d7dfc3c02d3efdb74aca0dbffd86d8b761a49 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4519: 3381a56be31defb3b5c23a4fbc19ac26a000c35b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9311: f29a0fb19046d895bcbd95fa165985af71e3b0b2 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9311/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx