[PATCH 1/2] drm/i915: use power well count instead of reading hw state when checking status

[PATCH 1/2] drm/i915: use power well count instead of reading hw state when checking status

[Jesse Barnes]

This saves many ms per call on my BYT by eliminating Punit communication
from the hw readout paths.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 452518f..09a3677 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5572,13 +5572,11 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
-		if (power_well->always_on)
+		if (power_well->always_on || power_well->count)
 			continue;
 
-		if (!power_well->ops->is_enabled(dev_priv, power_well)) {
-			is_enabled = false;
-			break;
-		}
+		is_enabled = false;
+		break;
 	}
 	mutex_unlock(&power_domains->lock);
 
-- 
1.8.4.2

[PATCH 2/2] drm/i915: rename is_enabled to hw_state

[Jesse Barnes]

Mainly useful for catching all the callers in the previous patch.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h |  4 ++--
 drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5d5e57d..3396a55 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -977,8 +977,8 @@ struct i915_power_well_ops {
 	void (*disable)(struct drm_i915_private *dev_priv,
 			struct i915_power_well *power_well);
 	/* Returns the hw enabled state. */
-	bool (*is_enabled)(struct drm_i915_private *dev_priv,
-			   struct i915_power_well *power_well);
+	bool (*hw_state)(struct drm_i915_private *dev_priv,
+			 struct i915_power_well *power_well);
 };
 
 /* Power well structure for haswell */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 09a3677..a29ef24 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5538,8 +5538,8 @@ void intel_suspend_hw(struct drm_device *dev)
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
-				   struct i915_power_well *power_well)
+static bool hsw_power_hw_state(struct drm_i915_private *dev_priv,
+			       struct i915_power_well *power_well)
 {
 	return I915_READ(HSW_PWR_WELL_DRIVER) ==
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
@@ -5716,8 +5716,8 @@ static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
 {
 }
 
-static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
-					     struct i915_power_well *power_well)
+static bool i9xx_always_on_power_hw_state(struct drm_i915_private *dev_priv,
+					  struct i915_power_well *power_well)
 {
 	return true;
 }
@@ -5819,8 +5819,8 @@ static void vlv_power_well_disable(struct drm_i915_private *dev_priv,
 	vlv_set_power_well(dev_priv, power_well, false);
 }
 
