[PATCH 01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[PATCH 01/11] 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>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2d53f7398a6d..fc7c36efd401 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.16.3

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

[PATCH 02/11] 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>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 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 5373b171bb96..a8d300280a2c 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 fc7c36efd401..bcaac9e69f8c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -838,53 +838,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;
 }
 
@@ -1094,6 +1108,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;
@@ -1101,5 +1116,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.16.3

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

Re: [PATCH 02/11] drm/i915/psr: Move PSR exit specific code to hardware specific function

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:27PM -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.
> 
> 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>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  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 5373b171bb96..a8d300280a2c 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 fc7c36efd401..bcaac9e69f8c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -838,53 +838,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;
>  }
>  
> @@ -1094,6 +1108,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;
> @@ -1101,5 +1116,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.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/11] drm/i915/psr: Share code between disable and exit

[José Roberto de Souza]

The disable and exit sequence are very similar with a lot common
code between both, so here sharing the code.

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>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_psr.c | 84 ++++++++++++++++++----------------------
 2 files changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a8d300280a2c..cb72ee27422f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -617,7 +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);
+	void (*exit)(struct intel_dp *intel_dp, bool disabling);
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index bcaac9e69f8c..d3451afeb8bb 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -672,23 +672,9 @@ static void vlv_psr_disable(struct intel_dp *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_EDP_PSR_IN_TRANS,
-					    0,
-					    1))
-			WARN(1, "PSR transition took longer than expected\n");
-
-		val = I915_READ(VLV_PSRCTL(crtc->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);
-
+		dev_priv->psr.exit(intel_dp, true);
 		dev_priv->psr.active = false;
 	} else {
 		WARN_ON(vlv_is_psr_active_on_pipe(dev, crtc->pipe));
@@ -703,31 +689,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	if (dev_priv->psr.active) {
-		i915_reg_t psr_status;
-		u32 psr_status_mask;
-
-		if (dev_priv->psr.psr2_enabled) {
-			psr_status = EDP_PSR2_STATUS;
-			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
-
-			I915_WRITE(EDP_PSR2_CTL,
-				   I915_READ(EDP_PSR2_CTL) &
-				   ~(EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE));
-
-		} else {
-			psr_status = EDP_PSR_STATUS;
-			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
-
-			I915_WRITE(EDP_PSR_CTL,
-				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
-		}
-
-		/* Wait till PSR is idle */
-		if (intel_wait_for_register(dev_priv,
-					    psr_status, psr_status_mask, 0,
-					    2000))
-			DRM_ERROR("Timed out waiting for PSR Idle State\n");
-
+		dev_priv->psr.exit(intel_dp, true);
 		dev_priv->psr.active = false;
 	} else {
 		if (dev_priv->psr.psr2_enabled)
@@ -838,25 +800,41 @@ static void intel_psr_work(struct work_struct *work)
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-static void hsw_psr_exit(struct intel_dp *intel_dp)
+static void hsw_psr_exit(struct intel_dp *intel_dp, bool disabling)
 {
 	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);
+	i915_reg_t psr_status;
+	u32 psr_status_mask;
 	u32 val;
 
 	if (dev_priv->psr.psr2_enabled) {
+		psr_status = EDP_PSR2_STATUS;
+		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
+
 		val = I915_READ(EDP_PSR2_CTL);
-		WARN_ON(!(val & EDP_PSR2_ENABLE));
-		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
+		WARN_ON(!disabling && !(val & EDP_PSR2_ENABLE));
+		val &= ~EDP_PSR2_ENABLE;
+		if (disabling)
+			val &= ~EDP_SU_TRACK_ENABLE;
+		I915_WRITE(EDP_PSR2_CTL, val);
 	} else {
+		psr_status = EDP_PSR_STATUS;
+		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
+
 		val = I915_READ(EDP_PSR_CTL);
-		WARN_ON(!(val & EDP_PSR_ENABLE));
+		WARN_ON(!disabling && !(val & EDP_PSR_ENABLE));
 		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
 	}
+
+	/* When disabling wait till PSR is idle */
+	if (disabling && intel_wait_for_register(dev_priv, psr_status,
+						 psr_status_mask, 0, 2000))
+		DRM_ERROR("Timed out waiting for PSR Idle State\n");
 }
 
-static void vlv_psr_exit(struct intel_dp *intel_dp)
+static void vlv_psr_exit(struct intel_dp *intel_dp, bool disabling)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = dig_port->base.base.dev;
@@ -865,10 +843,14 @@ static void vlv_psr_exit(struct intel_dp *intel_dp)
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 	u32 val;
 
+	if (disabling && intel_wait_for_register(dev_priv, VLV_PSRSTAT(pipe),
+						 VLV_EDP_PSR_IN_TRANS, 0, 1))
+		DRM_WARN("PSR transition took longer than expected\n");
+
 	val = I915_READ(VLV_PSRCTL(pipe));
 
 	/*
-	 * Here we do the transition drirectly from
+	 * Here we do the transition directly 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.
@@ -877,8 +859,16 @@ static void vlv_psr_exit(struct intel_dp *intel_dp)
 	 * Now we are at Same state after vlv_psr_enable_source.
 	 */
 	val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
+	if (disabling) {
+		/* Put VLV PSR back to PSR_state 0 (disabled). */
+		val &= ~VLV_EDP_PSR_ENABLE;
+		val &= ~VLV_EDP_PSR_MODE_MASK;
+	}
 	I915_WRITE(VLV_PSRCTL(pipe), val);
 
+	if (disabling)
+		return;
+
 	/*
 	 * Send AUX wake up - Spec says after transitioning to PSR
 	 * active we have to send AUX wake up by writing 01h in DPCD
@@ -898,7 +888,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 	if (!dev_priv->psr.active)
 		return;
 
-	dev_priv->psr.exit(intel_dp);
+	dev_priv->psr.exit(intel_dp, false);
 	dev_priv->psr.active = false;
 }
 
-- 
2.16.3

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

Re: [PATCH 03/11] drm/i915/psr: Share code between disable and exit

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:28PM -0700, José Roberto de Souza wrote:
> The disable and exit sequence are very similar with a lot common
> code between both, so here sharing the code.

I don't believe that we should do this.
Disable as is has some extra wait/timeouts that could slow things down.

Or at least not do this while we don't have a proper robuts test in place
and to test also performance impacts...

> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c | 84 ++++++++++++++++++----------------------
>  2 files changed, 38 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a8d300280a2c..cb72ee27422f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -617,7 +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);
> +	void (*exit)(struct intel_dp *intel_dp, bool disabling);
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index bcaac9e69f8c..d3451afeb8bb 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -672,23 +672,9 @@ static void vlv_psr_disable(struct intel_dp *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_EDP_PSR_IN_TRANS,
> -					    0,
> -					    1))
> -			WARN(1, "PSR transition took longer than expected\n");
> -
> -		val = I915_READ(VLV_PSRCTL(crtc->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);
> -
> +		dev_priv->psr.exit(intel_dp, true);
>  		dev_priv->psr.active = false;
>  	} else {
>  		WARN_ON(vlv_is_psr_active_on_pipe(dev, crtc->pipe));
> @@ -703,31 +689,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	if (dev_priv->psr.active) {
> -		i915_reg_t psr_status;
> -		u32 psr_status_mask;
> -
> -		if (dev_priv->psr.psr2_enabled) {
> -			psr_status = EDP_PSR2_STATUS;
> -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> -
> -			I915_WRITE(EDP_PSR2_CTL,
> -				   I915_READ(EDP_PSR2_CTL) &
> -				   ~(EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE));
> -
> -		} else {
> -			psr_status = EDP_PSR_STATUS;
> -			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> -
> -			I915_WRITE(EDP_PSR_CTL,
> -				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
> -		}
> -
> -		/* Wait till PSR is idle */
> -		if (intel_wait_for_register(dev_priv,
> -					    psr_status, psr_status_mask, 0,
> -					    2000))
> -			DRM_ERROR("Timed out waiting for PSR Idle State\n");
> -
> +		dev_priv->psr.exit(intel_dp, true);
>  		dev_priv->psr.active = false;
>  	} else {
>  		if (dev_priv->psr.psr2_enabled)
> @@ -838,25 +800,41 @@ static void intel_psr_work(struct work_struct *work)
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> -static void hsw_psr_exit(struct intel_dp *intel_dp)
> +static void hsw_psr_exit(struct intel_dp *intel_dp, bool disabling)
>  {
>  	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);
> +	i915_reg_t psr_status;
> +	u32 psr_status_mask;
>  	u32 val;
>  
>  	if (dev_priv->psr.psr2_enabled) {
> +		psr_status = EDP_PSR2_STATUS;
> +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> +
>  		val = I915_READ(EDP_PSR2_CTL);
> -		WARN_ON(!(val & EDP_PSR2_ENABLE));
> -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> +		WARN_ON(!disabling && !(val & EDP_PSR2_ENABLE));
> +		val &= ~EDP_PSR2_ENABLE;
> +		if (disabling)
> +			val &= ~EDP_SU_TRACK_ENABLE;
> +		I915_WRITE(EDP_PSR2_CTL, val);
>  	} else {
> +		psr_status = EDP_PSR_STATUS;
> +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> +
>  		val = I915_READ(EDP_PSR_CTL);
> -		WARN_ON(!(val & EDP_PSR_ENABLE));
> +		WARN_ON(!disabling && !(val & EDP_PSR_ENABLE));
>  		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
>  	}
> +
> +	/* When disabling wait till PSR is idle */
> +	if (disabling && intel_wait_for_register(dev_priv, psr_status,
> +						 psr_status_mask, 0, 2000))
> +		DRM_ERROR("Timed out waiting for PSR Idle State\n");
>  }
>  
> -static void vlv_psr_exit(struct intel_dp *intel_dp)
> +static void vlv_psr_exit(struct intel_dp *intel_dp, bool disabling)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
> @@ -865,10 +843,14 @@ static void vlv_psr_exit(struct intel_dp *intel_dp)
>  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  	u32 val;
>  
> +	if (disabling && intel_wait_for_register(dev_priv, VLV_PSRSTAT(pipe),
> +						 VLV_EDP_PSR_IN_TRANS, 0, 1))
> +		DRM_WARN("PSR transition took longer than expected\n");
> +
>  	val = I915_READ(VLV_PSRCTL(pipe));
>  
>  	/*
> -	 * Here we do the transition drirectly from
> +	 * Here we do the transition directly 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.
> @@ -877,8 +859,16 @@ static void vlv_psr_exit(struct intel_dp *intel_dp)
>  	 * Now we are at Same state after vlv_psr_enable_source.
>  	 */
>  	val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> +	if (disabling) {
> +		/* Put VLV PSR back to PSR_state 0 (disabled). */
> +		val &= ~VLV_EDP_PSR_ENABLE;
> +		val &= ~VLV_EDP_PSR_MODE_MASK;
> +	}
>  	I915_WRITE(VLV_PSRCTL(pipe), val);
>  
> +	if (disabling)
> +		return;
> +
>  	/*
>  	 * Send AUX wake up - Spec says after transitioning to PSR
>  	 * active we have to send AUX wake up by writing 01h in DPCD
> @@ -898,7 +888,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  	if (!dev_priv->psr.active)
>  		return;
>  
> -	dev_priv->psr.exit(intel_dp);
> +	dev_priv->psr.exit(intel_dp, false);
>  	dev_priv->psr.active = false;
>  }
>  
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/11] drm/i915/psr: Share code between disable and exit

[Souza, Jose]

On Mon, 2018-04-02 at 11:11 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 30, 2018 at 03:23:28PM -0700, José Roberto de Souza
> wrote:
> > The disable and exit sequence are very similar with a lot common
> > code between both, so here sharing the code.
> 
> I don't believe that we should do this.
> Disable as is has some extra wait/timeouts that could slow things
> down.
> 
> Or at least not do this while we don't have a proper robuts test in
> place
> and to test also performance impacts...

When the disabling argument is false it don't have the disable
wait/timeouts, I kept the same behavior to both just sharing the common
part.

> 
> > 
> > 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>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_psr.c | 84 ++++++++++++++++++--------
> > --------------
> >  2 files changed, 38 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index a8d300280a2c..cb72ee27422f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -617,7 +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);
> > +	void (*exit)(struct intel_dp *intel_dp, bool disabling);
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index bcaac9e69f8c..d3451afeb8bb 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -672,23 +672,9 @@ static void vlv_psr_disable(struct intel_dp
> > *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_EDP_PSR_IN_TRANS,
> > -					    0,
> > -					    1))
> > -			WARN(1, "PSR transition took longer than
> > expected\n");
> > -
> > -		val = I915_READ(VLV_PSRCTL(crtc->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);
> > -
> > +		dev_priv->psr.exit(intel_dp, true);
> >  		dev_priv->psr.active = false;
> >  	} else {
> >  		WARN_ON(vlv_is_psr_active_on_pipe(dev, crtc-
> > >pipe));
> > @@ -703,31 +689,7 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  	if (dev_priv->psr.active) {
> > -		i915_reg_t psr_status;
> > -		u32 psr_status_mask;
> > -
> > -		if (dev_priv->psr.psr2_enabled) {
> > -			psr_status = EDP_PSR2_STATUS;
> > -			psr_status_mask =
> > EDP_PSR2_STATUS_STATE_MASK;
> > -
> > -			I915_WRITE(EDP_PSR2_CTL,
> > -				   I915_READ(EDP_PSR2_CTL) &
> > -				   ~(EDP_PSR2_ENABLE |
> > EDP_SU_TRACK_ENABLE));
> > -
> > -		} else {
> > -			psr_status = EDP_PSR_STATUS;
> > -			psr_status_mask =
> > EDP_PSR_STATUS_STATE_MASK;
> > -
> > -			I915_WRITE(EDP_PSR_CTL,
> > -				   I915_READ(EDP_PSR_CTL) &
> > ~EDP_PSR_ENABLE);
> > -		}
> > -
> > -		/* Wait till PSR is idle */
> > -		if (intel_wait_for_register(dev_priv,
> > -					    psr_status,
> > psr_status_mask, 0,
> > -					    2000))
> > -			DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> > -
> > +		dev_priv->psr.exit(intel_dp, true);
> >  		dev_priv->psr.active = false;
> >  	} else {
> >  		if (dev_priv->psr.psr2_enabled)
> > @@ -838,25 +800,41 @@ static void intel_psr_work(struct work_struct
> > *work)
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > -static void hsw_psr_exit(struct intel_dp *intel_dp)
> > +static void hsw_psr_exit(struct intel_dp *intel_dp, bool
> > disabling)
> >  {
> >  	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);
> > +	i915_reg_t psr_status;
> > +	u32 psr_status_mask;
> >  	u32 val;
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > +		psr_status = EDP_PSR2_STATUS;
> > +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > +
> >  		val = I915_READ(EDP_PSR2_CTL);
> > -		WARN_ON(!(val & EDP_PSR2_ENABLE));
> > -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> > +		WARN_ON(!disabling && !(val & EDP_PSR2_ENABLE));
> > +		val &= ~EDP_PSR2_ENABLE;
> > +		if (disabling)
> > +			val &= ~EDP_SU_TRACK_ENABLE;
> > +		I915_WRITE(EDP_PSR2_CTL, val);
> >  	} else {
> > +		psr_status = EDP_PSR_STATUS;
> > +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > +
> >  		val = I915_READ(EDP_PSR_CTL);
> > -		WARN_ON(!(val & EDP_PSR_ENABLE));
> > +		WARN_ON(!disabling && !(val & EDP_PSR_ENABLE));
> >  		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> >  	}
> > +
> > +	/* When disabling wait till PSR is idle */
> > +	if (disabling && intel_wait_for_register(dev_priv,
> > psr_status,
> > +						 psr_status_mask,
> > 0, 2000))
> > +		DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> >  }
> >  
> > -static void vlv_psr_exit(struct intel_dp *intel_dp)
> > +static void vlv_psr_exit(struct intel_dp *intel_dp, bool
> > disabling)
> >  {
> >  	struct intel_digital_port *dig_port =
> > dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = dig_port->base.base.dev;
> > @@ -865,10 +843,14 @@ static void vlv_psr_exit(struct intel_dp
> > *intel_dp)
> >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >  	u32 val;
> >  
> > +	if (disabling && intel_wait_for_register(dev_priv,
> > VLV_PSRSTAT(pipe),
> > +						 VLV_EDP_PSR_IN_TR
> > ANS, 0, 1))
> > +		DRM_WARN("PSR transition took longer than
> > expected\n");
> > +
> >  	val = I915_READ(VLV_PSRCTL(pipe));
> >  
> >  	/*
> > -	 * Here we do the transition drirectly from
> > +	 * Here we do the transition directly 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.
> > @@ -877,8 +859,16 @@ static void vlv_psr_exit(struct intel_dp
> > *intel_dp)
> >  	 * Now we are at Same state after vlv_psr_enable_source.
> >  	 */
> >  	val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> > +	if (disabling) {
> > +		/* Put VLV PSR back to PSR_state 0 (disabled). */
> > +		val &= ~VLV_EDP_PSR_ENABLE;
> > +		val &= ~VLV_EDP_PSR_MODE_MASK;
> > +	}
> >  	I915_WRITE(VLV_PSRCTL(pipe), val);
> >  
> > +	if (disabling)
> > +		return;
> > +
> >  	/*
> >  	 * Send AUX wake up - Spec says after transitioning to PSR
> >  	 * active we have to send AUX wake up by writing 01h in
> > DPCD
> > @@ -898,7 +888,7 @@ static void intel_psr_exit(struct
> > drm_i915_private *dev_priv)
> >  	if (!dev_priv->psr.active)
> >  		return;
> >  
> > -	dev_priv->psr.exit(intel_dp);
> > +	dev_priv->psr.exit(intel_dp, false);
> >  	dev_priv->psr.active = false;
> >  }
> >  
> > -- 
> > 2.16.3
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/11] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[José Roberto de Souza]

It is not necessary as is possible to get the pipe information
from intel_dp.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cb72ee27422f..99af9169d792 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 d3451afeb8bb..c4720b0152c3 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -665,24 +665,23 @@ 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);
+	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
 	if (dev_priv->psr.active) {
 		dev_priv->psr.exit(intel_dp, true);
 		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, 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;
@@ -727,7 +726,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.16.3

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

Re: [PATCH 04/11] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:29PM -0700, José Roberto de Souza wrote:
> It is not necessary as is possible to get the pipe information
> from intel_dp.
> 
> 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

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
>  drivers/gpu/drm/i915/intel_psr.c | 13 ++++++-------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cb72ee27422f..99af9169d792 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 d3451afeb8bb..c4720b0152c3 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -665,24 +665,23 @@ 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);
> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;

Lucas, is this the one you noticed we are using wrongly?

> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  
>  	if (dev_priv->psr.active) {
>  		dev_priv->psr.exit(intel_dp, true);
>  		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, 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;
> @@ -727,7 +726,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.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/11] drm/i915/psr: Remove intel_crtc_state parameter from disable()

[Ville Syrjälä]

On Fri, Mar 30, 2018 at 03:23:29PM -0700, José Roberto de Souza wrote:
> It is not necessary as is possible to get the pipe information
> from intel_dp.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +--
>  drivers/gpu/drm/i915/intel_psr.c | 13 ++++++-------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cb72ee27422f..99af9169d792 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 d3451afeb8bb..c4720b0152c3 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -665,24 +665,23 @@ 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);
> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;

That is a legacy pointer that should not be used anymore.

> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  
>  	if (dev_priv->psr.active) {
>  		dev_priv->psr.exit(intel_dp, true);
>  		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, 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;
> @@ -727,7 +726,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.16.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 05/11] drm/i915/psr: Export intel_psr_activate/exit()

[José Roberto de Souza]

Export this functions so other modules can activate and exit PSR.

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>
---
 drivers/gpu/drm/i915/intel_drv.h |  2 +
 drivers/gpu/drm/i915/intel_psr.c | 94 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d1452fd2a58d..70026b772721 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1891,6 +1891,8 @@ 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_exit(struct intel_dp *intel_dp, bool wait_idle);
+void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
 
 /* 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 c4720b0152c3..906a12ea934d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,6 +56,8 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
+#define ACTIVATE_WORK_MSEC_DELAY 100
+
 static inline enum intel_display_power_domain
 psr_aux_domain(struct intel_dp *intel_dp)
 {
@@ -548,7 +550,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
 }
 
-static void intel_psr_activate(struct intel_dp *intel_dp)
+static void __intel_psr_activate(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;
@@ -645,7 +647,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	dev_priv->psr.enabled = intel_dp;
 
 	if (INTEL_GEN(dev_priv) >= 9) {
-		intel_psr_activate(intel_dp);
+		__intel_psr_activate(intel_dp);
 	} else {
 		/*
 		 * FIXME: Activation should happen immediately since this
@@ -794,7 +796,7 @@ static void intel_psr_work(struct work_struct *work)
 	if (dev_priv->psr.busy_frontbuffer_bits)
 		goto unlock;
 
-	intel_psr_activate(intel_dp);
+	__intel_psr_activate(intel_dp);
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
 }
@@ -880,7 +882,7 @@ static void vlv_psr_exit(struct intel_dp *intel_dp, bool disabling)
 			   DP_SET_POWER_D0);
 }
 
-static void intel_psr_exit(struct drm_i915_private *dev_priv)
+static void __intel_psr_exit(struct drm_i915_private *dev_priv)
 {
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 
@@ -977,7 +979,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
 	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
 
 	if (frontbuffer_bits)
-		intel_psr_exit(dev_priv);
+		__intel_psr_exit(dev_priv);
 
 	mutex_unlock(&dev_priv->psr.lock);
 }
@@ -1023,7 +1025,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	if (frontbuffer_bits) {
 		if (dev_priv->psr.psr2_enabled ||
 		    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-			intel_psr_exit(dev_priv);
+			__intel_psr_exit(dev_priv);
 		} else {
 			/*
 			 * Display WA #0884: all
@@ -1041,7 +1043,7 @@ 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));
+					      msecs_to_jiffies(ACTIVATE_WORK_MSEC_DELAY));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1108,3 +1110,81 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.exit = hsw_psr_exit;
 	}
 }
+
+/**
+ * intel_psr_exit - Exit PSR
+ * @intel_dp: DisplayPort that should have PSR exited if active
+ * @wait_idle: if true wait until PSR is inactive otherwise just request it
+ *
+ * This function exit PSR if enabled and active in this DisplayPort.
+ */
+void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle)
+{
+	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);
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (dev_priv->psr.enabled != intel_dp)
+		goto out;
+	if (!dev_priv->psr.active && !wait_idle)
+		goto out;
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		struct drm_crtc *crtc = dig_port->base.base.crtc;
+		enum pipe pipe = to_intel_crtc(crtc)->pipe;
+
+		__intel_psr_exit(dev_priv);
+
+		if (!wait_idle)
+			goto out;
+
+		if (intel_wait_for_register(dev_priv, VLV_PSRSTAT(pipe),
+					    VLV_EDP_PSR_IN_TRANS, 0, 2000))
+			DRM_ERROR("Timed out waiting for PSR Idle State\n");
+	} else {
+		/* HSW+ PSR disable is actually, exit + wait transition */
+		dev_priv->psr.exit(intel_dp, wait_idle);
+		dev_priv->psr.active = false;
+	}
+
+out:
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+/**
+ * intel_psr_activate - Activate PSR
+ * @intel_dp: DisplayPort that should have PSR activated if enabled
+ * @schedule: if true schedule the active to happens in a few milliseconds
+ * otherwise activate right now.
+ *
+ * This function activate PSR if enabled in this DisplayPort.
+ */
+void intel_psr_activate(struct intel_dp *intel_dp, bool schedule)
+{
+	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);
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (dev_priv->psr.enabled != intel_dp || dev_priv->psr.active)
+		goto out;
+
+	if (dev_priv->psr.busy_frontbuffer_bits)
+		goto out;
+
+	if (schedule) {
+		if (!work_busy(&dev_priv->psr.work.work))
+			schedule_delayed_work(&dev_priv->psr.work,
+					      msecs_to_jiffies(ACTIVATE_WORK_MSEC_DELAY));
+	} else
+		__intel_psr_activate(intel_dp);
+out:
+	mutex_unlock(&dev_priv->psr.lock);
+}
-- 
2.16.3

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

