| Message ID | 1382464076-5030-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/22 Imre Deak <imre.deak@intel.com>: > 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@intel.com> > --- > 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@intel.com> 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2013-10-23 at 11:56 -0200, Paulo Zanoni wrote: > 2013/10/22 Imre Deak <imre.deak@intel.com>: > > 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@intel.com> > > --- > > 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? Yes, that's a better place for it, especially since there is no point in duplicating it in every instance of the power well struct. Afaics the only real reason we need to store device is i915_{request,release}_power_well, but those could take it from power_domains and pass it to __intel_power_well_{get,put}. This is a somewhat bigger/independent change, so I'd follow up with a separate patch for it. > > - 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, > }; I was also thinking about naming the power wells somehow. They are different on different platforms, so the above enum would have the same values with different names, stg like: enum intel_power_wells { HASWELL_POWER_WELL = 0, VALLEYVIEW_POWER_WELL = 0, XX_POWER_WELL0 = 0, XX_POWER_WELL1 = 1, XX_POWER_WELL2 = 2, I915_MAX_POWER_WELLS, }; Which would work, but is a bit messy imo. Atm I don't see the point of having these names, since we will only reference them in terms of the power domains they support. > And then you switch all the code below that uses index zero for the > actual enum name. Yea, that's not so nice, but it would vanish as soon as we switch to reference the power wells in the above way. I have a preliminary patch for this see: https://github.com/ideak/linux/commit/5be8b8b4 > 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 :) See above, but perhaps for debugging purposes we could add a char *name, and make the state of power wells available through debugfs. > > + > > +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 :) Imo, this is safe now, since we only have a single power well. But the above preliminary patch has something similar to what you suggest, so if that's ok I'd address it there. > With or without my 3 bikesheds (although I would really like to see them): > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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. See the above patch. But basically yes, I would find the power well(s) based on the requested power domain and call __intel_power_well_{get,put} for each, and these would in turn call a platform dependent .set hook if needed. Thanks for the review, Imre
This patchset replaces "[PATCH 6/6] drm/i915: use power get/put instead of set for power on after init" from my previous patchset. I addressed the feedback from Daniel and Paulo (patch 1-3) and included a related cleanup (patch 4). Imre Deak (4): drm/i915: prepare for multiple power wells drm/i915: use power get/put instead of set for power on after init drm/i915: remove device field from struct power_well drm/i915: rename i915_init_power_well to i915_init_power_domains drivers/gpu/drm/i915/i915_dma.c | 10 ++-- drivers/gpu/drm/i915/i915_drv.c | 4 +- drivers/gpu/drm/i915/i915_drv.h | 20 +++++-- drivers/gpu/drm/i915/intel_display.c | 17 ++++++ drivers/gpu/drm/i915/intel_drv.h | 7 ++- drivers/gpu/drm/i915/intel_pm.c | 109 ++++++++++++++++------------------- 6 files changed, 92 insertions(+), 75 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; - struct mutex lock; /* power well enable/disable usage count */ int count; int i915_request; }; +#define I915_MAX_POWER_WELLS 1 + +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; 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); } /*
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@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 11 ++++++--- drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++---------------- 2 files changed, 42 insertions(+), 24 deletions(-)