On Thu, 5 Jun 2014 20:31:47 +0300
Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