Re: [PATCH 05/11] drm/i915/psr: Export intel_psr_activate/exit()

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:30PM -0700, José Roberto de Souza wrote:
> Export this functions so other modules can activate and exit PSR.

"module" is not a right word here.

But I also don't like the approach. In the past we had the use spread
on our driver and it was confusing and hard to deal with more corner
cases. frontbuffer tracking was created to fix that.

I understand where you are going because we need to disable it on aux
transactions, etc. But I don't believe the right way is to exporting
active/exit.

But to create one unified infrastructure to block and release psr
with atomic counters.

> 
> 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>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  drivers/gpu/drm/i915/intel_psr.c | 94 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d1452fd2a58d..70026b772721 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1891,6 +1891,8 @@ 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_exit(struct intel_dp *intel_dp, bool wait_idle);
> +void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
>  
>  /* 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 c4720b0152c3..906a12ea934d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,8 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +#define ACTIVATE_WORK_MSEC_DELAY 100
> +
>  static inline enum intel_display_power_domain
>  psr_aux_domain(struct intel_dp *intel_dp)
>  {
> @@ -548,7 +550,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
>  }
>  
> -static void intel_psr_activate(struct intel_dp *intel_dp)
> +static void __intel_psr_activate(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;
> @@ -645,7 +647,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	dev_priv->psr.enabled = intel_dp;
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_psr_activate(intel_dp);
> +		__intel_psr_activate(intel_dp);
>  	} else {
>  		/*
>  		 * FIXME: Activation should happen immediately since this
> @@ -794,7 +796,7 @@ static void intel_psr_work(struct work_struct *work)
>  	if (dev_priv->psr.busy_frontbuffer_bits)
>  		goto unlock;
>  
> -	intel_psr_activate(intel_dp);
> +	__intel_psr_activate(intel_dp);
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> @@ -880,7 +882,7 @@ static void vlv_psr_exit(struct intel_dp *intel_dp, bool disabling)
>  			   DP_SET_POWER_D0);
>  }
>  
> -static void intel_psr_exit(struct drm_i915_private *dev_priv)
> +static void __intel_psr_exit(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_dp *intel_dp = dev_priv->psr.enabled;
>  
> @@ -977,7 +979,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
>  
>  	if (frontbuffer_bits)
> -		intel_psr_exit(dev_priv);
> +		__intel_psr_exit(dev_priv);
>  
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> @@ -1023,7 +1025,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	if (frontbuffer_bits) {
>  		if (dev_priv->psr.psr2_enabled ||
>  		    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -			intel_psr_exit(dev_priv);
> +			__intel_psr_exit(dev_priv);
>  		} else {
>  			/*
>  			 * Display WA #0884: all
> @@ -1041,7 +1043,7 @@ 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));
> +					      msecs_to_jiffies(ACTIVATE_WORK_MSEC_DELAY));
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1108,3 +1110,81 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  		dev_priv->psr.exit = hsw_psr_exit;
>  	}
>  }
> +
> +/**
> + * intel_psr_exit - Exit PSR
> + * @intel_dp: DisplayPort that should have PSR exited if active
> + * @wait_idle: if true wait until PSR is inactive otherwise just request it
> + *
> + * This function exit PSR if enabled and active in this DisplayPort.
> + */
> +void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle)
> +{
> +	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);
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (dev_priv->psr.enabled != intel_dp)
> +		goto out;
> +	if (!dev_priv->psr.active && !wait_idle)
> +		goto out;
> +
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		struct drm_crtc *crtc = dig_port->base.base.crtc;
> +		enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +
> +		__intel_psr_exit(dev_priv);
> +
> +		if (!wait_idle)
> +			goto out;
> +
> +		if (intel_wait_for_register(dev_priv, VLV_PSRSTAT(pipe),
> +					    VLV_EDP_PSR_IN_TRANS, 0, 2000))
> +			DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +	} else {
> +		/* HSW+ PSR disable is actually, exit + wait transition */
> +		dev_priv->psr.exit(intel_dp, wait_idle);
> +		dev_priv->psr.active = false;
> +	}
> +
> +out:
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
> + * intel_psr_activate - Activate PSR
> + * @intel_dp: DisplayPort that should have PSR activated if enabled
> + * @schedule: if true schedule the active to happens in a few milliseconds
> + * otherwise activate right now.
> + *
> + * This function activate PSR if enabled in this DisplayPort.
> + */
> +void intel_psr_activate(struct intel_dp *intel_dp, bool schedule)
> +{
> +	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);
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (dev_priv->psr.enabled != intel_dp || dev_priv->psr.active)
> +		goto out;
> +
> +	if (dev_priv->psr.busy_frontbuffer_bits)
> +		goto out;
> +
> +	if (schedule) {
> +		if (!work_busy(&dev_priv->psr.work.work))
> +			schedule_delayed_work(&dev_priv->psr.work,
> +					      msecs_to_jiffies(ACTIVATE_WORK_MSEC_DELAY));
> +	} else
> +		__intel_psr_activate(intel_dp);
> +out:
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/11] drm/i915/psr: Add intel_psr_activate_block_get()/put()

