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

[Daniel Vetter <daniel@ffwll.ch>]

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