[PATCH v2 1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[PATCH v2 1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[José Roberto de Souza]

It was reading some random register in VLV and CHV.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---

No changes from v1.

 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 69a5b276f4d8..659180656f5b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -424,6 +424,11 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
+	if (dev_priv->psr.psr2_enabled)
+		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
+	else
+		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+
 	/* On HSW+ after we enable PSR on source it will activate it
 	 * as soon as it match configure idle_frame count. So
 	 * we just actually enable it here on activation time.
@@ -549,10 +554,6 @@ static void intel_psr_activate(struct intel_dp *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.psr2_enabled)
-		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-	else
-		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
-- 
2.17.0

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

[PATCH v2 2/9] drm/i915/psr: Move PSR exit specific code to hardware specific function

[José Roberto de Souza]

To proper execute PSR exit it was using 'if (HAS_DDI(dev_priv))' to
differentiate between VLV/CHV and HSW+ hardware, so here moving each
hardware handling to his own function.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---

No changes from v1.

 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 94 +++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8e8667d9b084..476bca872d48 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -617,6 +617,7 @@ struct i915_psr {
 	void (*enable_sink)(struct intel_dp *);
 	void (*activate)(struct intel_dp *);
 	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
+	void (*exit)(struct intel_dp *intel_dp);
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 659180656f5b..ebc47e2b08ca 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -844,53 +844,67 @@ static void intel_psr_work(struct work_struct *work)
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-static void intel_psr_exit(struct drm_i915_private *dev_priv)
+static void hsw_psr_exit(struct intel_dp *intel_dp)
 {
-	struct intel_dp *intel_dp = dev_priv->psr.enabled;
-	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	u32 val;
+
+	if (dev_priv->psr.psr2_enabled) {
+		val = I915_READ(EDP_PSR2_CTL);
+		WARN_ON(!(val & EDP_PSR2_ENABLE));
+		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
+	} else {
+		val = I915_READ(EDP_PSR_CTL);
+		WARN_ON(!(val & EDP_PSR_ENABLE));
+		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
+	}
+}
+
+static void vlv_psr_exit(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc *crtc = dig_port->base.base.crtc;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 	u32 val;
 
-	if (!dev_priv->psr.active)
-		return;
+	val = I915_READ(VLV_PSRCTL(pipe));
 
-	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_enabled) {
-			val = I915_READ(EDP_PSR2_CTL);
-			WARN_ON(!(val & EDP_PSR2_ENABLE));
-			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
-		} else {
-			val = I915_READ(EDP_PSR_CTL);
-			WARN_ON(!(val & EDP_PSR_ENABLE));
-			I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
-		}
-	} else {
-		val = I915_READ(VLV_PSRCTL(pipe));
+	/*
+	 * Here we do the transition drirectly from
+	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update) to
+	 * PSR_state 5 (exit).
+	 * PSR State 4 (active with single frame update) can be skipped.
+	 * On PSR_state 5 (exit) Hardware is responsible to transition
+	 * back to PSR_state 1 (inactive).
+	 * Now we are at Same state after vlv_psr_enable_source.
+	 */
+	val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
+	I915_WRITE(VLV_PSRCTL(pipe), val);
 
-		/*
-		 * Here we do the transition drirectly from
-		 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update) to
-		 * PSR_state 5 (exit).
-		 * PSR State 4 (active with single frame update) can be skipped.
-		 * On PSR_state 5 (exit) Hardware is responsible to transition
-		 * back to PSR_state 1 (inactive).
-		 * Now we are at Same state after vlv_psr_enable_source.
-		 */
-		val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
-		I915_WRITE(VLV_PSRCTL(pipe), val);
+	/*
+	 * Send AUX wake up - Spec says after transitioning to PSR
+	 * active we have to send AUX wake up by writing 01h in DPCD
+	 * 600h of sink device.
+	 * XXX: This might slow down the transition, but without this
+	 * HW doesn't complete the transition to PSR_state 1 and we
+	 * never get the screen updated.
+	 */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
+			   DP_SET_POWER_D0);
+}
 
-		/*
-		 * Send AUX wake up - Spec says after transitioning to PSR
-		 * active we have to send AUX wake up by writing 01h in DPCD
-		 * 600h of sink device.
-		 * XXX: This might slow down the transition, but without this
-		 * HW doesn't complete the transition to PSR_state 1 and we
-		 * never get the screen updated.
-		 */
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
-				   DP_SET_POWER_D0);
-	}
+static void intel_psr_exit(struct drm_i915_private *dev_priv)
+{
+	struct intel_dp *intel_dp = dev_priv->psr.enabled;
+
+	if (!dev_priv->psr.active)
+		return;
 
+	dev_priv->psr.exit(intel_dp);
 	dev_priv->psr.active = false;
 }
 
@@ -1100,6 +1114,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.enable_sink = vlv_psr_enable_sink;
 		dev_priv->psr.activate = vlv_psr_activate;
 		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
+		dev_priv->psr.exit = vlv_psr_exit;
 	} else {
 		dev_priv->psr.has_hw_tracking = true;
 		dev_priv->psr.enable_source = hsw_psr_enable_source;
@@ -1107,5 +1122,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
 		dev_priv->psr.activate = hsw_psr_activate;
 		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
+		dev_priv->psr.exit = hsw_psr_exit;
 	}
 }
-- 
2.17.0

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

Re: [PATCH v2 2/9] drm/i915/psr: Move PSR exit specific code to hardware specific function

[Dhinakaran Pandiyan]

On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> To proper execute PSR exit it was using 'if (HAS_DDI(dev_priv))' to
> differentiate between VLV/CHV and HSW+ hardware, so here moving each
> hardware handling to his own function.