[José Roberto de Souza]

intel_psr_activate_block_get() should be called when by some reason
PSR should not be activated for some time, it will increment counter
and it should the decremented by intel_psr_activate_block_put()
when PSR can be activated again.
intel_psr_activate_block_put() will not actually activate PSR, users
of this function should also call intel_psr_activate().

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>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_psr.c | 54 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 99af9169d792..41ebb144594e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -609,6 +609,7 @@ struct i915_psr {
 	bool has_hw_tracking;
 	bool psr2_enabled;
 	u8 sink_sync_latency;
+	unsigned int activate_block_count;
 
 	void (*enable_source)(struct intel_dp *,
 			      const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 70026b772721..020b96324135 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1893,6 +1893,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 			      struct intel_crtc_state *crtc_state);
 void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
 void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
+void intel_psr_activate_block_get(struct intel_dp *intel_dp);
+void intel_psr_activate_block_put(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 906a12ea934d..8702dbafb42d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -558,6 +558,8 @@ static void __intel_psr_activate(struct intel_dp *intel_dp)
 
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
+	if (dev_priv->psr.activate_block_count)
+		return;
 
 	dev_priv->psr.activate(intel_dp);
 	dev_priv->psr.active = true;
@@ -1188,3 +1190,55 @@ void intel_psr_activate(struct intel_dp *intel_dp, bool schedule)
 out:
 	mutex_unlock(&dev_priv->psr.lock);
 }
+
+/**
+ * intel_psr_activate_block_get - Block further attempts to activate PSR
+ * @intel_dp: DisplayPort that have PSR enabled
+ *
+ * It have a internal reference count, so each intel_psr_activate_block_get()
+ * should have a intel_psr_activate_block_put() counterpart.
+ */
+void intel_psr_activate_block_get(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);
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (dev_priv->psr.enabled != intel_dp)
+		goto out;
+
+	dev_priv->psr.activate_block_count++;
+out:
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+
+/**
+ * intel_psr_activate_block_put - Unblock further attempts to activate PSR
+ * @intel_dp: DisplayPort that have PSR enabled
+ *
+ * Decrease the reference counter incremented by intel_psr_activate_block_get()
+ * when zero it allows PSR be activate. To actually activate PSR when reference
+ * counter is zero intel_psr_activate() should be called.
+ */
+void intel_psr_activate_block_put(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);
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (dev_priv->psr.enabled != intel_dp)
+		goto out;
+
+	dev_priv->psr.activate_block_count--;
+out:
+	mutex_unlock(&dev_priv->psr.lock);
+}
-- 
2.16.3

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

