Re: [Intel-gfx] [CI 1/6] drm/i915/psr: New power domain for AUX IO.

[Rodrigo Vivi]

On Tue, Feb 27, 2018 at 10:24:27AM -0800, Nathan Ciobanu wrote:
> On Fri, Feb 23, 2018 at 02:15:15PM -0800, Dhinakaran Pandiyan wrote:
> > From: "Dhinakaran Pandiyan" 
> > 
> > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > channels re-use the existing AUX domains.
> > 
> > v4: Reword comment (Rodrigo and Ville)
> > Rename _get and _put functions to include aux_io substring(Rodrigo)
> > Remove unnecessary diff that got included.
> > v3: Extract aux domain selection into a function (Ville)
> > v2: Add AUX IO domain only for AUX-A
> > Rebased on top of Ville's AUX series.
> > 
> > Cc: Imre Deak 
> > Cc: Rodrigo Vivi 
> > Cc: Ville Syrjälä 
> > Suggested-by: Imre Deak 
> > Signed-off-by: Dhinakaran Pandiyan 
> > Reviewed-by: Rodrigo Vivi 
>  Tested-by: Nathan Ciobanu 

ops. I'm sorry for leaving this out. I end up merging the patches
before seeing this here.

all 6 patches merged on dinq now. Thanks for the patches and reviews.

> > ---
> >  drivers/gpu/drm/i915/intel_display.h|  1 +
> >  drivers/gpu/drm/i915/intel_psr.c| 41 
> > +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> >  3 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h 
> > b/drivers/gpu/drm/i915/intel_display.h
> > index f5733a2576e7..4e7418b345bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > POWER_DOMAIN_AUX_C,
> > POWER_DOMAIN_AUX_D,
> > POWER_DOMAIN_AUX_F,
> > +   POWER_DOMAIN_AUX_IO_A,
> > POWER_DOMAIN_GMBUS,
> > POWER_DOMAIN_MODESET,
> > POWER_DOMAIN_GT_IRQ,
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..04430d4c99c9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,6 +56,43 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > +static inline enum intel_display_power_domain
> > +psr_aux_domain(struct intel_dp *intel_dp)
> > +{
> > +   /* CNL HW requires corresponding AUX IOs to be powered up for PSR.
> > +* However, for non-A AUX ports the corresponding non-EDP transcoders
> > +* would have already enabled power well 2 and DC_OFF. This means we can
> > +* acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
> > +* specific AUX_IO reference without powering up any extra wells.
> > +* Note that PSR is enabled only on Port A even though this function
> > +* returns the correct domain for other ports too.
> > +*/
> > +   return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > + intel_dp->aux_power_domain;
> > +}
> > +
> > +static void psr_aux_io_power_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_GEN(dev_priv) < 10)
> > +   return;
> > +
> > +   intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > +}
> > +
> > +static void psr_aux_io_power_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_GEN(dev_priv) < 10)
> > +   return;
> > +
> > +   intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > +}
> > +
> >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -459,6 +496,8 @@ static void hsw_psr_enable_source(struct intel_dp 
> > *intel_dp,
> > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > u32 chicken;
> >  
> > +   psr_aux_io_power_get(intel_dp);
> > +
> > if (dev_priv->psr.psr2_support) {
> > chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > if (dev_priv->psr.y_cord_support)
> > @@ -617,6 +656,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > else
> > WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > }
> > +
> > +   psr_aux_io_power_put(intel_dp);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b7924feb9f27..53ea564f971e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ 

Re: [Intel-gfx] [CI 1/6] drm/i915/psr: New power domain for AUX IO.

[Nathan Ciobanu]

On Fri, Feb 23, 2018 at 02:15:15PM -0800, Dhinakaran Pandiyan wrote:
> From: "Dhinakaran Pandiyan" 
> 
> PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> for AUX-A enables DC_OFF well too. This is not required, so add a new
> AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> channels re-use the existing AUX domains.
> 
> v4: Reword comment (Rodrigo and Ville)
> Rename _get and _put functions to include aux_io substring(Rodrigo)
> Remove unnecessary diff that got included.
> v3: Extract aux domain selection into a function (Ville)
> v2: Add AUX IO domain only for AUX-A
> Rebased on top of Ville's AUX series.
> 
> Cc: Imre Deak 
> Cc: Rodrigo Vivi 
> Cc: Ville Syrjälä 
> Suggested-by: Imre Deak 
> Signed-off-by: Dhinakaran Pandiyan 
> Reviewed-by: Rodrigo Vivi 
 Tested-by: Nathan Ciobanu 
> ---
>  drivers/gpu/drm/i915/intel_display.h|  1 +
>  drivers/gpu/drm/i915/intel_psr.c| 41 
> +
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h 
> b/drivers/gpu/drm/i915/intel_display.h
> index f5733a2576e7..4e7418b345bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -186,6 +186,7 @@ enum intel_display_power_domain {
>   POWER_DOMAIN_AUX_C,
>   POWER_DOMAIN_AUX_D,
>   POWER_DOMAIN_AUX_F,
> + POWER_DOMAIN_AUX_IO_A,
>   POWER_DOMAIN_GMBUS,
>   POWER_DOMAIN_MODESET,
>   POWER_DOMAIN_GT_IRQ,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..04430d4c99c9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,43 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static inline enum intel_display_power_domain
> +psr_aux_domain(struct intel_dp *intel_dp)
> +{
> + /* CNL HW requires corresponding AUX IOs to be powered up for PSR.
> +  * However, for non-A AUX ports the corresponding non-EDP transcoders
> +  * would have already enabled power well 2 and DC_OFF. This means we can
> +  * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
> +  * specific AUX_IO reference without powering up any extra wells.
> +  * Note that PSR is enabled only on Port A even though this function
> +  * returns the correct domain for other ports too.
> +  */
> + return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> +   intel_dp->aux_power_domain;
> +}
> +
> +static void psr_aux_io_power_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_GEN(dev_priv) < 10)
> + return;
> +
> + intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> +}
> +
> +static void psr_aux_io_power_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_GEN(dev_priv) < 10)
> + return;
> +
> + intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> +}
> +
>  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -459,6 +496,8 @@ static void hsw_psr_enable_source(struct intel_dp 
> *intel_dp,
>   enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>   u32 chicken;
>  
> + psr_aux_io_power_get(intel_dp);
> +
>   if (dev_priv->psr.psr2_support) {
>   chicken = PSR2_VSC_ENABLE_PROG_HEADER;
>   if (dev_priv->psr.y_cord_support)
> @@ -617,6 +656,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>   else
>   WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>   }
> +
> + psr_aux_io_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b7924feb9f27..53ea564f971e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum 
> intel_display_power_domain domain)
>   return "AUX_D";
>   case POWER_DOMAIN_AUX_F:
>   return "AUX_F";
> + case POWER_DOMAIN_AUX_IO_A:
> + return "AUX_IO_A";
>   case POWER_DOMAIN_GMBUS:
>   return "GMBUS";
>   case POWER_DOMAIN_INIT:
> @@ -1853,6 