-static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
-				   struct i915_power_well *power_well)
+static bool vlv_power_hw_state(struct drm_i915_private *dev_priv,
+			       struct i915_power_well *power_well)
 {
 	int power_well_id = power_well->data;
 	bool enabled = false;
@@ -5904,7 +5904,7 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
 static void check_power_well_state(struct drm_i915_private *dev_priv,
 				   struct i915_power_well *power_well)
 {
-	bool enabled = power_well->ops->is_enabled(dev_priv, power_well);
+	bool enabled = power_well->ops->hw_state(dev_priv, power_well);
 
 	if (power_well->always_on || !i915.disable_power_well) {
 		if (!enabled)
@@ -6070,7 +6070,7 @@ static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
 	.sync_hw = i9xx_always_on_power_well_noop,
 	.enable = i9xx_always_on_power_well_noop,
 	.disable = i9xx_always_on_power_well_noop,
-	.is_enabled = i9xx_always_on_power_well_enabled,
+	.hw_state = i9xx_always_on_power_hw_state,
 };
 
 static struct i915_power_well i9xx_always_on_power_well[] = {
@@ -6086,7 +6086,7 @@ static const struct i915_power_well_ops hsw_power_well_ops = {
 	.sync_hw = hsw_power_well_sync_hw,
 	.enable = hsw_power_well_enable,
 	.disable = hsw_power_well_disable,
-	.is_enabled = hsw_power_well_enabled,
+	.hw_state = hsw_power_hw_state,
 };
 
 static struct i915_power_well hsw_power_wells[] = {
@@ -6121,14 +6121,14 @@ static const struct i915_power_well_ops vlv_display_power_well_ops = {
 	.sync_hw = vlv_power_well_sync_hw,
 	.enable = vlv_display_power_well_enable,
 	.disable = vlv_display_power_well_disable,
-	.is_enabled = vlv_power_well_enabled,
+	.hw_state = vlv_power_hw_state,
 };
 
 static const struct i915_power_well_ops vlv_dpio_power_well_ops = {
 	.sync_hw = vlv_power_well_sync_hw,
 	.enable = vlv_power_well_enable,
 	.disable = vlv_power_well_disable,
-	.is_enabled = vlv_power_well_enabled,
+	.hw_state = vlv_power_hw_state,
 };
 
 static struct i915_power_well vlv_power_wells[] = {
-- 
1.8.4.2

Re: [PATCH 1/2] drm/i915: use power well count instead of reading hw state when checking status

[Imre Deak]

On Wed, 2014-05-28 at 09:50 -0700, Jesse Barnes wrote:
> This saves many ms per call on my BYT by eliminating Punit communication
> from the hw readout paths.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 452518f..09a3677 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5572,13 +5572,11 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
>  
>  	mutex_lock(&power_domains->lock);
>  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> -		if (power_well->always_on)
> +		if (power_well->always_on || power_well->count)
>  			continue;
>  
> -		if (!power_well->ops->is_enabled(dev_priv, power_well)) {
> -			is_enabled = false;
> -			break;
> -		}
> +		is_enabled = false;
> +		break;
>  	}
>  	mutex_unlock(&power_domains->lock);

This was meant to return the HW state vs. the state based on the
refcount. It would work in the above way now, because we enable all
power wells for driver init time, but if remove that in the future the
two states may not match.

Perhaps we could maintain a cached version of the HW state in the
power_well struct that we set in the sync_hw handler and update whenever
we turn on/off the wells.

--Imre

Re: [PATCH 1/2] drm/i915: use power well count instead of reading hw state when checking status

[Jesse Barnes]

On Wed, 28 May 2014 21:09:27 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Wed, 2014-05-28 at 09:50 -0700, Jesse Barnes wrote:
> > This saves many ms per call on my BYT by eliminating Punit communication
> > from the hw readout paths.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 452518f..09a3677 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5572,13 +5572,11 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
> >  
> >  	mutex_lock(&power_domains->lock);
> >  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> > -		if (power_well->always_on)
> > +		if (power_well->always_on || power_well->count)
> >  			continue;
> >  
> > -		if (!power_well->ops->is_enabled(dev_priv, power_well)) {
> > -			is_enabled = false;
> > -			break;
> > -		}
> > +		is_enabled = false;
> > +		break;
> >  	}
> >  	mutex_unlock(&power_domains->lock);
> 
> This was meant to return the HW state vs. the state based on the
> refcount. It would work in the above way now, because we enable all
> power wells for driver init time, but if remove that in the future the
> two states may not match.
> 
> Perhaps we could maintain a cached version of the HW state in the
> power_well struct that we set in the sync_hw handler and update whenever
> we turn on/off the wells.

Yeah that works too, and I almost didn't send this out because I knew
the hw state is something we want to read out sometimes.

But on BYT it turns out to be pretty expensive to do the simple "get
power well" calls at the top of the display functions, so it would be
nice to find a way to avoid it.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: cache hw power well enabled state

[Imre Deak]

Jesse noticed that the punit communication needed to query the VLV power
well status can cause substantial delays. Since we can query the state
frequently, for example during I2C transfers, maintain a cached version
of the HW state to get rid of this delay.

This fixes at least one reported regression where boot time increased by
~4 seconds due to frequent power well state queries on VLV during eDP
EDID read.

Reported-by: Jesse Barnes <jesse.barnes@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c |  6 +++---
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
 drivers/gpu/drm/i915/intel_pm.c      | 37 +++++++++++++++---------------------
 4 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8631fb3..000a6ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -976,6 +976,8 @@ struct i915_power_well {
 	bool always_on;
 	/* power well enable/disable usage count */
 	int count;
+	/* cached hw enabled state */
+	bool hw_enabled;
 	unsigned long domains;
 	unsigned long data;
 	const struct i915_power_well_ops *ops;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb28..882d4f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12692,8 +12692,8 @@ intel_display_capture_error_state(struct drm_device *dev)
 
 	for_each_pipe(i) {
 		error->pipe[i].power_domain_on =
-			intel_display_power_enabled_sw(dev_priv,
-						       POWER_DOMAIN_PIPE(i));
+			intel_display_power_enabled_unlocked(dev_priv,
+							   POWER_DOMAIN_PIPE(i));
 		if (!error->pipe[i].power_domain_on)
 			continue;
 
@@ -12728,7 +12728,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 		enum transcoder cpu_transcoder = transcoders[i];
 
 		error->transcoder[i].power_domain_on =
-			intel_display_power_enabled_sw(dev_priv,
+			intel_display_power_enabled_unlocked(dev_priv,
 				POWER_DOMAIN_TRANSCODER(cpu_transcoder));
 		if (!error->transcoder[i].power_domain_on)
 			continue;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 78d4124..1455ddb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -948,8 +948,8 @@ int intel_power_domains_init(struct drm_i915_private *);
 void intel_power_domains_remove(struct drm_i915_private *);
 bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
 				 enum intel_display_power_domain domain);
-bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
-				    enum intel_display_power_domain domain);
+bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
+					  enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee27d74..96a2d31 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5801,8 +5801,8 @@ static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 }
 
-bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
-				    enum intel_display_power_domain domain)
+bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
+					  enum intel_display_power_domain domain)
 {
 	struct i915_power_domains *power_domains;
 	struct i915_power_well *power_well;
@@ -5813,16 +5813,19 @@ bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
 		return false;
 
 	power_domains = &dev_priv->power_domains;
+
 	is_enabled = true;
+
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
 		if (power_well->always_on)
 			continue;
 
-		if (!power_well->count) {
+		if (!power_well->hw_enabled) {
 			is_enabled = false;
 			break;
 		}
 	}
+
 	return is_enabled;
 }
 
@@ -5830,30 +5833,15 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
 				 enum intel_display_power_domain domain)
 {
 	struct i915_power_domains *power_domains;
-	struct i915_power_well *power_well;
-	bool is_enabled;
-	int i;
-
-	if (dev_priv->pm.suspended)
-		return false;
+	bool ret;
 
 	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->ops->is_enabled(dev_priv, power_well)) {
-			is_enabled = false;
-			break;
-		}
-	}
+	ret = intel_display_power_enabled_unlocked(dev_priv, domain);
 	mutex_unlock(&power_domains->lock);
 