Re: [PATCH 06/11] drm/i915/psr: Add intel_psr_activate_block_get()/put()

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:31PM -0700, José Roberto de Souza wrote:
> intel_psr_activate_block_get() should be called when by some reason
> PSR should not be activated for some time, it will increment counter
> and it should the decremented by intel_psr_activate_block_put()
> when PSR can be activated again.
> intel_psr_activate_block_put() will not actually activate PSR, users
> of this function should also call intel_psr_activate().

Ohh cool! you made the counter.
probably we will need to change things from mutex to spin locker.
But also the blocker functions here could already introduce the function
calls to really block and release psr.

> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_psr.c | 54 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 99af9169d792..41ebb144594e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -609,6 +609,7 @@ struct i915_psr {
>  	bool has_hw_tracking;
>  	bool psr2_enabled;
>  	u8 sink_sync_latency;
> +	unsigned int activate_block_count;
>  
>  	void (*enable_source)(struct intel_dp *,
>  			      const struct intel_crtc_state *);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 70026b772721..020b96324135 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1893,6 +1893,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state);
>  void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
>  void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
> +void intel_psr_activate_block_get(struct intel_dp *intel_dp);
> +void intel_psr_activate_block_put(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 906a12ea934d..8702dbafb42d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -558,6 +558,8 @@ static void __intel_psr_activate(struct intel_dp *intel_dp)
>  
>  	WARN_ON(dev_priv->psr.active);
>  	lockdep_assert_held(&dev_priv->psr.lock);
> +	if (dev_priv->psr.activate_block_count)
> +		return;
>  
>  	dev_priv->psr.activate(intel_dp);
>  	dev_priv->psr.active = true;
> @@ -1188,3 +1190,55 @@ void intel_psr_activate(struct intel_dp *intel_dp, bool schedule)
>  out:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> +
> +/**
> + * intel_psr_activate_block_get - Block further attempts to activate PSR
> + * @intel_dp: DisplayPort that have PSR enabled
> + *
> + * It have a internal reference count, so each intel_psr_activate_block_get()
> + * should have a intel_psr_activate_block_put() counterpart.
> + */
> +void intel_psr_activate_block_get(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);
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (dev_priv->psr.enabled != intel_dp)
> +		goto out;
> +
> +	dev_priv->psr.activate_block_count++;
> +out:
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +
> +/**
> + * intel_psr_activate_block_put - Unblock further attempts to activate PSR
> + * @intel_dp: DisplayPort that have PSR enabled
> + *
> + * Decrease the reference counter incremented by intel_psr_activate_block_get()
> + * when zero it allows PSR be activate. To actually activate PSR when reference
> + * counter is zero intel_psr_activate() should be called.
> + */
> +void intel_psr_activate_block_put(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);
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (dev_priv->psr.enabled != intel_dp)
> +		goto out;
> +
> +	dev_priv->psr.activate_block_count--;
> +out:
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/11] drm/i915/psr: Add intel_psr_activate_block_get()/put()

[Souza, Jose]

On Mon, 2018-04-02 at 11:20 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 30, 2018 at 03:23:31PM -0700, José Roberto de Souza
> wrote:
> > intel_psr_activate_block_get() should be called when by some reason
> > PSR should not be activated for some time, it will increment
> > counter
> > and it should the decremented by intel_psr_activate_block_put()
> > when PSR can be activated again.
> > intel_psr_activate_block_put() will not actually activate PSR,
> > users
> > of this function should also call intel_psr_activate().
> 
> Ohh cool! you made the counter.
> probably we will need to change things from mutex to spin locker.
> But also the blocker functions here could already introduce the
> function
> calls to really block and release psr.

Oh so drop the 'drm/i915/psr: Export intel_psr_activate/exit()' and
call PSR exit and activate from intel_psr_activate_block_get()/put()?

I dind't understand why you want to change struct mutex lock; to
spinlock_t?

> 
> > 
> > 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>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c | 54
> > ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 57 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 99af9169d792..41ebb144594e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -609,6 +609,7 @@ struct i915_psr {
> >  	bool has_hw_tracking;
> >  	bool psr2_enabled;
> >  	u8 sink_sync_latency;
> > +	unsigned int activate_block_count;
> >  
> >  	void (*enable_source)(struct intel_dp *,
> >  			      const struct intel_crtc_state *);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 70026b772721..020b96324135 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1893,6 +1893,8 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  			      struct intel_crtc_state
> > *crtc_state);
> >  void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
> >  void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
> > +void intel_psr_activate_block_get(struct intel_dp *intel_dp);
> > +void intel_psr_activate_block_put(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 906a12ea934d..8702dbafb42d 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -558,6 +558,8 @@ static void __intel_psr_activate(struct
> > intel_dp *intel_dp)
> >  
> >  	WARN_ON(dev_priv->psr.active);
> >  	lockdep_assert_held(&dev_priv->psr.lock);
> > +	if (dev_priv->psr.activate_block_count)
> > +		return;
> >  
> >  	dev_priv->psr.activate(intel_dp);
> >  	dev_priv->psr.active = true;
> > @@ -1188,3 +1190,55 @@ void intel_psr_activate(struct intel_dp
> > *intel_dp, bool schedule)
> >  out:
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> > +
> > +/**
> > + * intel_psr_activate_block_get - Block further attempts to
> > activate PSR
> > + * @intel_dp: DisplayPort that have PSR enabled
> > + *
> > + * It have a internal reference count, so each
> > intel_psr_activate_block_get()
> > + * should have a intel_psr_activate_block_put() counterpart.
> > + */
> > +void intel_psr_activate_block_get(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);
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +	if (dev_priv->psr.enabled != intel_dp)
> > +		goto out;
> > +
> > +	dev_priv->psr.activate_block_count++;
> > +out:
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > +
> > +/**
> > + * intel_psr_activate_block_put - Unblock further attempts to
> > activate PSR
> > + * @intel_dp: DisplayPort that have PSR enabled
> > + *
> > + * Decrease the reference counter incremented by
> > intel_psr_activate_block_get()
> > + * when zero it allows PSR be activate. To actually activate PSR
> > when reference
> > + * counter is zero intel_psr_activate() should be called.
> > + */
> > +void intel_psr_activate_block_put(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);
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +	if (dev_priv->psr.enabled != intel_dp)
> > +		goto out;
> > +
> > +	dev_priv->psr.activate_block_count--;
> > +out:
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > -- 
> > 2.16.3
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/11] drm/i915/psr: Add intel_psr_activate_block_get()/put()

[Rodrigo Vivi]

On Mon, Apr 02, 2018 at 03:11:54PM -0700, Souza, Jose wrote:
> On Mon, 2018-04-02 at 11:20 -0700, Rodrigo Vivi wrote:
> > On Fri, Mar 30, 2018 at 03:23:31PM -0700, José Roberto de Souza
> > wrote:
> > > intel_psr_activate_block_get() should be called when by some reason
> > > PSR should not be activated for some time, it will increment
> > > counter
> > > and it should the decremented by intel_psr_activate_block_put()
> > > when PSR can be activated again.
> > > intel_psr_activate_block_put() will not actually activate PSR,
> > > users
> > > of this function should also call intel_psr_activate().
> > 
> > Ohh cool! you made the counter.
> > probably we will need to change things from mutex to spin locker.
> > But also the blocker functions here could already introduce the
> > function
> > calls to really block and release psr.
> 
> Oh so drop the 'drm/i915/psr: Export intel_psr_activate/exit()' and
> call PSR exit and activate from intel_psr_activate_block_get()/put()?

yeap.

> 
> I dind't understand why you want to change struct mutex lock; to
> spinlock_t?

I'm not sure, but I believe that aux transactions can be called from
atomic areas protected with spin locks... if this assumption is true
than you cannot use any code that can sleep inside these areas, and
mutex can sleep. But in case no aux transaction is getting called
from atomic areas feel free to just ignore me ;)

