| Message ID | 1392674540-10915-17-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 18, 2014 at 12:02:17AM +0200, Imre Deak wrote: > Suggested by Daniel. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e81e7de..21ccf89 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5338,6 +5338,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > hsw_enable_package_c8(dev_priv); > } > > +static void check_power_well_state(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well) > +{ > + bool enabled; > + > + if (!power_well->ops->is_enabled) > + return; > + > + enabled = power_well->ops->is_enabled(dev_priv, power_well); > + > + if (enabled != (power_well->count > 0 || !i915.disable_power_well)) { Doesn't i915.disable_power_well==true mean "leave power wells always enabled"? So I think the '!' needs to be removed. > + DRM_ERROR("state mismatch for '%s' (hw state %d use-count %d disable_power_well %d\n", > + power_well->name, enabled, power_well->count, > + i915.disable_power_well); > + WARN_ON(1); > + } For an error message + backtrace, you could just use WARN(). > +} > + > void intel_display_power_get(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain) > { > @@ -5349,9 +5367,14 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, > > mutex_lock(&power_domains->lock); > > - for_each_power_well(i, power_well, BIT(domain), power_domains) > - if (!power_well->count++ && power_well->ops->enable) > + for_each_power_well(i, power_well, BIT(domain), power_domains) { > + if (!power_well->count++ && power_well->ops->enable) { > + DRM_DEBUG_KMS("enabling %s\n", power_well->name); > power_well->ops->enable(dev_priv, power_well); > + } > + > + check_power_well_state(dev_priv, power_well); > + } > > power_domains->domain_use_count[domain]++; > > @@ -5376,8 +5399,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > WARN_ON(!power_well->count); > > if (!--power_well->count && power_well->ops->disable && > - i915.disable_power_well) > + i915.disable_power_well) { > + DRM_DEBUG_KMS("disabling %s\n", power_well->name); > power_well->ops->disable(dev_priv, power_well); > + } > + > + check_power_well_state(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
On Tue, 2014-02-18 at 18:55 +0200, Ville Syrjälä wrote: > On Tue, Feb 18, 2014 at 12:02:17AM +0200, Imre Deak wrote: > > Suggested by Daniel. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++++--- > > 1 file changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index e81e7de..21ccf89 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5338,6 +5338,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > hsw_enable_package_c8(dev_priv); > > } > > > > +static void check_power_well_state(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + bool enabled; > > + > > + if (!power_well->ops->is_enabled) > > + return; > > + > > + enabled = power_well->ops->is_enabled(dev_priv, power_well); > > + > > + if (enabled != (power_well->count > 0 || !i915.disable_power_well)) { > > Doesn't i915.disable_power_well==true mean "leave power wells always > enabled"? So I think the '!' needs to be removed. No, i915.disable_power_well==true means disable power wells when the refcount goes to 0. Perhaps not the best name/semantics for this kind of option, the default for it should be 0 and mean normal operation, which is to disable power wells when possible. > > + DRM_ERROR("state mismatch for '%s' (hw state %d use-count %d disable_power_well %d\n", > > + power_well->name, enabled, power_well->count, > > + i915.disable_power_well); > > + WARN_ON(1); > > + } > > For an error message + backtrace, you could just use WARN(). Ok. > > +} > > + > > void intel_display_power_get(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain domain) > > { > > @@ -5349,9 +5367,14 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, > > > > mutex_lock(&power_domains->lock); > > > > - for_each_power_well(i, power_well, BIT(domain), power_domains) > > - if (!power_well->count++ && power_well->ops->enable) > > + for_each_power_well(i, power_well, BIT(domain), power_domains) { > > + if (!power_well->count++ && power_well->ops->enable) { > > + DRM_DEBUG_KMS("enabling %s\n", power_well->name); > > power_well->ops->enable(dev_priv, power_well); > > + } > > + > > + check_power_well_state(dev_priv, power_well); > > + } > > > > power_domains->domain_use_count[domain]++; > > > > @@ -5376,8 +5399,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > > WARN_ON(!power_well->count); > > > > if (!--power_well->count && power_well->ops->disable && > > - i915.disable_power_well) > > + i915.disable_power_well) { > > + DRM_DEBUG_KMS("disabling %s\n", power_well->name); > > power_well->ops->disable(dev_priv, power_well); > > + } > > + > > + check_power_well_state(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 >
On Tue, Feb 18, 2014 at 07:37:01PM +0200, Imre Deak wrote: > On Tue, 2014-02-18 at 18:55 +0200, Ville Syrjälä wrote: > > On Tue, Feb 18, 2014 at 12:02:17AM +0200, Imre Deak wrote: > > > Suggested by Daniel. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++++--- > > > 1 file changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index e81e7de..21ccf89 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -5338,6 +5338,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > > hsw_enable_package_c8(dev_priv); > > > } > > > > > > +static void check_power_well_state(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well) > > > +{ > > > + bool enabled; > > > + > > > + if (!power_well->ops->is_enabled) > > > + return; > > > + > > > + enabled = power_well->ops->is_enabled(dev_priv, power_well); > > > + > > > + if (enabled != (power_well->count > 0 || !i915.disable_power_well)) { > > > > Doesn't i915.disable_power_well==true mean "leave power wells always > > enabled"? So I think the '!' needs to be removed. > > No, i915.disable_power_well==true means disable power wells when the > refcount goes to 0. Perhaps not the best name/semantics for this kind of > option, the default for it should be 0 and mean normal operation, which > is to disable power wells when possible. Oh I had the impression it was the other way around, but you're right. Seems I keep getting confused by this thing. It has happened before and I'm guessing it will happen again after I've forgotten the details again. > > > > + DRM_ERROR("state mismatch for '%s' (hw state %d use-count %d disable_power_well %d\n", > > > + power_well->name, enabled, power_well->count, > > > + i915.disable_power_well); > > > + WARN_ON(1); > > > + } > > > > For an error message + backtrace, you could just use WARN(). > > Ok. > > > > +} > > > + > > > void intel_display_power_get(struct drm_i915_private *dev_priv, > > > enum intel_display_power_domain domain) > > > { > > > @@ -5349,9 +5367,14 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, > > > > > > mutex_lock(&power_domains->lock); > > > > > > - for_each_power_well(i, power_well, BIT(domain), power_domains) > > > - if (!power_well->count++ && power_well->ops->enable) > > > + for_each_power_well(i, power_well, BIT(domain), power_domains) { > > > + if (!power_well->count++ && power_well->ops->enable) { > > > + DRM_DEBUG_KMS("enabling %s\n", power_well->name); > > > power_well->ops->enable(dev_priv, power_well); > > > + } > > > + > > > + check_power_well_state(dev_priv, power_well); > > > + } > > > > > > power_domains->domain_use_count[domain]++; > > > > > > @@ -5376,8 +5399,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > > > WARN_ON(!power_well->count); > > > > > > if (!--power_well->count && power_well->ops->disable && > > > - i915.disable_power_well) > > > + i915.disable_power_well) { > > > + DRM_DEBUG_KMS("disabling %s\n", power_well->name); > > > power_well->ops->disable(dev_priv, power_well); > > > + } > > > + > > > + check_power_well_state(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 > > >
On Tue, Feb 18, 2014 at 07:59:05PM +0200, Ville Syrjälä wrote: > On Tue, Feb 18, 2014 at 07:37:01PM +0200, Imre Deak wrote: > > On Tue, 2014-02-18 at 18:55 +0200, Ville Syrjälä wrote: > > > On Tue, Feb 18, 2014 at 12:02:17AM +0200, Imre Deak wrote: > > > > Suggested by Daniel. > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++++--- > > > > 1 file changed, 30 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index e81e7de..21ccf89 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -5338,6 +5338,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > > > hsw_enable_package_c8(dev_priv); > > > > } > > > > > > > > +static void check_power_well_state(struct drm_i915_private *dev_priv, > > > > + struct i915_power_well *power_well) > > > > +{ > > > > + bool enabled; > > > > + > > > > + if (!power_well->ops->is_enabled) > > > > + return; > > > > + > > > > + enabled = power_well->ops->is_enabled(dev_priv, power_well); > > > > + > > > > + if (enabled != (power_well->count > 0 || !i915.disable_power_well)) { > > > > > > Doesn't i915.disable_power_well==true mean "leave power wells always > > > enabled"? So I think the '!' needs to be removed. > > > > No, i915.disable_power_well==true means disable power wells when the > > refcount goes to 0. Perhaps not the best name/semantics for this kind of > > option, the default for it should be 0 and mean normal operation, which > > is to disable power wells when possible. > > Oh I had the impression it was the other way around, but you're right. > Seems I keep getting confused by this thing. It has happened before and > I'm guessing it will happen again after I've forgotten the details > again. I concur that a module option rename pass is in order. Maybe once this has all settled a bit ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index e81e7de..21ccf89 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5338,6 +5338,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, hsw_enable_package_c8(dev_priv); } +static void check_power_well_state(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + bool enabled; + + if (!power_well->ops->is_enabled) + return; + + enabled = power_well->ops->is_enabled(dev_priv, power_well); + + if (enabled != (power_well->count > 0 || !i915.disable_power_well)) { + DRM_ERROR("state mismatch for '%s' (hw state %d use-count %d disable_power_well %d\n", + power_well->name, enabled, power_well->count, + i915.disable_power_well); + WARN_ON(1); + } +} + void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain) { @@ -5349,9 +5367,14 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, mutex_lock(&power_domains->lock); - for_each_power_well(i, power_well, BIT(domain), power_domains) - if (!power_well->count++ && power_well->ops->enable) + for_each_power_well(i, power_well, BIT(domain), power_domains) { + if (!power_well->count++ && power_well->ops->enable) { + DRM_DEBUG_KMS("enabling %s\n", power_well->name); power_well->ops->enable(dev_priv, power_well); + } + + check_power_well_state(dev_priv, power_well); + } power_domains->domain_use_count[domain]++; @@ -5376,8 +5399,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, WARN_ON(!power_well->count); if (!--power_well->count && power_well->ops->disable && - i915.disable_power_well) + i915.disable_power_well) { + DRM_DEBUG_KMS("disabling %s\n", power_well->name); power_well->ops->disable(dev_priv, power_well); + } + + check_power_well_state(dev_priv, power_well); } mutex_unlock(&power_domains->lock);
Suggested by Daniel. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)