[PATCH 0/8] support for multiple power wells

[PATCH 0/8] support for multiple power wells

[Imre Deak]

This patchset adds support for multiple dynamic power wells needed by
future platforms. Besides adding a debugfs entry for power domain info
it doesn't have any functional change.

I also included a related patch from Jesse that isn't applied yet, but
is a dependency for this patchset.

I did some smoke test on SNB/HSW/VLV.

Imre Deak (7):
  drm/i915: add audio power domain
  drm/i915: support for multiple power wells
  drm/i915: add always-on power wells instead of special casing them
  drm/i915: s/HAS_POWER_WELL/IS_HASWELL/ in
    intel_display_capture_error_state
  drm/i915: restrict HSW specific powerdomains HW init to HSW
  drm/i915: add a default always-on power well
  drm/i915: add a debugfs entry for power domain info

Jesse Barnes (1):
  drm/i915: protect HSW power well check with IS_HASWELL in
    redisable_vga

 drivers/gpu/drm/i915/i915_debugfs.c  |  68 +++++++++++++++
 drivers/gpu/drm/i915/i915_dma.c      |  18 ++--
 drivers/gpu/drm/i915/i915_drv.h      |  15 +++-
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 165 +++++++++++++++++++++--------------
 5 files changed, 190 insertions(+), 82 deletions(-)

-- 
1.8.4

[PATCH 1/8] drm/i915: add audio power domain

[Imre Deak]

This way the code is simpler and can also be used for other platforms
where the audio power domain->power well mapping is different.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 10 ++--------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c1921d..e2e72e8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -100,6 +100,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+	POWER_DOMAIN_AUDIO,
 	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 09ac9e7..34e1a8b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5653,10 +5653,7 @@ void i915_request_power_well(void)
 
 	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
 				power_domains);
-
-	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]);
-	mutex_unlock(&hsw_pwr->lock);
+	intel_display_power_get(dev_priv->dev, POWER_DOMAIN_AUDIO);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
 
@@ -5670,10 +5667,7 @@ void i915_release_power_well(void)
 
 	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
 				power_domains);
-
-	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]);
-	mutex_unlock(&hsw_pwr->lock);
+	intel_display_power_put(dev_priv->dev, POWER_DOMAIN_AUDIO);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
-- 
1.8.4

Re: [PATCH 1/8] drm/i915: add audio power domain

[Paulo Zanoni]

2013/11/1 Imre Deak <imre.deak@intel.com>:
> This way the code is simpler and can also be used for other platforms
> where the audio power domain->power well mapping is different.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Good idea!

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++--------
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2c1921d..e2e72e8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -100,6 +100,7 @@ enum intel_display_power_domain {
>         POWER_DOMAIN_TRANSCODER_C,
>         POWER_DOMAIN_TRANSCODER_EDP,
>         POWER_DOMAIN_VGA,
> +       POWER_DOMAIN_AUDIO,
>         POWER_DOMAIN_INIT,
>
>         POWER_DOMAIN_NUM,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 09ac9e7..34e1a8b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5653,10 +5653,7 @@ void i915_request_power_well(void)
>
>         dev_priv = container_of(hsw_pwr, struct drm_i915_private,
>                                 power_domains);
> -
> -       mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]);
> -       mutex_unlock(&hsw_pwr->lock);
> +       intel_display_power_get(dev_priv->dev, POWER_DOMAIN_AUDIO);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
>
> @@ -5670,10 +5667,7 @@ void i915_release_power_well(void)
>
>         dev_priv = container_of(hsw_pwr, struct drm_i915_private,
>                                 power_domains);
> -
> -       mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]);
> -       mutex_unlock(&hsw_pwr->lock);
> +       intel_display_power_put(dev_priv->dev, POWER_DOMAIN_AUDIO);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 2/8] drm/i915: support for multiple power wells

[Imre Deak]

HW generations so far had only one always-on power well and optionally
one dynamic power well. Upcoming HW gens may have multiple dynamic power
wells, so add some infrastructure to support them.

The idea is to keep the existing power domain API used by the rest of
the driver and create a mapping between these power domains and the
underlying power wells. This mapping can differ from one HW to another
but high level driver code doesn't need to know about this. Through the
existing get/put API it would just ask for a given power domain and the
power domain framework would make sure the relevant power wells get
enabled in the right order.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  12 +++--
 drivers/gpu/drm/i915/intel_pm.c | 113 ++++++++++++++++++++++++++++++++--------
 2 files changed, 99 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e2e72e8..038da2a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -913,21 +913,27 @@ struct intel_ilk_power_mgmt {
 
 /* Power well structure for haswell */
 struct i915_power_well {
+	const char *name;
 	/* power well enable/disable usage count */
 	int count;
+	unsigned long domains;
+	void *data;
+	void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
+		    bool enable);
+	bool (*is_enabled)(struct drm_device *dev,
+			   struct i915_power_well *power_well);
 };
 
-#define I915_MAX_POWER_WELLS 1
-
 struct i915_power_domains {
 	/*
 	 * Power wells needed for initialization at driver init and suspend
 	 * time are on. They are kept on until after the first modeset.
 	 */
 	bool init_power_on;
+	int power_well_count;
 
 	struct mutex lock;
-	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
+	struct i915_power_well *power_wells;
 };
 
 struct i915_dri1_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 34e1a8b..8640b78 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5521,27 +5521,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
 	return BIT(domain) & always_on_domains;
 }
 
+#define for_each_power_well(i, power_well, domain_mask, power_domains)	\
+	for (i = 0;							\
+	     i < (power_domains)->power_well_count &&			\
+		 ((power_well) = &(power_domains)->power_wells[i]);	\
+	     i++)							\
+		if ((power_well)->domains & (domain_mask))
+
+#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
+	for (i = (power_domains)->power_well_count - 1;			 \
+	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
+	     i--)							 \
+		if ((power_well)->domains & (domain_mask))
+
 /**
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-bool intel_display_power_enabled(struct drm_device *dev,
-				 enum intel_display_power_domain domain)
+static bool hsw_power_well_enabled(struct drm_device *dev,
+				   struct i915_power_well *power_well)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_POWER_WELL(dev))
-		return true;
-
-	if (is_always_on_power_domain(dev, domain))
-		return true;
-
 	return I915_READ(HSW_PWR_WELL_DRIVER) ==
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 }
 
-static void __intel_set_power_well(struct drm_device *dev, bool enable)
+bool intel_display_power_enabled(struct drm_device *dev,
+				 enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	bool is_enabled;
+	int i;
+
+	if (!HAS_POWER_WELL(dev))
+		return true;
+
+	if (is_always_on_power_domain(dev, domain))
+		return true;
+
+	power_domains = &dev_priv->power_domains;
+
+	is_enabled = true;
+
+	mutex_lock(&power_domains->lock);
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
+		if (!power_well->is_enabled(dev, power_well)) {
+			is_enabled = false;
+			break;
+		}
+	}
+	mutex_unlock(&power_domains->lock);
+
+	return is_enabled;
+}
+
+static void hsw_set_power_well(struct drm_device *dev,
+			       struct i915_power_well *power_well, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool is_enabled, enable_requested;
@@ -5591,16 +5630,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 static void __intel_power_well_get(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
-	if (!power_well->count++)
-		__intel_set_power_well(dev, true);
+	if (!power_well->count++ && power_well->set)
+		power_well->set(dev, power_well, true);
 }
 
 static void __intel_power_well_put(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
 	WARN_ON(!power_well->count);
-	if (!--power_well->count && i915_disable_power_well)
-		__intel_set_power_well(dev, false);
+
+	if (!--power_well->count && power_well->set && i915_disable_power_well)
+		power_well->set(dev, power_well, false);
 }
 
 void intel_display_power_get(struct drm_device *dev,
@@ -5608,6 +5648,8 @@ void intel_display_power_get(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5618,7 +5660,8 @@ void intel_display_power_get(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_get(dev, &power_domains->power_wells[0]);
+	for_each_power_well(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_get(dev, power_well);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5627,6 +5670,8 @@ void intel_display_power_put(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5637,7 +5682,8 @@ void intel_display_power_put(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_put(dev, &power_domains->power_wells[0]);
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_put(dev, power_well);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5671,17 +5717,37 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+static struct i915_power_well hsw_power_wells[] = {
+	{
+		.name = "display",
+		.domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
+		.is_enabled = hsw_power_well_enabled,
+		.set = hsw_set_power_well,
+	},
+};
+
 int intel_power_domains_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	struct i915_power_well *power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return 0;
 
 	mutex_init(&power_domains->lock);
-	hsw_pwr = power_domains;
 
-	power_well = &power_domains->power_wells[0];
-	power_well->count = 0;
+	/*
+	 * The enabling order will be from lower to higher indexed wells,
+	 * the disabling order is reversed.
+	 */
+	if (IS_HASWELL(dev)) {
+		power_domains->power_wells = hsw_power_wells;
+		power_domains->power_well_count = ARRAY_SIZE(hsw_power_wells);
+
+		hsw_pwr = power_domains;
+	} else {
+		WARN_ON(1);
+	}
 
 	return 0;
 }
@@ -5696,15 +5762,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
 	mutex_lock(&power_domains->lock);
-
-	power_well = &power_domains->power_wells[0];
-	__intel_set_power_well(dev, power_well->count > 0);
-
+	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
+		if (power_well->set)
+			power_well->set(dev, power_well, power_well->count > 0);
+	}
 	mutex_unlock(&power_domains->lock);
 }
 
-- 
1.8.4

[PATCH 3/8] drm/i915: add always-on power wells instead of special casing them

[Imre Deak]

Instead of using a separate function to check whether a power domain is
is always on, add an always-on power well covering all these power
domains and do the usual get/put on these unconditionally. Since we
don't assign a .set handler for these the get/put won't have any effect
besides the adjusted refcount.

