2013/10/22 Imre Deak <imre.deak@xxxxxxxxx>:
> In the future we'll need to support multiple power wells, so prepare for
> that here. Create a new power domains struct which contains all
> power domain/well specific fields. Since we'll have one lock protecting
> all power wells, move power_well->lock to the new struct too.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 11 ++++++---
> drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++----------------
> 2 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 80957ca..7315095 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -911,12 +911,18 @@ struct intel_ilk_power_mgmt {
> /* Power well structure for haswell */
> struct i915_power_well {
> struct drm_device *device;
Can't we also move "device" to i915_power_domains?
> - struct mutex lock;
> /* power well enable/disable usage count */
> int count;
> int i915_request;
> };
>
> +#define I915_MAX_POWER_WELLS 1
How about this?
enum intel_power_wells {
HASWELL_POWER_WELL = 0, /* Or any other more creative name,
since I guess we'll have an equivalent power well on other gens */
I915_MAX_POWER_WELLS,
};
And then you switch all the code below that uses index zero for the
actual enum name.
Maybe "NON_LPSP_POWER_WELL = 0" since the docs describe LPSP (Low
Power Single Pipe) as a feature that is enabled when the power well is
disabled. Feel free to invent better names :)
> +
> +struct i915_power_domains {
> + struct mutex lock;
> + struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> +};
> +
> struct i915_dri1_state {
> unsigned allow_batchbuffer : 1;
> u32 __iomem *gfx_hws_cpu_addr;
> @@ -1412,8 +1418,7 @@ typedef struct drm_i915_private {
> * mchdev_lock in intel_pm.c */
> struct intel_ilk_power_mgmt ips;
>
> - /* Haswell power well */
> - struct i915_power_well power_well;
> + struct i915_power_domains power_domains;
>
> struct i915_psr psr;
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3e140ab..09ca6f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5625,7 +5625,7 @@ void intel_display_power_get(struct drm_device *dev,
> enum intel_display_power_domain domain)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct i915_power_well *power_well = &dev_priv->power_well;
> + struct i915_power_domains *power_domains;
>
> if (!HAS_POWER_WELL(dev))
> return;
> @@ -5633,16 +5633,18 @@ void intel_display_power_get(struct drm_device *dev,
> if (is_always_on_power_domain(dev, domain))
> return;
>
> - mutex_lock(&power_well->lock);
> - __intel_power_well_get(power_well);
> - mutex_unlock(&power_well->lock);
> + power_domains = &dev_priv->power_domains;
> +
> + mutex_lock(&power_domains->lock);
> + __intel_power_well_get(&power_domains->power_wells[0]);
> + mutex_unlock(&power_domains->lock);
> }
>
> void intel_display_power_put(struct drm_device *dev,
> enum intel_display_power_domain domain)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct i915_power_well *power_well = &dev_priv->power_well;
> + struct i915_power_domains *power_domains;
>
> if (!HAS_POWER_WELL(dev))
> return;
> @@ -5650,12 +5652,14 @@ void intel_display_power_put(struct drm_device *dev,
> if (is_always_on_power_domain(dev, domain))
> return;
>
> - mutex_lock(&power_well->lock);
> - __intel_power_well_put(power_well);
> - mutex_unlock(&power_well->lock);
> + power_domains = &dev_priv->power_domains;
> +
> + mutex_lock(&power_domains->lock);
> + __intel_power_well_put(&power_domains->power_wells[0]);
> + mutex_unlock(&power_domains->lock);
> }
>
> -static struct i915_power_well *hsw_pwr;
> +static struct i915_power_domains *hsw_pwr;
>
> /* Display audio driver power well request */
> void i915_request_power_well(void)
> @@ -5664,7 +5668,7 @@ void i915_request_power_well(void)
> return;
>
> mutex_lock(&hsw_pwr->lock);
> - __intel_power_well_get(hsw_pwr);
> + __intel_power_well_get(&hsw_pwr->power_wells[0]);
> mutex_unlock(&hsw_pwr->lock);
> }
> EXPORT_SYMBOL_GPL(i915_request_power_well);
> @@ -5676,7 +5680,7 @@ void i915_release_power_well(void)
> return;
>
> mutex_lock(&hsw_pwr->lock);
> - __intel_power_well_put(hsw_pwr);
> + __intel_power_well_put(&hsw_pwr->power_wells[0]);
> mutex_unlock(&hsw_pwr->lock);
> }
> EXPORT_SYMBOL_GPL(i915_release_power_well);
> @@ -5684,12 +5688,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
> int i915_init_power_well(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;
>
> - hsw_pwr = &dev_priv->power_well;
> + mutex_init(&power_domains->lock);
> + hsw_pwr = power_domains;
>
> - hsw_pwr->device = dev;
> - mutex_init(&hsw_pwr->lock);
> - hsw_pwr->count = 0;
> + power_well = &power_domains->power_wells[0];
> + power_well->device = dev;
> + power_well->count = 0;
How about this?
for (i = 0; i < I915_MAX_POWER_WELLS; i++) {
power_well = &power_domains->power_wells[0];
power_well->device = dev; /* If you don't move this to the
power_domains struct. */
power_well->count = 0;
}
Then we'll always be safe :)
With or without my 3 bikesheds (although I would really like to see them):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
By the way, do you also plan to change some functions so they take
struct i915_power_well (or an index representing the power well
number) as a parameter? E.g., intel_set_power_well.
>
> return 0;
> }
> @@ -5702,7 +5709,8 @@ void i915_remove_power_well(struct drm_device *dev)
> void intel_set_power_well(struct drm_device *dev, bool enable)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct i915_power_well *power_well = &dev_priv->power_well;
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> + struct i915_power_well *power_well;
>
> if (!HAS_POWER_WELL(dev))
> return;
> @@ -5710,8 +5718,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> if (!i915_disable_power_well && !enable)
> return;
>
> - mutex_lock(&power_well->lock);
> + mutex_lock(&power_domains->lock);
>
> + power_well = &power_domains->power_wells[0];
> /*
> * This function will only ever contribute one
> * to the power well reference count. i915_request
> @@ -5729,20 +5738,24 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> __intel_power_well_put(power_well);
>
> out:
> - mutex_unlock(&power_well->lock);
> + mutex_unlock(&power_domains->lock);
> }
>
> static void intel_resume_power_well(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct i915_power_well *power_well = &dev_priv->power_well;
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> + struct i915_power_well *power_well;
>
> if (!HAS_POWER_WELL(dev))
> return;
>
> - mutex_lock(&power_well->lock);
> + mutex_lock(&power_domains->lock);
> +
> + power_well = &power_domains->power_wells[0];
> __intel_set_power_well(dev, power_well->count > 0);
> - mutex_unlock(&power_well->lock);
> +
> + 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