> 
> > 
> > > 
> > > 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>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > >  drivers/gpu/drm/i915/intel_psr.c | 54
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 57 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 99af9169d792..41ebb144594e 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -609,6 +609,7 @@ struct i915_psr {
> > >  	bool has_hw_tracking;
> > >  	bool psr2_enabled;
> > >  	u8 sink_sync_latency;
> > > +	unsigned int activate_block_count;
> > >  
> > >  	void (*enable_source)(struct intel_dp *,
> > >  			      const struct intel_crtc_state *);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 70026b772721..020b96324135 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1893,6 +1893,8 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >  			      struct intel_crtc_state
> > > *crtc_state);
> > >  void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
> > >  void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
> > > +void intel_psr_activate_block_get(struct intel_dp *intel_dp);
> > > +void intel_psr_activate_block_put(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 906a12ea934d..8702dbafb42d 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -558,6 +558,8 @@ static void __intel_psr_activate(struct
> > > intel_dp *intel_dp)
> > >  
> > >  	WARN_ON(dev_priv->psr.active);
> > >  	lockdep_assert_held(&dev_priv->psr.lock);
> > > +	if (dev_priv->psr.activate_block_count)
> > > +		return;
> > >  
> > >  	dev_priv->psr.activate(intel_dp);
> > >  	dev_priv->psr.active = true;
> > > @@ -1188,3 +1190,55 @@ void intel_psr_activate(struct intel_dp
> > > *intel_dp, bool schedule)
> > >  out:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > > +
> > > +/**
> > > + * intel_psr_activate_block_get - Block further attempts to
> > > activate PSR
> > > + * @intel_dp: DisplayPort that have PSR enabled
> > > + *
> > > + * It have a internal reference count, so each
> > > intel_psr_activate_block_get()
> > > + * should have a intel_psr_activate_block_put() counterpart.
> > > + */
> > > +void intel_psr_activate_block_get(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);
> > > +
> > > +	if (!CAN_PSR(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +	if (dev_priv->psr.enabled != intel_dp)
> > > +		goto out;
> > > +
> > > +	dev_priv->psr.activate_block_count++;
> > > +out:
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > +
> > > +
> > > +/**
> > > + * intel_psr_activate_block_put - Unblock further attempts to
> > > activate PSR
> > > + * @intel_dp: DisplayPort that have PSR enabled
> > > + *
> > > + * Decrease the reference counter incremented by
> > > intel_psr_activate_block_get()
> > > + * when zero it allows PSR be activate. To actually activate PSR
> > > when reference
> > > + * counter is zero intel_psr_activate() should be called.
> > > + */
> > > +void intel_psr_activate_block_put(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);
> > > +
> > > +	if (!CAN_PSR(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +	if (dev_priv->psr.enabled != intel_dp)
> > > +		goto out;
> > > +
> > > +	dev_priv->psr.activate_block_count--;
> > > +out:
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > -- 
> > > 2.16.3
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[José Roberto de Souza]

When PSR/PSR2 is enabled hardware can do aux ch transactions by it
self.
Spec requires that PSR is inactive before do any aux ch transaction
in HSW and BDW, for skl+ there is a aux ch mutex but the use is not
recommended.
So exiting PSR/PSR2 and waiting the transition to inactive to prevent
any concurrent access between hardware and software in aux ch
registers.

VLV and CHV hardware don't do any aux as software is responsible to
do all the buffer tracking and it sends the wake up aux ch message
to sink.

BSpec: 7530, 799 and 7532

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
 3 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..fedee4e7ed24 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
 	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
 }
 
+/**
+ * intel_dp_aux_ch_get - Get total control of aux ch registers
+ *
+ * By exiting or disabling any hardware feature that can also use the aux ch
+ * registers at the same time as driver, this function will give total control
+ * of aux ch registers to driver.
+ */
+static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
+{
+	if (!intel_dp->exit_psr_on_aux_ch_xfer)
+		return;
+
+	intel_psr_activate_block_get(intel_dp);
+	intel_psr_exit(intel_dp, true);
+}
+
+/**
+ * intel_dp_aux_ch_put - Release aux ch registers control
+ *
+ * It is the intel_dp_aux_ch_get() counterpart.
+ */
+static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
+{
+	if (!intel_dp->exit_psr_on_aux_ch_xfer)
+		return;
+
+	intel_psr_activate_block_put(intel_dp);
+	/* Usually more than just one aux ch transaction is executed when
+	 * handling some event, activating PSR right way would cause several
+	 * msecs of delay waiting PSR to exit for each aux ch transaction, so
+	 * here asking it to be scheduled.
+	 */
+	intel_psr_activate(intel_dp, true);
+}
+
 static int
 intel_dp_aux_xfer(struct intel_dp *intel_dp,
 		  const uint8_t *send, int send_bytes,
@@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
+	intel_dp_aux_ch_get(intel_dp);
+
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
 		status = I915_READ_NOTRACE(ch_ctl);
@@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 
 	ret = recv_bytes;
 out:
+	intel_dp_aux_ch_put(intel_dp);
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
 
 	if (vdd)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 020b96324135..177478f0b032 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1139,6 +1139,8 @@ struct intel_dp {
 
 	/* Displayport compliance testing */
 	struct intel_dp_compliance compliance;
+
+	bool exit_psr_on_aux_ch_xfer;
 };
 
 struct intel_lspcon {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 8702dbafb42d..f88f12246a23 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
 	}
 
+	/* From all platforms that supports PSR/PSR2 this 2 is the ones that
+	 * don't have restrictions about use of the aux ch while PSR/PSR2 is
+	 * enabled.
+	 */
+	if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
+		intel_dp->exit_psr_on_aux_ch_xfer = true;
+
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
 }
@@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 
 	dev_priv->psr.disable_source(intel_dp);
 
+	intel_dp->exit_psr_on_aux_ch_xfer = false;
 	/* Disable PSR on Sink */
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
 
-- 
2.16.3

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

Re: [PATCH 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza wrote:
> When PSR/PSR2 is enabled hardware can do aux ch transactions by it
> self.
> Spec requires that PSR is inactive before do any aux ch transaction
> in HSW and BDW, for skl+ there is a aux ch mutex but the use is not
> recommended.
> So exiting PSR/PSR2 and waiting the transition to inactive to prevent
> any concurrent access between hardware and software in aux ch
> registers.
> 
> VLV and CHV hardware don't do any aux as software is responsible to
> do all the buffer tracking and it sends the wake up aux ch message
> to sink.
>

ahh cool... I get back what I said on some previous patch.
I like where it is going, but...

> BSpec: 7530, 799 and 7532
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..fedee4e7ed24 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
>  }
>  
> +/**
> + * intel_dp_aux_ch_get - Get total control of aux ch registers
> + *
> + * By exiting or disabling any hardware feature that can also use the aux ch
> + * registers at the same time as driver, this function will give total control
> + * of aux ch registers to driver.
> + */
> +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
> +{
> +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> +		return;
> +
> +	intel_psr_activate_block_get(intel_dp);
> +	intel_psr_exit(intel_dp, true);

decision on exit and activate should be inside the block_get and block_put,
based on current state of the counters.

> +}
> +
> +/**
> + * intel_dp_aux_ch_put - Release aux ch registers control
> + *
> + * It is the intel_dp_aux_ch_get() counterpart.
> + */
> +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
> +{
> +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> +		return;
> +
> +	intel_psr_activate_block_put(intel_dp);
> +	/* Usually more than just one aux ch transaction is executed when
> +	 * handling some event, activating PSR right way would cause several
> +	 * msecs of delay waiting PSR to exit for each aux ch transaction, so
> +	 * here asking it to be scheduled.
> +	 */
> +	intel_psr_activate(intel_dp, true);
> +}
> +
>  static int
>  intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  		  const uint8_t *send, int send_bytes,
> @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> +	intel_dp_aux_ch_get(intel_dp);
> +
>  	/* Try to wait for any previous AUX channel activity */
>  	for (try = 0; try < 3; try++) {
>  		status = I915_READ_NOTRACE(ch_ctl);
> @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  
>  	ret = recv_bytes;
>  out:
> +	intel_dp_aux_ch_put(intel_dp);
>  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>  
>  	if (vdd)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 020b96324135..177478f0b032 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1139,6 +1139,8 @@ struct intel_dp {
>  
>  	/* Displayport compliance testing */
>  	struct intel_dp_compliance compliance;
> +
> +	bool exit_psr_on_aux_ch_xfer;
>  };
>  
>  struct intel_lspcon {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 8702dbafb42d..f88f12246a23 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
>  	}
>  
> +	/* From all platforms that supports PSR/PSR2 this 2 is the ones that
> +	 * don't have restrictions about use of the aux ch while PSR/PSR2 is
> +	 * enabled.
> +	 */
> +	if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
> +		intel_dp->exit_psr_on_aux_ch_xfer = true;
> +
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  
>  	dev_priv->psr.disable_source(intel_dp);
>  
> +	intel_dp->exit_psr_on_aux_ch_xfer = false;
>  	/* Disable PSR on Sink */
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
>  
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[Pandiyan, Dhinakaran]

On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza wrote:
> > When PSR/PSR2 is enabled hardware can do aux ch transactions by it
> > self.
> > Spec requires that PSR is inactive before do any aux ch transaction
> > in HSW and BDW, for skl+ there is a aux ch mutex but the use is not
> > recommended.
> > So exiting PSR/PSR2 and waiting the transition to inactive to prevent
> > any concurrent access between hardware and software in aux ch
> > registers.
> > 
> > VLV and CHV hardware don't do any aux as software is responsible to
> > do all the buffer tracking and it sends the wake up aux ch message
> > to sink.
> >
> 
> ahh cool... I get back what I said on some previous patch.
> I like where it is going, but...
> 
> > BSpec: 7530, 799 and 7532
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 38 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
> >  3 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4298ac..fedee4e7ed24 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> >  }
> >  
> > +/**
> > + * intel_dp_aux_ch_get - Get total control of aux ch registers
> > + *
> > + * By exiting or disabling any hardware feature that can also use the aux ch
> > + * registers at the same time as driver, this function will give total control
> > + * of aux ch registers to driver.
> > + */
> > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
> > +{
> > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > +		return;
> > +
> > +	intel_psr_activate_block_get(intel_dp);
> > +	intel_psr_exit(intel_dp, true);
> 
> decision on exit and activate should be inside the block_get and block_put,
> based on current state of the counters.
> 

I haven't read the patches yet, but you could use the kref infrastructure for 
reference counting.

> > +}
> > +
> > +/**
> > + * intel_dp_aux_ch_put - Release aux ch registers control
> > + *
> > + * It is the intel_dp_aux_ch_get() counterpart.
> > + */
> > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
> > +{
> > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > +		return;
> > +
> > +	intel_psr_activate_block_put(intel_dp);
> > +	/* Usually more than just one aux ch transaction is executed when
> > +	 * handling some event, activating PSR right way would cause several
> > +	 * msecs of delay waiting PSR to exit for each aux ch transaction, so
> > +	 * here asking it to be scheduled.
> > +	 */
> > +	intel_psr_activate(intel_dp, true);
> > +}
> > +
> >  static int
> >  intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  		  const uint8_t *send, int send_bytes,
> > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > +	intel_dp_aux_ch_get(intel_dp);
> > +
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> >  		status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  
> >  	ret = recv_bytes;
> >  out:
> > +	intel_dp_aux_ch_put(intel_dp);
> >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >  
> >  	if (vdd)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 020b96324135..177478f0b032 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1139,6 +1139,8 @@ struct intel_dp {
> >  
> >  	/* Displayport compliance testing */
> >  	struct intel_dp_compliance compliance;
> > +
> > +	bool exit_psr_on_aux_ch_xfer;
> >  };
> >  
> >  struct intel_lspcon {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 8702dbafb42d..f88f12246a23 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
> >  	}
> >  
> > +	/* From all platforms that supports PSR/PSR2 this 2 is the ones that
> > +	 * don't have restrictions about use of the aux ch while PSR/PSR2 is
> > +	 * enabled.
> > +	 */
> > +	if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
> > +		intel_dp->exit_psr_on_aux_ch_xfer = true;
> > +
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >  
> >  	dev_priv->psr.disable_source(intel_dp);
> >  
> > +	intel_dp->exit_psr_on_aux_ch_xfer = false;
> >  	/* Disable PSR on Sink */
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> >  
> > -- 
> > 2.16.3
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[Souza, Jose]

On Mon, 2018-04-02 at 11:42 -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote:
> > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza
> > wrote:
> > > When PSR/PSR2 is enabled hardware can do aux ch transactions by
> > > it
> > > self.
> > > Spec requires that PSR is inactive before do any aux ch
> > > transaction
> > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is
> > > not
> > > recommended.
> > > So exiting PSR/PSR2 and waiting the transition to inactive to
> > > prevent
> > > any concurrent access between hardware and software in aux ch
> > > registers.
> > > 
> > > VLV and CHV hardware don't do any aux as software is responsible
> > > to
> > > do all the buffer tracking and it sends the wake up aux ch
> > > message
> > > to sink.
> > > 
> > 
> > ahh cool... I get back what I said on some previous patch.
> > I like where it is going, but...
> > 
> > > BSpec: 7530, 799 and 7532
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  | 38
> > > ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > >  drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
> > >  3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 62f82c4298ac..fedee4e7ed24 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1062,6 +1062,41 @@ static uint32_t
> > > skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> > >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> > >  }
> > >  
> > > +/**
> > > + * intel_dp_aux_ch_get - Get total control of aux ch registers
> > > + *
> > > + * By exiting or disabling any hardware feature that can also
> > > use the aux ch
> > > + * registers at the same time as driver, this function will give
> > > total control
> > > + * of aux ch registers to driver.
> > > + */
> > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
> > > +{
> > > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > > +		return;
> > > +
> > > +	intel_psr_activate_block_get(intel_dp);
> > > +	intel_psr_exit(intel_dp, true);
> > 
> > decision on exit and activate should be inside the block_get and
> > block_put,
> > based on current state of the counters.
> > 
> 
> I haven't read the patches yet, but you could use the kref
> infrastructure for 
> reference counting.

Cool I will use it, thanks.

> 
> > > +}
> > > +
> > > +/**
> > > + * intel_dp_aux_ch_put - Release aux ch registers control
> > > + *
> > > + * It is the intel_dp_aux_ch_get() counterpart.
> > > + */
> > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
> > > +{
> > > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > > +		return;
> > > +
> > > +	intel_psr_activate_block_put(intel_dp);
> > > +	/* Usually more than just one aux ch transaction is
> > > executed when
> > > +	 * handling some event, activating PSR right way would
> > > cause several
> > > +	 * msecs of delay waiting PSR to exit for each aux ch
> > > transaction, so
> > > +	 * here asking it to be scheduled.
> > > +	 */
> > > +	intel_psr_activate(intel_dp, true);
> > > +}
> > > +
> > >  static int
> > >  intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > >  		  const uint8_t *send, int send_bytes,
> > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > +	intel_dp_aux_ch_get(intel_dp);
> > > +
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	ret = recv_bytes;
> > >  out:
> > > +	intel_dp_aux_ch_put(intel_dp);
> > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >  
> > >  	if (vdd)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 020b96324135..177478f0b032 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1139,6 +1139,8 @@ struct intel_dp {
> > >  
> > >  	/* Displayport compliance testing */
> > >  	struct intel_dp_compliance compliance;
> > > +
> > > +	bool exit_psr_on_aux_ch_xfer;
> > >  };
> > >  
> > >  struct intel_lspcon {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 8702dbafb42d..f88f12246a23 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > >  				      msecs_to_jiffies(intel_dp-
> > > >panel_power_cycle_delay * 5));
> > >  	}
> > >  
> > > +	/* From all platforms that supports PSR/PSR2 this 2 is
> > > the ones that
> > > +	 * don't have restrictions about use of the aux ch while
> > > PSR/PSR2 is
> > > +	 * enabled.
> > > +	 */
> > > +	if (!(IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv)))
> > > +		intel_dp->exit_psr_on_aux_ch_xfer = true;
> > > +
> > >  unlock:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	dev_priv->psr.disable_source(intel_dp);
> > >  
> > > +	intel_dp->exit_psr_on_aux_ch_xfer = false;
> > >  	/* Disable PSR on Sink */
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> > >  
> > > -- 
> > > 2.16.3
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[Souza, Jose]

On Mon, 2018-04-02 at 22:16 +0000, Souza, Jose wrote:
> On Mon, 2018-04-02 at 11:42 -0700, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote:
> > > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza
> > > wrote:
> > > > When PSR/PSR2 is enabled hardware can do aux ch transactions by
> > > > it
> > > > self.
> > > > Spec requires that PSR is inactive before do any aux ch
> > > > transaction
> > > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is
> > > > not
> > > > recommended.
> > > > So exiting PSR/PSR2 and waiting the transition to inactive to
> > > > prevent
> > > > any concurrent access between hardware and software in aux ch
> > > > registers.
> > > > 
> > > > VLV and CHV hardware don't do any aux as software is
> > > > responsible
> > > > to
> > > > do all the buffer tracking and it sends the wake up aux ch
> > > > message
> > > > to sink.
> > > > 
> > > 
> > > ahh cool... I get back what I said on some previous patch.
> > > I like where it is going, but...
> > > 
> > > > BSpec: 7530, 799 and 7532
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 38
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > > >  drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
> > > >  3 files changed, 48 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 62f82c4298ac..fedee4e7ed24 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1062,6 +1062,41 @@ static uint32_t
> > > > skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> > > >  }
> > > >  
> > > > +/**
> > > > + * intel_dp_aux_ch_get - Get total control of aux ch registers
> > > > + *
> > > > + * By exiting or disabling any hardware feature that can also
> > > > use the aux ch
> > > > + * registers at the same time as driver, this function will
> > > > give
> > > > total control
> > > > + * of aux ch registers to driver.
> > > > + */
> > > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
> > > > +{
> > > > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > > > +		return;
> > > > +
> > > > +	intel_psr_activate_block_get(intel_dp);
> > > > +	intel_psr_exit(intel_dp, true);
> > > 
> > > decision on exit and activate should be inside the block_get and
> > > block_put,
> > > based on current state of the counters.
> > > 
> > 
> > I haven't read the patches yet, but you could use the kref
> > infrastructure for 
> > reference counting.
> 
> Cool I will use it, thanks.

To use kref I would need to do something like this in intel_psr_init():

kref_init(&dev_priv->psr.activate_block_kref);
/* kref_init() initialize the reference as 1, so calling kref_put() to
 * keep it as 0 but this will cause the release callback to be executed
 * but it will not be fully executed as PSR is not enabled yet.
 */
kref_put(&dev_priv->psr.activate_block_kref,
intel_psr_activate_block_release);

That is okay? keep it?

> 
> > 
> > > > +}
> > > > +
> > > > +/**
> > > > + * intel_dp_aux_ch_put - Release aux ch registers control
> > > > + *
> > > > + * It is the intel_dp_aux_ch_get() counterpart.
> > > > + */
> > > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
> > > > +{
> > > > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > > > +		return;
> > > > +
> > > > +	intel_psr_activate_block_put(intel_dp);
> > > > +	/* Usually more than just one aux ch transaction is
> > > > executed when
> > > > +	 * handling some event, activating PSR right way would
> > > > cause several
> > > > +	 * msecs of delay waiting PSR to exit for each aux ch
> > > > transaction, so
> > > > +	 * here asking it to be scheduled.
> > > > +	 */
> > > > +	intel_psr_activate(intel_dp, true);
> > > > +}
> > > > +
> > > >  static int
> > > >  intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > > >  		  const uint8_t *send, int send_bytes,
> > > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	intel_dp_check_edp(intel_dp);
> > > >  
> > > > +	intel_dp_aux_ch_get(intel_dp);
> > > > +
> > > >  	/* Try to wait for any previous AUX channel activity
> > > > */
> > > >  	for (try = 0; try < 3; try++) {
> > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	ret = recv_bytes;
> > > >  out:
> > > > +	intel_dp_aux_ch_put(intel_dp);
> > > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > > PM_QOS_DEFAULT_VALUE);
> > > >  
> > > >  	if (vdd)
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 020b96324135..177478f0b032 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1139,6 +1139,8 @@ struct intel_dp {
> > > >  
> > > >  	/* Displayport compliance testing */
> > > >  	struct intel_dp_compliance compliance;
> > > > +
> > > > +	bool exit_psr_on_aux_ch_xfer;
> > > >  };
> > > >  
> > > >  struct intel_lspcon {
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 8702dbafb42d..f88f12246a23 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > >  				      msecs_to_jiffies(intel_d
> > > > p-
> > > > > panel_power_cycle_delay * 5));
> > > > 
> > > >  	}
> > > >  
> > > > +	/* From all platforms that supports PSR/PSR2 this 2 is
> > > > the ones that
> > > > +	 * don't have restrictions about use of the aux ch
> > > > while
> > > > PSR/PSR2 is
> > > > +	 * enabled.
> > > > +	 */
> > > > +	if (!(IS_VALLEYVIEW(dev_priv) ||
> > > > IS_CHERRYVIEW(dev_priv)))
> > > > +		intel_dp->exit_psr_on_aux_ch_xfer = true;
> > > > +
> > > >  unlock:
> > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > >  }
> > > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	dev_priv->psr.disable_source(intel_dp);
> > > >  
> > > > +	intel_dp->exit_psr_on_aux_ch_xfer = false;
> > > >  	/* Disable PSR on Sink */
> > > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> > > >  
> > > > -- 
> > > > 2.16.3
> > > > 
> 
> _______________________________________________
> 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 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[Pandiyan, Dhinakaran]