-	return is_enabled;
+	return ret;
 }
 
 /*
@@ -6174,6 +6162,7 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 		if (!power_well->count++) {
 			DRM_DEBUG_KMS("enabling %s\n", power_well->name);
 			power_well->ops->enable(dev_priv, power_well);
+			power_well->hw_enabled = true;
 		}
 
 		check_power_well_state(dev_priv, power_well);
@@ -6203,6 +6192,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 
 		if (!--power_well->count && i915.disable_power_well) {
 			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
+			power_well->hw_enabled = false;
 			power_well->ops->disable(dev_priv, power_well);
 		}
 
@@ -6463,8 +6453,11 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
 	int i;
 
 	mutex_lock(&power_domains->lock);
-	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains)
+	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
 		power_well->ops->sync_hw(dev_priv, power_well);
+		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
+								     power_well);
+	}
 	mutex_unlock(&power_domains->lock);
 }
 
-- 
1.8.4

Re: [PATCH] drm/i915: cache hw power well enabled state

[Jesse Barnes]

On Thu,  5 Jun 2014 20:31:47 +0300
Imre Deak <imre.deak@intel.com> wrote:

> Jesse noticed that the punit communication needed to query the VLV power
> well status can cause substantial delays. Since we can query the state
> frequently, for example during I2C transfers, maintain a cached version
> of the HW state to get rid of this delay.
> 
> This fixes at least one reported regression where boot time increased by
> ~4 seconds due to frequent power well state queries on VLV during eDP
> EDID read.
> 
> Reported-by: Jesse Barnes <jesse.barnes@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c |  6 +++---
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  drivers/gpu/drm/i915/intel_pm.c      | 37 +++++++++++++++---------------------
>  4 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8631fb3..000a6ce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -976,6 +976,8 @@ struct i915_power_well {
>  	bool always_on;
>  	/* power well enable/disable usage count */
>  	int count;
> +	/* cached hw enabled state */
> +	bool hw_enabled;
>  	unsigned long domains;
>  	unsigned long data;
>  	const struct i915_power_well_ops *ops;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..882d4f5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12692,8 +12692,8 @@ intel_display_capture_error_state(struct drm_device *dev)
>  
>  	for_each_pipe(i) {
>  		error->pipe[i].power_domain_on =
> -			intel_display_power_enabled_sw(dev_priv,
> -						       POWER_DOMAIN_PIPE(i));
> +			intel_display_power_enabled_unlocked(dev_priv,
> +							   POWER_DOMAIN_PIPE(i));
>  		if (!error->pipe[i].power_domain_on)
>  			continue;
>  
> @@ -12728,7 +12728,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		enum transcoder cpu_transcoder = transcoders[i];
>  
>  		error->transcoder[i].power_domain_on =
> -			intel_display_power_enabled_sw(dev_priv,
> +			intel_display_power_enabled_unlocked(dev_priv,
>  				POWER_DOMAIN_TRANSCODER(cpu_transcoder));
>  		if (!error->transcoder[i].power_domain_on)
>  			continue;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 78d4124..1455ddb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -948,8 +948,8 @@ int intel_power_domains_init(struct drm_i915_private *);
>  void intel_power_domains_remove(struct drm_i915_private *);
>  bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
>  				 enum intel_display_power_domain domain);
> -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> -				    enum intel_display_power_domain domain);
> +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
> +					  enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee27d74..96a2d31 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5801,8 +5801,8 @@ static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
>  		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>  }
>  
> -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> -				    enum intel_display_power_domain domain)
> +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
> +					  enum intel_display_power_domain domain)
>  {
>  	struct i915_power_domains *power_domains;
>  	struct i915_power_well *power_well;
> @@ -5813,16 +5813,19 @@ bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
>  		return false;
>  
>  	power_domains = &dev_priv->power_domains;
> +
>  	is_enabled = true;
> +
>  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>  		if (power_well->always_on)
>  			continue;
>  
> -		if (!power_well->count) {
> +		if (!power_well->hw_enabled) {
>  			is_enabled = false;
>  			break;
>  		}
>  	}
> +
>  	return is_enabled;
>  }
>  
> @@ -5830,30 +5833,15 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
>  				 enum intel_display_power_domain domain)
>  {
>  	struct i915_power_domains *power_domains;
> -	struct i915_power_well *power_well;
> -	bool is_enabled;
> -	int i;
> -
> -	if (dev_priv->pm.suspended)
> -		return false;
> +	bool ret;
>  
>  	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->ops->is_enabled(dev_priv, power_well)) {
> -			is_enabled = false;
> -			break;
> -		}
> -	}
> +	ret = intel_display_power_enabled_unlocked(dev_priv, domain);
>  	mutex_unlock(&power_domains->lock);
>  
> -	return is_enabled;
> +	return ret;
>  }
>  
>  /*
> @@ -6174,6 +6162,7 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
>  		if (!power_well->count++) {
>  			DRM_DEBUG_KMS("enabling %s\n", power_well->name);
>  			power_well->ops->enable(dev_priv, power_well);
> +			power_well->hw_enabled = true;
>  		}
>  
>  		check_power_well_state(dev_priv, power_well);
> @@ -6203,6 +6192,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  		if (!--power_well->count && i915.disable_power_well) {
>  			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> +			power_well->hw_enabled = false;
>  			power_well->ops->disable(dev_priv, power_well);
>  		}
>  
> @@ -6463,8 +6453,11 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
>  	int i;
>  
>  	mutex_lock(&power_domains->lock);
> -	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains)
> +	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
>  		power_well->ops->sync_hw(dev_priv, power_well);
> +		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
> +								     power_well);
> +	}
>  	mutex_unlock(&power_domains->lock);
>  }
>  

I guess we have check_power_well_state for the cross checking, so:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache hw power well enabled state

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 7363 bytes --]

On Thu, 2014-06-05 at 10:35 -0700, Jesse Barnes wrote:
> On Thu,  5 Jun 2014 20:31:47 +0300
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > Jesse noticed that the punit communication needed to query the VLV power
> > well status can cause substantial delays. Since we can query the state
> > frequently, for example during I2C transfers, maintain a cached version
> > of the HW state to get rid of this delay.
> > 
> > This fixes at least one reported regression where boot time increased by
> > ~4 seconds due to frequent power well state queries on VLV during eDP
> > EDID read.
> > 
> > Reported-by: Jesse Barnes <jesse.barnes@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> >  drivers/gpu/drm/i915/intel_display.c |  6 +++---
> >  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
> >  drivers/gpu/drm/i915/intel_pm.c      | 37 +++++++++++++++---------------------
> >  4 files changed, 22 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8631fb3..000a6ce 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -976,6 +976,8 @@ struct i915_power_well {
> >  	bool always_on;
> >  	/* power well enable/disable usage count */
> >  	int count;
> > +	/* cached hw enabled state */
> > +	bool hw_enabled;
> >  	unsigned long domains;
> >  	unsigned long data;
> >  	const struct i915_power_well_ops *ops;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b5cbb28..882d4f5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12692,8 +12692,8 @@ intel_display_capture_error_state(struct drm_device *dev)
> >  
> >  	for_each_pipe(i) {
> >  		error->pipe[i].power_domain_on =
> > -			intel_display_power_enabled_sw(dev_priv,
> > -						       POWER_DOMAIN_PIPE(i));
> > +			intel_display_power_enabled_unlocked(dev_priv,
> > +							   POWER_DOMAIN_PIPE(i));
> >  		if (!error->pipe[i].power_domain_on)
> >  			continue;
> >  
> > @@ -12728,7 +12728,7 @@ intel_display_capture_error_state(struct drm_device *dev)
> >  		enum transcoder cpu_transcoder = transcoders[i];
> >  
> >  		error->transcoder[i].power_domain_on =
> > -			intel_display_power_enabled_sw(dev_priv,
> > +			intel_display_power_enabled_unlocked(dev_priv,
> >  				POWER_DOMAIN_TRANSCODER(cpu_transcoder));
> >  		if (!error->transcoder[i].power_domain_on)
> >  			continue;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 78d4124..1455ddb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -948,8 +948,8 @@ int intel_power_domains_init(struct drm_i915_private *);
> >  void intel_power_domains_remove(struct drm_i915_private *);
> >  bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
> >  				 enum intel_display_power_domain domain);
> > -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> > -				    enum intel_display_power_domain domain);
> > +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
> > +					  enum intel_display_power_domain domain);
> >  void intel_display_power_get(struct drm_i915_private *dev_priv,
> >  			     enum intel_display_power_domain domain);
> >  void intel_display_power_put(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ee27d74..96a2d31 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5801,8 +5801,8 @@ static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
> >  		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
> >  }
> >  
> > -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> > -				    enum intel_display_power_domain domain)
> > +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
> > +					  enum intel_display_power_domain domain)
> >  {
> >  	struct i915_power_domains *power_domains;
> >  	struct i915_power_well *power_well;
> > @@ -5813,16 +5813,19 @@ bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> >  		return false;
> >  
> >  	power_domains = &dev_priv->power_domains;
> > +
> >  	is_enabled = true;
> > +
> >  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> >  		if (power_well->always_on)
> >  			continue;
> >  
> > -		if (!power_well->count) {
> > +		if (!power_well->hw_enabled) {
> >  			is_enabled = false;
> >  			break;
> >  		}
> >  	}
> > +
> >  	return is_enabled;
> >  }
> >  
> > @@ -5830,30 +5833,15 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
> >  				 enum intel_display_power_domain domain)
> >  {
> >  	struct i915_power_domains *power_domains;
> > -	struct i915_power_well *power_well;
> > -	bool is_enabled;
> > -	int i;
> > -
> > -	if (dev_priv->pm.suspended)
> > -		return false;
> > +	bool ret;
> >  
> >  	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->ops->is_enabled(dev_priv, power_well)) {
> > -			is_enabled = false;
> > -			break;
> > -		}
> > -	}
> > +	ret = intel_display_power_enabled_unlocked(dev_priv, domain);
> >  	mutex_unlock(&power_domains->lock);
> >  
> > -	return is_enabled;
> > +	return ret;
> >  }
> >  
> >  /*
> > @@ -6174,6 +6162,7 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
> >  		if (!power_well->count++) {
> >  			DRM_DEBUG_KMS("enabling %s\n", power_well->name);
> >  			power_well->ops->enable(dev_priv, power_well);
> > +			power_well->hw_enabled = true;
> >  		}
> >  
> >  		check_power_well_state(dev_priv, power_well);
> > @@ -6203,6 +6192,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  
> >  		if (!--power_well->count && i915.disable_power_well) {
> >  			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> > +			power_well->hw_enabled = false;
> >  			power_well->ops->disable(dev_priv, power_well);
> >  		}
> >  
> > @@ -6463,8 +6453,11 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
> >  	int i;
> >  
> >  	mutex_lock(&power_domains->lock);
> > -	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains)
> > +	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> >  		power_well->ops->sync_hw(dev_priv, power_well);
> > +		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
> > +								     power_well);
> > +	}
> >  	mutex_unlock(&power_domains->lock);
> >  }
> >  
> 
> I guess we have check_power_well_state for the cross checking, so:

Yea, that still reads the real HW state, and it's done only during 0->1
1->0 transitions, so (hopefully) not a big overhead.

I also tested this lightly on HSW and VLV.

--Imre

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: cache hw power well enabled state

[Daniel Vetter]

On Thu, Jun 05, 2014 at 08:31:47PM +0300, Imre Deak wrote:
> Jesse noticed that the punit communication needed to query the VLV power
> well status can cause substantial delays. Since we can query the state
> frequently, for example during I2C transfers, maintain a cached version
> of the HW state to get rid of this delay.
> 
> This fixes at least one reported regression where boot time increased by
> ~4 seconds due to frequent power well state queries on VLV during eDP
> EDID read.
> 
> Reported-by: Jesse Barnes <jesse.barnes@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

A citation for the regressing commit here (i.e. the one that enabled vlv
runtime pm) would be good so that Jani can pick it up.

Aside: intel_rpm.c with the runtime pm infrastructure + some overview
kerneldoc would be really, really nice.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c |  6 +++---
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  drivers/gpu/drm/i915/intel_pm.c      | 37 +++++++++++++++---------------------
>  4 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8631fb3..000a6ce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -976,6 +976,8 @@ struct i915_power_well {
>  	bool always_on;
>  	/* power well enable/disable usage count */
>  	int count;
> +	/* cached hw enabled state */
> +	bool hw_enabled;
>  	unsigned long domains;
>  	unsigned long data;
>  	const struct i915_power_well_ops *ops;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..882d4f5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12692,8 +12692,8 @@ intel_display_capture_error_state(struct drm_device *dev)
>  
>  	for_each_pipe(i) {
>  		error->pipe[i].power_domain_on =
> -			intel_display_power_enabled_sw(dev_priv,
> -						       POWER_DOMAIN_PIPE(i));
> +			intel_display_power_enabled_unlocked(dev_priv,
> +							   POWER_DOMAIN_PIPE(i));
>  		if (!error->pipe[i].power_domain_on)
>  			continue;
>  
> @@ -12728,7 +12728,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		enum transcoder cpu_transcoder = transcoders[i];
>  
>  		error->transcoder[i].power_domain_on =
> -			intel_display_power_enabled_sw(dev_priv,
> +			intel_display_power_enabled_unlocked(dev_priv,
>  				POWER_DOMAIN_TRANSCODER(cpu_transcoder));
>  		if (!error->transcoder[i].power_domain_on)
>  			continue;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 78d4124..1455ddb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -948,8 +948,8 @@ int intel_power_domains_init(struct drm_i915_private *);
>  void intel_power_domains_remove(struct drm_i915_private *);
>  bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
>  				 enum intel_display_power_domain domain);
> -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> -				    enum intel_display_power_domain domain);
> +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
> +					  enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee27d74..96a2d31 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5801,8 +5801,8 @@ static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
>  		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>  }
>  
> -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
> -				    enum intel_display_power_domain domain)
> +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev_priv,
> +					  enum intel_display_power_domain domain)
>  {
>  	struct i915_power_domains *power_domains;
>  	struct i915_power_well *power_well;
> @@ -5813,16 +5813,19 @@ bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv,
>  		return false;
>  
>  	power_domains = &dev_priv->power_domains;
> +
>  	is_enabled = true;
> +
>  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>  		if (power_well->always_on)
>  			continue;
>  
> -		if (!power_well->count) {
> +		if (!power_well->hw_enabled) {
>  			is_enabled = false;
>  			break;
>  		}
>  	}
> +
>  	return is_enabled;
>  }
>  
> @@ -5830,30 +5833,15 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
>  				 enum intel_display_power_domain domain)
>  {
>  	struct i915_power_domains *power_domains;
> -	struct i915_power_well *power_well;
> -	bool is_enabled;
> -	int i;
> -
> -	if (dev_priv->pm.suspended)
> -		return false;
> +	bool ret;
>  
>  	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->ops->is_enabled(dev_priv, power_well)) {
> -			is_enabled = false;
> -			break;
> -		}
> -	}
> +	ret = intel_display_power_enabled_unlocked(dev_priv, domain);
>  	mutex_unlock(&power_domains->lock);
>  
> -	return is_enabled;
> +	return ret;
>  }
>  
>  /*
> @@ -6174,6 +6162,7 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
>  		if (!power_well->count++) {
>  			DRM_DEBUG_KMS("enabling %s\n", power_well->name);
>  			power_well->ops->enable(dev_priv, power_well);
> +			power_well->hw_enabled = true;
>  		}
>  
>  		check_power_well_state(dev_priv, power_well);
> @@ -6203,6 +6192,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  		if (!--power_well->count && i915.disable_power_well) {
>  			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> +			power_well->hw_enabled = false;
>  			power_well->ops->disable(dev_priv, power_well);
>  		}
>  
> @@ -6463,8 +6453,11 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
>  	int i;
>  
>  	mutex_lock(&power_domains->lock);
> -	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains)
> +	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
>  		power_well->ops->sync_hw(dev_priv, power_well);
> +		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
> +								     power_well);
> +	}
>  	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

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

Re: [PATCH] drm/i915: cache hw power well enabled state

[Imre Deak]

On Fri, 2014-06-06 at 19:19 +0200, Daniel Vetter wrote:
> On Thu, Jun 05, 2014 at 08:31:47PM +0300, Imre Deak wrote:
> > Jesse noticed that the punit communication needed to query the VLV power
> > well status can cause substantial delays. Since we can query the state
> > frequently, for example during I2C transfers, maintain a cached version
> > of the HW state to get rid of this delay.
> > 
> > This fixes at least one reported regression where boot time increased by
> > ~4 seconds due to frequent power well state queries on VLV during eDP
> > EDID read.
> > 
> > Reported-by: Jesse Barnes <jesse.barnes@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> A citation for the regressing commit here (i.e. the one that enabled vlv
> runtime pm) would be good so that Jani can pick it up.

I'm not aware of any other case where we had a significant overhead, so
for the above particular issue I'd say

this was introduced by

commit bb4932c4f17b68f34645ffbcf845e4c29d17290b
Author: Imre Deak <imre.deak@intel.com>
Date:   Mon Apr 14 20:24:33 2014 +0300

    drm/i915: vlv: check port power domain instead of only D0 for eDP
VDD on

> Aside: intel_rpm.c with the runtime pm infrastructure + some overview
> kerneldoc would be really, really nice.

You mean to move out the power well and rpm stuff to this new file?
Agreed, intel_pm.c is rather big already.

--Imre

Re: [PATCH] drm/i915: cache hw power well enabled state

[Daniel Vetter]

On Fri, Jun 06, 2014 at 08:37:48PM +0300, Imre Deak wrote:
> On Fri, 2014-06-06 at 19:19 +0200, Daniel Vetter wrote:
> > On Thu, Jun 05, 2014 at 08:31:47PM +0300, Imre Deak wrote:
> > > Jesse noticed that the punit communication needed to query the VLV power
> > > well status can cause substantial delays. Since we can query the state
> > > frequently, for example during I2C transfers, maintain a cached version
> > > of the HW state to get rid of this delay.
> > > 
> > > This fixes at least one reported regression where boot time increased by
> > > ~4 seconds due to frequent power well state queries on VLV during eDP
> > > EDID read.
> > > 
> > > Reported-by: Jesse Barnes <jesse.barnes@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > A citation for the regressing commit here (i.e. the one that enabled vlv
> > runtime pm) would be good so that Jani can pick it up.
> 
> I'm not aware of any other case where we had a significant overhead, so
> for the above particular issue I'd say
> 
> this was introduced by
> 
> commit bb4932c4f17b68f34645ffbcf845e4c29d17290b
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Mon Apr 14 20:24:33 2014 +0300
> 
>     drm/i915: vlv: check port power domain instead of only D0 for eDP
> VDD on
> 
> > Aside: intel_rpm.c with the runtime pm infrastructure + some overview
> > kerneldoc would be really, really nice.
> 
> You mean to move out the power well and rpm stuff to this new file?
> Agreed, intel_pm.c is rather big already.

Well moving it out alone isn't that useful, but using that chance to add a
bit of documentation for the main functions and an overview would be
really good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: cache hw power well enabled state

[Daniel Vetter]

On Fri, Jun 06, 2014 at 08:37:48PM +0300, Imre Deak wrote:
> On Fri, 2014-06-06 at 19:19 +0200, Daniel Vetter wrote:
> > On Thu, Jun 05, 2014 at 08:31:47PM +0300, Imre Deak wrote:
> > > Jesse noticed that the punit communication needed to query the VLV power
> > > well status can cause substantial delays. Since we can query the state
> > > frequently, for example during I2C transfers, maintain a cached version
> > > of the HW state to get rid of this delay.
> > > 
> > > This fixes at least one reported regression where boot time increased by
> > > ~4 seconds due to frequent power well state queries on VLV during eDP
> > > EDID read.
> > > 
> > > Reported-by: Jesse Barnes <jesse.barnes@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > A citation for the regressing commit here (i.e. the one that enabled vlv
> > runtime pm) would be good so that Jani can pick it up.
> 
> I'm not aware of any other case where we had a significant overhead, so
> for the above particular issue I'd say
> 
> this was introduced by
> 
> commit bb4932c4f17b68f34645ffbcf845e4c29d17290b
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Mon Apr 14 20:24:33 2014 +0300
> 
>     drm/i915: vlv: check port power domain instead of only D0 for eDP
> VDD on

Ok since Jani's out I've merged this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch