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

[Imre Deak <imre.deak@intel.com>]

[-- 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