When https://patchwork.freedesktop.org/patch/222230/ lands, we don't
have to worry about this :)
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 
> No changes from v1.
> 
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 94 +++++++++++++++++++-----------
> --
>  2 files changed, 56 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 8e8667d9b084..476bca872d48 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -617,6 +617,7 @@ struct i915_psr {
>  	void (*enable_sink)(struct intel_dp *);
>  	void (*activate)(struct intel_dp *);
>  	void (*setup_vsc)(struct intel_dp *, const struct
> intel_crtc_state *);
> +	void (*exit)(struct intel_dp *intel_dp);
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 659180656f5b..ebc47e2b08ca 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -844,53 +844,67 @@ static void intel_psr_work(struct work_struct
> *work)
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> -static void intel_psr_exit(struct drm_i915_private *dev_priv)
> +static void hsw_psr_exit(struct intel_dp *intel_dp)
>  {
> -	struct intel_dp *intel_dp = dev_priv->psr.enabled;
> -	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)-
> >base.base.crtc;
> +	struct intel_digital_port *dig_port =
> dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	u32 val;
> +
> +	if (dev_priv->psr.psr2_enabled) {
> +		val = I915_READ(EDP_PSR2_CTL);
> +		WARN_ON(!(val & EDP_PSR2_ENABLE));
> +		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> +	} else {
> +		val = I915_READ(EDP_PSR_CTL);
> +		WARN_ON(!(val & EDP_PSR_ENABLE));
> +		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> +	}
> +}
> +
> +static void vlv_psr_exit(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port =
> dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_crtc *crtc = dig_port->base.base.crtc;
>  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  	u32 val;
>  
> -	if (!dev_priv->psr.active)
> -		return;
> +	val = I915_READ(VLV_PSRCTL(pipe));
>  
> -	if (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_enabled) {
> -			val = I915_READ(EDP_PSR2_CTL);
> -			WARN_ON(!(val & EDP_PSR2_ENABLE));
> -			I915_WRITE(EDP_PSR2_CTL, val &
> ~EDP_PSR2_ENABLE);
> -		} else {
> -			val = I915_READ(EDP_PSR_CTL);
> -			WARN_ON(!(val & EDP_PSR_ENABLE));
> -			I915_WRITE(EDP_PSR_CTL, val &
> ~EDP_PSR_ENABLE);
> -		}
> -	} else {
> -		val = I915_READ(VLV_PSRCTL(pipe));
> +	/*
> +	 * Here we do the transition drirectly from
> +	 * PSR_state 3 (active - no Remote Frame Buffer (RFB)
> update) to
> +	 * PSR_state 5 (exit).
> +	 * PSR State 4 (active with single frame update) can be
> skipped.
> +	 * On PSR_state 5 (exit) Hardware is responsible to
> transition
> +	 * back to PSR_state 1 (inactive).
> +	 * Now we are at Same state after vlv_psr_enable_source.
> +	 */
> +	val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> +	I915_WRITE(VLV_PSRCTL(pipe), val);
>  
> -		/*
> -		 * Here we do the transition drirectly from
> -		 * PSR_state 3 (active - no Remote Frame Buffer
> (RFB) update) to
> -		 * PSR_state 5 (exit).
> -		 * PSR State 4 (active with single frame update) can
> be skipped.
> -		 * On PSR_state 5 (exit) Hardware is responsible to
> transition
> -		 * back to PSR_state 1 (inactive).
> -		 * Now we are at Same state after
> vlv_psr_enable_source.
> -		 */
> -		val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> -		I915_WRITE(VLV_PSRCTL(pipe), val);
> +	/*
> +	 * Send AUX wake up - Spec says after transitioning to PSR
> +	 * active we have to send AUX wake up by writing 01h in DPCD
> +	 * 600h of sink device.
> +	 * XXX: This might slow down the transition, but without
> this
> +	 * HW doesn't complete the transition to PSR_state 1 and we
> +	 * never get the screen updated.
> +	 */
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> +			   DP_SET_POWER_D0);
> +}
>  
> -		/*
> -		 * Send AUX wake up - Spec says after transitioning
> to PSR
> -		 * active we have to send AUX wake up by writing 01h
> in DPCD
> -		 * 600h of sink device.
> -		 * XXX: This might slow down the transition, but
> without this
> -		 * HW doesn't complete the transition to PSR_state 1
> and we
> -		 * never get the screen updated.
> -		 */
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> -				   DP_SET_POWER_D0);
> -	}
> +static void intel_psr_exit(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_dp *intel_dp = dev_priv->psr.enabled;
> +
> +	if (!dev_priv->psr.active)
> +		return;
>  
> +	dev_priv->psr.exit(intel_dp);
>  	dev_priv->psr.active = false;
>  }
>  
> @@ -1100,6 +1114,7 @@ void intel_psr_init(struct drm_i915_private
> *dev_priv)
>  		dev_priv->psr.enable_sink = vlv_psr_enable_sink;
>  		dev_priv->psr.activate = vlv_psr_activate;
>  		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
> +		dev_priv->psr.exit = vlv_psr_exit;
>  	} else {
>  		dev_priv->psr.has_hw_tracking = true;
>  		dev_priv->psr.enable_source = hsw_psr_enable_source;
> @@ -1107,5 +1122,6 @@ void intel_psr_init(struct drm_i915_private
> *dev_priv)
>  		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
>  		dev_priv->psr.activate = hsw_psr_activate;
>  		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
> +		dev_priv->psr.exit = hsw_psr_exit;
>  	}
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/9] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[José Roberto de Souza]

It is only used by VLV/CHV and we can get this information from
intel_dp for those platforms.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---

Changes from v1:
- not using legacy drm_crtc pointer(struct drm_crtc *crtc = intel_dig_port->base.base.crtc)

 drivers/gpu/drm/i915/i915_drv.h  |  3 +--
 drivers/gpu/drm/i915/intel_psr.c | 17 +++++++----------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 476bca872d48..f5ffb3d72cef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -612,8 +612,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 ebc47e2b08ca..934498505356 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -665,38 +665,35 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-static void vlv_psr_disable(struct intel_dp *intel_dp,
-			    const struct intel_crtc_state *old_crtc_state)
+static void vlv_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);
-	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	uint32_t val;
 
 	if (dev_priv->psr.active) {
 		/* Put VLV PSR back to PSR_state 0 (disabled). */
 		if (intel_wait_for_register(dev_priv,
-					    VLV_PSRSTAT(crtc->pipe),
+					    VLV_PSRSTAT(intel_dp->active_pipe),
 					    VLV_EDP_PSR_IN_TRANS,
 					    0,
 					    1))
 			WARN(1, "PSR transition took longer than expected\n");
 
-		val = I915_READ(VLV_PSRCTL(crtc->pipe));
+		val = I915_READ(VLV_PSRCTL(intel_dp->active_pipe));
 		val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
 		val &= ~VLV_EDP_PSR_ENABLE;
 		val &= ~VLV_EDP_PSR_MODE_MASK;
-		I915_WRITE(VLV_PSRCTL(crtc->pipe), val);
+		I915_WRITE(VLV_PSRCTL(intel_dp->active_pipe), val);
 
 		dev_priv->psr.active = false;
 	} else {
-		WARN_ON(vlv_is_psr_active_on_pipe(dev, crtc->pipe));
+		WARN_ON(vlv_is_psr_active_on_pipe(dev, intel_dp->active_pipe));
 	}
 }
 
-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;
@@ -765,7 +762,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.0

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

Re: [PATCH v2 3/9] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Ville Syrjälä]

On Wed, Apr 18, 2018 at 03:43:05PM -0700, José Roberto de Souza wrote:
> It is only used by VLV/CHV and we can get this information from
> intel_dp for those platforms.

But why? Abusing active_pipe (which is there just for the pps
tracking) is not a good idea IMO.

> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> 
> Changes from v1:
> - not using legacy drm_crtc pointer(struct drm_crtc *crtc = intel_dig_port->base.base.crtc)
> 
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
>  drivers/gpu/drm/i915/intel_psr.c | 17 +++++++----------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 476bca872d48..f5ffb3d72cef 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -612,8 +612,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 ebc47e2b08ca..934498505356 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -665,38 +665,35 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> -static void vlv_psr_disable(struct intel_dp *intel_dp,
> -			    const struct intel_crtc_state *old_crtc_state)
> +static void vlv_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);
> -	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>  	uint32_t val;
>  
>  	if (dev_priv->psr.active) {
>  		/* Put VLV PSR back to PSR_state 0 (disabled). */
>  		if (intel_wait_for_register(dev_priv,
> -					    VLV_PSRSTAT(crtc->pipe),
> +					    VLV_PSRSTAT(intel_dp->active_pipe),
>  					    VLV_EDP_PSR_IN_TRANS,
>  					    0,
>  					    1))
>  			WARN(1, "PSR transition took longer than expected\n");
>  
> -		val = I915_READ(VLV_PSRCTL(crtc->pipe));
> +		val = I915_READ(VLV_PSRCTL(intel_dp->active_pipe));
>  		val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
>  		val &= ~VLV_EDP_PSR_ENABLE;
>  		val &= ~VLV_EDP_PSR_MODE_MASK;
> -		I915_WRITE(VLV_PSRCTL(crtc->pipe), val);
> +		I915_WRITE(VLV_PSRCTL(intel_dp->active_pipe), val);
>  
>  		dev_priv->psr.active = false;
>  	} else {
> -		WARN_ON(vlv_is_psr_active_on_pipe(dev, crtc->pipe));
> +		WARN_ON(vlv_is_psr_active_on_pipe(dev, intel_dp->active_pipe));
>  	}
>  }
>  
> -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;
> @@ -765,7 +762,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.0

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

[PATCH v2 4/9] 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.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---

Changes from v1:
- printing a debug message when sink assert a error

 drivers/gpu/drm/i915/intel_dp.c  |  2 ++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 48 +++++++++++++++++++++++++++++---
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..701963a192ee 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 	if (intel_dp_needs_link_retrain(intel_dp))
 		return false;
 