[Intel-gfx] [CI 1/6] drm/i915/psr: New power domain for AUX IO.

[Dhinakaran Pandiyan]

From: "Dhinakaran Pandiyan" 

PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
for AUX-A enables DC_OFF well too. This is not required, so add a new
AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
channels re-use the existing AUX domains.

v4: Reword comment (Rodrigo and Ville)
Rename _get and _put functions to include aux_io substring(Rodrigo)
Remove unnecessary diff that got included.
v3: Extract aux domain selection into a function (Ville)
v2: Add AUX IO domain only for AUX-A
Rebased on top of Ville's AUX series.

Cc: Imre Deak 
Cc: Rodrigo Vivi 
Cc: Ville Syrjälä 
Suggested-by: Imre Deak 
Signed-off-by: Dhinakaran Pandiyan 
Reviewed-by: Rodrigo Vivi 
---
 drivers/gpu/drm/i915/intel_display.h|  1 +
 drivers/gpu/drm/i915/intel_psr.c| 41 +
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.h 
b/drivers/gpu/drm/i915/intel_display.h
index f5733a2576e7..4e7418b345bc 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -186,6 +186,7 @@ enum intel_display_power_domain {
POWER_DOMAIN_AUX_C,
POWER_DOMAIN_AUX_D,
POWER_DOMAIN_AUX_F,
+   POWER_DOMAIN_AUX_IO_A,
POWER_DOMAIN_GMBUS,
POWER_DOMAIN_MODESET,
POWER_DOMAIN_GT_IRQ,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..04430d4c99c9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,6 +56,43 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
+static inline enum intel_display_power_domain
+psr_aux_domain(struct intel_dp *intel_dp)
+{
+   /* CNL HW requires corresponding AUX IOs to be powered up for PSR.
+* However, for non-A AUX ports the corresponding non-EDP transcoders
+* would have already enabled power well 2 and DC_OFF. This means we can
+* acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
+* specific AUX_IO reference without powering up any extra wells.
+* Note that PSR is enabled only on Port A even though this function
+* returns the correct domain for other ports too.
+*/
+   return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+ intel_dp->aux_power_domain;
+}
+
+static void psr_aux_io_power_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_GEN(dev_priv) < 10)
+   return;
+
+   intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
+}
+
+static void psr_aux_io_power_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_GEN(dev_priv) < 10)
+   return;
+
+   intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
+}
+
 static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
 {
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -459,6 +496,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
u32 chicken;
 
+   psr_aux_io_power_get(intel_dp);
+
if (dev_priv->psr.psr2_support) {
chicken = PSR2_VSC_ENABLE_PROG_HEADER;
if (dev_priv->psr.y_cord_support)
@@ -617,6 +656,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
else
WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
}
+
+   psr_aux_io_power_put(intel_dp);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b7924feb9f27..53ea564f971e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -130,6 +130,8 @@ intel_display_power_domain_str(enum 
intel_display_power_domain domain)
return "AUX_D";
case POWER_DOMAIN_AUX_F:
return "AUX_F";
+   case POWER_DOMAIN_AUX_IO_A:
+   return "AUX_IO_A";
case POWER_DOMAIN_GMBUS:
return "GMBUS";
case POWER_DOMAIN_INIT:
@@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private 
*dev_priv,
BIT_ULL(POWER_DOMAIN_INIT))
 #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (  \
BIT_ULL(POWER_DOMAIN_AUX_A) |   \
+   BIT_ULL(POWER_DOMAIN_AUX_IO_A) |\