On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza wrote:
> > When PSR/PSR2 is enabled hardware can do aux ch transactions by it
> > self.
> > Spec requires that PSR is inactive before do any aux ch transaction
> > in HSW and BDW, for skl+ there is a aux ch mutex but the use is not
> > recommended.
> > So exiting PSR/PSR2 and waiting the transition to inactive to prevent
> > any concurrent access between hardware and software in aux ch
> > registers.

Wouldn't the debugfs interface that you added to read sink PSR status
always show PSR as disabled? Or do you intend to set
intel_dp->exit_psr_on_aux_ch_xfer = true? 


> > 
> > VLV and CHV hardware don't do any aux as software is responsible to
> > do all the buffer tracking and it sends the wake up aux ch message
> > to sink.
> >
> 
> ahh cool... I get back what I said on some previous patch.
> I like where it is going, but...
> 
> > BSpec: 7530, 799 and 7532
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 38 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
> >  3 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4298ac..fedee4e7ed24 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> >  }
> >  
> > +/**
> > + * intel_dp_aux_ch_get - Get total control of aux ch registers
> > + *
> > + * By exiting or disabling any hardware feature that can also use the aux ch
> > + * registers at the same time as driver, this function will give total control
> > + * of aux ch registers to driver.
> > + */
> > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
> > +{
> > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > +		return;
> > +
> > +	intel_psr_activate_block_get(intel_dp);
> > +	intel_psr_exit(intel_dp, true);
> 
> decision on exit and activate should be inside the block_get and block_put,
> based on current state of the counters.
> 
> > +}
> > +
> > +/**
> > + * intel_dp_aux_ch_put - Release aux ch registers control
> > + *
> > + * It is the intel_dp_aux_ch_get() counterpart.
> > + */
> > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
> > +{
> > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > +		return;
> > +
> > +	intel_psr_activate_block_put(intel_dp);
> > +	/* Usually more than just one aux ch transaction is executed when
> > +	 * handling some event, activating PSR right way would cause several
> > +	 * msecs of delay waiting PSR to exit for each aux ch transaction, so
> > +	 * here asking it to be scheduled.
> > +	 */
> > +	intel_psr_activate(intel_dp, true);
> > +}
> > +
> >  static int
> >  intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  		  const uint8_t *send, int send_bytes,
> > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > +	intel_dp_aux_ch_get(intel_dp);
> > +
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> >  		status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >  
> >  	ret = recv_bytes;
> >  out:
> > +	intel_dp_aux_ch_put(intel_dp);
> >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >  
> >  	if (vdd)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 020b96324135..177478f0b032 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1139,6 +1139,8 @@ struct intel_dp {
> >  
> >  	/* Displayport compliance testing */
> >  	struct intel_dp_compliance compliance;
> > +
> > +	bool exit_psr_on_aux_ch_xfer;
> >  };
> >  
> >  struct intel_lspcon {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 8702dbafb42d..f88f12246a23 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
> >  	}
> >  
> > +	/* From all platforms that supports PSR/PSR2 this 2 is the ones that
> > +	 * don't have restrictions about use of the aux ch while PSR/PSR2 is
> > +	 * enabled.
> > +	 */
> > +	if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))
> > +		intel_dp->exit_psr_on_aux_ch_xfer = true;
> > +
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >  
> >  	dev_priv->psr.disable_source(intel_dp);
> >  
> > +	intel_dp->exit_psr_on_aux_ch_xfer = false;
> >  	/* Disable PSR on Sink */
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> >  
> > -- 
> > 2.16.3
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/11] drm/i915/dp: Exit PSR before do a aux channel transaction