This makes the code more readable and provides debug info also on the
use of always-on power wells (once the relevant debugfs entry is added.)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++--------------------------
 2 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 038da2a..555a6fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -914,6 +914,7 @@ struct intel_ilk_power_mgmt {
 /* Power well structure for haswell */
 struct i915_power_well {
 	const char *name;
+	unsigned always_on:1;
 	/* power well enable/disable usage count */
 	int count;
 	unsigned long domains;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8640b78..ab85e89 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5504,23 +5504,6 @@ void intel_suspend_hw(struct drm_device *dev)
 		lpt_suspend_hw(dev);
 }
 
-static bool is_always_on_power_domain(struct drm_device *dev,
-				      enum intel_display_power_domain domain)
-{
-	unsigned long always_on_domains;
-
-	BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK);
-
-	if (IS_HASWELL(dev)) {
-		always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS;
-	} else {
-		WARN_ON(1);
-		return true;
-	}
-
-	return BIT(domain) & always_on_domains;
-}
-
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
 	for (i = 0;							\
 	     i < (power_domains)->power_well_count &&			\
@@ -5560,15 +5543,15 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return true;
 
-	if (is_always_on_power_domain(dev, domain))
-		return true;
-
 	power_domains = &dev_priv->power_domains;
 
 	is_enabled = true;
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
+		if (power_well->always_on)
+			continue;
+
 		if (!power_well->is_enabled(dev, power_well)) {
 			is_enabled = false;
 			break;
@@ -5654,9 +5637,6 @@ void intel_display_power_get(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	if (is_always_on_power_domain(dev, domain))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5676,9 +5656,6 @@ void intel_display_power_put(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	if (is_always_on_power_domain(dev, domain))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5719,6 +5696,11 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
 
 static struct i915_power_well hsw_power_wells[] = {
 	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = HSW_ALWAYS_ON_POWER_DOMAINS,
+	},
+	{
 		.name = "display",
 		.domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
 		.is_enabled = hsw_power_well_enabled,
-- 
1.8.4

[PATCH 4/8] drm/i915: s/HAS_POWER_WELL/IS_HASWELL/ in intel_display_capture_error_state

[Imre Deak]

Some upcoming platforms with power wells don't have this register, so
check for Haswell instead.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8f40ae3..929ecce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11101,7 +11101,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 	if (error == NULL)
 		return NULL;
 
-	if (HAS_POWER_WELL(dev))
+	if (IS_HASWELL(dev))
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
@@ -11171,7 +11171,7 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 		return;
 
 	err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
-	if (HAS_POWER_WELL(dev))
+	if (IS_HASWELL(dev))
 		err_printf(m, "PWR_WELL_CTL2: %08x\n",
 			   error->power_well_driver);
 	for_each_pipe(i) {
-- 
1.8.4

Re: [PATCH 4/8] drm/i915: s/HAS_POWER_WELL/IS_HASWELL/ in intel_display_capture_error_state

[Daniel Vetter]

On Fri, Nov 01, 2013 at 07:19:50PM +0200, Imre Deak wrote:
> Some upcoming platforms with power wells don't have this register, so
> check for Haswell instead.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

The point of HAS_POWER_WELL was to make bdw work (for which we now have
approval apparently). Depending upon how this all shapes out it's probably
better to stall this series for a little bit until the bdw stuff has
landed.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8f40ae3..929ecce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11101,7 +11101,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  	if (error == NULL)
>  		return NULL;
>  
> -	if (HAS_POWER_WELL(dev))
> +	if (IS_HASWELL(dev))
>  		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>  
>  	for_each_pipe(i) {
> @@ -11171,7 +11171,7 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  		return;
>  
>  	err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
> -	if (HAS_POWER_WELL(dev))
> +	if (IS_HASWELL(dev))
>  		err_printf(m, "PWR_WELL_CTL2: %08x\n",
>  			   error->power_well_driver);
>  	for_each_pipe(i) {
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/8] drm/i915: s/HAS_POWER_WELL/IS_HASWELL/ in intel_display_capture_error_state

[Imre Deak]

On Fri, 2013-11-01 at 20:41 +0100, Daniel Vetter wrote:
> On Fri, Nov 01, 2013 at 07:19:50PM +0200, Imre Deak wrote:
> > Some upcoming platforms with power wells don't have this register, so
> > check for Haswell instead.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> The point of HAS_POWER_WELL was to make bdw work (for which we now have
> approval apparently). Depending upon how this all shapes out it's probably
> better to stall this series for a little bit until the bdw stuff has
> landed.

Ok, rebasing this on top of the BDW stuff is minimal work. But comments
on it would still be appreciated.

--Imre

[PATCH 5/8] drm/i915: protect HSW power well check with IS_HASWELL in redisable_vga

[Imre Deak]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

From: Jesse Barnes <jbarnes@virtuousgeek.org>

This may need work if other platforms do the same thing, but in the
meantime we should avoid looking at HSW specific bits in this generic
function.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 929ecce..58cc2f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10773,7 +10773,7 @@ void i915_redisable_vga(struct drm_device *dev)
 	 * level, just check if the power well is enabled instead of trying to
 	 * follow the "don't touch the power well if we don't need it" policy
 	 * the rest of the driver uses. */
-	if (HAS_POWER_WELL(dev) &&
+	if (IS_HASWELL(dev) &&
 	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE_ENABLED) == 0)
 		return;
 
-- 
1.8.4

[PATCH 6/8] drm/i915: restrict HSW specific powerdomains HW init to HSW

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ab85e89..ba4baf1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5774,6 +5774,9 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 	intel_display_set_init_power(dev, true);
 	intel_power_domains_resume(dev);
 
+	if (!(IS_HASWELL(dev)))
+		return;
+
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
 	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
-- 
1.8.4

[PATCH 7/8] drm/i915: add a default always-on power well

[Imre Deak]

So far we distinguished platforms without a dynamic power well with
the HAS_POWER_WELL macro and for such platforms we didn't call any power
domain functions. Instead of doing this check we can add an always-on
power well for these platforms and call the power domain functions
unconditionally. For always-on power wells we only increase/decrease
their refcounts, otherwise they are a nop.

This makes high level driver code more readable and as a bonus provides
some idea of the current power domains state for all platforms (once
the relevant debugfs entry is added).

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 18 +++++++-----------
 drivers/gpu/drm/i915/i915_drv.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++-------------------
 3 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0cab2d0..b57c3da 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1639,8 +1639,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	if (HAS_POWER_WELL(dev))
-		intel_power_domains_init(dev);
+	intel_power_domains_init(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
@@ -1667,8 +1666,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	return 0;
 
 out_power_well:
-	if (HAS_POWER_WELL(dev))
-		intel_power_domains_remove(dev);
+	intel_power_domains_remove(dev);
 	drm_vblank_cleanup(dev);
 out_gem_unload:
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
@@ -1706,13 +1704,11 @@ int i915_driver_unload(struct drm_device *dev)
 
 	intel_gpu_ips_teardown();
 
-	if (HAS_POWER_WELL(dev)) {
-		/* The i915.ko module is still not prepared to be loaded when
-		 * the power well is not enabled, so just enable it in case
-		 * we're going to unload/reload. */
-		intel_display_set_init_power(dev, true);
-		intel_power_domains_remove(dev);
-	}
+	/* The i915.ko module is still not prepared to be loaded when
+	 * the power well is not enabled, so just enable it in case
+	 * we're going to unload/reload. */
+	intel_display_set_init_power(dev, true);
+	intel_power_domains_remove(dev);
 
 	i915_teardown_sysfs(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 555a6fc..05365e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1795,7 +1795,6 @@ struct drm_i915_file_private {
 #define HAS_IPS(dev)		(IS_ULT(dev))
 
 #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
-#define HAS_POWER_WELL(dev)	(IS_HASWELL(dev))
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev))
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ba4baf1..f35cb5d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5540,9 +5540,6 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	bool is_enabled;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return true;
-
 	power_domains = &dev_priv->power_domains;
 
 	is_enabled = true;
@@ -5634,9 +5631,6 @@ void intel_display_power_get(struct drm_device *dev,
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5653,9 +5647,6 @@ void intel_display_power_put(struct drm_device *dev,
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5694,6 +5685,14 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+static struct i915_power_well intel_power_wells[] = {
+	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = POWER_DOMAIN_MASK,
+	},
+};
+
 static struct i915_power_well hsw_power_wells[] = {
 	{
 		.name = "always-on",
@@ -5713,9 +5712,6 @@ int intel_power_domains_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 
-	if (!HAS_POWER_WELL(dev))
-		return 0;
-
 	mutex_init(&power_domains->lock);
 
 	/*
@@ -5728,7 +5724,8 @@ int intel_power_domains_init(struct drm_device *dev)
 
 		hsw_pwr = power_domains;
 	} else {
-		WARN_ON(1);
+		power_domains->power_wells = intel_power_wells;
+		power_domains->power_well_count = ARRAY_SIZE(intel_power_wells);
 	}
 
 	return 0;
@@ -5746,9 +5743,6 @@ static void intel_power_domains_resume(struct drm_device *dev)
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	mutex_lock(&power_domains->lock);
 	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
 		if (power_well->set)
@@ -5767,9 +5761,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	/* For now, we need the power well to be always enabled. */
 	intel_display_set_init_power(dev, true);
 	intel_power_domains_resume(dev);
-- 
1.8.4

[PATCH 8/8] drm/i915: add a debugfs entry for power domain info

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 68 +++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5c45e9e..a32f3c4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1760,6 +1760,73 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static const char *power_domain_str(enum intel_display_power_domain domain)
+{
+#define PWRD(p)	{ POWER_DOMAIN_ ## p, # p }
+	static struct {
+		enum intel_display_power_domain domain;
+		const char *name;
+	} table[] = {
+		PWRD(PIPE_A),
+		PWRD(PIPE_B),
+		PWRD(PIPE_C),
+		PWRD(PIPE_A_PANEL_FITTER),
+		PWRD(PIPE_B_PANEL_FITTER),
+		PWRD(PIPE_C_PANEL_FITTER),
+		PWRD(TRANSCODER_A),
+		PWRD(TRANSCODER_B),
+		PWRD(TRANSCODER_C),
+		PWRD(TRANSCODER_EDP),
+		PWRD(VGA),
+		PWRD(AUDIO),
+		PWRD(INIT),
+	};
+#undef PWRD
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(table); i++)
+		if (table[i].domain == domain)
+			return table[i].name;
+
+	WARN_ON(1);
+
+	return "?";
+}
+
+static int i915_power_domain_info(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	int i;
+
+	mutex_lock(&power_domains->lock);
+
+	for (i = 0; i < power_domains->power_well_count; i++) {
+		struct i915_power_well *power_well;
+		enum intel_display_power_domain power_domain;
+
+		power_well = &power_domains->power_wells[i];
+		seq_printf(m, "Power well: %s\n", power_well->name);
+		seq_printf(m, "  Use count: %d\n", power_well->count);
+		seq_printf(m, "  Power domains:\n");
+
+		for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
+		     power_domain++) {
+			if (!(BIT(power_domain) & power_well->domains))
+				continue;
+
+			seq_printf(m, "    %s\n",
+				   power_domain_str(power_domain));
+		}
+	}
+
+	mutex_unlock(&power_domains->lock);
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -2820,6 +2887,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
+	{"i915_power_domain_info", i915_power_domain_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.8.4

[PATCH v2 0/8] support for multiple power wells

[Imre Deak]

This is just a rebase on the latest kernel and additional debug info on
domain use counts in patch 8.

Imre Deak (7):
  drm/i915: add audio power domain
  drm/i915: support for multiple power wells
  drm/i915: add always-on power wells instead of special casing them
  drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL
  drm/i915: don't do BDW/HSW specific powerdomains init on other
    platforms
  drm/i915: add a default always-on power well
  drm/i915: add a debugfs entry for power domain info

Jesse Barnes (1):
  drm/i915: protect HSW power well check with IS_HASWELL in
    redisable_vga

 drivers/gpu/drm/i915/i915_debugfs.c  |  69 ++++++++++++
 drivers/gpu/drm/i915/i915_dma.c      |  18 ++--
 drivers/gpu/drm/i915/i915_drv.h      |  19 +++-
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 198 +++++++++++++++++++++++------------
 5 files changed, 224 insertions(+), 86 deletions(-)

-- 
1.8.4

[PATCH v3 0/8] support for multiple power wells

[Imre Deak]

v2: rebase on latest kernel + debug info on domain use counts
v3: address review comments from Paulo

Imre Deak (7):
  drm/i915: add audio power domain
  drm/i915: support for multiple power wells
  drm/i915: add always-on power wells instead of special casing them
  drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL
  drm/i915: don't do BDW/HSW specific powerdomains init on other
    platforms
  drm/i915: add a default always-on power well
  drm/i915: add a debugfs entry for power domain info

Jesse Barnes (1):
  drm/i915: protect HSW power well check with IS_HASWELL in
    redisable_vga

 drivers/gpu/drm/i915/i915_debugfs.c  |  71 +++++++++++++
 drivers/gpu/drm/i915/i915_dma.c      |  18 ++--
 drivers/gpu/drm/i915/i915_drv.h      |  18 +++-
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 198 +++++++++++++++++++++++------------
 5 files changed, 227 insertions(+), 84 deletions(-)

-- 
1.8.4

Re: [PATCH v3 0/8] support for multiple power wells

[Paulo Zanoni]

2013/11/25 Imre Deak <imre.deak@intel.com>:
> v2: rebase on latest kernel + debug info on domain use counts
> v3: address review comments from Paulo
>
All my comments got implemented or got some nice explanations, so, for
the series:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> Imre Deak (7):
>   drm/i915: add audio power domain
>   drm/i915: support for multiple power wells
>   drm/i915: add always-on power wells instead of special casing them
>   drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL
>   drm/i915: don't do BDW/HSW specific powerdomains init on other
>     platforms
>   drm/i915: add a default always-on power well
>   drm/i915: add a debugfs entry for power domain info
>
> Jesse Barnes (1):
>   drm/i915: protect HSW power well check with IS_HASWELL in
>     redisable_vga
>
>  drivers/gpu/drm/i915/i915_debugfs.c  |  71 +++++++++++++
>  drivers/gpu/drm/i915/i915_dma.c      |  18 ++--
>  drivers/gpu/drm/i915/i915_drv.h      |  18 +++-
>  drivers/gpu/drm/i915/intel_display.c |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 198 +++++++++++++++++++++++------------
>  5 files changed, 227 insertions(+), 84 deletions(-)
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v3 0/8] support for multiple power wells

[Daniel Vetter]

On Tue, Nov 26, 2013 at 04:46:54PM -0200, Paulo Zanoni wrote:
> 2013/11/25 Imre Deak <imre.deak@intel.com>:
> > v2: rebase on latest kernel + debug info on domain use counts
> > v3: address review comments from Paulo
> >
> All my comments got implemented or got some nice explanations, so, for
> the series:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Slurped them all in, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3 1/8] drm/i915: add audio power domain

[Imre Deak]

This way the code is simpler and can also be used for other platforms
where the audio power domain->power well mapping is different.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Paulo Zanoni <paulo.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 10 ++--------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4377523..7007b9b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -113,6 +113,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+	POWER_DOMAIN_AUDIO,
 	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 172efa0..bcca995 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5773,10 +5773,7 @@ void i915_request_power_well(void)
 
 	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
 				power_domains);
-
-	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]);
-	mutex_unlock(&hsw_pwr->lock);
+	intel_display_power_get(dev_priv->dev, POWER_DOMAIN_AUDIO);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
 
@@ -5790,10 +5787,7 @@ void i915_release_power_well(void)
 
 	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
 				power_domains);
-
-	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]);
-	mutex_unlock(&hsw_pwr->lock);
+	intel_display_power_put(dev_priv->dev, POWER_DOMAIN_AUDIO);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
-- 
1.8.4

[PATCH v3 2/8] drm/i915: support for multiple power wells

[Imre Deak]

HW generations so far had only one always-on power well and optionally
one dynamic power well. Upcoming HW gens may have multiple dynamic power
wells, so add some infrastructure to support them.

The idea is to keep the existing power domain API used by the rest of
the driver and create a mapping between these power domains and the
underlying power wells. This mapping can differ from one HW to another
but high level driver code doesn't need to know about this. Through the
existing get/put API it would just ask for a given power domain and the
power domain framework would make sure the relevant power wells get
enabled in the right order.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  12 +++-
 drivers/gpu/drm/i915/intel_pm.c | 128 ++++++++++++++++++++++++++++++++--------
 2 files changed, 114 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7007b9b..b20016c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -945,21 +945,27 @@ struct intel_ilk_power_mgmt {
 
 /* Power well structure for haswell */
 struct i915_power_well {
+	const char *name;
 	/* power well enable/disable usage count */
 	int count;
+	unsigned long domains;
+	void *data;
+	void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
+		    bool enable);
+	bool (*is_enabled)(struct drm_device *dev,
+			   struct i915_power_well *power_well);
 };
 
-#define I915_MAX_POWER_WELLS 1
-
 struct i915_power_domains {
 	/*
 	 * Power wells needed for initialization at driver init and suspend
 	 * time are on. They are kept on until after the first modeset.
 	 */
 	bool init_power_on;
+	int power_well_count;
 
 	struct mutex lock;
-	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
+	struct i915_power_well *power_wells;
 };
 
 struct i915_dri1_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bcca995..2b39a9c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5625,27 +5625,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
 	return BIT(domain) & always_on_domains;
 }
 
+#define for_each_power_well(i, power_well, domain_mask, power_domains)	\
+	for (i = 0;							\
+	     i < (power_domains)->power_well_count &&			\
+		 ((power_well) = &(power_domains)->power_wells[i]);	\
+	     i++)							\
+		if ((power_well)->domains & (domain_mask))
+
+#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
+	for (i = (power_domains)->power_well_count - 1;			 \
+	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
+	     i--)							 \
+		if ((power_well)->domains & (domain_mask))
+
 /**
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-bool intel_display_power_enabled(struct drm_device *dev,
-				 enum intel_display_power_domain domain)
+static bool hsw_power_well_enabled(struct drm_device *dev,
+				   struct i915_power_well *power_well)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_POWER_WELL(dev))
-		return true;
-
-	if (is_always_on_power_domain(dev, domain))
-		return true;
-
 	return I915_READ(HSW_PWR_WELL_DRIVER) ==
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 }
 
-static void __intel_set_power_well(struct drm_device *dev, bool enable)
+bool intel_display_power_enabled(struct drm_device *dev,
+				 enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	bool is_enabled;
+	int i;
+
+	if (!HAS_POWER_WELL(dev))
+		return true;
+
+	if (is_always_on_power_domain(dev, domain))
+		return true;
+
+	power_domains = &dev_priv->power_domains;
+
+	is_enabled = true;
+
+	mutex_lock(&power_domains->lock);
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
+		if (!power_well->is_enabled(dev, power_well)) {
+			is_enabled = false;
+			break;
+		}
+	}
+	mutex_unlock(&power_domains->lock);
+
+	return is_enabled;
+}
+
+static void hsw_set_power_well(struct drm_device *dev,
+			       struct i915_power_well *power_well, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool is_enabled, enable_requested;
@@ -5711,16 +5750,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 static void __intel_power_well_get(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
-	if (!power_well->count++)
-		__intel_set_power_well(dev, true);
+	if (!power_well->count++ && power_well->set)
+		power_well->set(dev, power_well, true);
 }
 
 static void __intel_power_well_put(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
 	WARN_ON(!power_well->count);
-	if (!--power_well->count && i915_disable_power_well)
-		__intel_set_power_well(dev, false);
+
+	if (!--power_well->count && power_well->set && i915_disable_power_well)
+		power_well->set(dev, power_well, false);
 }
 
 void intel_display_power_get(struct drm_device *dev,
@@ -5728,6 +5768,8 @@ void intel_display_power_get(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5738,7 +5780,8 @@ void intel_display_power_get(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_get(dev, &power_domains->power_wells[0]);
+	for_each_power_well(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_get(dev, power_well);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5747,6 +5790,8 @@ void intel_display_power_put(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5757,7 +5802,8 @@ void intel_display_power_put(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_put(dev, &power_domains->power_wells[0]);
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_put(dev, power_well);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5791,17 +5837,52 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+static struct i915_power_well hsw_power_wells[] = {
+	{
+		.name = "display",
+		.domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
+		.is_enabled = hsw_power_well_enabled,
+		.set = hsw_set_power_well,
+	},
+};
+
+static struct i915_power_well bdw_power_wells[] = {
+	{
+		.name = "display",
+		.domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
+		.is_enabled = hsw_power_well_enabled,
+		.set = hsw_set_power_well,
+	},
+};
+
+#define set_power_wells(power_domains, __power_wells) ({		\
+	(power_domains)->power_wells = (__power_wells);			\
+	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
+})
+
 int intel_power_domains_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	struct i915_power_well *power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return 0;
 
 	mutex_init(&power_domains->lock);
-	hsw_pwr = power_domains;
 
-	power_well = &power_domains->power_wells[0];
-	power_well->count = 0;
+	/*
+	 * The enabling order will be from lower to higher indexed wells,
+	 * the disabling order is reversed.
+	 */
+	if (IS_HASWELL(dev)) {
+		set_power_wells(power_domains, hsw_power_wells);
+		hsw_pwr = power_domains;
+	} else if (IS_BROADWELL(dev)) {
+		set_power_wells(power_domains, bdw_power_wells);
+		hsw_pwr = power_domains;
+	} else {
+		WARN_ON(1);
+	}
 
 	return 0;
 }
@@ -5816,15 +5897,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
 	mutex_lock(&power_domains->lock);
-
-	power_well = &power_domains->power_wells[0];
-	__intel_set_power_well(dev, power_well->count > 0);
-
+	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
+		if (power_well->set)
+			power_well->set(dev, power_well, power_well->count > 0);
+	}
 	mutex_unlock(&power_domains->lock);
 }
 
