| Message ID | 1384434635-3077-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/11/14 Imre Deak <imre.deak@intel.com>: > Instead of using a separate function to check whether a power domain is > is always on, add an always-on power well covering all these power > domains and do the usual get/put on these unconditionally. Since we > don't assign a .set handler for these the get/put won't have any effect > besides the adjusted refcount. Oh, now I see why you had all those checks for the existence of the "set" function :) > > This makes the code more readable and provides debug info also on the > use of always-on power wells (once the relevant debugfs entry is added.) > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++---------------------------- > 2 files changed, 14 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b20016c..ff3314d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt { > /* Power well structure for haswell */ > struct i915_power_well { > const char *name; > + unsigned always_on:1; On our driver we have many cases where we just use many "bool" variables, and we also have many cases where we use single-bit variables like this. On this specific case we're not gaining anything by using the single-bit variable, so I'm not sure if it's the most appropriate thing to use. I wish we had a guideline telling us which one is preferred on each case :) Anyway: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > /* power well enable/disable usage count */ > int count; > unsigned long domains; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2b39a9c..ee5aeb1 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5606,25 +5606,6 @@ void intel_suspend_hw(struct drm_device *dev) > lpt_suspend_hw(dev); > } > > -static bool is_always_on_power_domain(struct drm_device *dev, > - enum intel_display_power_domain domain) > -{ > - unsigned long always_on_domains; > - > - BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK); > - > - if (IS_BROADWELL(dev)) { > - always_on_domains = BDW_ALWAYS_ON_POWER_DOMAINS; > - } else if (IS_HASWELL(dev)) { > - always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS; > - } else { > - WARN_ON(1); > - return true; > - } > - > - return BIT(domain) & always_on_domains; > -} > - > #define for_each_power_well(i, power_well, domain_mask, power_domains) \ > for (i = 0; \ > i < (power_domains)->power_well_count && \ > @@ -5664,15 +5645,15 @@ bool intel_display_power_enabled(struct drm_device *dev, > if (!HAS_POWER_WELL(dev)) > return true; > > - if (is_always_on_power_domain(dev, domain)) > - return true; > - > 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->is_enabled(dev, power_well)) { > is_enabled = false; > break; > @@ -5774,9 +5755,6 @@ void intel_display_power_get(struct drm_device *dev, > if (!HAS_POWER_WELL(dev)) > return; > > - if (is_always_on_power_domain(dev, domain)) > - return; > - > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > @@ -5796,9 +5774,6 @@ void intel_display_power_put(struct drm_device *dev, > if (!HAS_POWER_WELL(dev)) > return; > > - if (is_always_on_power_domain(dev, domain)) > - return; > - > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > @@ -5839,6 +5814,11 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); > > static struct i915_power_well hsw_power_wells[] = { > { > + .name = "always-on", > + .always_on = 1, > + .domains = HSW_ALWAYS_ON_POWER_DOMAINS, > + }, > + { > .name = "display", > .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS, > .is_enabled = hsw_power_well_enabled, > @@ -5848,6 +5828,11 @@ static struct i915_power_well hsw_power_wells[] = { > > static struct i915_power_well bdw_power_wells[] = { > { > + .name = "always-on", > + .always_on = 1, > + .domains = BDW_ALWAYS_ON_POWER_DOMAINS, > + }, > + { > .name = "display", > .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS, > .is_enabled = hsw_power_well_enabled, > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 22, 2013 at 02:04:02PM -0200, Paulo Zanoni wrote: > 2013/11/14 Imre Deak <imre.deak@intel.com>: > > Instead of using a separate function to check whether a power domain is > > is always on, add an always-on power well covering all these power > > domains and do the usual get/put on these unconditionally. Since we > > don't assign a .set handler for these the get/put won't have any effect > > besides the adjusted refcount. > > Oh, now I see why you had all those checks for the existence of the > "set" function :) > > > > > > This makes the code more readable and provides debug info also on the > > use of always-on power wells (once the relevant debugfs entry is added.) > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++---------------------------- > > 2 files changed, 14 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index b20016c..ff3314d 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt { > > /* Power well structure for haswell */ > > struct i915_power_well { > > const char *name; > > + unsigned always_on:1; > > On our driver we have many cases where we just use many "bool" > variables, and we also have many cases where we use single-bit > variables like this. On this specific case we're not gaining anything > by using the single-bit variable, so I'm not sure if it's the most > appropriate thing to use. I wish we had a guideline telling us which > one is preferred on each case :) I wish we'd use 'bool foo:1' for single bit bitfields. Otherwise stuff like 'foo = 2' doesn't work (you get false instead of true).
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b20016c..ff3314d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -946,6 +946,7 @@ struct intel_ilk_power_mgmt { /* Power well structure for haswell */ struct i915_power_well { const char *name; + unsigned always_on:1; /* power well enable/disable usage count */ int count; unsigned long domains; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2b39a9c..ee5aeb1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5606,25 +5606,6 @@ void intel_suspend_hw(struct drm_device *dev) lpt_suspend_hw(dev); } -static bool is_always_on_power_domain(struct drm_device *dev, - enum intel_display_power_domain domain) -{ - unsigned long always_on_domains; - - BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK); - - if (IS_BROADWELL(dev)) { - always_on_domains = BDW_ALWAYS_ON_POWER_DOMAINS; - } else if (IS_HASWELL(dev)) { - always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS; - } else { - WARN_ON(1); - return true; - } - - return BIT(domain) & always_on_domains; -} - #define for_each_power_well(i, power_well, domain_mask, power_domains) \ for (i = 0; \ i < (power_domains)->power_well_count && \ @@ -5664,15 +5645,15 @@ bool intel_display_power_enabled(struct drm_device *dev, if (!HAS_POWER_WELL(dev)) return true; - if (is_always_on_power_domain(dev, domain)) - return true; - 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->is_enabled(dev, power_well)) { is_enabled = false; break; @@ -5774,9 +5755,6 @@ void intel_display_power_get(struct drm_device *dev, if (!HAS_POWER_WELL(dev)) return; - if (is_always_on_power_domain(dev, domain)) - return; - power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); @@ -5796,9 +5774,6 @@ void intel_display_power_put(struct drm_device *dev, if (!HAS_POWER_WELL(dev)) return; - if (is_always_on_power_domain(dev, domain)) - return; - power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); @@ -5839,6 +5814,11 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); static struct i915_power_well hsw_power_wells[] = { { + .name = "always-on", + .always_on = 1, + .domains = HSW_ALWAYS_ON_POWER_DOMAINS, + }, + { .name = "display", .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS, .is_enabled = hsw_power_well_enabled, @@ -5848,6 +5828,11 @@ static struct i915_power_well hsw_power_wells[] = { static struct i915_power_well bdw_power_wells[] = { { + .name = "always-on", + .always_on = 1, + .domains = BDW_ALWAYS_ON_POWER_DOMAINS, + }, + { .name = "display", .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS, .is_enabled = hsw_power_well_enabled,
Instead of using a separate function to check whether a power domain is is always on, add an always-on power well covering all these power domains and do the usual get/put on these unconditionally. Since we don't assign a .set handler for these the get/put won't have any effect besides the adjusted refcount. This makes the code more readable and provides debug info also on the use of always-on power wells (once the relevant debugfs entry is added.) Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 41 +++++++++++++---------------------------- 2 files changed, 14 insertions(+), 28 deletions(-)