+	intel_psr_hpd_short_pulse_handle(intel_dp);
+
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
 		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
 		/* Send a Hotplug Uevent to userspace to start modeset */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5bd2263407b2..b79e15ecd052 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
 				   unsigned frontbuffer_bits);
 void intel_psr_compute_config(struct intel_dp *intel_dp,
 			      struct intel_crtc_state *crtc_state);
+void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 934498505356..4cb27faab707 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -996,6 +996,15 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
+static void intel_psr_schedule_activate_work(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->psr.active || dev_priv->psr.busy_frontbuffer_bits
+	    || work_busy(&dev_priv->psr.work.work))
+		return;
+
+	schedule_delayed_work(&dev_priv->psr.work, msecs_to_jiffies(100));
+}
+
 /**
  * intel_psr_flush - Flush PSR
  * @dev_priv: i915 device
@@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 		}
 	}
 
-	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		if (!work_busy(&dev_priv->psr.work.work))
-			schedule_delayed_work(&dev_priv->psr.work,
-					      msecs_to_jiffies(100));
+	intel_psr_schedule_activate_work(dev_priv);
+
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.exit = hsw_psr_exit;
 	}
 }
+
+void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct i915_psr *psr = &dev_priv->psr;
+	uint8_t val;
+
+	if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
+		return;
+
+	mutex_lock(&psr->lock);
+
+	if (psr->enabled != intel_dp)
+		goto not_enabled;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) {
+		DRM_DEBUG_KMS("PSR_STATUS read failed\n");
+		goto dpcd_error;
+	}
+
+	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) {
+		DRM_DEBUG_KMS("PSR sink internal error, exiting PSR\n");
+		intel_psr_exit(dev_priv);
+	}
+
+	/* TODO: handle other PSR/PSR2 errors */
+dpcd_error:
+	intel_psr_schedule_activate_work(dev_priv);
+not_enabled:
+	mutex_unlock(&psr->lock);
+}
-- 
2.17.0

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

Re: [PATCH v2 4/9] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Dhinakaran Pandiyan]

On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> eDP spec states that sink device will do a short pulse in HPD
> line when there is a PSR/PSR2 error that needs to be handled by
> source, this is handling the first and most simples error:
> DP_PSR_SINK_INTERNAL_ERROR.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 
> Changes from v1:
> - printing a debug message when sink assert a error
> 
>  drivers/gpu/drm/i915/intel_dp.c  |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 48 +++++++++++++++++++++++++++++---
>  3 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..701963a192ee 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  	if (intel_dp_needs_link_retrain(intel_dp))
>  		return false;
>  
> +	intel_psr_hpd_short_pulse_handle(intel_dp);
	 intel_psr_short_pulse() should be sufficient.

> +
>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>  		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
>  		/* Send a Hotplug Uevent to userspace to start modeset */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5bd2263407b2..b79e15ecd052 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
>  				   unsigned frontbuffer_bits);
>  void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state);
> +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 934498505356..4cb27faab707 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -996,6 +996,15 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> +static void intel_psr_schedule_activate_work(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->psr.active || dev_priv->psr.busy_frontbuffer_bits
> +	    || work_busy(&dev_priv->psr.work.work))
> +		return;
> +
> +	schedule_delayed_work(&dev_priv->psr.work, msecs_to_jiffies(100));
> +}
> +
>  /**
>   * intel_psr_flush - Flush PSR
>   * @dev_priv: i915 device
> @@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  		}
>  	}
>  
> -	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> -		if (!work_busy(&dev_priv->psr.work.work))
> -			schedule_delayed_work(&dev_priv->psr.work,
> -					      msecs_to_jiffies(100));
> +	intel_psr_schedule_activate_work(dev_priv);
> +
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  		dev_priv->psr.exit = hsw_psr_exit;
>  	}
>  }
> +
> +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct i915_psr *psr = &dev_priv->psr;
> +	uint8_t val;
> +
> +	if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> +		return;
> +
> +	mutex_lock(&psr->lock);
> +
> +	if (psr->enabled != intel_dp)
> +		goto not_enabled;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) {
> +		DRM_DEBUG_KMS("PSR_STATUS read failed\n");

Since we can't handle the irq without reading the dpcd and the irq is
potentially an error condition, I think this should be DRM_ERROR.


> +		goto dpcd_error;
> +	}
> +
> +	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) {
> +		DRM_DEBUG_KMS("PSR sink internal error, exiting PSR\n");
> +		intel_psr_exit(dev_priv);

Shouldn't this be disabling PSR? Exit will allow for re-activation
immediately. An unknown error in the sink IMO should disable PSR for
good.

> +	}
> +
> +	/* TODO: handle other PSR/PSR2 errors */
> +dpcd_error:
> +	intel_psr_schedule_activate_work(dev_priv);

Why? There is no intel_psr_exit() before the goto.


> +not_enabled:
> +	mutex_unlock(&psr->lock);
> +}

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

Re: [PATCH v2 4/9] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Souza, Jose]

On Thu, 2018-04-26 at 15:29 -0700, Dhinakaran Pandiyan wrote:
> 
> 
> On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> > eDP spec states that sink device will do a short pulse in HPD
> > line when there is a PSR/PSR2 error that needs to be handled by
> > source, this is handling the first and most simples error:
> > DP_PSR_SINK_INTERNAL_ERROR.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > 
> > Changes from v1:
> > - printing a debug message when sink assert a error
> > 
> >  drivers/gpu/drm/i915/intel_dp.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 48
> > +++++++++++++++++++++++++++++---
> >  3 files changed, 47 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4298ac..701963a192ee 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4462,6 +4462,8 @@ intel_dp_short_pulse(struct intel_dp
> > *intel_dp)
> >  	if (intel_dp_needs_link_retrain(intel_dp))
> >  		return false;
> >  
> > +	intel_psr_hpd_short_pulse_handle(intel_dp);
> 
> 	 intel_psr_short_pulse() should be sufficient.

done

> 
> > +
> >  	if (intel_dp->compliance.test_type ==
> > DP_TEST_LINK_TRAINING) {
> >  		DRM_DEBUG_KMS("Link Training Compliance Test
> > requested\n");
> >  		/* Send a Hotplug Uevent to userspace to start
> > modeset */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 5bd2263407b2..b79e15ecd052 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1901,6 +1901,7 @@ void intel_psr_single_frame_update(struct
> > drm_i915_private *dev_priv,
> >  				   unsigned frontbuffer_bits);
> >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  			      struct intel_crtc_state
> > *crtc_state);
> > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 934498505356..4cb27faab707 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -996,6 +996,15 @@ void intel_psr_invalidate(struct
> > drm_i915_private *dev_priv,
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > +static void intel_psr_schedule_activate_work(struct
> > drm_i915_private *dev_priv)
> > +{
> > +	if (dev_priv->psr.active || dev_priv-
> > >psr.busy_frontbuffer_bits
> > +	    || work_busy(&dev_priv->psr.work.work))
> > +		return;
> > +
> > +	schedule_delayed_work(&dev_priv->psr.work,
> > msecs_to_jiffies(100));
> > +}
> > +
> >  /**
> >   * intel_psr_flush - Flush PSR
> >   * @dev_priv: i915 device
> > @@ -1052,10 +1061,8 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> >  		}
> >  	}
> >  
> > -	if (!dev_priv->psr.active && !dev_priv-
> > >psr.busy_frontbuffer_bits)
> > -		if (!work_busy(&dev_priv->psr.work.work))
> > -			schedule_delayed_work(&dev_priv->psr.work,
> > -					      msecs_to_jiffies(100
> > ));
> > +	intel_psr_schedule_activate_work(dev_priv);
> > +
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > @@ -1122,3 +1129,36 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> >  		dev_priv->psr.exit = hsw_psr_exit;
> >  	}
> >  }
> > +
> > +void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct i915_psr *psr = &dev_priv->psr;
> > +	uint8_t val;
> > +
> > +	if (!HAS_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> > +		return;
> > +
> > +	mutex_lock(&psr->lock);
> > +
> > +	if (psr->enabled != intel_dp)
> > +		goto not_enabled;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val)
> > != 1) {
> > +		DRM_DEBUG_KMS("PSR_STATUS read failed\n");
> 
> Since we can't handle the irq without reading the dpcd and the irq is
> potentially an error condition, I think this should be DRM_ERROR.

Done

> 
> 
> > +		goto dpcd_error;
> > +	}
> > +
> > +	if ((val & DP_PSR_SINK_STATE_MASK) ==
> > DP_PSR_SINK_INTERNAL_ERROR) {
> > +		DRM_DEBUG_KMS("PSR sink internal error, exiting
> > PSR\n");
> > +		intel_psr_exit(dev_priv);
> 
> Shouldn't this be disabling PSR? Exit will allow for re-activation
> immediately. An unknown error in the sink IMO should disable PSR for
> good.


I was not able to reproduce any of the PSR errors even once, I guess
only exit and enter PSR should be enough to sink fix the error but I
guess you are right, better be safe and avoid any rendering problems.

> 
> > +	}
> > +
> > +	/* TODO: handle other PSR/PSR2 errors */
> > +dpcd_error:
> > +	intel_psr_schedule_activate_work(dev_priv);
> 
> Why? There is no intel_psr_exit() before the goto.

yep, it can be removed. It was a left over from the series with the
patches to exit psr on aux ch transfer.
Also now disabling PSR it will not be need.

> 
> 
> > +not_enabled:
> > +	mutex_unlock(&psr->lock);
> > +}
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 5/9] 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.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---

