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

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

When https://patchwork.freedesktop.org/patch/30/ lands, we don't
have to worry about this :)
> 
> Signed-off-by: José Roberto de Souza 
> Cc: Dhinakaran Pandiyan 
> Reviewed-by: Rodrigo Vivi 
> ---
> 
> No changes from v1.
> 
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 94 +++---
> --
>  2 files changed, 56 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 8e8667d9b084..476bca872d48 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -617,6 +617,7 @@ struct i915_psr {
>   void (*enable_sink)(struct intel_dp *);
>   void (*activate)(struct intel_dp *);
>   void (*setup_vsc)(struct intel_dp *, const struct
> intel_crtc_state *);
> + void (*exit)(struct intel_dp *intel_dp);
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 659180656f5b..ebc47e2b08ca 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -844,53 +844,67 @@ static void intel_psr_work(struct work_struct
> *work)
>   mutex_unlock(_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.
> +

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

2018-04-18 Thread 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 
Cc: Dhinakaran Pandiyan 
Reviewed-by: Rodrigo Vivi 
---

No changes from v1.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8e8667d9b084..476bca872d48 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -617,6 +617,7 @@ struct i915_psr {
void (*enable_sink)(struct intel_dp *);
void (*activate)(struct intel_dp *);
void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
+   void (*exit)(struct intel_dp *intel_dp);
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 659180656f5b..ebc47e2b08ca 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -844,53 +844,67 @@ static void intel_psr_work(struct work_struct *work)
mutex_unlock(_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(_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
-