-- 
1.8.4

[PATCH v3 3/8] drm/i915: add always-on power wells instead of special casing them

[Imre Deak]

Instead of using a separate function to check whether a power domain is
is always on, add an always-on power well covering all these power
domains and do the usual get/put on these unconditionally. Since we
don't assign a .set handler for these the get/put won't have any effect
besides the adjusted refcount.

This makes the code more readable and provides debug info also on the
use of always-on power wells (once the relevant debugfs entry is added.)

v3: make is_always_on to be bool instead of a bit field (Paulo)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++----------------------------
 2 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b20016c..701a7fb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt {
 /* Power well structure for haswell */
 struct i915_power_well {
 	const char *name;
+	bool always_on;
 	/* power well enable/disable usage count */
 	int count;
 	unsigned long domains;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2b39a9c..ee5aeb1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5606,25 +5606,6 @@ void intel_suspend_hw(struct drm_device *dev)
 		lpt_suspend_hw(dev);
 }
 
-static bool is_always_on_power_domain(struct drm_device *dev,
-				      enum intel_display_power_domain domain)
-{
-	unsigned long always_on_domains;
-
-	BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK);
-
-	if (IS_BROADWELL(dev)) {
-		always_on_domains = BDW_ALWAYS_ON_POWER_DOMAINS;
-	} else if (IS_HASWELL(dev)) {
-		always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS;
-	} else {
-		WARN_ON(1);
-		return true;
-	}
-
-	return BIT(domain) & always_on_domains;
-}
-
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
 	for (i = 0;							\
 	     i < (power_domains)->power_well_count &&			\
@@ -5664,15 +5645,15 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return true;
 
-	if (is_always_on_power_domain(dev, domain))
-		return true;
-
 	power_domains = &dev_priv->power_domains;
 
 	is_enabled = true;
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
+		if (power_well->always_on)
+			continue;
+
 		if (!power_well->is_enabled(dev, power_well)) {
 			is_enabled = false;
 			break;
@@ -5774,9 +5755,6 @@ void intel_display_power_get(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	if (is_always_on_power_domain(dev, domain))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5796,9 +5774,6 @@ void intel_display_power_put(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	if (is_always_on_power_domain(dev, domain))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5839,6 +5814,11 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
 
 static struct i915_power_well hsw_power_wells[] = {
 	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = HSW_ALWAYS_ON_POWER_DOMAINS,
+	},
+	{
 		.name = "display",
 		.domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
 		.is_enabled = hsw_power_well_enabled,
@@ -5848,6 +5828,11 @@ static struct i915_power_well hsw_power_wells[] = {
 
 static struct i915_power_well bdw_power_wells[] = {
 	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = BDW_ALWAYS_ON_POWER_DOMAINS,
+	},
+	{
 		.name = "display",
 		.domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
 		.is_enabled = hsw_power_well_enabled,
-- 
1.8.4

[PATCH v3 4/8] drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL

[Imre Deak]

In intel_display_capture_error_state we use HAS_POWER_WELL to check if
we are running on Haswell/Broadwell when accessing HSW_PWR_WELL_DRIVER
which is specific to these platforms. Future platforms with power wells
don't have this register, so HAS_POWER_WELL won't work there any more.
Use IS_HASWELL/IS_BROADWELL instead.

v3: fix using logical || instead of bitwise | (Paulo)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2df2366..64f096e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11370,7 +11370,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 	if (error == NULL)
 		return NULL;
 
-	if (HAS_POWER_WELL(dev))
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
@@ -11441,7 +11441,7 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 		return;
 
 	err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
-	if (HAS_POWER_WELL(dev))
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		err_printf(m, "PWR_WELL_CTL2: %08x\n",
 			   error->power_well_driver);
 	for_each_pipe(i) {
-- 
1.8.4

[PATCH v3 5/8] drm/i915: protect HSW power well check with IS_HASWELL in redisable_vga

[Imre Deak]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

This may need work if other platforms do the same thing, but in the
meantime we should avoid looking at HSW specific bits in this generic
function.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
[added IS_BROADWELL too as that needs the same handling (Imre)]
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 64f096e..fab7d35 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11042,7 +11042,7 @@ void i915_redisable_vga(struct drm_device *dev)
 	 * level, just check if the power well is enabled instead of trying to
 	 * follow the "don't touch the power well if we don't need it" policy
 	 * the rest of the driver uses. */
-	if (HAS_POWER_WELL(dev) &&
+	if ((IS_HASWELL(dev) || IS_BROADWELL(dev)) &&
 	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE_ENABLED) == 0)
 		return;
 
-- 
1.8.4

[PATCH v3 6/8] drm/i915: don't do BDW/HSW specific powerdomains init on other platforms

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee5aeb1..d5eacd8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5912,6 +5912,9 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 	intel_display_set_init_power(dev, true);
 	intel_power_domains_resume(dev);
 
+	if (!(IS_HASWELL(dev) || IS_BROADWELL(dev)))
+		return;
+
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
 	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
-- 
1.8.4

[PATCH v3 7/8] drm/i915: add a default always-on power well

[Imre Deak]

So far we distinguished platforms without a dynamic power well with
the HAS_POWER_WELL macro and for such platforms we didn't call any power
domain functions. Instead of doing this check we can add an always-on
power well for these platforms and call the power domain functions
unconditionally. For always-on power wells we only increase/decrease
their refcounts, otherwise they are nop.

This makes high level driver code more readable and as a bonus provides
some idea of the current power domains state for all platforms (once
the relevant debugfs entry is added).

v3: rename intel_power_wells to i9xx_always_on_power_well (Paulo)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 18 +++++++-----------
 drivers/gpu/drm/i915/i915_drv.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++-------------------
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 00d74f8..95dd3db 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1639,8 +1639,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	if (HAS_POWER_WELL(dev))
-		intel_power_domains_init(dev);
+	intel_power_domains_init(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
@@ -1667,8 +1666,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	return 0;
 
 out_power_well:
-	if (HAS_POWER_WELL(dev))
-		intel_power_domains_remove(dev);
+	intel_power_domains_remove(dev);
 	drm_vblank_cleanup(dev);
 out_gem_unload:
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
@@ -1706,13 +1704,11 @@ int i915_driver_unload(struct drm_device *dev)
 
 	intel_gpu_ips_teardown();
 
-	if (HAS_POWER_WELL(dev)) {
-		/* The i915.ko module is still not prepared to be loaded when
-		 * the power well is not enabled, so just enable it in case
-		 * we're going to unload/reload. */
-		intel_display_set_init_power(dev, true);
-		intel_power_domains_remove(dev);
-	}
+	/* The i915.ko module is still not prepared to be loaded when
+	 * the power well is not enabled, so just enable it in case
+	 * we're going to unload/reload. */
+	intel_display_set_init_power(dev, true);
+	intel_power_domains_remove(dev);
 
 	i915_teardown_sysfs(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 701a7fb..20d3b79 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1834,7 +1834,6 @@ struct drm_i915_file_private {
 #define HAS_IPS(dev)		(IS_ULT(dev) || IS_BROADWELL(dev))
 
 #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
-#define HAS_POWER_WELL(dev)	(IS_HASWELL(dev) || IS_BROADWELL(dev))
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d5eacd8..ff798b8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5642,9 +5642,6 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	bool is_enabled;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return true;
-
 	power_domains = &dev_priv->power_domains;
 
 	is_enabled = true;
@@ -5752,9 +5749,6 @@ void intel_display_power_get(struct drm_device *dev,
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5771,9 +5765,6 @@ void intel_display_power_put(struct drm_device *dev,
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5812,6 +5803,14 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+static struct i915_power_well i9xx_always_on_power_well[] = {
+	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = POWER_DOMAIN_MASK,
+	},
+};
+
 static struct i915_power_well hsw_power_wells[] = {
 	{
 		.name = "always-on",
@@ -5850,9 +5849,6 @@ int intel_power_domains_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 
-	if (!HAS_POWER_WELL(dev))
-		return 0;
-
 	mutex_init(&power_domains->lock);
 
 	/*
@@ -5866,7 +5862,7 @@ int intel_power_domains_init(struct drm_device *dev)
 		set_power_wells(power_domains, bdw_power_wells);
 		hsw_pwr = power_domains;
 	} else {
-		WARN_ON(1);
+		set_power_wells(power_domains, i9xx_always_on_power_well);
 	}
 
 	return 0;
@@ -5884,9 +5880,6 @@ static void intel_power_domains_resume(struct drm_device *dev)
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	mutex_lock(&power_domains->lock);
 	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
 		if (power_well->set)
@@ -5905,9 +5898,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	/* For now, we need the power well to be always enabled. */
 	intel_display_set_init_power(dev, true);
 	intel_power_domains_resume(dev);
-- 
1.8.4

[PATCH v3 8/8] drm/i915: add a debugfs entry for power domain info

[Imre Deak]

Add a debugfs entry showing the use-count for all power domains of each
power well.

v3: address comments from Paulo:
- simplify power_domain_str() by using a switch table
- move power_well::domain_count to power_domains
- WARN_ON decrementing a 0 refcount

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 71 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_pm.c     | 12 +++++++
 3 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6875b7a..68d9ed0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1844,6 +1844,76 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static const char *power_domain_str(enum intel_display_power_domain domain)
+{
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+		return "PIPE_A";
+	case POWER_DOMAIN_PIPE_B:
+		return "PIPE_B";
+	case POWER_DOMAIN_PIPE_C:
+		return "PIPE_C";
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+		return "PIPE_A_PANEL_FITTER";
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+		return "PIPE_B_PANEL_FITTER";
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+		return "PIPE_C_PANEL_FITTER";
+	case POWER_DOMAIN_TRANSCODER_A:
+		return "TRANSCODER_A";
+	case POWER_DOMAIN_TRANSCODER_B:
+		return "TRANSCODER_B";
+	case POWER_DOMAIN_TRANSCODER_C:
+		return "TRANSCODER_C";
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return "TRANSCODER_EDP";
+	case POWER_DOMAIN_VGA:
+		return "VGA";
+	case POWER_DOMAIN_AUDIO:
+		return "AUDIO";
+	case POWER_DOMAIN_INIT:
+		return "INIT";
+	default:
+		WARN_ON(1);
+		return "?";
+	}
+}
+
+static int i915_power_domain_info(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	int i;
+
+	mutex_lock(&power_domains->lock);
+
+	seq_printf(m, "%-25s %s\n", "Power well/domain", "Use count");
+	for (i = 0; i < power_domains->power_well_count; i++) {
+		struct i915_power_well *power_well;
+		enum intel_display_power_domain power_domain;
+
+		power_well = &power_domains->power_wells[i];
+		seq_printf(m, "%-25s %d\n", power_well->name,
+			   power_well->count);
+
+		for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
+		     power_domain++) {
+			if (!(BIT(power_domain) & power_well->domains))
+				continue;
+
+			seq_printf(m, "  %-23s %d\n",
+				 power_domain_str(power_domain),
+				 power_domains->domain_use_count[power_domain]);
+		}
+	}
+
+	mutex_unlock(&power_domains->lock);
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -3076,6 +3146,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
+	{"i915_power_domain_info", i915_power_domain_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20d3b79..47b8fd1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -966,6 +966,9 @@ struct i915_power_domains {
 	int power_well_count;
 
 	struct mutex lock;
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	int domain_use_count[POWER_DOMAIN_NUM];
+#endif
 	struct i915_power_well *power_wells;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ff798b8..a1a54ab 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5752,8 +5752,13 @@ void intel_display_power_get(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	power_domains->domain_use_count[domain]++;
+#endif
 	for_each_power_well(i, power_well, BIT(domain), power_domains)
 		__intel_power_well_get(dev, power_well);
+
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5768,8 +5773,15 @@ void intel_display_power_put(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
+
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
 		__intel_power_well_put(dev, power_well);
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	WARN_ON(!power_domains->domain_use_count[domain]);
+	power_domains->domain_use_count[domain]--;
+#endif
+
 	mutex_unlock(&power_domains->lock);
 }
 
-- 
1.8.4

[PATCH v2 1/8] drm/i915: add audio power domain

[Imre Deak]

This way the code is simpler and can also be used for other platforms
where the audio power domain->power well mapping is different.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Paulo Zanoni <paulo.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 10 ++--------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4377523..7007b9b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -113,6 +113,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+	POWER_DOMAIN_AUDIO,
 	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 172efa0..bcca995 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5773,10 +5773,7 @@ void i915_request_power_well(void)
 
 	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
 				power_domains);
-
-	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]);
-	mutex_unlock(&hsw_pwr->lock);
+	intel_display_power_get(dev_priv->dev, POWER_DOMAIN_AUDIO);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
 
@@ -5790,10 +5787,7 @@ void i915_release_power_well(void)
 
 	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
 				power_domains);
-
-	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]);
-	mutex_unlock(&hsw_pwr->lock);
+	intel_display_power_put(dev_priv->dev, POWER_DOMAIN_AUDIO);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
-- 
1.8.4

[PATCH v2 2/8] drm/i915: support for multiple power wells

[Imre Deak]

HW generations so far had only one always-on power well and optionally
one dynamic power well. Upcoming HW gens may have multiple dynamic power
wells, so add some infrastructure to support them.

The idea is to keep the existing power domain API used by the rest of
the driver and create a mapping between these power domains and the
underlying power wells. This mapping can differ from one HW to another
but high level driver code doesn't need to know about this. Through the
existing get/put API it would just ask for a given power domain and the
power domain framework would make sure the relevant power wells get
enabled in the right order.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  12 +++-
 drivers/gpu/drm/i915/intel_pm.c | 128 ++++++++++++++++++++++++++++++++--------
 2 files changed, 114 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7007b9b..b20016c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -945,21 +945,27 @@ struct intel_ilk_power_mgmt {
 
 /* Power well structure for haswell */
 struct i915_power_well {
+	const char *name;
 	/* power well enable/disable usage count */
 	int count;
+	unsigned long domains;
+	void *data;
+	void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
+		    bool enable);
+	bool (*is_enabled)(struct drm_device *dev,
+			   struct i915_power_well *power_well);
 };
 
-#define I915_MAX_POWER_WELLS 1
-
 struct i915_power_domains {
 	/*
 	 * Power wells needed for initialization at driver init and suspend
 	 * time are on. They are kept on until after the first modeset.
 	 */
 	bool init_power_on;
+	int power_well_count;
 
 	struct mutex lock;
-	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
+	struct i915_power_well *power_wells;
 };
 
 struct i915_dri1_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bcca995..2b39a9c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5625,27 +5625,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
 	return BIT(domain) & always_on_domains;
 }
 
+#define for_each_power_well(i, power_well, domain_mask, power_domains)	\
+	for (i = 0;							\
+	     i < (power_domains)->power_well_count &&			\
+		 ((power_well) = &(power_domains)->power_wells[i]);	\
+	     i++)							\
+		if ((power_well)->domains & (domain_mask))
+
+#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
+	for (i = (power_domains)->power_well_count - 1;			 \
+	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
+	     i--)							 \
+		if ((power_well)->domains & (domain_mask))
+
 /**
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-bool intel_display_power_enabled(struct drm_device *dev,
-				 enum intel_display_power_domain domain)
+static bool hsw_power_well_enabled(struct drm_device *dev,
+				   struct i915_power_well *power_well)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_POWER_WELL(dev))
-		return true;
-
-	if (is_always_on_power_domain(dev, domain))
-		return true;
-
 	return I915_READ(HSW_PWR_WELL_DRIVER) ==
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 }
 
-static void __intel_set_power_well(struct drm_device *dev, bool enable)
+bool intel_display_power_enabled(struct drm_device *dev,
+				 enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	bool is_enabled;
+	int i;
+
+	if (!HAS_POWER_WELL(dev))
+		return true;
+
+	if (is_always_on_power_domain(dev, domain))
+		return true;
+
+	power_domains = &dev_priv->power_domains;
+
+	is_enabled = true;
+
+	mutex_lock(&power_domains->lock);
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
+		if (!power_well->is_enabled(dev, power_well)) {
+			is_enabled = false;
+			break;
+		}
+	}
+	mutex_unlock(&power_domains->lock);
+
+	return is_enabled;
+}
+
+static void hsw_set_power_well(struct drm_device *dev,
+			       struct i915_power_well *power_well, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool is_enabled, enable_requested;
@@ -5711,16 +5750,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 static void __intel_power_well_get(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
-	if (!power_well->count++)
-		__intel_set_power_well(dev, true);
+	if (!power_well->count++ && power_well->set)
+		power_well->set(dev, power_well, true);
 }
 
 static void __intel_power_well_put(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
 	WARN_ON(!power_well->count);
-	if (!--power_well->count && i915_disable_power_well)
-		__intel_set_power_well(dev, false);
+
+	if (!--power_well->count && power_well->set && i915_disable_power_well)
+		power_well->set(dev, power_well, false);
 }
 
 void intel_display_power_get(struct drm_device *dev,
@@ -5728,6 +5768,8 @@ void intel_display_power_get(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5738,7 +5780,8 @@ void intel_display_power_get(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_get(dev, &power_domains->power_wells[0]);
+	for_each_power_well(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_get(dev, power_well);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5747,6 +5790,8 @@ void intel_display_power_put(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains;
+	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5757,7 +5802,8 @@ void intel_display_power_put(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_put(dev, &power_domains->power_wells[0]);
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_put(dev, power_well);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5791,17 +5837,52 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+static struct i915_power_well hsw_power_wells[] = {
+	{
+		.name = "display",
+		.domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
+		.is_enabled = hsw_power_well_enabled,
+		.set = hsw_set_power_well,
+	},
+};
+
+static struct i915_power_well bdw_power_wells[] = {
+	{
+		.name = "display",
+		.domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
+		.is_enabled = hsw_power_well_enabled,
+		.set = hsw_set_power_well,
+	},
+};
+
+#define set_power_wells(power_domains, __power_wells) ({		\
+	(power_domains)->power_wells = (__power_wells);			\
+	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
+})
+
 int intel_power_domains_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	struct i915_power_well *power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return 0;
 
 	mutex_init(&power_domains->lock);
-	hsw_pwr = power_domains;
 
-	power_well = &power_domains->power_wells[0];
-	power_well->count = 0;
+	/*
+	 * The enabling order will be from lower to higher indexed wells,
+	 * the disabling order is reversed.
+	 */
+	if (IS_HASWELL(dev)) {
+		set_power_wells(power_domains, hsw_power_wells);
+		hsw_pwr = power_domains;
+	} else if (IS_BROADWELL(dev)) {
+		set_power_wells(power_domains, bdw_power_wells);
+		hsw_pwr = power_domains;
+	} else {
+		WARN_ON(1);
+	}
 
 	return 0;
 }
@@ -5816,15 +5897,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
+	int i;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
 	mutex_lock(&power_domains->lock);
-
-	power_well = &power_domains->power_wells[0];
-	__intel_set_power_well(dev, power_well->count > 0);
-
+	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
+		if (power_well->set)
+			power_well->set(dev, power_well, power_well->count > 0);
+	}
 	mutex_unlock(&power_domains->lock);
 }
 
-- 
1.8.4

Re: [PATCH v2 2/8] drm/i915: support for multiple power wells

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak@intel.com>:
> HW generations so far had only one always-on power well and optionally
> one dynamic power well. Upcoming HW gens may have multiple dynamic power
> wells, so add some infrastructure to support them.
>
> The idea is to keep the existing power domain API used by the rest of
> the driver and create a mapping between these power domains and the
> underlying power wells. This mapping can differ from one HW to another
> but high level driver code doesn't need to know about this. Through the
> existing get/put API it would just ask for a given power domain and the
> power domain framework would make sure the relevant power wells get
> enabled in the right order.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  12 +++-
>  drivers/gpu/drm/i915/intel_pm.c | 128 ++++++++++++++++++++++++++++++++--------
>  2 files changed, 114 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7007b9b..b20016c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -945,21 +945,27 @@ struct intel_ilk_power_mgmt {
>
>  /* Power well structure for haswell */
>  struct i915_power_well {
> +       const char *name;
>         /* power well enable/disable usage count */
>         int count;
> +       unsigned long domains;
> +       void *data;

This "data" field is completely unused.


> +       void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
> +                   bool enable);
> +       bool (*is_enabled)(struct drm_device *dev,
> +                          struct i915_power_well *power_well);

The "power_well" argument of both functions is also unused.


>  };
>
> -#define I915_MAX_POWER_WELLS 1
> -
>  struct i915_power_domains {
>         /*
>          * Power wells needed for initialization at driver init and suspend
>          * time are on. They are kept on until after the first modeset.
>          */
>         bool init_power_on;
> +       int power_well_count;
>
>         struct mutex lock;
> -       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> +       struct i915_power_well *power_wells;
>  };
>
>  struct i915_dri1_state {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bcca995..2b39a9c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5625,27 +5625,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
>         return BIT(domain) & always_on_domains;
>  }
>
> +#define for_each_power_well(i, power_well, domain_mask, power_domains) \
> +       for (i = 0;                                                     \
> +            i < (power_domains)->power_well_count &&                   \
> +                ((power_well) = &(power_domains)->power_wells[i]);     \
> +            i++)                                                       \
> +               if ((power_well)->domains & (domain_mask))
> +
> +#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
> +       for (i = (power_domains)->power_well_count - 1;                  \
> +            i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> +            i--)                                                        \
> +               if ((power_well)->domains & (domain_mask))
> +
>  /**
>   * We should only use the power well if we explicitly asked the hardware to
>   * enable it, so check if it's enabled and also check if we've requested it to
>   * be enabled.
>   */
> -bool intel_display_power_enabled(struct drm_device *dev,
> -                                enum intel_display_power_domain domain)
> +static bool hsw_power_well_enabled(struct drm_device *dev,
> +                                  struct i915_power_well *power_well)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return true;
> -
> -       if (is_always_on_power_domain(dev, domain))
> -               return true;
> -
>         return I915_READ(HSW_PWR_WELL_DRIVER) ==
>                      (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>  }
>
> -static void __intel_set_power_well(struct drm_device *dev, bool enable)
> +bool intel_display_power_enabled(struct drm_device *dev,
> +                                enum intel_display_power_domain domain)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_domains *power_domains;
> +       struct i915_power_well *power_well;
> +       bool is_enabled;
> +       int i;
> +
> +       if (!HAS_POWER_WELL(dev))
> +               return true;
> +
> +       if (is_always_on_power_domain(dev, domain))
> +               return true;
> +
> +       power_domains = &dev_priv->power_domains;
> +
> +       is_enabled = true;
> +
> +       mutex_lock(&power_domains->lock);
> +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> +               if (!power_well->is_enabled(dev, power_well)) {
> +                       is_enabled = false;
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&power_domains->lock);
> +
> +       return is_enabled;
> +}
> +
> +static void hsw_set_power_well(struct drm_device *dev,
> +                              struct i915_power_well *power_well, bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         bool is_enabled, enable_requested;
> @@ -5711,16 +5750,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>  static void __intel_power_well_get(struct drm_device *dev,
>                                    struct i915_power_well *power_well)
>  {
> -       if (!power_well->count++)
> -               __intel_set_power_well(dev, true);
> +       if (!power_well->count++ && power_well->set)
> +               power_well->set(dev, power_well, true);

The "set" function always exists for all power wells, so we shouldn't
check for it. It's better if we get a segfault that tells us we forgot
to implement the set() function than if we just silently not run
anything.


>  }
>
>  static void __intel_power_well_put(struct drm_device *dev,
>                                    struct i915_power_well *power_well)
>  {
>         WARN_ON(!power_well->count);
> -       if (!--power_well->count && i915_disable_power_well)
> -               __intel_set_power_well(dev, false);
> +
> +       if (!--power_well->count && power_well->set && i915_disable_power_well)

Same here.


> +               power_well->set(dev, power_well, false);
>  }
>
>  void intel_display_power_get(struct drm_device *dev,
> @@ -5728,6 +5768,8 @@ void intel_display_power_get(struct drm_device *dev,
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains;
> +       struct i915_power_well *power_well;
> +       int i;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5738,7 +5780,8 @@ void intel_display_power_get(struct drm_device *dev,
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> -       __intel_power_well_get(dev, &power_domains->power_wells[0]);
> +       for_each_power_well(i, power_well, BIT(domain), power_domains)
> +               __intel_power_well_get(dev, power_well);
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5747,6 +5790,8 @@ void intel_display_power_put(struct drm_device *dev,
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains;
> +       struct i915_power_well *power_well;
> +       int i;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5757,7 +5802,8 @@ void intel_display_power_put(struct drm_device *dev,
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> -       __intel_power_well_put(dev, &power_domains->power_wells[0]);
> +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> +               __intel_power_well_put(dev, power_well);
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5791,17 +5837,52 @@ void i915_release_power_well(void)
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>
> +static struct i915_power_well hsw_power_wells[] = {
> +       {
> +               .name = "display",
> +               .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
> +               .is_enabled = hsw_power_well_enabled,
> +               .set = hsw_set_power_well,
> +       },
> +};
> +
> +static struct i915_power_well bdw_power_wells[] = {
> +       {
> +               .name = "display",
> +               .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
> +               .is_enabled = hsw_power_well_enabled,
> +               .set = hsw_set_power_well,
> +       },
> +};
> +
> +#define set_power_wells(power_domains, __power_wells) ({               \
> +       (power_domains)->power_wells = (__power_wells);                 \
> +       (power_domains)->power_well_count = ARRAY_SIZE(__power_wells);  \
> +})

IMHO this macro is just over-complicating things. Please just do the
two assignments directly instead of calling the macro.


> +
>  int intel_power_domains_init(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> -       struct i915_power_well *power_well;
> +
> +       if (!HAS_POWER_WELL(dev))
> +               return 0;
>
>         mutex_init(&power_domains->lock);
> -       hsw_pwr = power_domains;
>
> -       power_well = &power_domains->power_wells[0];
> -       power_well->count = 0;
> +       /*
> +        * The enabling order will be from lower to higher indexed wells,
> +        * the disabling order is reversed.
> +        */
> +       if (IS_HASWELL(dev)) {
> +               set_power_wells(power_domains, hsw_power_wells);
> +               hsw_pwr = power_domains;
> +       } else if (IS_BROADWELL(dev)) {
> +               set_power_wells(power_domains, bdw_power_wells);
> +               hsw_pwr = power_domains;

I wonder if there's a way to treat Haswell and Broadwell as exactly
the same thing, except for one single "if" statement somewhere.

> +       } else {
> +               WARN_ON(1);
> +       }
>
>         return 0;
>  }
> @@ -5816,15 +5897,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
>         struct i915_power_well *power_well;
> +       int i;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
>
>         mutex_lock(&power_domains->lock);
> -
> -       power_well = &power_domains->power_wells[0];
> -       __intel_set_power_well(dev, power_well->count > 0);
> -
> +       for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> +               if (power_well->set)

Same here: the set() function should always exist.

> +                       power_well->set(dev, power_well, power_well->count > 0);
> +       }
>         mutex_unlock(&power_domains->lock);
>  }
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 2/8] drm/i915: support for multiple power wells

[Imre Deak]

On Fri, 2013-11-22 at 13:53 -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > HW generations so far had only one always-on power well and optionally
> > one dynamic power well. Upcoming HW gens may have multiple dynamic power
> > wells, so add some infrastructure to support them.
> >
> > The idea is to keep the existing power domain API used by the rest of
> > the driver and create a mapping between these power domains and the
> > underlying power wells. This mapping can differ from one HW to another
> > but high level driver code doesn't need to know about this. Through the
> > existing get/put API it would just ask for a given power domain and the
> > power domain framework would make sure the relevant power wells get
> > enabled in the right order.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  12 +++-
> >  drivers/gpu/drm/i915/intel_pm.c | 128 ++++++++++++++++++++++++++++++++--------
> >  2 files changed, 114 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7007b9b..b20016c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -945,21 +945,27 @@ struct intel_ilk_power_mgmt {
> >
> >  /* Power well structure for haswell */
> >  struct i915_power_well {
> > +       const char *name;
> >         /* power well enable/disable usage count */
> >         int count;
> > +       unsigned long domains;
> > +       void *data;
> 
> This "data" field is completely unused.
> 
> 
> > +       void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
> > +                   bool enable);
> > +       bool (*is_enabled)(struct drm_device *dev,
> > +                          struct i915_power_well *power_well);
> 
> The "power_well" argument of both functions is also unused.

These are for platforms that handle multiple power wells with a
single .set handler, determining the exact power well via the .data
member. This should've been in the log as it's not clear from the
current patchset.

> >  };
> >
> > -#define I915_MAX_POWER_WELLS 1
> > -
> >  struct i915_power_domains {
> >         /*
> >          * Power wells needed for initialization at driver init and suspend
> >          * time are on. They are kept on until after the first modeset.
> >          */
> >         bool init_power_on;
> > +       int power_well_count;
> >
> >         struct mutex lock;
> > -       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> > +       struct i915_power_well *power_wells;
> >  };
> >
> >  struct i915_dri1_state {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bcca995..2b39a9c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5625,27 +5625,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
> >         return BIT(domain) & always_on_domains;
> >  }
> >
> > +#define for_each_power_well(i, power_well, domain_mask, power_domains) \
> > +       for (i = 0;                                                     \
> > +            i < (power_domains)->power_well_count &&                   \
> > +                ((power_well) = &(power_domains)->power_wells[i]);     \
> > +            i++)                                                       \
> > +               if ((power_well)->domains & (domain_mask))
> > +
> > +#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
> > +       for (i = (power_domains)->power_well_count - 1;                  \
> > +            i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> > +            i--)                                                        \
> > +               if ((power_well)->domains & (domain_mask))
> > +
> >  /**
> >   * We should only use the power well if we explicitly asked the hardware to
> >   * enable it, so check if it's enabled and also check if we've requested it to
> >   * be enabled.
> >   */
> > -bool intel_display_power_enabled(struct drm_device *dev,
> > -                                enum intel_display_power_domain domain)
> > +static bool hsw_power_well_enabled(struct drm_device *dev,
> > +                                  struct i915_power_well *power_well)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > -       if (!HAS_POWER_WELL(dev))
> > -               return true;
> > -
> > -       if (is_always_on_power_domain(dev, domain))
> > -               return true;
> > -
> >         return I915_READ(HSW_PWR_WELL_DRIVER) ==
> >                      (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
> >  }
> >
> > -static void __intel_set_power_well(struct drm_device *dev, bool enable)
> > +bool intel_display_power_enabled(struct drm_device *dev,
> > +                                enum intel_display_power_domain domain)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_domains *power_domains;
> > +       struct i915_power_well *power_well;
> > +       bool is_enabled;
> > +       int i;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return true;
> > +
> > +       if (is_always_on_power_domain(dev, domain))
> > +               return true;
> > +
> > +       power_domains = &dev_priv->power_domains;
> > +
> > +       is_enabled = true;
> > +
> > +       mutex_lock(&power_domains->lock);
> > +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> > +               if (!power_well->is_enabled(dev, power_well)) {
> > +                       is_enabled = false;
> > +                       break;
> > +               }
> > +       }
> > +       mutex_unlock(&power_domains->lock);
> > +
> > +       return is_enabled;
> > +}
> > +
> > +static void hsw_set_power_well(struct drm_device *dev,
> > +                              struct i915_power_well *power_well, bool enable)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         bool is_enabled, enable_requested;
> > @@ -5711,16 +5750,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> >  static void __intel_power_well_get(struct drm_device *dev,
> >                                    struct i915_power_well *power_well)
> >  {
> > -       if (!power_well->count++)
> > -               __intel_set_power_well(dev, true);
> > +       if (!power_well->count++ && power_well->set)
> > +               power_well->set(dev, power_well, true);
> 
> The "set" function always exists for all power wells, so we shouldn't
> check for it. It's better if we get a segfault that tells us we forgot
> to implement the set() function than if we just silently not run
> anything.

These are for the default power wells w/o any need to poke at the HW as
you later point out.

> >  }
> >
> >  static void __intel_power_well_put(struct drm_device *dev,
> >                                    struct i915_power_well *power_well)
> >  {
> >         WARN_ON(!power_well->count);
> > -       if (!--power_well->count && i915_disable_power_well)
> > -               __intel_set_power_well(dev, false);
> > +
> > +       if (!--power_well->count && power_well->set && i915_disable_power_well)
> 
> Same here.
> 
> 
> > +               power_well->set(dev, power_well, false);
> >  }
> >
> >  void intel_display_power_get(struct drm_device *dev,
> > @@ -5728,6 +5768,8 @@ void intel_display_power_get(struct drm_device *dev,
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains;
> > +       struct i915_power_well *power_well;
> > +       int i;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> > @@ -5738,7 +5780,8 @@ void intel_display_power_get(struct drm_device *dev,
> >         power_domains = &dev_priv->power_domains;
> >
> >         mutex_lock(&power_domains->lock);
> > -       __intel_power_well_get(dev, &power_domains->power_wells[0]);
> > +       for_each_power_well(i, power_well, BIT(domain), power_domains)
> > +               __intel_power_well_get(dev, power_well);
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > @@ -5747,6 +5790,8 @@ void intel_display_power_put(struct drm_device *dev,
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains;
> > +       struct i915_power_well *power_well;
> > +       int i;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> > @@ -5757,7 +5802,8 @@ void intel_display_power_put(struct drm_device *dev,
> >         power_domains = &dev_priv->power_domains;
> >
> >         mutex_lock(&power_domains->lock);
> > -       __intel_power_well_put(dev, &power_domains->power_wells[0]);
> > +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> > +               __intel_power_well_put(dev, power_well);
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > @@ -5791,17 +5837,52 @@ void i915_release_power_well(void)
> >  }
> >  EXPORT_SYMBOL_GPL(i915_release_power_well);
> >
> > +static struct i915_power_well hsw_power_wells[] = {
> > +       {
> > +               .name = "display",
> > +               .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
> > +               .is_enabled = hsw_power_well_enabled,
> > +               .set = hsw_set_power_well,
> > +       },
> > +};
> > +
> > +static struct i915_power_well bdw_power_wells[] = {
> > +       {
> > +               .name = "display",
> > +               .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
> > +               .is_enabled = hsw_power_well_enabled,
> > +               .set = hsw_set_power_well,
> > +       },
> > +};
> > +
> > +#define set_power_wells(power_domains, __power_wells) ({               \
> > +       (power_domains)->power_wells = (__power_wells);                 \
> > +       (power_domains)->power_well_count = ARRAY_SIZE(__power_wells);  \
> > +})
> 
> IMHO this macro is just over-complicating things. Please just do the
> two assignments directly instead of calling the macro.

At the moment yes, but we'll have quite a few more power wells later
where we need to do the same thing. Again not obvious from this patchset
alone.

> > +
> >  int intel_power_domains_init(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > -       struct i915_power_well *power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return 0;
> >
> >         mutex_init(&power_domains->lock);
> > -       hsw_pwr = power_domains;
> >
> > -       power_well = &power_domains->power_wells[0];
> > -       power_well->count = 0;
> > +       /*
> > +        * The enabling order will be from lower to higher indexed wells,
> > +        * the disabling order is reversed.
> > +        */
> > +       if (IS_HASWELL(dev)) {
> > +               set_power_wells(power_domains, hsw_power_wells);
> > +               hsw_pwr = power_domains;
> > +       } else if (IS_BROADWELL(dev)) {
> > +               set_power_wells(power_domains, bdw_power_wells);
> > +               hsw_pwr = power_domains;
> 
> I wonder if there's a way to treat Haswell and Broadwell as exactly
> the same thing, except for one single "if" statement somewhere.

They are the same except a different .domains mask.

--Imre

> > +       } else {
> > +               WARN_ON(1);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -5816,15 +5897,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >         struct i915_power_well *power_well;
> > +       int i;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> >
> >         mutex_lock(&power_domains->lock);
> > -
> > -       power_well = &power_domains->power_wells[0];
> > -       __intel_set_power_well(dev, power_well->count > 0);
> > -
> > +       for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> > +               if (power_well->set)
> 
> Same here: the set() function should always exist.
> 
> > +                       power_well->set(dev, power_well, power_well->count > 0);
> > +       }
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

Re: [PATCH v2 2/8] drm/i915: support for multiple power wells

[Paulo Zanoni]

2013/11/22 Imre Deak <imre.deak@intel.com>:
> On Fri, 2013-11-22 at 13:53 -0200, Paulo Zanoni wrote:
>> 2013/11/14 Imre Deak <imre.deak@intel.com>:
>> > HW generations so far had only one always-on power well and optionally
>> > one dynamic power well. Upcoming HW gens may have multiple dynamic power
>> > wells, so add some infrastructure to support them.
>> >
>> > The idea is to keep the existing power domain API used by the rest of
>> > the driver and create a mapping between these power domains and the
>> > underlying power wells. This mapping can differ from one HW to another
>> > but high level driver code doesn't need to know about this. Through the
>> > existing get/put API it would just ask for a given power domain and the
>> > power domain framework would make sure the relevant power wells get
>> > enabled in the right order.
>> >
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h |  12 +++-
>> >  drivers/gpu/drm/i915/intel_pm.c | 128 ++++++++++++++++++++++++++++++++--------
>> >  2 files changed, 114 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > index 7007b9b..b20016c 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -945,21 +945,27 @@ struct intel_ilk_power_mgmt {
>> >
>> >  /* Power well structure for haswell */
>> >  struct i915_power_well {
>> > +       const char *name;
>> >         /* power well enable/disable usage count */
>> >         int count;
>> > +       unsigned long domains;
>> > +       void *data;
>>
>> This "data" field is completely unused.
>>
>>
>> > +       void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
>> > +                   bool enable);
>> > +       bool (*is_enabled)(struct drm_device *dev,
>> > +                          struct i915_power_well *power_well);
>>
>> The "power_well" argument of both functions is also unused.
>
> These are for platforms that handle multiple power wells with a
> single .set handler, determining the exact power well via the .data
> member. This should've been in the log as it's not clear from the
> current patchset.

Oh, ok. My main worry with this kind of approach is that, for example,
in the future we may bikeshed the patch that uses the "data" field so
it will no longer use that field, then we'll forget to remove the
now-unused "data" field.


>
>> >  };
>> >
>> > -#define I915_MAX_POWER_WELLS 1
>> > -
>> >  struct i915_power_domains {
>> >         /*
>> >          * Power wells needed for initialization at driver init and suspend
>> >          * time are on. They are kept on until after the first modeset.
>> >          */
>> >         bool init_power_on;
>> > +       int power_well_count;
>> >
>> >         struct mutex lock;
>> > -       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
>> > +       struct i915_power_well *power_wells;
>> >  };
>> >
>> >  struct i915_dri1_state {
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > index bcca995..2b39a9c 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -5625,27 +5625,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
>> >         return BIT(domain) & always_on_domains;
>> >  }
>> >
>> > +#define for_each_power_well(i, power_well, domain_mask, power_domains) \
>> > +       for (i = 0;                                                     \
>> > +            i < (power_domains)->power_well_count &&                   \
>> > +                ((power_well) = &(power_domains)->power_wells[i]);     \
>> > +            i++)                                                       \
>> > +               if ((power_well)->domains & (domain_mask))
>> > +
>> > +#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
>> > +       for (i = (power_domains)->power_well_count - 1;                  \
>> > +            i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
>> > +            i--)                                                        \
>> > +               if ((power_well)->domains & (domain_mask))
>> > +
>> >  /**
>> >   * We should only use the power well if we explicitly asked the hardware to
>> >   * enable it, so check if it's enabled and also check if we've requested it to
>> >   * be enabled.
>> >   */
>> > -bool intel_display_power_enabled(struct drm_device *dev,
>> > -                                enum intel_display_power_domain domain)
>> > +static bool hsw_power_well_enabled(struct drm_device *dev,
>> > +                                  struct i915_power_well *power_well)
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >
>> > -       if (!HAS_POWER_WELL(dev))
>> > -               return true;
>> > -
>> > -       if (is_always_on_power_domain(dev, domain))
>> > -               return true;
>> > -
>> >         return I915_READ(HSW_PWR_WELL_DRIVER) ==
>> >                      (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>> >  }
>> >
>> > -static void __intel_set_power_well(struct drm_device *dev, bool enable)
>> > +bool intel_display_power_enabled(struct drm_device *dev,
>> > +                                enum intel_display_power_domain domain)
>> > +{
>> > +       struct drm_i915_private *dev_priv = dev->dev_private;
>> > +       struct i915_power_domains *power_domains;
>> > +       struct i915_power_well *power_well;
>> > +       bool is_enabled;
>> > +       int i;
>> > +
>> > +       if (!HAS_POWER_WELL(dev))
>> > +               return true;
>> > +
>> > +       if (is_always_on_power_domain(dev, domain))
>> > +               return true;
>> > +
>> > +       power_domains = &dev_priv->power_domains;
>> > +
>> > +       is_enabled = true;
>> > +
>> > +       mutex_lock(&power_domains->lock);
>> > +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>> > +               if (!power_well->is_enabled(dev, power_well)) {
>> > +                       is_enabled = false;
>> > +                       break;
>> > +               }
>> > +       }
>> > +       mutex_unlock(&power_domains->lock);
>> > +
>> > +       return is_enabled;
>> > +}
>> > +
>> > +static void hsw_set_power_well(struct drm_device *dev,
>> > +                              struct i915_power_well *power_well, bool enable)
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >         bool is_enabled, enable_requested;
>> > @@ -5711,16 +5750,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>> >  static void __intel_power_well_get(struct drm_device *dev,
>> >                                    struct i915_power_well *power_well)
>> >  {
>> > -       if (!power_well->count++)
>> > -               __intel_set_power_well(dev, true);
>> > +       if (!power_well->count++ && power_well->set)
>> > +               power_well->set(dev, power_well, true);
>>
>> The "set" function always exists for all power wells, so we shouldn't
>> check for it. It's better if we get a segfault that tells us we forgot
>> to implement the set() function than if we just silently not run
>> anything.
>
> These are for the default power wells w/o any need to poke at the HW as
> you later point out.
>
>> >  }
>> >
>> >  static void __intel_power_well_put(struct drm_device *dev,
>> >                                    struct i915_power_well *power_well)
>> >  {
>> >         WARN_ON(!power_well->count);
>> > -       if (!--power_well->count && i915_disable_power_well)
>> > -               __intel_set_power_well(dev, false);
>> > +
>> > +       if (!--power_well->count && power_well->set && i915_disable_power_well)
>>
>> Same here.
>>
>>
>> > +               power_well->set(dev, power_well, false);
>> >  }
>> >
>> >  void intel_display_power_get(struct drm_device *dev,
>> > @@ -5728,6 +5768,8 @@ void intel_display_power_get(struct drm_device *dev,
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >         struct i915_power_domains *power_domains;
>> > +       struct i915_power_well *power_well;
>> > +       int i;
>> >
>> >         if (!HAS_POWER_WELL(dev))
>> >                 return;
>> > @@ -5738,7 +5780,8 @@ void intel_display_power_get(struct drm_device *dev,
>> >         power_domains = &dev_priv->power_domains;
>> >
>> >         mutex_lock(&power_domains->lock);
>> > -       __intel_power_well_get(dev, &power_domains->power_wells[0]);
>> > +       for_each_power_well(i, power_well, BIT(domain), power_domains)
>> > +               __intel_power_well_get(dev, power_well);
>> >         mutex_unlock(&power_domains->lock);
>> >  }
>> >
>> > @@ -5747,6 +5790,8 @@ void intel_display_power_put(struct drm_device *dev,
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >         struct i915_power_domains *power_domains;
>> > +       struct i915_power_well *power_well;
>> > +       int i;
>> >
>> >         if (!HAS_POWER_WELL(dev))
>> >                 return;
>> > @@ -5757,7 +5802,8 @@ void intel_display_power_put(struct drm_device *dev,
>> >         power_domains = &dev_priv->power_domains;
>> >
>> >         mutex_lock(&power_domains->lock);
>> > -       __intel_power_well_put(dev, &power_domains->power_wells[0]);
>> > +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
>> > +               __intel_power_well_put(dev, power_well);
>> >         mutex_unlock(&power_domains->lock);
>> >  }
>> >
>> > @@ -5791,17 +5837,52 @@ void i915_release_power_well(void)
>> >  }
>> >  EXPORT_SYMBOL_GPL(i915_release_power_well);
>> >
>> > +static struct i915_power_well hsw_power_wells[] = {
>> > +       {
>> > +               .name = "display",
>> > +               .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
>> > +               .is_enabled = hsw_power_well_enabled,
>> > +               .set = hsw_set_power_well,
>> > +       },
>> > +};
>> > +
>> > +static struct i915_power_well bdw_power_wells[] = {
>> > +       {
>> > +               .name = "display",
>> > +               .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
>> > +               .is_enabled = hsw_power_well_enabled,
>> > +               .set = hsw_set_power_well,
>> > +       },
>> > +};
>> > +
>> > +#define set_power_wells(power_domains, __power_wells) ({               \
>> > +       (power_domains)->power_wells = (__power_wells);                 \
>> > +       (power_domains)->power_well_count = ARRAY_SIZE(__power_wells);  \
>> > +})
>>
>> IMHO this macro is just over-complicating things. Please just do the
>> two assignments directly instead of calling the macro.
>
> At the moment yes, but we'll have quite a few more power wells later
> where we need to do the same thing. Again not obvious from this patchset
> alone.
>
>> > +
>> >  int intel_power_domains_init(struct drm_device *dev)
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >         struct i915_power_domains *power_domains = &dev_priv->power_domains;
>> > -       struct i915_power_well *power_well;
>> > +
>> > +       if (!HAS_POWER_WELL(dev))
>> > +               return 0;
>> >
>> >         mutex_init(&power_domains->lock);
>> > -       hsw_pwr = power_domains;
>> >
>> > -       power_well = &power_domains->power_wells[0];
>> > -       power_well->count = 0;
>> > +       /*
>> > +        * The enabling order will be from lower to higher indexed wells,
>> > +        * the disabling order is reversed.
>> > +        */
>> > +       if (IS_HASWELL(dev)) {
>> > +               set_power_wells(power_domains, hsw_power_wells);
>> > +               hsw_pwr = power_domains;
>> > +       } else if (IS_BROADWELL(dev)) {
>> > +               set_power_wells(power_domains, bdw_power_wells);
>> > +               hsw_pwr = power_domains;
>>
>> I wonder if there's a way to treat Haswell and Broadwell as exactly
>> the same thing, except for one single "if" statement somewhere.
>
> They are the same except a different .domains mask.
>
> --Imre
>
>> > +       } else {
>> > +               WARN_ON(1);
>> > +       }
>> >
>> >         return 0;
>> >  }
>> > @@ -5816,15 +5897,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >         struct i915_power_domains *power_domains = &dev_priv->power_domains;
>> >         struct i915_power_well *power_well;
>> > +       int i;
>> >
>> >         if (!HAS_POWER_WELL(dev))
>> >                 return;
>> >
>> >         mutex_lock(&power_domains->lock);
>> > -
>> > -       power_well = &power_domains->power_wells[0];
>> > -       __intel_set_power_well(dev, power_well->count > 0);
>> > -
>> > +       for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
>> > +               if (power_well->set)
>>
>> Same here: the set() function should always exist.
>>
>> > +                       power_well->set(dev, power_well, power_well->count > 0);
>> > +       }
>> >         mutex_unlock(&power_domains->lock);
>> >  }
>> >
>> > --
>> > 1.8.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>
>



-- 
Paulo Zanoni

[PATCH v2 3/8] drm/i915: add always-on power wells instead of special casing them

[Imre Deak]

Instead of using a separate function to check whether a power domain is
is always on, add an always-on power well covering all these power
domains and do the usual get/put on these unconditionally. Since we
don't assign a .set handler for these the get/put won't have any effect
besides the adjusted refcount.

This makes the code more readable and provides debug info also on the
use of always-on power wells (once the relevant debugfs entry is added.)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++----------------------------
 2 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b20016c..ff3314d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt {
 /* Power well structure for haswell */
 struct i915_power_well {
 	const char *name;
+	unsigned always_on:1;
 	/* power well enable/disable usage count */
 	int count;
 	unsigned long domains;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2b39a9c..ee5aeb1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5606,25 +5606,6 @@ void intel_suspend_hw(struct drm_device *dev)
 		lpt_suspend_hw(dev);
 }
 
-static bool is_always_on_power_domain(struct drm_device *dev,
-				      enum intel_display_power_domain domain)
-{
-	unsigned long always_on_domains;
-
-	BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK);
-
-	if (IS_BROADWELL(dev)) {
-		always_on_domains = BDW_ALWAYS_ON_POWER_DOMAINS;
-	} else if (IS_HASWELL(dev)) {
-		always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS;
-	} else {
-		WARN_ON(1);
-		return true;
-	}
-
-	return BIT(domain) & always_on_domains;
-}
-
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
 	for (i = 0;							\
 	     i < (power_domains)->power_well_count &&			\
@@ -5664,15 +5645,15 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return true;
 
-	if (is_always_on_power_domain(dev, domain))
-		return true;
-
 	power_domains = &dev_priv->power_domains;
 
 	is_enabled = true;
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
+		if (power_well->always_on)
+			continue;
+
 		if (!power_well->is_enabled(dev, power_well)) {
 			is_enabled = false;
 			break;
@@ -5774,9 +5755,6 @@ void intel_display_power_get(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	if (is_always_on_power_domain(dev, domain))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5796,9 +5774,6 @@ void intel_display_power_put(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	if (is_always_on_power_domain(dev, domain))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5839,6 +5814,11 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
 
 static struct i915_power_well hsw_power_wells[] = {
 	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = HSW_ALWAYS_ON_POWER_DOMAINS,
+	},
+	{
 		.name = "display",
 		.domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
 		.is_enabled = hsw_power_well_enabled,
@@ -5848,6 +5828,11 @@ static struct i915_power_well hsw_power_wells[] = {
 
 static struct i915_power_well bdw_power_wells[] = {
 	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = BDW_ALWAYS_ON_POWER_DOMAINS,
+	},
+	{
 		.name = "display",
 		.domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
 		.is_enabled = hsw_power_well_enabled,
-- 
1.8.4

Re: [PATCH v2 3/8] drm/i915: add always-on power wells instead of special casing them

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak@intel.com>:
> Instead of using a separate function to check whether a power domain is
> is always on, add an always-on power well covering all these power
> domains and do the usual get/put on these unconditionally. Since we
> don't assign a .set handler for these the get/put won't have any effect
> besides the adjusted refcount.

Oh, now I see why you had all those checks for the existence of the
"set" function :)


>
> This makes the code more readable and provides debug info also on the
> use of always-on power wells (once the relevant debugfs entry is added.)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++----------------------------
>  2 files changed, 14 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b20016c..ff3314d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt {
>  /* Power well structure for haswell */
>  struct i915_power_well {
>         const char *name;
> +       unsigned always_on:1;

On our driver we have many cases where we just use many "bool"
variables, and we also have many cases where we use single-bit
variables like this. On this specific case we're not gaining anything
by using the single-bit variable, so I'm not sure if it's the most
appropriate thing to use. I wish we had a guideline telling us which
one is preferred on each case :)

Anyway: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>         /* power well enable/disable usage count */
>         int count;
>         unsigned long domains;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2b39a9c..ee5aeb1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5606,25 +5606,6 @@ void intel_suspend_hw(struct drm_device *dev)
>                 lpt_suspend_hw(dev);
>  }
>
> -static bool is_always_on_power_domain(struct drm_device *dev,
> -                                     enum intel_display_power_domain domain)
> -{
> -       unsigned long always_on_domains;
> -
> -       BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK);
> -
> -       if (IS_BROADWELL(dev)) {
> -               always_on_domains = BDW_ALWAYS_ON_POWER_DOMAINS;
> -       } else if (IS_HASWELL(dev)) {
> -               always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS;
> -       } else {
> -               WARN_ON(1);
> -               return true;
> -       }
> -
> -       return BIT(domain) & always_on_domains;
> -}
> -
>  #define for_each_power_well(i, power_well, domain_mask, power_domains) \
>         for (i = 0;                                                     \
>              i < (power_domains)->power_well_count &&                   \
> @@ -5664,15 +5645,15 @@ bool intel_display_power_enabled(struct drm_device *dev,
>         if (!HAS_POWER_WELL(dev))
>                 return true;
>
> -       if (is_always_on_power_domain(dev, domain))
> -               return true;
> -
>         power_domains = &dev_priv->power_domains;
>
>         is_enabled = true;
>
>         mutex_lock(&power_domains->lock);
>         for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> +               if (power_well->always_on)
> +                       continue;
> +
>                 if (!power_well->is_enabled(dev, power_well)) {
>                         is_enabled = false;
>                         break;
> @@ -5774,9 +5755,6 @@ void intel_display_power_get(struct drm_device *dev,
>         if (!HAS_POWER_WELL(dev))
>                 return;
>
> -       if (is_always_on_power_domain(dev, domain))
> -               return;
> -
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> @@ -5796,9 +5774,6 @@ void intel_display_power_put(struct drm_device *dev,
>         if (!HAS_POWER_WELL(dev))
>                 return;
>
> -       if (is_always_on_power_domain(dev, domain))
> -               return;
> -
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> @@ -5839,6 +5814,11 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
>
>  static struct i915_power_well hsw_power_wells[] = {
>         {
> +               .name = "always-on",
> +               .always_on = 1,
> +               .domains = HSW_ALWAYS_ON_POWER_DOMAINS,
> +       },
> +       {
>                 .name = "display",
>                 .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
>                 .is_enabled = hsw_power_well_enabled,
> @@ -5848,6 +5828,11 @@ static struct i915_power_well hsw_power_wells[] = {
>
>  static struct i915_power_well bdw_power_wells[] = {
>         {
> +               .name = "always-on",
> +               .always_on = 1,
> +               .domains = BDW_ALWAYS_ON_POWER_DOMAINS,
> +       },
> +       {
>                 .name = "display",
>                 .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
>                 .is_enabled = hsw_power_well_enabled,
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 3/8] drm/i915: add always-on power wells instead of special casing them

[Ville Syrjälä]

On Fri, Nov 22, 2013 at 02:04:02PM -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > Instead of using a separate function to check whether a power domain is
> > is always on, add an always-on power well covering all these power
> > domains and do the usual get/put on these unconditionally. Since we
> > don't assign a .set handler for these the get/put won't have any effect
> > besides the adjusted refcount.
> 
> Oh, now I see why you had all those checks for the existence of the
> "set" function :)
> 
> 
> >
> > This makes the code more readable and provides debug info also on the
> > use of always-on power wells (once the relevant debugfs entry is added.)
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++----------------------------
> >  2 files changed, 14 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b20016c..ff3314d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt {
> >  /* Power well structure for haswell */
> >  struct i915_power_well {
> >         const char *name;
> > +       unsigned always_on:1;
> 
> On our driver we have many cases where we just use many "bool"
> variables, and we also have many cases where we use single-bit
> variables like this. On this specific case we're not gaining anything
> by using the single-bit variable, so I'm not sure if it's the most
> appropriate thing to use. I wish we had a guideline telling us which
> one is preferred on each case :)

I wish we'd use 'bool foo:1' for single bit bitfields. Otherwise stuff
like 'foo = 2' doesn't work (you get false instead of true).

-- 
Ville Syrjälä
Intel OTC

[PATCH v2 4/8] drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL

[Imre Deak]

In intel_display_capture_error_state we use HAS_POWER_WELL to check if
we are running on Haswell/Broadwell when accessing HSW_PWR_WELL_DRIVER
which is specific to these platforms. Future platforms with power wells
don't have this register, so HAS_POWER_WELL won't work there any more.
Use IS_HASWELL/IS_BROADWELL instead.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2df2366..bb5e4e9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11370,7 +11370,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 	if (error == NULL)
 		return NULL;
 
-	if (HAS_POWER_WELL(dev))
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
@@ -11441,7 +11441,7 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 		return;
 
 	err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
-	if (HAS_POWER_WELL(dev))
+	if (IS_HASWELL(dev) | IS_BROADWELL(dev))
 		err_printf(m, "PWR_WELL_CTL2: %08x\n",
 			   error->power_well_driver);
 	for_each_pipe(i) {
-- 
1.8.4

Re: [PATCH v2 4/8] drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak@intel.com>:
> In intel_display_capture_error_state we use HAS_POWER_WELL to check if
> we are running on Haswell/Broadwell when accessing HSW_PWR_WELL_DRIVER
> which is specific to these platforms. Future platforms with power wells
> don't have this register, so HAS_POWER_WELL won't work there any more.
> Use IS_HASWELL/IS_BROADWELL instead.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2df2366..bb5e4e9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11370,7 +11370,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>         if (error == NULL)
>                 return NULL;
>
> -       if (HAS_POWER_WELL(dev))
> +       if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>                 error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>
>         for_each_pipe(i) {
> @@ -11441,7 +11441,7 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>                 return;
>
>         err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
> -       if (HAS_POWER_WELL(dev))
> +       if (IS_HASWELL(dev) | IS_BROADWELL(dev))

Please use the logical OR, instead of the bit operation.

Thanks,
Paulo

>                 err_printf(m, "PWR_WELL_CTL2: %08x\n",
>                            error->power_well_driver);
>         for_each_pipe(i) {
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 4/8] drm/i915: use IS_HASWELL/BROADWELL instead of HAS_POWER_WELL

[Imre Deak]

On Fri, 2013-11-22 at 13:41 -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > In intel_display_capture_error_state we use HAS_POWER_WELL to check if
> > we are running on Haswell/Broadwell when accessing HSW_PWR_WELL_DRIVER
> > which is specific to these platforms. Future platforms with power wells
> > don't have this register, so HAS_POWER_WELL won't work there any more.
> > Use IS_HASWELL/IS_BROADWELL instead.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2df2366..bb5e4e9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11370,7 +11370,7 @@ intel_display_capture_error_state(struct drm_device *dev)
> >         if (error == NULL)
> >                 return NULL;
> >
> > -       if (HAS_POWER_WELL(dev))
> > +       if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >                 error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
> >
> >         for_each_pipe(i) {
> > @@ -11441,7 +11441,7 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
> >                 return;
> >
> >         err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
> > -       if (HAS_POWER_WELL(dev))
> > +       if (IS_HASWELL(dev) | IS_BROADWELL(dev))
> 
> Please use the logical OR, instead of the bit operation.

Good catch, it was a typo.

--Imre

[PATCH v2 5/8] drm/i915: protect HSW power well check with IS_HASWELL in redisable_vga

[Imre Deak]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

This may need work if other platforms do the same thing, but in the
meantime we should avoid looking at HSW specific bits in this generic
function.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
[added IS_BROADWELL too as that needs the same handling (Imre)]
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bb5e4e9..7b28137 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11042,7 +11042,7 @@ void i915_redisable_vga(struct drm_device *dev)
 	 * level, just check if the power well is enabled instead of trying to
 	 * follow the "don't touch the power well if we don't need it" policy
 	 * the rest of the driver uses. */
-	if (HAS_POWER_WELL(dev) &&
+	if ((IS_HASWELL(dev) || IS_BROADWELL(dev)) &&
 	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE_ENABLED) == 0)
 		return;
 
-- 
1.8.4

[PATCH v2 6/8] drm/i915: don't do BDW/HSW specific powerdomains init on other platforms

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee5aeb1..d5eacd8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5912,6 +5912,9 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 	intel_display_set_init_power(dev, true);
 	intel_power_domains_resume(dev);
 
+	if (!(IS_HASWELL(dev) || IS_BROADWELL(dev)))
+		return;
+
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
 	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
-- 
1.8.4

Re: [PATCH v2 6/8] drm/i915: don't do BDW/HSW specific powerdomains init on other platforms

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak@intel.com>:
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Since I assume all power wells will require some kind of
register-checking and hardware-touching at the init paths, shouldn't
we add power_well->init_hw() and call it once for each power well? The
problem with this series is that we're doing code changes for a case
we still didn't try to implement (multiple power wells), so sometimes
it gets hard to predict what exactly will be needed/used.

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee5aeb1..d5eacd8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5912,6 +5912,9 @@ void intel_power_domains_init_hw(struct drm_device *dev)
>         intel_display_set_init_power(dev, true);
>         intel_power_domains_resume(dev);
>
> +       if (!(IS_HASWELL(dev) || IS_BROADWELL(dev)))
> +               return;
> +
>         /* We're taking over the BIOS, so clear any requests made by it since
>          * the driver is in charge now. */
>         if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 6/8] drm/i915: don't do BDW/HSW specific powerdomains init on other platforms

[Imre Deak]

On Fri, 2013-11-22 at 14:09 -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Since I assume all power wells will require some kind of
> register-checking and hardware-touching at the init paths, shouldn't
> we add power_well->init_hw() and call it once for each power well?

So far HSW/BDW is the only instance that I know of, so we'd end up
having empty stubs for the rest.

> The problem with this series is that we're doing code changes for a case
> we still didn't try to implement (multiple power wells), so sometimes
> it gets hard to predict what exactly will be needed/used.

Yea, I realize this is somewhat of a problem. This a preparation for
future stuff, for which I do have an implementation only in my local
tree.. But I tried to make clarifications in the logs to this effect. 

--Imre

> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ee5aeb1..d5eacd8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5912,6 +5912,9 @@ void intel_power_domains_init_hw(struct drm_device *dev)
> >         intel_display_set_init_power(dev, true);
> >         intel_power_domains_resume(dev);
> >
> > +       if (!(IS_HASWELL(dev) || IS_BROADWELL(dev)))
> > +               return;
> > +
> >         /* We're taking over the BIOS, so clear any requests made by it since
> >          * the driver is in charge now. */
> >         if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 7/8] drm/i915: add a default always-on power well

[Imre Deak]

So far we distinguished platforms without a dynamic power well with
the HAS_POWER_WELL macro and for such platforms we didn't call any power
domain functions. Instead of doing this check we can add an always-on
power well for these platforms and call the power domain functions
unconditionally. For always-on power wells we only increase/decrease
their refcounts, otherwise they are nop.

This makes high level driver code more readable and as a bonus provides
some idea of the current power domains state for all platforms (once
the relevant debugfs entry is added).

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 18 +++++++-----------
 drivers/gpu/drm/i915/i915_drv.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++-------------------
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 00d74f8..95dd3db 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1639,8 +1639,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 			goto out_gem_unload;
 	}
 
-	if (HAS_POWER_WELL(dev))
-		intel_power_domains_init(dev);
+	intel_power_domains_init(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
@@ -1667,8 +1666,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	return 0;
 
 out_power_well:
-	if (HAS_POWER_WELL(dev))
-		intel_power_domains_remove(dev);
+	intel_power_domains_remove(dev);
 	drm_vblank_cleanup(dev);
 out_gem_unload:
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
@@ -1706,13 +1704,11 @@ int i915_driver_unload(struct drm_device *dev)
 
 	intel_gpu_ips_teardown();
 
-	if (HAS_POWER_WELL(dev)) {
-		/* The i915.ko module is still not prepared to be loaded when
-		 * the power well is not enabled, so just enable it in case
-		 * we're going to unload/reload. */
-		intel_display_set_init_power(dev, true);
-		intel_power_domains_remove(dev);
-	}
+	/* The i915.ko module is still not prepared to be loaded when
+	 * the power well is not enabled, so just enable it in case
+	 * we're going to unload/reload. */
+	intel_display_set_init_power(dev, true);
+	intel_power_domains_remove(dev);
 
 	i915_teardown_sysfs(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff3314d..06f47bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1834,7 +1834,6 @@ struct drm_i915_file_private {
 #define HAS_IPS(dev)		(IS_ULT(dev) || IS_BROADWELL(dev))
 
 #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
-#define HAS_POWER_WELL(dev)	(IS_HASWELL(dev) || IS_BROADWELL(dev))
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d5eacd8..d252453 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5642,9 +5642,6 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	bool is_enabled;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return true;
-
 	power_domains = &dev_priv->power_domains;
 
 	is_enabled = true;
@@ -5752,9 +5749,6 @@ void intel_display_power_get(struct drm_device *dev,
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5771,9 +5765,6 @@ void intel_display_power_put(struct drm_device *dev,
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5812,6 +5803,14 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+static struct i915_power_well intel_power_wells[] = {
+	{
+		.name = "always-on",
+		.always_on = 1,
+		.domains = POWER_DOMAIN_MASK,
+	},
+};
+
 static struct i915_power_well hsw_power_wells[] = {
 	{
 		.name = "always-on",
@@ -5850,9 +5849,6 @@ int intel_power_domains_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 
-	if (!HAS_POWER_WELL(dev))
-		return 0;
-
 	mutex_init(&power_domains->lock);
 
 	/*
@@ -5866,7 +5862,7 @@ int intel_power_domains_init(struct drm_device *dev)
 		set_power_wells(power_domains, bdw_power_wells);
 		hsw_pwr = power_domains;
 	} else {
-		WARN_ON(1);
+		set_power_wells(power_domains, intel_power_wells);
 	}
 
 	return 0;
@@ -5884,9 +5880,6 @@ static void intel_power_domains_resume(struct drm_device *dev)
 	struct i915_power_well *power_well;
 	int i;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	mutex_lock(&power_domains->lock);
 	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
 		if (power_well->set)
@@ -5905,9 +5898,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
 	/* For now, we need the power well to be always enabled. */
 	intel_display_set_init_power(dev, true);
 	intel_power_domains_resume(dev);
-- 
1.8.4

Re: [PATCH v2 7/8] drm/i915: add a default always-on power well

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak@intel.com>:
> So far we distinguished platforms without a dynamic power well with
> the HAS_POWER_WELL macro and for such platforms we didn't call any power
> domain functions. Instead of doing this check we can add an always-on
> power well for these platforms and call the power domain functions
> unconditionally. For always-on power wells we only increase/decrease
> their refcounts, otherwise they are nop.
>
> This makes high level driver code more readable and as a bonus provides
> some idea of the current power domains state for all platforms (once
> the relevant debugfs entry is added).

I took a similar approach with the PC8 code - platforms that don't
support PC8 would never reach zero refcount - but Chris didn't like it
and then sent a patch that added lots of "if (!HAS_PC8(dev))" checks
everywhere. I personally don't have any strong opinions: I'm fine
either with or without this patch. If it helps us catch unbalanced
refcounts somehow, then I'm all in favor of it :)


>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 18 +++++++-----------
>  drivers/gpu/drm/i915/i915_drv.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++-------------------
>  3 files changed, 16 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 00d74f8..95dd3db 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1639,8 +1639,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>                         goto out_gem_unload;
>         }
>
> -       if (HAS_POWER_WELL(dev))
> -               intel_power_domains_init(dev);
> +       intel_power_domains_init(dev);
>
>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>                 ret = i915_load_modeset_init(dev);
> @@ -1667,8 +1666,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>         return 0;
>
>  out_power_well:
> -       if (HAS_POWER_WELL(dev))
> -               intel_power_domains_remove(dev);
> +       intel_power_domains_remove(dev);
>         drm_vblank_cleanup(dev);
>  out_gem_unload:
>         if (dev_priv->mm.inactive_shrinker.scan_objects)
> @@ -1706,13 +1704,11 @@ int i915_driver_unload(struct drm_device *dev)
>
>         intel_gpu_ips_teardown();
>
> -       if (HAS_POWER_WELL(dev)) {
> -               /* The i915.ko module is still not prepared to be loaded when
> -                * the power well is not enabled, so just enable it in case
> -                * we're going to unload/reload. */
> -               intel_display_set_init_power(dev, true);
> -               intel_power_domains_remove(dev);
> -       }
> +       /* The i915.ko module is still not prepared to be loaded when
> +        * the power well is not enabled, so just enable it in case
> +        * we're going to unload/reload. */
> +       intel_display_set_init_power(dev, true);
> +       intel_power_domains_remove(dev);
>
>         i915_teardown_sysfs(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff3314d..06f47bf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1834,7 +1834,6 @@ struct drm_i915_file_private {
>  #define HAS_IPS(dev)           (IS_ULT(dev) || IS_BROADWELL(dev))
>
>  #define HAS_DDI(dev)           (INTEL_INFO(dev)->has_ddi)
> -#define HAS_POWER_WELL(dev)    (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  #define HAS_FPGA_DBG_UNCLAIMED(dev)    (INTEL_INFO(dev)->has_fpga_dbg)
>  #define HAS_PSR(dev)           (IS_HASWELL(dev) || IS_BROADWELL(dev))
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d5eacd8..d252453 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5642,9 +5642,6 @@ bool intel_display_power_enabled(struct drm_device *dev,
>         bool is_enabled;
>         int i;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return true;
> -
>         power_domains = &dev_priv->power_domains;
>
>         is_enabled = true;
> @@ -5752,9 +5749,6 @@ void intel_display_power_get(struct drm_device *dev,
>         struct i915_power_well *power_well;
>         int i;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return;
> -
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> @@ -5771,9 +5765,6 @@ void intel_display_power_put(struct drm_device *dev,
>         struct i915_power_well *power_well;
>         int i;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return;
> -
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> @@ -5812,6 +5803,14 @@ void i915_release_power_well(void)
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>
> +static struct i915_power_well intel_power_wells[] = {

I would rename this to something like "fake_power_wells",
"legacy_power_wells", "old_power_wells", "gen2_power_wells" or
something else.

With or without the rename: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +       {
> +               .name = "always-on",
> +               .always_on = 1,
> +               .domains = POWER_DOMAIN_MASK,
> +       },
> +};
> +
>  static struct i915_power_well hsw_power_wells[] = {
>         {
>                 .name = "always-on",
> @@ -5850,9 +5849,6 @@ int intel_power_domains_init(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return 0;
> -
>         mutex_init(&power_domains->lock);
>
>         /*
> @@ -5866,7 +5862,7 @@ int intel_power_domains_init(struct drm_device *dev)
>                 set_power_wells(power_domains, bdw_power_wells);
>                 hsw_pwr = power_domains;
>         } else {
> -               WARN_ON(1);
> +               set_power_wells(power_domains, intel_power_wells);
>         }
>
>         return 0;
> @@ -5884,9 +5880,6 @@ static void intel_power_domains_resume(struct drm_device *dev)
>         struct i915_power_well *power_well;
>         int i;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return;
> -
>         mutex_lock(&power_domains->lock);
>         for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
>                 if (power_well->set)
> @@ -5905,9 +5898,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       if (!HAS_POWER_WELL(dev))
> -               return;
> -
>         /* For now, we need the power well to be always enabled. */
>         intel_display_set_init_power(dev, true);
>         intel_power_domains_resume(dev);
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH v2 8/8] drm/i915: add a debugfs entry for power domain info

[Imre Deak]

Add a debugfs entry showing the use-count for all power domains of each
power well.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  4 +++
 drivers/gpu/drm/i915/intel_pm.c     | 16 ++++++---
 3 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6875b7a..a6555cd 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1844,6 +1844,74 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static const char *power_domain_str(enum intel_display_power_domain domain)
+{
+#define PWRD(p)	{ POWER_DOMAIN_ ## p, # p }
+	static struct {
+		enum intel_display_power_domain domain;
+		const char *name;
+	} table[] = {
+		PWRD(PIPE_A),
+		PWRD(PIPE_B),
+		PWRD(PIPE_C),
+		PWRD(PIPE_A_PANEL_FITTER),
+		PWRD(PIPE_B_PANEL_FITTER),
+		PWRD(PIPE_C_PANEL_FITTER),
+		PWRD(TRANSCODER_A),
+		PWRD(TRANSCODER_B),
+		PWRD(TRANSCODER_C),
+		PWRD(TRANSCODER_EDP),
+		PWRD(VGA),
+		PWRD(AUDIO),
+		PWRD(INIT),
+	};
+#undef PWRD
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(table); i++)
+		if (table[i].domain == domain)
+			return table[i].name;
+
+	WARN_ON(1);
+
+	return "?";
+}
+
+static int i915_power_domain_info(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	int i;
+
+	mutex_lock(&power_domains->lock);
+
+	seq_printf(m, "%-25s %s\n", "Power well/domain", "Use count");
+	for (i = 0; i < power_domains->power_well_count; i++) {
+		struct i915_power_well *power_well;
+		enum intel_display_power_domain power_domain;
+
+		power_well = &power_domains->power_wells[i];
+		seq_printf(m, "%-25s %d\n", power_well->name,
+			   power_well->count);
+
+		for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
+		     power_domain++) {
+			if (!(BIT(power_domain) & power_well->domains))
+				continue;
+
+			seq_printf(m, "  %-23s %d\n",
+				   power_domain_str(power_domain),
+				   power_well->domain_count[power_domain]);
+		}
+	}
+
+	mutex_unlock(&power_domains->lock);
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -3076,6 +3144,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
+	{"i915_power_domain_info", i915_power_domain_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 06f47bf..194e39f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -950,6 +950,10 @@ struct i915_power_well {
 	/* power well enable/disable usage count */
 	int count;
 	unsigned long domains;
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	/* usage count for each power domain in the domains mask */
+	int domain_count[POWER_DOMAIN_NUM];
+#endif
 	void *data;
 	void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
 		    bool enable);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d252453..99210c1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5726,17 +5726,25 @@ static void hsw_set_power_well(struct drm_device *dev,
 }
 
 static void __intel_power_well_get(struct drm_device *dev,
-				   struct i915_power_well *power_well)
+				   struct i915_power_well *power_well,
+				   enum intel_display_power_domain domain)
 {
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	power_well->domain_count[domain]++;
+#endif
 	if (!power_well->count++ && power_well->set)
 		power_well->set(dev, power_well, true);
 }
 
 static void __intel_power_well_put(struct drm_device *dev,
-				   struct i915_power_well *power_well)
+				   struct i915_power_well *power_well,
+				   enum intel_display_power_domain domain)
 {
 	WARN_ON(!power_well->count);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	power_well->domain_count[domain]--;
+#endif
 	if (!--power_well->count && power_well->set && i915_disable_power_well)
 		power_well->set(dev, power_well, false);
 }
@@ -5753,7 +5761,7 @@ void intel_display_power_get(struct drm_device *dev,
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well(i, power_well, BIT(domain), power_domains)
-		__intel_power_well_get(dev, power_well);
+		__intel_power_well_get(dev, power_well, domain);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5769,7 +5777,7 @@ void intel_display_power_put(struct drm_device *dev,
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
-		__intel_power_well_put(dev, power_well);
+		__intel_power_well_put(dev, power_well, domain);
 	mutex_unlock(&power_domains->lock);
 }
 
-- 
1.8.4

Re: [PATCH v2 8/8] drm/i915: add a debugfs entry for power domain info

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak@intel.com>:
> Add a debugfs entry showing the use-count for all power domains of each
> power well.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |  4 +++
>  drivers/gpu/drm/i915/intel_pm.c     | 16 ++++++---
>  3 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6875b7a..a6555cd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1844,6 +1844,74 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static const char *power_domain_str(enum intel_display_power_domain domain)
> +{
> +#define PWRD(p)        { POWER_DOMAIN_ ## p, # p }
> +       static struct {
> +               enum intel_display_power_domain domain;
> +               const char *name;
> +       } table[] = {
> +               PWRD(PIPE_A),
> +               PWRD(PIPE_B),
> +               PWRD(PIPE_C),
> +               PWRD(PIPE_A_PANEL_FITTER),
> +               PWRD(PIPE_B_PANEL_FITTER),
> +               PWRD(PIPE_C_PANEL_FITTER),
> +               PWRD(TRANSCODER_A),
> +               PWRD(TRANSCODER_B),
> +               PWRD(TRANSCODER_C),
> +               PWRD(TRANSCODER_EDP),
> +               PWRD(VGA),
> +               PWRD(AUDIO),
> +               PWRD(INIT),
> +       };
> +#undef PWRD
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(table); i++)
> +               if (table[i].domain == domain)
> +                       return table[i].name;

Why not just "return table[domain].name" instead of the 3 lines above?

Our driver has a few of these functions returning strings for enums
and they usually just contain a "switch" statement on the enum,
without using complicated macros. I tend to prefer the simple things.


> +
> +       WARN_ON(1);
> +
> +       return "?";
> +}
> +
> +static int i915_power_domain_info(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       int i;
> +
> +       mutex_lock(&power_domains->lock);
> +
> +       seq_printf(m, "%-25s %s\n", "Power well/domain", "Use count");
> +       for (i = 0; i < power_domains->power_well_count; i++) {
> +               struct i915_power_well *power_well;
> +               enum intel_display_power_domain power_domain;
> +
> +               power_well = &power_domains->power_wells[i];
> +               seq_printf(m, "%-25s %d\n", power_well->name,
> +                          power_well->count);
> +
> +               for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
> +                    power_domain++) {
> +                       if (!(BIT(power_domain) & power_well->domains))
> +                               continue;
> +
> +                       seq_printf(m, "  %-23s %d\n",
> +                                  power_domain_str(power_domain),
> +                                  power_well->domain_count[power_domain]);
> +               }
> +       }
> +
> +       mutex_unlock(&power_domains->lock);
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -3076,6 +3144,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_edp_psr_status", i915_edp_psr_status, 0},
>         {"i915_energy_uJ", i915_energy_uJ, 0},
>         {"i915_pc8_status", i915_pc8_status, 0},
> +       {"i915_power_domain_info", i915_power_domain_info, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 06f47bf..194e39f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -950,6 +950,10 @@ struct i915_power_well {
>         /* power well enable/disable usage count */
>         int count;
>         unsigned long domains;
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +       /* usage count for each power domain in the domains mask */
> +       int domain_count[POWER_DOMAIN_NUM];

Shouldn't we track this in struct power_domains, since when we get a
domain we don't get it for a specific power well, but just "get the
domain"? I think it makes more sense.


> +#endif
>         void *data;
>         void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
>                     bool enable);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d252453..99210c1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5726,17 +5726,25 @@ static void hsw_set_power_well(struct drm_device *dev,
>  }
>
>  static void __intel_power_well_get(struct drm_device *dev,
> -                                  struct i915_power_well *power_well)
> +                                  struct i915_power_well *power_well,
> +                                  enum intel_display_power_domain domain)
>  {
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +       power_well->domain_count[domain]++;
> +#endif
>         if (!power_well->count++ && power_well->set)
>                 power_well->set(dev, power_well, true);
>  }
>
>  static void __intel_power_well_put(struct drm_device *dev,
> -                                  struct i915_power_well *power_well)
> +                                  struct i915_power_well *power_well,
> +                                  enum intel_display_power_domain domain)
>  {
>         WARN_ON(!power_well->count);
>
> +#if IS_ENABLED(CONFIG_DEBUG_FS)

WARN_ON(power_well->domain_count[domain] == 0);


> +       power_well->domain_count[domain]--;
> +#endif
>         if (!--power_well->count && power_well->set && i915_disable_power_well)
>                 power_well->set(dev, power_well, false);
>  }
> @@ -5753,7 +5761,7 @@ void intel_display_power_get(struct drm_device *dev,
>
>         mutex_lock(&power_domains->lock);
>         for_each_power_well(i, power_well, BIT(domain), power_domains)
> -               __intel_power_well_get(dev, power_well);
> +               __intel_power_well_get(dev, power_well, domain);
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5769,7 +5777,7 @@ void intel_display_power_put(struct drm_device *dev,
>
>         mutex_lock(&power_domains->lock);
>         for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> -               __intel_power_well_put(dev, power_well);
> +               __intel_power_well_put(dev, power_well, domain);
>         mutex_unlock(&power_domains->lock);
>  }
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 8/8] drm/i915: add a debugfs entry for power domain info

[Imre Deak]

On Fri, 2013-11-22 at 15:32 -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > Add a debugfs entry showing the use-count for all power domains of each
> > power well.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.h     |  4 +++
> >  drivers/gpu/drm/i915/intel_pm.c     | 16 ++++++---
> >  3 files changed, 85 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 6875b7a..a6555cd 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1844,6 +1844,74 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
> >         return 0;
> >  }
> >
> > +static const char *power_domain_str(enum intel_display_power_domain domain)
> > +{
> > +#define PWRD(p)        { POWER_DOMAIN_ ## p, # p }
> > +       static struct {
> > +               enum intel_display_power_domain domain;
> > +               const char *name;
> > +       } table[] = {
> > +               PWRD(PIPE_A),
> > +               PWRD(PIPE_B),
> > +               PWRD(PIPE_C),
> > +               PWRD(PIPE_A_PANEL_FITTER),
> > +               PWRD(PIPE_B_PANEL_FITTER),
> > +               PWRD(PIPE_C_PANEL_FITTER),
> > +               PWRD(TRANSCODER_A),
> > +               PWRD(TRANSCODER_B),
> > +               PWRD(TRANSCODER_C),
> > +               PWRD(TRANSCODER_EDP),
> > +               PWRD(VGA),
> > +               PWRD(AUDIO),
> > +               PWRD(INIT),
> > +       };
> > +#undef PWRD
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(table); i++)
> > +               if (table[i].domain == domain)
> > +                       return table[i].name;
> 
> Why not just "return table[domain].name" instead of the 3 lines above?

In the first version I had the POWER_DOMAIN_* values redefined to be
bitmasks, where this wouldn't have worked. 

> Our driver has a few of these functions returning strings for enums
> and they usually just contain a "switch" statement on the enum,
> without using complicated macros. I tend to prefer the simple things.

I think this is mostly a matter of taste, having a switch table allows
for mistypings for example. But I don't mind rewriting it if that's a
more standard way in the driver.

> > +
> > +       WARN_ON(1);
> > +
> > +       return "?";
> > +}
> > +
> > +static int i915_power_domain_info(struct seq_file *m, void *unused)
> > +{
> > +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> > +       struct drm_device *dev = node->minor->dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +       int i;
> > +
> > +       mutex_lock(&power_domains->lock);
> > +
> > +       seq_printf(m, "%-25s %s\n", "Power well/domain", "Use count");
> > +       for (i = 0; i < power_domains->power_well_count; i++) {
> > +               struct i915_power_well *power_well;
> > +               enum intel_display_power_domain power_domain;
> > +
> > +               power_well = &power_domains->power_wells[i];
> > +               seq_printf(m, "%-25s %d\n", power_well->name,
> > +                          power_well->count);
> > +
> > +               for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
> > +                    power_domain++) {
> > +                       if (!(BIT(power_domain) & power_well->domains))
> > +                               continue;
> > +
> > +                       seq_printf(m, "  %-23s %d\n",
> > +                                  power_domain_str(power_domain),
> > +                                  power_well->domain_count[power_domain]);
> > +               }
> > +       }
> > +
> > +       mutex_unlock(&power_domains->lock);
> > +
> > +       return 0;
> > +}
> > +
> >  struct pipe_crc_info {
> >         const char *name;
> >         struct drm_device *dev;
> > @@ -3076,6 +3144,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
> >         {"i915_edp_psr_status", i915_edp_psr_status, 0},
> >         {"i915_energy_uJ", i915_energy_uJ, 0},
> >         {"i915_pc8_status", i915_pc8_status, 0},
> > +       {"i915_power_domain_info", i915_power_domain_info, 0},
> >  };
> >  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 06f47bf..194e39f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -950,6 +950,10 @@ struct i915_power_well {
> >         /* power well enable/disable usage count */
> >         int count;
> >         unsigned long domains;
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +       /* usage count for each power domain in the domains mask */
> > +       int domain_count[POWER_DOMAIN_NUM];
> 
> Shouldn't we track this in struct power_domains, since when we get a
> domain we don't get it for a specific power well, but just "get the
> domain"? I think it makes more sense.

It belongs to the power well. For instance in theory we could have a
power domain that needs more than a single power well to be on.

> > +#endif
> >         void *data;
> >         void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
> >                     bool enable);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d252453..99210c1 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5726,17 +5726,25 @@ static void hsw_set_power_well(struct drm_device *dev,
> >  }
> >
> >  static void __intel_power_well_get(struct drm_device *dev,
> > -                                  struct i915_power_well *power_well)
> > +                                  struct i915_power_well *power_well,
> > +                                  enum intel_display_power_domain domain)
> >  {
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +       power_well->domain_count[domain]++;
> > +#endif
> >         if (!power_well->count++ && power_well->set)
> >                 power_well->set(dev, power_well, true);
> >  }
> >
> >  static void __intel_power_well_put(struct drm_device *dev,
> > -                                  struct i915_power_well *power_well)
> > +                                  struct i915_power_well *power_well,
> > +                                  enum intel_display_power_domain domain)
> >  {
> >         WARN_ON(!power_well->count);
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> 
> WARN_ON(power_well->domain_count[domain] == 0);

Ok, will add this.

--Imre

> 
> > +       power_well->domain_count[domain]--;
> > +#endif
> >         if (!--power_well->count && power_well->set && i915_disable_power_well)
> >                 power_well->set(dev, power_well, false);
> >  }
> > @@ -5753,7 +5761,7 @@ void intel_display_power_get(struct drm_device *dev,
> >
> >         mutex_lock(&power_domains->lock);
> >         for_each_power_well(i, power_well, BIT(domain), power_domains)
> > -               __intel_power_well_get(dev, power_well);
> > +               __intel_power_well_get(dev, power_well, domain);
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > @@ -5769,7 +5777,7 @@ void intel_display_power_put(struct drm_device *dev,
> >
> >         mutex_lock(&power_domains->lock);
> >         for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> > -               __intel_power_well_put(dev, power_well);
> > +               __intel_power_well_put(dev, power_well, domain);
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

Re: [PATCH v2 8/8] drm/i915: add a debugfs entry for power domain info

[Imre Deak]

On Fri, 2013-11-22 at 21:04 +0200, Imre Deak wrote:
> On Fri, 2013-11-22 at 15:32 -0200, Paulo Zanoni wrote:
> > 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > > Add a debugfs entry showing the use-count for all power domains of each
> > > power well.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_drv.h     |  4 +++
> > >  drivers/gpu/drm/i915/intel_pm.c     | 16 ++++++---
> > >  3 files changed, 85 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 6875b7a..a6555cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1844,6 +1844,74 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
> > >         return 0;
> > >  }
> > >
> > > +static const char *power_domain_str(enum intel_display_power_domain domain)
> > > +{
> > > +#define PWRD(p)        { POWER_DOMAIN_ ## p, # p }
> > > +       static struct {
> > > +               enum intel_display_power_domain domain;
> > > +               const char *name;
> > > +       } table[] = {
> > > +               PWRD(PIPE_A),
> > > +               PWRD(PIPE_B),
> > > +               PWRD(PIPE_C),
> > > +               PWRD(PIPE_A_PANEL_FITTER),
> > > +               PWRD(PIPE_B_PANEL_FITTER),
> > > +               PWRD(PIPE_C_PANEL_FITTER),
> > > +               PWRD(TRANSCODER_A),
> > > +               PWRD(TRANSCODER_B),
> > > +               PWRD(TRANSCODER_C),
> > > +               PWRD(TRANSCODER_EDP),
> > > +               PWRD(VGA),
> > > +               PWRD(AUDIO),
> > > +               PWRD(INIT),
> > > +       };
> > > +#undef PWRD
> > > +       int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(table); i++)
> > > +               if (table[i].domain == domain)
> > > +                       return table[i].name;
> > 
> > Why not just "return table[domain].name" instead of the 3 lines above?
> 
> In the first version I had the POWER_DOMAIN_* values redefined to be
> bitmasks, where this wouldn't have worked. 
> 
> > Our driver has a few of these functions returning strings for enums
> > and they usually just contain a "switch" statement on the enum,
> > without using complicated macros. I tend to prefer the simple things.
> 
> I think this is mostly a matter of taste, having a switch table allows
> for mistypings for example. But I don't mind rewriting it if that's a
> more standard way in the driver.
> 
> > > +
> > > +       WARN_ON(1);
> > > +
> > > +       return "?";
> > > +}
> > > +
> > > +static int i915_power_domain_info(struct seq_file *m, void *unused)
> > > +{
> > > +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> > > +       struct drm_device *dev = node->minor->dev;
> > > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > > +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > > +       int i;
> > > +
> > > +       mutex_lock(&power_domains->lock);
> > > +
> > > +       seq_printf(m, "%-25s %s\n", "Power well/domain", "Use count");
> > > +       for (i = 0; i < power_domains->power_well_count; i++) {
> > > +               struct i915_power_well *power_well;
> > > +               enum intel_display_power_domain power_domain;
> > > +
> > > +               power_well = &power_domains->power_wells[i];
> > > +               seq_printf(m, "%-25s %d\n", power_well->name,
> > > +                          power_well->count);
> > > +
> > > +               for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
> > > +                    power_domain++) {
> > > +                       if (!(BIT(power_domain) & power_well->domains))
> > > +                               continue;
> > > +
> > > +                       seq_printf(m, "  %-23s %d\n",
> > > +                                  power_domain_str(power_domain),
> > > +                                  power_well->domain_count[power_domain]);
> > > +               }
> > > +       }
> > > +
> > > +       mutex_unlock(&power_domains->lock);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  struct pipe_crc_info {
> > >         const char *name;
> > >         struct drm_device *dev;
> > > @@ -3076,6 +3144,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
> > >         {"i915_edp_psr_status", i915_edp_psr_status, 0},
> > >         {"i915_energy_uJ", i915_energy_uJ, 0},
> > >         {"i915_pc8_status", i915_pc8_status, 0},
> > > +       {"i915_power_domain_info", i915_power_domain_info, 0},
> > >  };
> > >  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 06f47bf..194e39f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -950,6 +950,10 @@ struct i915_power_well {
> > >         /* power well enable/disable usage count */
> > >         int count;
> > >         unsigned long domains;
> > > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > > +       /* usage count for each power domain in the domains mask */
> > > +       int domain_count[POWER_DOMAIN_NUM];
> > 
> > Shouldn't we track this in struct power_domains, since when we get a
> > domain we don't get it for a specific power well, but just "get the
> > domain"? I think it makes more sense.
> 
> It belongs to the power well. For instance in theory we could have a
> power domain that needs more than a single power well to be on.

Actually you were right. Even though I want to show the refcount for a
given domain for each power well, this refcount will always be the same
for each power well. So I'll move domain_count to power_domains as you
suggested.

--Imre

> 
> > > +#endif
> > >         void *data;
> > >         void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
> > >                     bool enable);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index d252453..99210c1 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5726,17 +5726,25 @@ static void hsw_set_power_well(struct drm_device *dev,
> > >  }
> > >
> > >  static void __intel_power_well_get(struct drm_device *dev,
> > > -                                  struct i915_power_well *power_well)
> > > +                                  struct i915_power_well *power_well,
> > > +                                  enum intel_display_power_domain domain)
> > >  {
> > > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > > +       power_well->domain_count[domain]++;
> > > +#endif
> > >         if (!power_well->count++ && power_well->set)
> > >                 power_well->set(dev, power_well, true);
> > >  }
> > >
> > >  static void __intel_power_well_put(struct drm_device *dev,
> > > -                                  struct i915_power_well *power_well)
> > > +                                  struct i915_power_well *power_well,
> > > +                                  enum intel_display_power_domain domain)
> > >  {
> > >         WARN_ON(!power_well->count);
> > >
> > > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > 
> > WARN_ON(power_well->domain_count[domain] == 0);
> 
> Ok, will add this.
> 
> --Imre
> 
> > 
> > > +       power_well->domain_count[domain]--;
> > > +#endif
> > >         if (!--power_well->count && power_well->set && i915_disable_power_well)
> > >                 power_well->set(dev, power_well, false);
> > >  }
> > > @@ -5753,7 +5761,7 @@ void intel_display_power_get(struct drm_device *dev,
> > >
> > >         mutex_lock(&power_domains->lock);
> > >         for_each_power_well(i, power_well, BIT(domain), power_domains)
> > > -               __intel_power_well_get(dev, power_well);
> > > +               __intel_power_well_get(dev, power_well, domain);
> > >         mutex_unlock(&power_domains->lock);
> > >  }
> > >
> > > @@ -5769,7 +5777,7 @@ void intel_display_power_put(struct drm_device *dev,
> > >
> > >         mutex_lock(&power_domains->lock);
> > >         for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> > > -               __intel_power_well_put(dev, power_well);
> > > +               __intel_power_well_put(dev, power_well, domain);
> > >         mutex_unlock(&power_domains->lock);
> > >  }
> > >
> > > --
> > > 1.8.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
>