[Souza, Jose]

On Mon, 2018-04-02 at 12:00 -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote:
> > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza
> > wrote:
> > > When PSR/PSR2 is enabled hardware can do aux ch transactions by
> > > it
> > > self.
> > > Spec requires that PSR is inactive before do any aux ch
> > > transaction
> > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is
> > > not
> > > recommended.
> > > So exiting PSR/PSR2 and waiting the transition to inactive to
> > > prevent
> > > any concurrent access between hardware and software in aux ch
> > > registers.
> 
> Wouldn't the debugfs interface that you added to read sink PSR status
> always show PSR as disabled? Or do you intend to set
> intel_dp->exit_psr_on_aux_ch_xfer = true? 

Yeah it will always be disabled, unless when the hack in 'drm/i915:
Keep IGT PSR and frontbuffer tests functional' is set, Rodrigo asked to
remove the debugfs file that was controlling the state of the hack and
enable/disable it around CRC read so I could do that for the whole
i915_edp_psr_status too.

> 
> 
> > > 
> > > VLV and CHV hardware don't do any aux as software is responsible
> > > to
> > > do all the buffer tracking and it sends the wake up aux ch
> > > message
> > > to sink.
> > > 
> > 
> > ahh cool... I get back what I said on some previous patch.
> > I like where it is going, but...
> > 
> > > BSpec: 7530, 799 and 7532
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  | 38
> > > ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > >  drivers/gpu/drm/i915/intel_psr.c |  8 ++++++++
> > >  3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 62f82c4298ac..fedee4e7ed24 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1062,6 +1062,41 @@ static uint32_t
> > > skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> > >  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> > >  }
> > >  
> > > +/**
> > > + * intel_dp_aux_ch_get - Get total control of aux ch registers
> > > + *
> > > + * By exiting or disabling any hardware feature that can also
> > > use the aux ch
> > > + * registers at the same time as driver, this function will give
> > > total control
> > > + * of aux ch registers to driver.
> > > + */
> > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
> > > +{
> > > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > > +		return;
> > > +
> > > +	intel_psr_activate_block_get(intel_dp);
> > > +	intel_psr_exit(intel_dp, true);
> > 
> > decision on exit and activate should be inside the block_get and
> > block_put,
> > based on current state of the counters.
> > 
> > > +}
> > > +
> > > +/**
> > > + * intel_dp_aux_ch_put - Release aux ch registers control
> > > + *
> > > + * It is the intel_dp_aux_ch_get() counterpart.
> > > + */
> > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp)
> > > +{
> > > +	if (!intel_dp->exit_psr_on_aux_ch_xfer)
> > > +		return;
> > > +
> > > +	intel_psr_activate_block_put(intel_dp);
> > > +	/* Usually more than just one aux ch transaction is
> > > executed when
> > > +	 * handling some event, activating PSR right way would
> > > cause several
> > > +	 * msecs of delay waiting PSR to exit for each aux ch
> > > transaction, so
> > > +	 * here asking it to be scheduled.
> > > +	 */
> > > +	intel_psr_activate(intel_dp, true);
> > > +}
> > > +
> > >  static int
> > >  intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > >  		  const uint8_t *send, int send_bytes,
> > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > +	intel_dp_aux_ch_get(intel_dp);
> > > +
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	ret = recv_bytes;
> > >  out:
> > > +	intel_dp_aux_ch_put(intel_dp);
> > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >  
> > >  	if (vdd)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 020b96324135..177478f0b032 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1139,6 +1139,8 @@ struct intel_dp {
> > >  
> > >  	/* Displayport compliance testing */
> > >  	struct intel_dp_compliance compliance;
> > > +
> > > +	bool exit_psr_on_aux_ch_xfer;
> > >  };
> > >  
> > >  struct intel_lspcon {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 8702dbafb42d..f88f12246a23 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > >  				      msecs_to_jiffies(intel_dp-
> > > >panel_power_cycle_delay * 5));
> > >  	}
> > >  
> > > +	/* From all platforms that supports PSR/PSR2 this 2 is
> > > the ones that
> > > +	 * don't have restrictions about use of the aux ch while
> > > PSR/PSR2 is
> > > +	 * enabled.
> > > +	 */
> > > +	if (!(IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv)))
> > > +		intel_dp->exit_psr_on_aux_ch_xfer = true;
> > > +
> > >  unlock:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	dev_priv->psr.disable_source(intel_dp);
> > >  
> > > +	intel_dp->exit_psr_on_aux_ch_xfer = false;
> > >  	/* Disable PSR on Sink */
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> > >  
> > > -- 
> > > 2.16.3
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/11] drm/i915: Keep IGT PSR and frontbuffer tests functional

[José Roberto de Souza]

'drm/i915/dp: Exit PSR before do a aux channel transaction' cause all
IGT PSR and frontbuffer tracking tests to not be userful.
Those tests depend in reading the calculated CRC value of the
frontbuffer in sink and doing a aux transaction now causes the PSR
to exit.
So any bug in software and hardware buffer tracking will be masked by
this forced PSR exit.

This is a dirty workaround that makes those tests functional again
at the risk of causing concurrent access in the aux ch registers
while running_igt_psr_tests is set.

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_dp.c     | 18 ++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1dba2c451255..519f67598d44 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4732,6 +4732,27 @@ static int i915_drrs_ctl_set(void *data, u64 val)
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_drrs_ctl_fops, NULL, i915_drrs_ctl_set, "%llu\n");
 