Changes from v1:
- printing a debug message when sink assert a error

 drivers/gpu/drm/i915/intel_psr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4cb27faab707..558b08a43f9e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1156,6 +1156,18 @@ void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
 		intel_psr_exit(dev_priv);
 	}
 
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS, &val) != 1) {
+		DRM_DEBUG_KMS("PSR_ERROR_STATUS read failed\n");
+		goto dpcd_error;
+	}
+
+	if (val & DP_PSR_RFB_STORAGE_ERROR) {
+		DRM_DEBUG_KMS("PSR RFB storage error, exiting PSR\n");
+		intel_psr_exit(dev_priv);
+	}
+	/* clear status register */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
+
 	/* TODO: handle other PSR/PSR2 errors */
 dpcd_error:
 	intel_psr_schedule_activate_work(dev_priv);
-- 
2.17.0

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

Re: [PATCH v2 5/9] drm/i915/psr: Handle PSR RFB storage error

[Dhinakaran Pandiyan]

On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> Sink will interrupt source when it have any problem saving or reading
> the remote frame buffer.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 
> Changes from v1:
> - printing a debug message when sink assert a error
> 
>  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 4cb27faab707..558b08a43f9e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1156,6 +1156,18 @@ void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
>  		intel_psr_exit(dev_priv);
>  	}
>  
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS, &val) != 1) {
> +		DRM_DEBUG_KMS("PSR_ERROR_STATUS read failed\n");
> +		goto dpcd_error;
> +	}
> +
> +	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> +		DRM_DEBUG_KMS("PSR RFB storage error, exiting PSR\n");
> +		intel_psr_exit(dev_priv);

What do we achieve with an exit? Resetting PSR? I don't think that's
enough if the sink has storage errors. I think we should just disable
PSR here too.

> +	}
> +	/* clear status register */
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);

So the other two errors are not handled, silently clearing them isn't
right. How about at least printing a debug with the read value and
saying the error wasn't handled?

> +
>  	/* TODO: handle other PSR/PSR2 errors */
>  dpcd_error:
>  	intel_psr_schedule_activate_work(dev_priv);

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

Re: [PATCH v2 5/9] drm/i915/psr: Handle PSR RFB storage error

[Souza, Jose]

On Thu, 2018-04-26 at 15:37 -0700, Dhinakaran Pandiyan wrote:
> 
> 
> On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> > Sink will interrupt source when it have any problem saving or
> > reading
> > the remote frame buffer.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > 
> > Changes from v1:
> > - printing a debug message when sink assert a error
> > 
> >  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 4cb27faab707..558b08a43f9e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -1156,6 +1156,18 @@ void intel_psr_hpd_short_pulse_handle(struct
> > intel_dp *intel_dp)
> >  		intel_psr_exit(dev_priv);
> >  	}
> >  
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> > &val) != 1) {
> > +		DRM_DEBUG_KMS("PSR_ERROR_STATUS read failed\n");
> > +		goto dpcd_error;
> > +	}
> > +
> > +	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> > +		DRM_DEBUG_KMS("PSR RFB storage error, exiting
> > PSR\n");
> > +		intel_psr_exit(dev_priv);
> 
> What do we achieve with an exit? Resetting PSR? I don't think that's
> enough if the sink has storage errors. I think we should just disable
> PSR here too.

Disabling now.

> 
> > +	}
> > +	/* clear status register */
> > +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> > val);
> 
> So the other two errors are not handled, silently clearing them isn't
> right. How about at least printing a debug with the read value and
> saying the error wasn't handled?

DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR is only applicable for PSR2 and to
sink report DP_PSR_LINK_CRC_ERROR a bit needs to be set in
DP_PSR_EN_CFG(both done in the next patch).

Do you still think that would be nice to print?

> 
> > +
> >  	/* TODO: handle other PSR/PSR2 errors */
> >  dpcd_error:
> >  	intel_psr_schedule_activate_work(dev_priv);
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/9] drm/i915/psr: Handle PSR RFB storage error

[Dhinakaran Pandiyan]

On Mon, 2018-04-30 at 23:28 +0000, Souza, Jose wrote:
> On Thu, 2018-04-26 at 15:37 -0700, Dhinakaran Pandiyan wrote:
> > 
> > 
> > 
> > On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> > > 
> > > Sink will interrupt source when it have any problem saving or
> > > reading
> > > the remote frame buffer.
> > > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > > 
> > > Changes from v1:
> > > - printing a debug message when sink assert a error
> > > 
> > >  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 4cb27faab707..558b08a43f9e 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -1156,6 +1156,18 @@ void
> > > intel_psr_hpd_short_pulse_handle(struct
> > > intel_dp *intel_dp)
> > >  		intel_psr_exit(dev_priv);
> > >  	}
> > >  
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > DP_PSR_ERROR_STATUS,
> > > &val) != 1) {
> > > +		DRM_DEBUG_KMS("PSR_ERROR_STATUS read failed\n");
> > > +		goto dpcd_error;
> > > +	}
> > > +
> > > +	if (val & DP_PSR_RFB_STORAGE_ERROR) {
> > > +		DRM_DEBUG_KMS("PSR RFB storage error, exiting
> > > PSR\n");
> > > +		intel_psr_exit(dev_priv);
> > What do we achieve with an exit? Resetting PSR? I don't think
> > that's
> > enough if the sink has storage errors. I think we should just
> > disable
> > PSR here too.
> Disabling now.
> 
> > 
> > 
> > > 
> > > +	}
> > > +	/* clear status register */
> > > +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> > > val);
> > So the other two errors are not handled, silently clearing them
> > isn't
> > right. How about at least printing a debug with the read value and
> > saying the error wasn't handled?
> DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR is only applicable for PSR2
Okay, but we do allow PSR2 with i915.enable_psr=1 right?

>  and to
> sink report DP_PSR_LINK_CRC_ERROR a bit needs to be set in
> DP_PSR_EN_CFG(both done in the next patch).
> 
> Do you still think that would be nice to print?
> > 
> > 
> > > 
> > > +
> > >  	/* TODO: handle other PSR/PSR2 errors */
> > >  dpcd_error:
> > >  	intel_psr_schedule_activate_work(dev_priv);
> > 
> _______________________________________________
> 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 v2 6/9] 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 on DP_PSR_ERROR_STATUS.

Also spec recommends to disable MAX_SLEEP as a trigger to exit PSR when
CRC check is enabled to improve power savings.

Spec: 7723

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---

