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.
> };
>
> -#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.
> }
>
> 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.
> +
> 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.
> + } 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