+static int i915_running_igt_psr_tests_get(void *data, u64 *val)
+{
+	struct drm_i915_private *dev_priv = data;
+
+	*val = dev_priv->running_igt_psr_tests;
+	return 0;
+}
+
+static int i915_running_igt_psr_tests_set(void *data, u64 val)
+{
+	struct drm_i915_private *dev_priv = data;
+
+	dev_priv->running_igt_psr_tests = !!val;
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_running_igt_psr_tests_fops,
+			i915_running_igt_psr_tests_get,
+			i915_running_igt_psr_tests_set,
+			"%llu\n");
+
 static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -4812,7 +4833,8 @@ static const struct i915_debugfs_files {
 	{"i915_guc_log_relay", &i915_guc_log_relay_fops},
 	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
 	{"i915_ipc_status", &i915_ipc_status_fops},
-	{"i915_drrs_ctl", &i915_drrs_ctl_fops}
+	{"i915_drrs_ctl", &i915_drrs_ctl_fops},
+	{"i915_running_igt_psr_tests", &i915_running_igt_psr_tests_fops}
 };
 
 int i915_debugfs_register(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 41ebb144594e..9e0a5e29f48e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2110,6 +2110,8 @@ struct drm_i915_private {
 
 	struct i915_pmu pmu;
 
+	bool running_igt_psr_tests;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fedee4e7ed24..c655f6c08a02 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1071,8 +1071,20 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
  */
 static void intel_dp_aux_ch_get(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);
+
 	if (!intel_dp->exit_psr_on_aux_ch_xfer)
 		return;
+	/*
+	 * HACK: This is a workaround to keep IGT PSR and frontbuffer tracking
+	 * tests functional, otherwise when IGT request the CRC of the
+	 * frontbuffer to sink it would cause this function to complete execute
+	 * and exit PSR.
+	 */
+	if (dev_priv->running_igt_psr_tests)
+		return;
 
 	intel_psr_activate_block_get(intel_dp);
 	intel_psr_exit(intel_dp, true);
@@ -1085,8 +1097,14 @@ static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
  */
 static void intel_dp_aux_ch_put(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);
+
 	if (!intel_dp->exit_psr_on_aux_ch_xfer)
 		return;
+	if (dev_priv->running_igt_psr_tests)
+		return;
 
 	intel_psr_activate_block_put(intel_dp);
 	/* Usually more than just one aux ch transaction is executed when
-- 
2.16.3

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

Re: [PATCH 08/11] drm/i915: Keep IGT PSR and frontbuffer tests functional

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:33PM -0700, José Roberto de Souza wrote:
> 'drm/i915/dp: Exit PSR before do a aux channel transaction' cause all
> IGT PSR and frontbuffer tracking tests to not be userful.
> Those tests depend in reading the calculated CRC value of the
> frontbuffer in sink and doing a aux transaction now causes the PSR
> to exit.
> So any bug in software and hardware buffer tracking will be masked by
> this forced PSR exit.
> 
> This is a dirty workaround that makes those tests functional again
> at the risk of causing concurrent access in the aux ch registers
> while running_igt_psr_tests is set.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c     | 18 ++++++++++++++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1dba2c451255..519f67598d44 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4732,6 +4732,27 @@ static int i915_drrs_ctl_set(void *data, u64 val)
>  
>  DEFINE_SIMPLE_ATTRIBUTE(i915_drrs_ctl_fops, NULL, i915_drrs_ctl_set, "%llu\n");
>  
> +static int i915_running_igt_psr_tests_get(void *data, u64 *val)
> +{
> +	struct drm_i915_private *dev_priv = data;
> +
> +	*val = dev_priv->running_igt_psr_tests;
> +	return 0;
> +}
> +
> +static int i915_running_igt_psr_tests_set(void *data, u64 val)
> +{
> +	struct drm_i915_private *dev_priv = data;
> +
> +	dev_priv->running_igt_psr_tests = !!val;
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_running_igt_psr_tests_fops,
> +			i915_running_igt_psr_tests_get,
> +			i915_running_igt_psr_tests_set,
> +			"%llu\n");
> +
>  static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_capabilities", i915_capabilities, 0},
>  	{"i915_gem_objects", i915_gem_object_info, 0},
> @@ -4812,7 +4833,8 @@ static const struct i915_debugfs_files {
>  	{"i915_guc_log_relay", &i915_guc_log_relay_fops},
>  	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
>  	{"i915_ipc_status", &i915_ipc_status_fops},
> -	{"i915_drrs_ctl", &i915_drrs_ctl_fops}
> +	{"i915_drrs_ctl", &i915_drrs_ctl_fops},
> +	{"i915_running_igt_psr_tests", &i915_running_igt_psr_tests_fops}

we don't need new ops. All we want is to block the exit when running
sink_crc tests, so add the blocker wa there...

>  };
>  
>  int i915_debugfs_register(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 41ebb144594e..9e0a5e29f48e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2110,6 +2110,8 @@ struct drm_i915_private {
>  
>  	struct i915_pmu pmu;
>  
> +	bool running_igt_psr_tests;
> +
>  	/*
>  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>  	 * will be rejected. Instead look for a better place.
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fedee4e7ed24..c655f6c08a02 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1071,8 +1071,20 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>   */
>  static void intel_dp_aux_ch_get(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);
> +
>  	if (!intel_dp->exit_psr_on_aux_ch_xfer)
>  		return;
> +	/*
> +	 * HACK: This is a workaround to keep IGT PSR and frontbuffer tracking
> +	 * tests functional, otherwise when IGT request the CRC of the
> +	 * frontbuffer to sink it would cause this function to complete execute
> +	 * and exit PSR.
> +	 */
> +	if (dev_priv->running_igt_psr_tests)
> +		return;
>  
>  	intel_psr_activate_block_get(intel_dp);
>  	intel_psr_exit(intel_dp, true);
> @@ -1085,8 +1097,14 @@ static void intel_dp_aux_ch_get(struct intel_dp *intel_dp)
>   */
>  static void intel_dp_aux_ch_put(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);
> +
>  	if (!intel_dp->exit_psr_on_aux_ch_xfer)
>  		return;
> +	if (dev_priv->running_igt_psr_tests)
> +		return;
>  
>  	intel_psr_activate_block_put(intel_dp);
>  	/* Usually more than just one aux ch transaction is executed when
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/11] 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>
---
 drivers/gpu/drm/i915/intel_dp.c  |  2 ++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 31 +++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c655f6c08a02..fc8b86bc0120 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4518,6 +4518,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 177478f0b032..d4c7e1e0fb86 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1897,6 +1897,7 @@ void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
 void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
 void intel_psr_activate_block_get(struct intel_dp *intel_dp);
 void intel_psr_activate_block_put(struct intel_dp *intel_dp);
+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 f88f12246a23..85d201fce287 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1250,3 +1250,34 @@ void intel_psr_activate_block_put(struct intel_dp *intel_dp)
 out:
 	mutex_unlock(&dev_priv->psr.lock);
 }
+
+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))
+		return;
+
+	mutex_lock(&psr->lock);
+	if (psr->enabled != intel_dp)
+		goto out;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) {
+		DRM_DEBUG_KMS("PSR_STATUS read failed\n");
+		goto dpcd_read_error;
+	}
+
+	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR)
+		__intel_psr_exit(dev_priv);
+
+	/* TODO: handle other PSR/PSR2 errors */
+dpcd_read_error:
+	if (!dev_priv->psr.busy_frontbuffer_bits)
+		__intel_psr_activate(intel_dp);
+out:
+	mutex_unlock(&psr->lock);
+}
-- 
2.16.3

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

Re: [PATCH 09/11] drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:34PM -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>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c655f6c08a02..fc8b86bc0120 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4518,6 +4518,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 177478f0b032..d4c7e1e0fb86 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1897,6 +1897,7 @@ void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
>  void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
>  void intel_psr_activate_block_get(struct intel_dp *intel_dp);
>  void intel_psr_activate_block_put(struct intel_dp *intel_dp);
> +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 f88f12246a23..85d201fce287 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1250,3 +1250,34 @@ void intel_psr_activate_block_put(struct intel_dp *intel_dp)
>  out:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> +
> +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))
> +		return;
> +
> +	mutex_lock(&psr->lock);
> +	if (psr->enabled != intel_dp)
> +		goto out;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) {
> +		DRM_DEBUG_KMS("PSR_STATUS read failed\n");
> +		goto dpcd_read_error;
> +	}
> +
> +	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR)
> +		__intel_psr_exit(dev_priv);

shouldn't we add a debug msg here?

> +
> +	/* TODO: handle other PSR/PSR2 errors */
> +dpcd_read_error:
> +	if (!dev_priv->psr.busy_frontbuffer_bits)
> +		__intel_psr_activate(intel_dp);
> +out:
> +	mutex_unlock(&psr->lock);
> +}
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/11] 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>
---
 drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 85d201fce287..d76eece63ebd 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1274,6 +1274,16 @@ void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
 	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR)
 		__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_read_error;
+	}
+
+	if (val & DP_PSR_RFB_STORAGE_ERROR)
+		__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_read_error:
 	if (!dev_priv->psr.busy_frontbuffer_bits)
-- 
2.16.3

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

[PATCH 11/11] 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.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 176dca6554f4..3bd8f5edee87 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4001,6 +4001,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 d76eece63ebd..1f4bf079dd39 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -292,6 +292,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);
@@ -379,6 +381,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);
 }
@@ -1279,12 +1284,12 @@ void intel_psr_hpd_short_pulse_handle(struct intel_dp *intel_dp)
 		goto dpcd_read_error;
 	}
 
-	if (val & DP_PSR_RFB_STORAGE_ERROR)
+	if (val & (DP_PSR_RFB_STORAGE_ERROR | DP_PSR_LINK_CRC_ERROR))
 		__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_read_error:
 	if (!dev_priv->psr.busy_frontbuffer_bits)
 		__intel_psr_activate(intel_dp);
-- 
2.16.3

_______________________________________________
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 [01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Patchwork]

== Series Details ==

Series: series starting with [01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
URL   : https://patchwork.freedesktop.org/series/40978/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
499cc3f929a4 drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
5f0bc3f1c19d drm/i915/psr: Move PSR exit specific code to hardware specific function
00837d74661e drm/i915/psr: Share code between disable and exit
384712a1fb4a drm/i915/psr: Remove intel_crtc_state parameter from disable()
af1fc4a61d32 drm/i915/psr: Export intel_psr_activate/exit()
-:177: CHECK:BRACES: braces {} should be used on all arms of this statement
#177: FILE: drivers/gpu/drm/i915/intel_psr.c:1182:
+	if (schedule) {
[...]
+	} else
[...]

-:181: CHECK:BRACES: Unbalanced braces around else statement
#181: FILE: drivers/gpu/drm/i915/intel_psr.c:1186:
+	} else

total: 0 errors, 0 warnings, 2 checks, 153 lines checked
6d6bc90eccc3 drm/i915/psr: Add intel_psr_activate_block_get()/put()
-:88: CHECK:LINE_SPACING: Please don't use multiple blank lines
#88: FILE: drivers/gpu/drm/i915/intel_psr.c:1219:
+
+

total: 0 errors, 0 warnings, 1 checks, 78 lines checked
f42690dc389a drm/i915/dp: Exit PSR before do a aux channel transaction
ac18ae2ade2c drm/i915: Keep IGT PSR and frontbuffer tests functional
191d9682d215 drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
164ed46e42e1 drm/i915/psr: Handle PSR RFB storage error
67745738f4d0 drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
-:29: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#29: FILE: drivers/gpu/drm/i915/i915_reg.h:4004:
+#define   EDP_PSR_CRC_ENABLE			(1<<10) /* BDW+ */
                             			  ^

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

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

✗ Fi.CI.BAT: warning for series starting with [01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Patchwork]

== Series Details ==

Series: series starting with [01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
URL   : https://patchwork.freedesktop.org/series/40978/
State : warning

== Summary ==

Series 40978v1 series starting with [01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function
https://patchwork.freedesktop.org/api/1.0/series/40978/revisions/1/mbox/

---- Possible new issues:

Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> DMESG-WARN (fi-skl-6700k2)
                pass       -> DMESG-WARN (fi-skl-guc)
        Subgroup basic-flip-c:
                pass       -> DMESG-WARN (fi-kbl-7567u)

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:544s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:295s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:515s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:506s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:517s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:508s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-cfl-s3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:564s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:513s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:588s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:418s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:319s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:403s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:419s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:467s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:428s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:464s
fi-kbl-7567u     total:285  pass:264  dwarn:1   dfail:0   fail:0   skip:20  time:457s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:509s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:660s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:437s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:531s
fi-skl-6700k2    total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:508s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-skl-guc       total:285  pass:256  dwarn:1   dfail:0   fail:0   skip:28  time:428s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:565s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:407s
Blacklisted hosts:
fi-cnl-psr       total:285  pass:256  dwarn:3   dfail:0   fail:0   skip:26  time:505s
fi-glk-j4005     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:487s

c46052cde6a50c5459e00791ffc4d5aa1ec58a9e drm-tip: 2018y-03m-30d-18h-56m-26s UTC integration manifest
67745738f4d0 drm/i915/psr/bdw+: Enable CRC check in the static frame on the sink side
164ed46e42e1 drm/i915/psr: Handle PSR RFB storage error
191d9682d215 drm/i915/psr: Begin to handle PSR/PSR2 errors set by sink
ac18ae2ade2c drm/i915: Keep IGT PSR and frontbuffer tests functional
f42690dc389a drm/i915/dp: Exit PSR before do a aux channel transaction
6d6bc90eccc3 drm/i915/psr: Add intel_psr_activate_block_get()/put()
af1fc4a61d32 drm/i915/psr: Export intel_psr_activate/exit()
384712a1fb4a drm/i915/psr: Remove intel_crtc_state parameter from disable()
00837d74661e drm/i915/psr: Share code between disable and exit
5f0bc3f1c19d drm/i915/psr: Move PSR exit specific code to hardware specific function
499cc3f929a4 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_8554/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915/psr: Move specific HSW+ WARN_ON to HSW+ function

[Rodrigo Vivi]

On Fri, Mar 30, 2018 at 03:23:26PM -0700, 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>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

I'm sure that my patch [1] will conflict with this series.

[1] https://patchwork.freedesktop.org/series/39650/

But the approach here is right and I couldn't get back there
yet and address latest DK's requests. So:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_psr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2d53f7398a6d..fc7c36efd401 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.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx