On Fri, 2013-11-22 at 13:53 -0200, Paulo Zanoni wrote:
> 2013/11/14 Imre Deak <imre.deak@xxxxxxxxx>:
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
