2013/11/22 Imre Deak <imre.deak@xxxxxxxxx>:
> 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.
Oh, ok. My main worry with this kind of approach is that, for example,
in the future we may bikeshed the patch that uses the "data" field so
it will no longer use that field, then we'll forget to remove the
now-unused "data" field.
>
>> > };
>> >
>> > -#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
>>
>>
>>
>
>
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
