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@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:
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@xxxxxxxxxxxxxxxx>
>
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