Changes from v1:
- printing a debug message when sink assert a error

 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 24 +++++++++++++++++-------
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fb106026a1f4..d3efbd654889 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4016,6 +4016,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 558b08a43f9e..1920e7d03e06 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -290,6 +290,8 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 		dpcd_val |= DP_PSR_ENABLE_PSR2;
 	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);
@@ -377,6 +379,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);
 }
@@ -602,10 +607,12 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 		 * preventing  other hw tracking issues now we can rely
 		 * on frontbuffer tracking.
 		 */
-		I915_WRITE(EDP_PSR_DEBUG,
-			   EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD |
-			   EDP_PSR_DEBUG_MASK_LPSP);
+		u32 val = EDP_PSR_DEBUG_MASK_MEMUP | EDP_PSR_DEBUG_MASK_HPD
+			  | EDP_PSR_DEBUG_MASK_LPSP;
+
+		if (INTEL_GEN(dev_priv) >= 8)
+			val |= EDP_PSR_DEBUG_MASK_MAX_SLEEP;
+		I915_WRITE(EDP_PSR_DEBUG, val);
 	}
 }
 
@@ -1161,14 +1168,17 @@ void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
 		goto dpcd_error;
 	}
 
-	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, exiting PSR\n");
+		if (val & DP_PSR_LINK_CRC_ERROR)
+			DRM_DEBUG_KMS("PSR Link CRC error, exiting PSR\n");
 		intel_psr_exit(dev_priv);
 	}
 	/* 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 */
 dpcd_error:
 	intel_psr_schedule_activate_work(dev_priv);
 not_enabled:
-- 
2.17.0

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

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

[Rodrigo Vivi]

On Wed, Apr 18, 2018 at 03:43:08PM -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 on DP_PSR_ERROR_STATUS.

good idea, but first we need to stop using sink crc for the tests :(

and make sure the interrupts are being handled correct...

(I was going to mention an extra requirement that was to make sure
that you implement the needed Wa, but I noticed you already took care
of this)

> 
> Also spec recommends to disable MAX_SLEEP as a trigger to exit PSR when
> CRC check is enabled to improve power savings.

my first impression was that this should be a separated patch
in case we needed to revert it doesn't revert both functionalities...

however I saw this was Display WA #0388 with a mention:
"This becomes standard required programming for all subsequent projects."

so yeah, it belongs here. Maybe good to at least referrence this wa number.

> 
> Spec: 7723
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 
> Changes from v1:
> - printing a debug message when sink assert a error
> 
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 24 +++++++++++++++++-------
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fb106026a1f4..d3efbd654889 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4016,6 +4016,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 558b08a43f9e..1920e7d03e06 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -290,6 +290,8 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  		dpcd_val |= DP_PSR_ENABLE_PSR2;
>  	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);
> @@ -377,6 +379,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);
>  }
> @@ -602,10 +607,12 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  		 * preventing  other hw tracking issues now we can rely
>  		 * on frontbuffer tracking.
>  		 */
> -		I915_WRITE(EDP_PSR_DEBUG,
> -			   EDP_PSR_DEBUG_MASK_MEMUP |
> -			   EDP_PSR_DEBUG_MASK_HPD |
> -			   EDP_PSR_DEBUG_MASK_LPSP);
> +		u32 val = EDP_PSR_DEBUG_MASK_MEMUP | EDP_PSR_DEBUG_MASK_HPD
> +			  | EDP_PSR_DEBUG_MASK_LPSP;
> +
> +		if (INTEL_GEN(dev_priv) >= 8)
> +			val |= EDP_PSR_DEBUG_MASK_MAX_SLEEP;
> +		I915_WRITE(EDP_PSR_DEBUG, val);
>  	}
>  }
>  
> @@ -1161,14 +1168,17 @@ void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
>  		goto dpcd_error;
>  	}
>  
> -	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, exiting PSR\n");
> +		if (val & DP_PSR_LINK_CRC_ERROR)
> +			DRM_DEBUG_KMS("PSR Link CRC error, exiting PSR\n");
>  		intel_psr_exit(dev_priv);
>  	}
>  	/* 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 */
>  dpcd_error:
>  	intel_psr_schedule_activate_work(dev_priv);
>  not_enabled:
> -- 
> 2.17.0
> 
> _______________________________________________
> 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

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

[Souza, Jose]

On Fri, 2018-04-20 at 14:16 -0700, Rodrigo Vivi wrote:
> On Wed, Apr 18, 2018 at 03:43:08PM -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 on DP_PSR_ERROR_STATUS.
> 
> good idea, but first we need to stop using sink crc for the tests :(

We do the sink crc tests to check if PSR really exited and the screen
was updated with the new frame right? I don't see how this would cause
any problems.

> 
> and make sure the interrupts are being handled correct...
> 
> (I was going to mention an extra requirement that was to make sure
> that you implement the needed Wa, but I noticed you already took care
> of this)
> 
> > 
> > Also spec recommends to disable MAX_SLEEP as a trigger to exit PSR
> > when
> > CRC check is enabled to improve power savings.
> 
> my first impression was that this should be a separated patch
> in case we needed to revert it doesn't revert both functionalities...
> 
> however I saw this was Display WA #0388 with a mention:
> "This becomes standard required programming for all subsequent
> projects."
> 
> so yeah, it belongs here. Maybe good to at least referrence this wa
> number.

I will add this.

> 
> > 
> > Spec: 7723
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > 
> > Changes from v1:
> > - printing a debug message when sink assert a error
> > 
> >  drivers/gpu/drm/i915/i915_reg.h  |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 24 +++++++++++++++++-------
> >  2 files changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index fb106026a1f4..d3efbd654889 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4016,6 +4016,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 558b08a43f9e..1920e7d03e06 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -290,6 +290,8 @@ static void hsw_psr_enable_sink(struct intel_dp
> > *intel_dp)
> >  		dpcd_val |= DP_PSR_ENABLE_PSR2;
> >  	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);
> > @@ -377,6 +379,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);
> >  }
> > @@ -602,10 +607,12 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  		 * preventing  other hw tracking issues now we can
> > rely
> >  		 * on frontbuffer tracking.
> >  		 */
> > -		I915_WRITE(EDP_PSR_DEBUG,
> > -			   EDP_PSR_DEBUG_MASK_MEMUP |
> > -			   EDP_PSR_DEBUG_MASK_HPD |
> > -			   EDP_PSR_DEBUG_MASK_LPSP);
> > +		u32 val = EDP_PSR_DEBUG_MASK_MEMUP |
> > EDP_PSR_DEBUG_MASK_HPD
> > +			  | EDP_PSR_DEBUG_MASK_LPSP;
> > +
> > +		if (INTEL_GEN(dev_priv) >= 8)
> > +			val |= EDP_PSR_DEBUG_MASK_MAX_SLEEP;
> > +		I915_WRITE(EDP_PSR_DEBUG, val);
> >  	}
> >  }
> >  
> > @@ -1161,14 +1168,17 @@ void
> > intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
> >  		goto dpcd_error;
> >  	}
> >  
> > -	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,
> > exiting PSR\n");
> > +		if (val & DP_PSR_LINK_CRC_ERROR)
> > +			DRM_DEBUG_KMS("PSR Link CRC error, exiting
> > PSR\n");
> >  		intel_psr_exit(dev_priv);
> >  	}
> >  	/* 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 */
> >  dpcd_error:
> >  	intel_psr_schedule_activate_work(dev_priv);
> >  not_enabled:
> > -- 
> > 2.17.0
> > 
> > _______________________________________________
> > 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

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

[Dhinakaran Pandiyan]

On Wed, 2018-04-18 at 15:43 -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 on DP_PSR_ERROR_STATUS.
> 
> Also spec recommends to disable MAX_SLEEP as a trigger to exit PSR
> when
> CRC check is enabled to improve power savings.

Let's mask this irrespective of CRC's, i.e., for HSW too. It'll make
PSR exit more deterministic.

Cc: Ville
I remember Ville was asking if there's a way to disable the timeouts a
while ago.



