2013/11/14 Imre Deak <imre.deak@xxxxxxxxx>:
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> /* 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
