| Message ID | 1392674540-10915-3-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Feb 2014 00:02:03 +0200 Imre Deak <imre.deak@intel.com> wrote: > These functions are used only by a single call site and are simple > enough to just fold them in. > > No functional change. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++------------------------ > 1 file changed, 13 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index aa9c2df..db48d55 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5305,27 +5305,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, > } > } > > -static void __intel_power_well_get(struct drm_i915_private *dev_priv, > - struct i915_power_well *power_well) > -{ > - if (!power_well->count++ && power_well->set) { > - hsw_disable_package_c8(dev_priv); > - power_well->set(dev_priv, power_well, true); > - } > -} > - > -static void __intel_power_well_put(struct drm_i915_private *dev_priv, > - struct i915_power_well *power_well) > -{ > - WARN_ON(!power_well->count); > - > - if (!--power_well->count && power_well->set && > - i915.disable_power_well) { > - power_well->set(dev_priv, power_well, false); > - hsw_enable_package_c8(dev_priv); > - } > -} > - > void intel_display_power_get(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain) > { > @@ -5338,7 +5317,10 @@ 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) > - __intel_power_well_get(dev_priv, power_well); > + if (!power_well->count++ && power_well->set) { > + hsw_disable_package_c8(dev_priv); > + power_well->set(dev_priv, power_well, true); > + } > > power_domains->domain_use_count[domain]++; > > @@ -5359,8 +5341,15 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > WARN_ON(!power_domains->domain_use_count[domain]); > power_domains->domain_use_count[domain]--; > > - for_each_power_well_rev(i, power_well, BIT(domain), power_domains) > - __intel_power_well_put(dev_priv, power_well); > + for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { > + WARN_ON(!power_well->count); > + > + if (!--power_well->count && power_well->set && > + i915.disable_power_well) { > + power_well->set(dev_priv, power_well, false); > + hsw_enable_package_c8(dev_priv); > + } > + } > > mutex_unlock(&power_domains->lock); > } Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Thu, Feb 20, 2014 at 11:17:31AM -0800, Jesse Barnes wrote: > On Tue, 18 Feb 2014 00:02:03 +0200 > Imre Deak <imre.deak@intel.com> wrote: > > > These functions are used only by a single call site and are simple > > enough to just fold them in. > > > > No functional change. There should be a much better reason (or any reason at all!) for removing self-descriptive code. -Chris
2014-02-17 19:02 GMT-03:00 Imre Deak <imre.deak@intel.com>: > These functions are used only by a single call site and are simple > enough to just fold them in. > > No functional change. > > Signed-off-by: Imre Deak <imre.deak@intel.com> IMHO, this makes the code harder to read. I would prefer to not have this change. > --- > drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++------------------------ > 1 file changed, 13 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index aa9c2df..db48d55 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5305,27 +5305,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, > } > } > > -static void __intel_power_well_get(struct drm_i915_private *dev_priv, > - struct i915_power_well *power_well) > -{ > - if (!power_well->count++ && power_well->set) { > - hsw_disable_package_c8(dev_priv); > - power_well->set(dev_priv, power_well, true); > - } > -} > - > -static void __intel_power_well_put(struct drm_i915_private *dev_priv, > - struct i915_power_well *power_well) > -{ > - WARN_ON(!power_well->count); > - > - if (!--power_well->count && power_well->set && > - i915.disable_power_well) { > - power_well->set(dev_priv, power_well, false); > - hsw_enable_package_c8(dev_priv); > - } > -} > - > void intel_display_power_get(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain) > { > @@ -5338,7 +5317,10 @@ 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) > - __intel_power_well_get(dev_priv, power_well); > + if (!power_well->count++ && power_well->set) { > + hsw_disable_package_c8(dev_priv); > + power_well->set(dev_priv, power_well, true); > + } > > power_domains->domain_use_count[domain]++; > > @@ -5359,8 +5341,15 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > WARN_ON(!power_domains->domain_use_count[domain]); > power_domains->domain_use_count[domain]--; > > - for_each_power_well_rev(i, power_well, BIT(domain), power_domains) > - __intel_power_well_put(dev_priv, power_well); > + for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { > + WARN_ON(!power_well->count); > + > + if (!--power_well->count && power_well->set && > + i915.disable_power_well) { > + power_well->set(dev_priv, power_well, false); > + hsw_enable_package_c8(dev_priv); > + } > + } > > 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 Mon, 2014-02-24 at 10:23 -0300, Paulo Zanoni wrote: > 2014-02-17 19:02 GMT-03:00 Imre Deak <imre.deak@intel.com>: > > These functions are used only by a single call site and are simple > > enough to just fold them in. > > > > No functional change. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > IMHO, this makes the code harder to read. I would prefer to not have > this change. I find intel_display_power_put() after patch 19/19 simple enough to not have the separate functions. I'd say that in this case readability is actually better with the inlined version, as you don't have to jump between two functions when reading the code. --Imre > > --- > > drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++------------------------ > > 1 file changed, 13 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index aa9c2df..db48d55 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5305,27 +5305,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, > > } > > } > > > > -static void __intel_power_well_get(struct drm_i915_private *dev_priv, > > - struct i915_power_well *power_well) > > -{ > > - if (!power_well->count++ && power_well->set) { > > - hsw_disable_package_c8(dev_priv); > > - power_well->set(dev_priv, power_well, true); > > - } > > -} > > - > > -static void __intel_power_well_put(struct drm_i915_private *dev_priv, > > - struct i915_power_well *power_well) > > -{ > > - WARN_ON(!power_well->count); > > - > > - if (!--power_well->count && power_well->set && > > - i915.disable_power_well) { > > - power_well->set(dev_priv, power_well, false); > > - hsw_enable_package_c8(dev_priv); > > - } > > -} > > - > > void intel_display_power_get(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain domain) > > { > > @@ -5338,7 +5317,10 @@ 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) > > - __intel_power_well_get(dev_priv, power_well); > > + if (!power_well->count++ && power_well->set) { > > + hsw_disable_package_c8(dev_priv); > > + power_well->set(dev_priv, power_well, true); > > + } > > > > power_domains->domain_use_count[domain]++; > > > > @@ -5359,8 +5341,15 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > > WARN_ON(!power_domains->domain_use_count[domain]); > > power_domains->domain_use_count[domain]--; > > > > - for_each_power_well_rev(i, power_well, BIT(domain), power_domains) > > - __intel_power_well_put(dev_priv, power_well); > > + for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { > > + WARN_ON(!power_well->count); > > + > > + if (!--power_well->count && power_well->set && > > + i915.disable_power_well) { > > + power_well->set(dev_priv, power_well, false); > > + hsw_enable_package_c8(dev_priv); > > + } > > + } > > > > 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 > > >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index aa9c2df..db48d55 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5305,27 +5305,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, } } -static void __intel_power_well_get(struct drm_i915_private *dev_priv, - struct i915_power_well *power_well) -{ - if (!power_well->count++ && power_well->set) { - hsw_disable_package_c8(dev_priv); - power_well->set(dev_priv, power_well, true); - } -} - -static void __intel_power_well_put(struct drm_i915_private *dev_priv, - struct i915_power_well *power_well) -{ - WARN_ON(!power_well->count); - - if (!--power_well->count && power_well->set && - i915.disable_power_well) { - power_well->set(dev_priv, power_well, false); - hsw_enable_package_c8(dev_priv); - } -} - void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain) { @@ -5338,7 +5317,10 @@ 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) - __intel_power_well_get(dev_priv, power_well); + if (!power_well->count++ && power_well->set) { + hsw_disable_package_c8(dev_priv); + power_well->set(dev_priv, power_well, true); + } power_domains->domain_use_count[domain]++; @@ -5359,8 +5341,15 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, WARN_ON(!power_domains->domain_use_count[domain]); power_domains->domain_use_count[domain]--; - for_each_power_well_rev(i, power_well, BIT(domain), power_domains) - __intel_power_well_put(dev_priv, power_well); + for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { + WARN_ON(!power_well->count); + + if (!--power_well->count && power_well->set && + i915.disable_power_well) { + power_well->set(dev_priv, power_well, false); + hsw_enable_package_c8(dev_priv); + } + } mutex_unlock(&power_domains->lock); }
These functions are used only by a single call site and are simple enough to just fold them in. No functional change. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)