> 
> Spec: 7723
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 
> Changes from v1:
> - printing a debug message when sink assert a error
> 
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 24 +++++++++++++++++-------
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index fb106026a1f4..d3efbd654889 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4016,6 +4016,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 558b08a43f9e..1920e7d03e06 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -290,6 +290,8 @@ static void hsw_psr_enable_sink(struct intel_dp
> *intel_dp)
>  		dpcd_val |= DP_PSR_ENABLE_PSR2;
>  	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);
> @@ -377,6 +379,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);
>  }
> @@ -602,10 +607,12 @@ static void hsw_psr_enable_source(struct
> intel_dp *intel_dp,
>  		 * preventing  other hw tracking issues now we can
> rely
>  		 * on frontbuffer tracking.
>  		 */
> -		I915_WRITE(EDP_PSR_DEBUG,
> -			   EDP_PSR_DEBUG_MASK_MEMUP |
> -			   EDP_PSR_DEBUG_MASK_HPD |
> -			   EDP_PSR_DEBUG_MASK_LPSP);
> +		u32 val = EDP_PSR_DEBUG_MASK_MEMUP |
> EDP_PSR_DEBUG_MASK_HPD
> +			  | EDP_PSR_DEBUG_MASK_LPSP;
> +
> +		if (INTEL_GEN(dev_priv) >= 8)
> +			val |= EDP_PSR_DEBUG_MASK_MAX_SLEEP;
> +		I915_WRITE(EDP_PSR_DEBUG, val);
>  	}
>  }
>  
> @@ -1161,14 +1168,17 @@ void intel_psr_hpd_short_pulse_handle(struct
> intel_dp *intel_dp)
>  		goto dpcd_error;
>  	}
>  
> -	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,
> exiting PSR\n");
> +		if (val & DP_PSR_LINK_CRC_ERROR)
> +			DRM_DEBUG_KMS("PSR Link CRC error, exiting
> PSR\n");
>  		intel_psr_exit(dev_priv);

Not sure how exit will help here.

We should print this as DRM_ERROR, check if CRC errors repeat more than
a few times and disable PSR altogether if they do. We can probably
start with a small threshold and then tune it.

Sorry for the delay in reviewing this patch.


>  	}
>  	/* 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 */
>  dpcd_error:
>  	intel_psr_schedule_activate_work(dev_priv);
>  not_enabled:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[José Roberto de Souza]

This reduces the spaghetti that intel_dp_aux_xfer().

Moved doing less changes possible here, improvements to the new
function in further patch.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---

New patch in this series.

 drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 701963a192ee..a11465c62950 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1062,6 +1062,38 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
 	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
 }
 
