[Intel-gfx] [PATCH v2 3/8] drm/i915: add always-on power wells instead of special casing them

[Paulo Zanoni]

2013/11/14 Imre Deak <imre.deak at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni