Re: [PATCH v2 2/8] drm/i915: support for multiple power wells

[Imre Deak <imre.deak@intel.com>]

On Fri, 2013-11-22 at 13:53 -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@intel.com>:
> > HW generations so far had only one always-on power well and optionally
> > one dynamic power well. Upcoming HW gens may have multiple dynamic power
> > wells, so add some infrastructure to support them.
> >
> > The idea is to keep the existing power domain API used by the rest of
> > the driver and create a mapping between these power domains and the
> > underlying power wells. This mapping can differ from one HW to another
> > but high level driver code doesn't need to know about this. Through the
> > existing get/put API it would just ask for a given power domain and the
> > power domain framework would make sure the relevant power wells get
> > enabled in the right order.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  12 +++-
> >  drivers/gpu/drm/i915/intel_pm.c | 128 ++++++++++++++++++++++++++++++++--------
> >  2 files changed, 114 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7007b9b..b20016c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -945,21 +945,27 @@ struct intel_ilk_power_mgmt {
> >
> >  /* Power well structure for haswell */
> >  struct i915_power_well {
> > +       const char *name;
> >         /* power well enable/disable usage count */
> >         int count;
> > +       unsigned long domains;
> > +       void *data;
> 
> This "data" field is completely unused.
> 
> 
> > +       void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
> > +                   bool enable);
> > +       bool (*is_enabled)(struct drm_device *dev,
> > +                          struct i915_power_well *power_well);
> 
> The "power_well" argument of both functions is also unused.

These are for platforms that handle multiple power wells with a
single .set handler, determining the exact power well via the .data
member. This should've been in the log as it's not clear from the
current patchset.

> >  };
> >
> > -#define I915_MAX_POWER_WELLS 1
> > -
> >  struct i915_power_domains {
> >         /*
> >          * Power wells needed for initialization at driver init and suspend
> >          * time are on. They are kept on until after the first modeset.
> >          */
> >         bool init_power_on;
> > +       int power_well_count;
> >
> >         struct mutex lock;
> > -       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> > +       struct i915_power_well *power_wells;
> >  };
> >
> >  struct i915_dri1_state {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bcca995..2b39a9c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5625,27 +5625,66 @@ static bool is_always_on_power_domain(struct drm_device *dev,
> >         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 &&                   \
> > +                ((power_well) = &(power_domains)->power_wells[i]);     \
> > +            i++)                                                       \
> > +               if ((power_well)->domains & (domain_mask))
> > +
> > +#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
> > +       for (i = (power_domains)->power_well_count - 1;                  \
> > +            i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> > +            i--)                                                        \
> > +               if ((power_well)->domains & (domain_mask))
> > +
> >  /**
> >   * We should only use the power well if we explicitly asked the hardware to
> >   * enable it, so check if it's enabled and also check if we've requested it to
> >   * be enabled.
> >   */
> > -bool intel_display_power_enabled(struct drm_device *dev,
> > -                                enum intel_display_power_domain domain)
> > +static bool hsw_power_well_enabled(struct drm_device *dev,
> > +                                  struct i915_power_well *power_well)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > -       if (!HAS_POWER_WELL(dev))
> > -               return true;
> > -
> > -       if (is_always_on_power_domain(dev, domain))
> > -               return true;
> > -
> >         return I915_READ(HSW_PWR_WELL_DRIVER) ==
> >                      (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
> >  }
> >
> > -static void __intel_set_power_well(struct drm_device *dev, bool enable)
> > +bool intel_display_power_enabled(struct drm_device *dev,
> > +                                enum intel_display_power_domain domain)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_domains *power_domains;
> > +       struct i915_power_well *power_well;
> > +       bool is_enabled;
> > +       int i;
> > +
> > +       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->is_enabled(dev, power_well)) {
> > +                       is_enabled = false;
> > +                       break;
> > +               }
> > +       }
> > +       mutex_unlock(&power_domains->lock);
> > +
> > +       return is_enabled;
> > +}
> > +
> > +static void hsw_set_power_well(struct drm_device *dev,
> > +                              struct i915_power_well *power_well, bool enable)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         bool is_enabled, enable_requested;
> > @@ -5711,16 +5750,17 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> >  static void __intel_power_well_get(struct drm_device *dev,
> >                                    struct i915_power_well *power_well)
> >  {
> > -       if (!power_well->count++)
> > -               __intel_set_power_well(dev, true);
> > +       if (!power_well->count++ && power_well->set)
> > +               power_well->set(dev, power_well, true);
> 
> The "set" function always exists for all power wells, so we shouldn't
> check for it. It's better if we get a segfault that tells us we forgot
> to implement the set() function than if we just silently not run
> anything.

These are for the default power wells w/o any need to poke at the HW as
you later point out.

> >  }
> >
> >  static void __intel_power_well_put(struct drm_device *dev,
> >                                    struct i915_power_well *power_well)
> >  {
> >         WARN_ON(!power_well->count);
> > -       if (!--power_well->count && i915_disable_power_well)
> > -               __intel_set_power_well(dev, false);
> > +
> > +       if (!--power_well->count && power_well->set && i915_disable_power_well)
> 
> Same here.
> 
> 
> > +               power_well->set(dev, power_well, false);
> >  }
> >
> >  void intel_display_power_get(struct drm_device *dev,
> > @@ -5728,6 +5768,8 @@ void intel_display_power_get(struct drm_device *dev,
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains;
> > +       struct i915_power_well *power_well;
> > +       int i;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> > @@ -5738,7 +5780,8 @@ void intel_display_power_get(struct drm_device *dev,
> >         power_domains = &dev_priv->power_domains;
> >
> >         mutex_lock(&power_domains->lock);
> > -       __intel_power_well_get(dev, &power_domains->power_wells[0]);
> > +       for_each_power_well(i, power_well, BIT(domain), power_domains)
> > +               __intel_power_well_get(dev, power_well);
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > @@ -5747,6 +5790,8 @@ void intel_display_power_put(struct drm_device *dev,
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains;
> > +       struct i915_power_well *power_well;
> > +       int i;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> > @@ -5757,7 +5802,8 @@ void intel_display_power_put(struct drm_device *dev,
> >         power_domains = &dev_priv->power_domains;
> >
> >         mutex_lock(&power_domains->lock);
> > -       __intel_power_well_put(dev, &power_domains->power_wells[0]);
> > +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> > +               __intel_power_well_put(dev, power_well);
> >         mutex_unlock(&power_domains->lock);
> >  }
> >
> > @@ -5791,17 +5837,52 @@ void i915_release_power_well(void)
> >  }
> >  EXPORT_SYMBOL_GPL(i915_release_power_well);
> >
> > +static struct i915_power_well hsw_power_wells[] = {
> > +       {
> > +               .name = "display",
> > +               .domains = POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS,
> > +               .is_enabled = hsw_power_well_enabled,
> > +               .set = hsw_set_power_well,
> > +       },
> > +};
> > +
> > +static struct i915_power_well bdw_power_wells[] = {
> > +       {
> > +               .name = "display",
> > +               .domains = POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS,
> > +               .is_enabled = hsw_power_well_enabled,
> > +               .set = hsw_set_power_well,
> > +       },
> > +};
> > +
> > +#define set_power_wells(power_domains, __power_wells) ({               \
> > +       (power_domains)->power_wells = (__power_wells);                 \
> > +       (power_domains)->power_well_count = ARRAY_SIZE(__power_wells);  \
> > +})
> 
> IMHO this macro is just over-complicating things. Please just do the
> two assignments directly instead of calling the macro.

At the moment yes, but we'll have quite a few more power wells later
where we need to do the same thing. Again not obvious from this patchset
alone.

> > +
> >  int intel_power_domains_init(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > -       struct i915_power_well *power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return 0;
> >
> >         mutex_init(&power_domains->lock);
> > -       hsw_pwr = power_domains;
> >
> > -       power_well = &power_domains->power_wells[0];
> > -       power_well->count = 0;
> > +       /*
> > +        * The enabling order will be from lower to higher indexed wells,
> > +        * the disabling order is reversed.
> > +        */
> > +       if (IS_HASWELL(dev)) {
> > +               set_power_wells(power_domains, hsw_power_wells);
> > +               hsw_pwr = power_domains;
> > +       } else if (IS_BROADWELL(dev)) {
> > +               set_power_wells(power_domains, bdw_power_wells);
> > +               hsw_pwr = power_domains;
> 
> I wonder if there's a way to treat Haswell and Broadwell as exactly
> the same thing, except for one single "if" statement somewhere.

They are the same except a different .domains mask.

--Imre

> > +       } else {
> > +               WARN_ON(1);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -5816,15 +5897,16 @@ static void intel_power_domains_resume(struct drm_device *dev)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >         struct i915_power_well *power_well;
> > +       int i;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> >
> >         mutex_lock(&power_domains->lock);
> > -
> > -       power_well = &power_domains->power_wells[0];
> > -       __intel_set_power_well(dev, power_well->count > 0);
> > -
> > +       for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> > +               if (power_well->set)
> 
> Same here: the set() function should always exist.
> 
> > +                       power_well->set(dev, power_well, power_well->count > 0);
> > +       }
> >         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
> 
> 
>