+static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv =
+			to_i915(intel_dig_port->base.base.dev);
+	i915_reg_t ch_ctl;
+	uint32_t status;
+	int try;
+
+	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
+
+	for (try = 0; try < 3; try++) {
+		status = I915_READ_NOTRACE(ch_ctl);
+		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
+			break;
+		msleep(1);
+	}
+
+	if (try == 3) {
+		static u32 last_status = -1;
+		const u32 status = I915_READ(ch_ctl);
+
+		if (status != last_status) {
+			WARN(1, "dp_aux_ch not started status 0x%08x\n",
+			     status);
+			last_status = status;
+		}
+	}
+
+	return true;
+}
+
 static int
 intel_dp_aux_xfer(struct intel_dp *intel_dp,
 		  const uint8_t *send, int send_bytes,
@@ -1074,7 +1106,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 	i915_reg_t ch_ctl, ch_data[5];
 	uint32_t aux_clock_divider;
 	int i, ret, recv_bytes;
-	uint32_t status;
+	uint32_t status = 0;
 	int try, clock = 0;
 	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
 	bool vdd;
@@ -1102,23 +1134,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 	intel_dp_check_edp(intel_dp);
 
 	/* Try to wait for any previous AUX channel activity */
-	for (try = 0; try < 3; try++) {
-		status = I915_READ_NOTRACE(ch_ctl);
-		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
-			break;
-		msleep(1);
-	}
-
-	if (try == 3) {
-		static u32 last_status = -1;
-		const u32 status = I915_READ(ch_ctl);
-
-		if (status != last_status) {
-			WARN(1, "dp_aux_ch not started status 0x%08x\n",
-			     status);
-			last_status = status;
-		}
-
+	if (intel_dp_aux_is_busy(intel_dp)) {
 		ret = -EBUSY;
 		goto out;
 	}
-- 
2.17.0

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

Re: [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[Dhinakaran Pandiyan]

On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> This reduces the spaghetti that intel_dp_aux_xfer().
> 
> Moved doing less changes possible here, improvements to the new
> function in further patch.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> 
> New patch in this series.
> 
>  drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 701963a192ee..a11465c62950 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1062,6 +1062,38 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
>  }
>  
> +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +	i915_reg_t ch_ctl;
> +	uint32_t status;
> +	int try;
> +
> +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +
> +	for (try = 0; try < 3; try++) {
> +		status = I915_READ_NOTRACE(ch_ctl);
> +		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> +			break;

Did you mean to return false here?

Anyway, this code here looks very similar to intel_dp_aux_wait_done().
Might be worth checking if it can be reused.

> +		msleep(1);
> +	}
> +
> +	if (try == 3) {
> +		static u32 last_status = -1;
> +		const u32 status = I915_READ(ch_ctl);
> +
> +		if (status != last_status) {
> +			WARN(1, "dp_aux_ch not started status 0x%08x\n",
> +			     status);
> +			last_status = status;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static int
>  intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  		  const uint8_t *send, int send_bytes,
> @@ -1074,7 +1106,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  	i915_reg_t ch_ctl, ch_data[5];
>  	uint32_t aux_clock_divider;
>  	int i, ret, recv_bytes;
> -	uint32_t status;
> +	uint32_t status = 0;
>  	int try, clock = 0;
>  	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
>  	bool vdd;
> @@ -1102,23 +1134,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  	intel_dp_check_edp(intel_dp);
>  
>  	/* Try to wait for any previous AUX channel activity */
> -	for (try = 0; try < 3; try++) {
> -		status = I915_READ_NOTRACE(ch_ctl);
> -		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> -			break;
> -		msleep(1);
> -	}
> -
> -	if (try == 3) {
> -		static u32 last_status = -1;
> -		const u32 status = I915_READ(ch_ctl);
> -
> -		if (status != last_status) {
> -			WARN(1, "dp_aux_ch not started status 0x%08x\n",
> -			     status);
> -			last_status = status;
> -		}
> -
> +	if (intel_dp_aux_is_busy(intel_dp)) {
>  		ret = -EBUSY;
>  		goto out;
>  	}

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

Re: [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[Souza, Jose]

On Thu, 2018-04-26 at 15:51 -0700, Dhinakaran Pandiyan wrote:
> 
> 
> On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> > This reduces the spaghetti that intel_dp_aux_xfer().
> > 
> > Moved doing less changes possible here, improvements to the new
> > function in further patch.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> > 
> > New patch in this series.
> > 
> >  drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------
> > ------
> >  1 file changed, 34 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 701963a192ee..a11465c62950 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1062,6 +1062,38 @@ static uint32_t skl_get_aux_send_ctl(struct
> > intel_dp *intel_dp,
> >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> >  }
> >  
> > +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +	i915_reg_t ch_ctl;
> > +	uint32_t status;
> > +	int try;
> > +
> > +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > +
> > +	for (try = 0; try < 3; try++) {
> > +		status = I915_READ_NOTRACE(ch_ctl);
> > +		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > +			break;
> 
> Did you mean to return false here?

Oh thanks, I just fixed this in the next patch.

> 
> Anyway, this code here looks very similar to
> intel_dp_aux_wait_done().
> Might be worth checking if it can be reused.

I'm not sure that hardware will send a interruption when it finish a
aux ch transaction that started by it self, that is why I'm going safe
here and just keep what is working for now.

> 
> > +		msleep(1);
> > +	}
> > +
> > +	if (try == 3) {
> > +		static u32 last_status = -1;
> > +		const u32 status = I915_READ(ch_ctl);
> > +
> > +		if (status != last_status) {
> > +			WARN(1, "dp_aux_ch not started status
> > 0x%08x\n",
> > +			     status);
> > +			last_status = status;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static int
> >  intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  		  const uint8_t *send, int send_bytes,
> > @@ -1074,7 +1106,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  	i915_reg_t ch_ctl, ch_data[5];
> >  	uint32_t aux_clock_divider;
> >  	int i, ret, recv_bytes;
> > -	uint32_t status;
> > +	uint32_t status = 0;
> >  	int try, clock = 0;
> >  	bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
> >  	bool vdd;
> > @@ -1102,23 +1134,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  	intel_dp_check_edp(intel_dp);
> >  
> >  	/* Try to wait for any previous AUX channel activity */
> > -	for (try = 0; try < 3; try++) {
> > -		status = I915_READ_NOTRACE(ch_ctl);
> > -		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > -			break;
> > -		msleep(1);
> > -	}
> > -
> > -	if (try == 3) {
> > -		static u32 last_status = -1;
> > -		const u32 status = I915_READ(ch_ctl);
> > -
> > -		if (status != last_status) {
> > -			WARN(1, "dp_aux_ch not started status
> > 0x%08x\n",
> > -			     status);
> > -			last_status = status;
> > -		}
> > -
> > +	if (intel_dp_aux_is_busy(intel_dp)) {
> >  		ret = -EBUSY;
> >  		goto out;
> >  	}
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 7/9] drm/i915/dp: Move code to check if aux ch is busy to a function

[Pandiyan, Dhinakaran]

On Mon, 2018-04-30 at 23:39 +0000, Souza, Jose wrote:
> On Thu, 2018-04-26 at 15:51 -0700, Dhinakaran Pandiyan wrote:
> > 
> > 
> > 
> > On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> > > 
> > > This reduces the spaghetti that intel_dp_aux_xfer().
> > > 
> > > Moved doing less changes possible here, improvements to the new
> > > function in further patch.
> > > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > > 
> > > New patch in this series.
> > > 
> > >  drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++------
> > > ------
> > >  1 file changed, 34 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 701963a192ee..a11465c62950 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1062,6 +1062,38 @@ static uint32_t
> > > skl_get_aux_send_ctl(struct
> > > intel_dp *intel_dp,
> > >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> > >  }
> > >  
> > > +static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +	i915_reg_t ch_ctl;
> > > +	uint32_t status;
> > > +	int try;
> > > +
> > > +	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > > +
> > > +	for (try = 0; try < 3; try++) {
> > > +		status = I915_READ_NOTRACE(ch_ctl);
> > > +		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > > +			break;
> > Did you mean to return false here?
> Oh thanks, I just fixed this in the next patch.
> 
> > 
> > 
> > Anyway, this code here looks very similar to
> > intel_dp_aux_wait_done().
> > Might be worth checking if it can be reused.
> I'm not sure that hardware will send a interruption when it finish a
> aux ch transaction that started by it self, that is why I'm going
> safe
> here and just keep what is working for now.

I should have clarified I meant intel_dp_aux_wait_done(intel_dp,
false).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 8/9] drm/i915/dp: Improve intel_dp_aux_is_busy()

[José Roberto de Souza]

- Doing earlier return when not busy
- using u32 instead of uint32_t
- counting from 3 to 0 as it is is the most common in the driver
- using DRM_WARN() instead of WARN()
- adding aux port name to the debug message
- nuking last_status, it is one static variable to all DP ports
also 2 different aux transactions with the same message size would
have the same ch_ctl value, if really desired to reduce the number
of debug messages it should be implemented a per aux ch last_status
- no need to sleep in the last try
- sleeping for 1 aux ch transaction time

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---

New patch in this series.

 drivers/gpu/drm/i915/intel_dp.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a11465c62950..258e23961456 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1068,28 +1068,21 @@ static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv =
 			to_i915(intel_dig_port->base.base.dev);
 	i915_reg_t ch_ctl;
-	uint32_t status;
-	int try;
+	u32 status;
+	unsigned int try;
 
 	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
 
-	for (try = 0; try < 3; try++) {
+	for (try = 3; try; try--) {
 		status = I915_READ_NOTRACE(ch_ctl);
 		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
-			break;
-		msleep(1);
+			return false;
+		if (try > 1)
+			usleep_range(400, 500);
 	}
 
-	if (try == 3) {
-		static u32 last_status = -1;
-		const u32 status = I915_READ(ch_ctl);
-
-		if (status != last_status) {
-			WARN(1, "dp_aux_ch not started status 0x%08x\n",
-			     status);
-			last_status = status;
-		}
-	}
+	DRM_WARN("DP aux %c is busy 0x%08x\n",
+		 aux_ch_name(intel_dp->aux_ch), status);
 
 	return true;
 }
-- 
2.17.0

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

Re: [PATCH v2 8/9] drm/i915/dp: Improve intel_dp_aux_is_busy()

[Dhinakaran Pandiyan]

On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote:
> - Doing earlier return when not busy
> - using u32 instead of uint32_t
> - counting from 3 to 0 as it is is the most common in the driver
Hmm. I see more instances that increment the loop variable than the
opposite.

> - using DRM_WARN() instead of WARN()
Why? WARN and WARN_ON's are common afaict

> - adding aux port name to the debug message
Sounds like a good idea.

> - nuking last_status, it is one static variable to all DP ports
> also 2 different aux transactions with the same message size would
> have the same ch_ctl value, if really desired to reduce the number
> of debug messages it should be implemented a per aux ch last_status
Makes sense to do this.

> - no need to sleep in the last try
> - sleeping for 1 aux ch transaction time
Was this determined based on the clock speed or is it in the spec? I
see numbers for timeout, nothing for the transaction itself.


This patch mixes up functional changes with non-functional ones. I'd
prefer you split these changes.


> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> 
> New patch in this series.
> 
>  drivers/gpu/drm/i915/intel_dp.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index a11465c62950..258e23961456 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1068,28 +1068,21 @@ static bool intel_dp_aux_is_busy(struct
> intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv =
>  			to_i915(intel_dig_port->base.base.dev);
>  	i915_reg_t ch_ctl;
> -	uint32_t status;
> -	int try;
> +	u32 status;
> +	unsigned int try;
>  
>  	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>  
> -	for (try = 0; try < 3; try++) {
> +	for (try = 3; try; try--) {
>  		status = I915_READ_NOTRACE(ch_ctl);
>  		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> -			break;
> -		msleep(1);
> +			return false;
> +		if (try > 1)
> +			usleep_range(400, 500);
>  	}
>  
> -	if (try == 3) {
> -		static u32 last_status = -1;
> -		const u32 status = I915_READ(ch_ctl);
> -
> -		if (status != last_status) {
> -			WARN(1, "dp_aux_ch not started status
> 0x%08x\n",
> -			     status);
> -			last_status = status;
> -		}
> -	}
> +	DRM_WARN("DP aux %c is busy 0x%08x\n",
> +		 aux_ch_name(intel_dp->aux_ch), status);
>  
>  	return true;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 9/9] drm/i915/dp: Avoid concurrent access when HW is using aux ch

[José Roberto de Souza]

This will avoid some cases of concurrent access to aux ch registers when
hardware is using it(HW uses it when PSR, GTC and aux frame is enabled).

It is just first step to see if this scenario happens, if so it will be
properly handled as described in bspec.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---

New patch in this series, this is replacing to the patches in this series that
was exiting PSR before a aux transaction.
As discussed with Dhinakaran, let's check first if this scenary happens if
so I will bring those patches back.

 drivers/gpu/drm/i915/intel_dp.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 258e23961456..74abd4cd93dd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1062,7 +1062,7 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
 	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
 }
 
-static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
+static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp, unsigned int tries)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv =
@@ -1073,7 +1073,7 @@ static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp)
 
 	ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
 
-	for (try = 3; try; try--) {
+	for (try = tries; try; try--) {
 		status = I915_READ_NOTRACE(ch_ctl);
 		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
 			return false;
@@ -1127,7 +1127,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 	intel_dp_check_edp(intel_dp);
 
 	/* Try to wait for any previous AUX channel activity */
-	if (intel_dp_aux_is_busy(intel_dp)) {
+	if (intel_dp_aux_is_busy(intel_dp, 3)) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -1148,6 +1148,18 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
+			/* WA: try to avoid concurrent access to aux ch
+			 * registers while hardware is using it, the other
+			 * way is not handled at all.
+			 */
+			if (intel_dp_aux_is_busy(intel_dp, 1)) {
+				DRM_ERROR("Aux ch %c is busy, hw is using it",
+					  aux_ch_name(intel_dp->aux_ch));
+				/* sleep for a transaction time */
+				usleep_range(400, 500);
+				continue;
+			}
+
 			/* Load the send data into the aux channel data registers */
 			for (i = 0; i < send_bytes; i += 4)
 				I915_WRITE(ch_data[i >> 2],
-- 
2.17.0

_______________________________________________
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 [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
URL   : https://patchwork.freedesktop.org/series/41923/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9dc8e92b44ac drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
f0dbcd5555f6 drm/i915/psr: Move PSR exit specific code to hardware specific function
0a466f97bd5a drm/i915/psr: Remove intel_crtc_state parameter from disable()
440ab6965b08 drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
-:54: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
#54: FILE: drivers/gpu/drm/i915/intel_psr.c:1002:
+	if (dev_priv->psr.active || dev_priv->psr.busy_frontbuffer_bits
+	    || work_busy(&dev_priv->psr.work.work))

total: 0 errors, 0 warnings, 1 checks, 78 lines checked
e463ec258771 drm/i915/psr: Handle PSR RFB storage error
16a97441a986 drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
-:32: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#32: FILE: drivers/gpu/drm/i915/i915_reg.h:4019:
+#define   EDP_PSR_CRC_ENABLE			(1<<10) /* BDW+ */
                             			  ^

total: 0 errors, 0 warnings, 1 checks, 60 lines checked
5a9cc41f7e89 drm/i915/dp: Move code to check if aux ch is busy to a function
-:41: WARNING:MSLEEP: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
#41: FILE: drivers/gpu/drm/i915/intel_dp.c:1080:
+		msleep(1);

total: 0 errors, 1 warnings, 0 checks, 70 lines checked
87008545c0f1 drm/i915/dp: Improve intel_dp_aux_is_busy()
524271645ddc drm/i915/dp: Avoid concurrent access when HW is using aux ch

_______________________________________________
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 [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
URL   : https://patchwork.freedesktop.org/series/41923/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
Okay!

Commit: drm/i915/psr: Move PSR exit specific code to hardware specific function
-drivers/gpu/drm/i915/selftests/../i915_drv.h:2207:33: warning: constant 0xffffea0000000000 is so big it is unsigned long
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3655:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:2208:33: warning: constant 0xffffea0000000000 is so big it is unsigned long
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3656:16: warning: expression using sizeof(void)

Commit: drm/i915/psr: Remove intel_crtc_state parameter from disable()
-drivers/gpu/drm/i915/selftests/../i915_drv.h:2208:33: warning: constant 0xffffea0000000000 is so big it is unsigned long
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3656:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:2207:33: warning: constant 0xffffea0000000000 is so big it is unsigned long
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3655: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/bdw+: Enable CRC check in the static frame on the sink side
Okay!

Commit: drm/i915/dp: Move code to check if aux ch is busy to a function
Okay!

Commit: drm/i915/dp: Improve intel_dp_aux_is_busy()
Okay!

Commit: drm/i915/dp: Avoid concurrent access when HW is using aux ch
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 [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
URL   : https://patchwork.freedesktop.org/series/41923/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4066 -> Patchwork_8743 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713


== Participating hosts (33 -> 31) ==

  Additional (2): fi-glk-j4005 fi-bxt-dsi 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-glk-1 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8743

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8743: 524271645ddca56c7333fb096cb311eb98a5cbc3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

524271645ddc drm/i915/dp: Avoid concurrent access when HW is using aux ch
87008545c0f1 drm/i915/dp: Improve intel_dp_aux_is_busy()
5a9cc41f7e89 drm/i915/dp: Move code to check if aux ch is busy to a function
16a97441a986 drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
e463ec258771 drm/i915/psr: Handle PSR RFB storage error
440ab6965b08 drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
0a466f97bd5a drm/i915/psr: Remove intel_crtc_state parameter from disable()
f0dbcd5555f6 drm/i915/psr: Move PSR exit specific code to hardware specific function
9dc8e92b44ac drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
URL   : https://patchwork.freedesktop.org/series/41923/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4066_full -> Patchwork_8743_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8743_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8743_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8743_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_eio@suspend:
      shard-hsw:          PASS -> DMESG-WARN

    
    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-dirty-render:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_mmap_write_crc:
      shard-apl:          PASS -> FAIL (fdo#103286)

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    igt@prime_vgem@basic-fence-flip:
      shard-apl:          PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

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

    igt@kms_frontbuffer_tracking@fbc-farfromfence:
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +2

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103286 https://bugs.freedesktop.org/show_bug.cgi?id=103286
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (9 -> 4) ==

  Missing    (5): shard-glk8 shard-glk6 shard-glk7 shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8743

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8743: 524271645ddca56c7333fb096cb311eb98a5cbc3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v2 1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Dhinakaran Pandiyan]

On Wednesday, April 18, 2018 3:43:03 PM PDT José Roberto de Souza wrote:
> It was reading some random register in VLV and CHV.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 
> No changes from v1.
> 
>  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 69a5b276f4d8..659180656f5b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -424,6 +424,11 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
> struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> 
> +	if (dev_priv->psr.psr2_enabled)
> +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> +	else
> +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> +

Why not move the checks under hsw_activate_psr2 and hsw_activate_psr1?  This 
is just duplicating another  psr2_enabled branch that is right below.

>  	/* On HSW+ after we enable PSR on source it will activate it
>  	 * as soon as it match configure idle_frame count. So
>  	 * we just actually enable it here on activation time.
> @@ -549,10 +554,6 @@ static void intel_psr_activate(struct intel_dp
> *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.psr2_enabled)
> -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> -	else
> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  	WARN_ON(dev_priv->psr.active);
>  	lockdep_assert_held(&dev_priv->psr.lock);


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

Re: [PATCH v2 1/9] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Souza, Jose]

On Wed, 2018-04-25 at 17:41 -0700, Dhinakaran Pandiyan wrote:
> On Wednesday, April 18, 2018 3:43:03 PM PDT José Roberto de Souza
> wrote:
> > It was reading some random register in VLV and CHV.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > 
> > No changes from v1.
> > 
> >  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 69a5b276f4d8..659180656f5b
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -424,6 +424,11 @@ static void hsw_psr_activate(struct intel_dp
> > *intel_dp)
> > struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > 
> > +	if (dev_priv->psr.psr2_enabled)
> > +		WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > +	else
> > +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > +
> 
> Why not move the checks under hsw_activate_psr2 and
> hsw_activate_psr1?  This 
> is just duplicating another  psr2_enabled branch that is right below.

Okay, done.

> 
> >  	/* On HSW+ after we enable PSR on source it will activate
> > it
> >  	 * as soon as it match configure idle_frame count. So
> >  	 * we just actually enable it here on activation time.
> > @@ -549,10 +554,6 @@ static void intel_psr_activate(struct intel_dp
> > *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.psr2_enabled)
> > -		WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > -	else
> > -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> >  	WARN_ON(dev_priv->psr.active);
> >  	lockdep_assert_held(&dev_priv->psr.lock);
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx