| Message ID | 1382711810-25881-4-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/25 Imre Deak <imre.deak@intel.com>: > The only real need for this field was in > i915_{request,release}_power_well, but there we can get at it by a > container_of magic. Also since in the future we'll have multiple power > wells each with its own power_well struct it makes sense to remove the > field from there where it'd be just redundancy. > > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com> My original idea was to just move it from i915_power_well to i915_power_domains, so hsw_pwr (which is the new external static thing) would still have a pointer to our driver. This way we wouldn't need the container_of magic. But your solution works too, and saves 4/8 bytes :) Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++--------- > 2 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3565db2..2731fbb 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -911,7 +911,6 @@ struct intel_ilk_power_mgmt { > > /* Power well structure for haswell */ > struct i915_power_well { > - struct drm_device *device; > /* power well enable/disable usage count */ > int count; > }; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 4c38e28..4f84a4b 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5608,17 +5608,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable) > } > } > > -static void __intel_power_well_get(struct i915_power_well *power_well) > +static void __intel_power_well_get(struct drm_device *dev, > + struct i915_power_well *power_well) > { > if (!power_well->count++) > - __intel_set_power_well(power_well->device, true); > + __intel_set_power_well(dev, true); > } > > -static void __intel_power_well_put(struct i915_power_well *power_well) > +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) > - __intel_set_power_well(power_well->device, false); > + __intel_set_power_well(dev, false); > } > > void intel_display_power_get(struct drm_device *dev, > @@ -5636,7 +5638,7 @@ void intel_display_power_get(struct drm_device *dev, > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > - __intel_power_well_get(&power_domains->power_wells[0]); > + __intel_power_well_get(dev, &power_domains->power_wells[0]); > mutex_unlock(&power_domains->lock); > } > > @@ -5655,7 +5657,7 @@ void intel_display_power_put(struct drm_device *dev, > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > - __intel_power_well_put(&power_domains->power_wells[0]); > + __intel_power_well_put(dev, &power_domains->power_wells[0]); > mutex_unlock(&power_domains->lock); > } > > @@ -5664,11 +5666,16 @@ static struct i915_power_domains *hsw_pwr; > /* Display audio driver power well request */ > void i915_request_power_well(void) > { > + struct drm_i915_private *dev_priv; > + > if (WARN_ON(!hsw_pwr)) > return; > > + dev_priv = container_of(hsw_pwr, struct drm_i915_private, > + power_domains); > + > mutex_lock(&hsw_pwr->lock); > - __intel_power_well_get(&hsw_pwr->power_wells[0]); > + __intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]); > mutex_unlock(&hsw_pwr->lock); > } > EXPORT_SYMBOL_GPL(i915_request_power_well); > @@ -5676,11 +5683,16 @@ EXPORT_SYMBOL_GPL(i915_request_power_well); > /* Display audio driver power well release */ > void i915_release_power_well(void) > { > + struct drm_i915_private *dev_priv; > + > if (WARN_ON(!hsw_pwr)) > return; > > + dev_priv = container_of(hsw_pwr, struct drm_i915_private, > + power_domains); > + > mutex_lock(&hsw_pwr->lock); > - __intel_power_well_put(&hsw_pwr->power_wells[0]); > + __intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]); > mutex_unlock(&hsw_pwr->lock); > } > EXPORT_SYMBOL_GPL(i915_release_power_well); > @@ -5695,7 +5707,6 @@ int i915_init_power_well(struct drm_device *dev) > hsw_pwr = power_domains; > > power_well = &power_domains->power_wells[0]; > - power_well->device = dev; > power_well->count = 0; > > return 0; > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 25, 2013 at 05:50:18PM -0200, Paulo Zanoni wrote: > 2013/10/25 Imre Deak <imre.deak@intel.com>: > > The only real need for this field was in > > i915_{request,release}_power_well, but there we can get at it by a > > container_of magic. Also since in the future we'll have multiple power > > wells each with its own power_well struct it makes sense to remove the > > field from there where it'd be just redundancy. > > > > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com> > > My original idea was to just move it from i915_power_well to > i915_power_domains, so hsw_pwr (which is the new external static > thing) would still have a pointer to our driver. This way we wouldn't > need the container_of magic. But your solution works too, and saves > 4/8 bytes :) > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> First 3 patches merged, thanks. -Daniel
2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Oct 25, 2013 at 05:50:18PM -0200, Paulo Zanoni wrote: >> 2013/10/25 Imre Deak <imre.deak@intel.com>: >> > The only real need for this field was in >> > i915_{request,release}_power_well, but there we can get at it by a >> > container_of magic. Also since in the future we'll have multiple power >> > wells each with its own power_well struct it makes sense to remove the >> > field from there where it'd be just redundancy. >> > >> > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com> >> >> My original idea was to just move it from i915_power_well to >> i915_power_domains, so hsw_pwr (which is the new external static >> thing) would still have a pointer to our driver. This way we wouldn't >> need the container_of magic. But your solution works too, and saves >> 4/8 bytes :) >> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > First 3 patches merged, thanks. I just realized that at some point we accidentally killed the i915.disable_power_well option... I see the i915_disable_power_well variable is not being used anywhere. Imre, can you please investigate that? Thanks, Paulo > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, 2013-10-28 at 15:41 -0200, Paulo Zanoni wrote: > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > > On Fri, Oct 25, 2013 at 05:50:18PM -0200, Paulo Zanoni wrote: > >> 2013/10/25 Imre Deak <imre.deak@intel.com>: > >> > The only real need for this field was in > >> > i915_{request,release}_power_well, but there we can get at it by a > >> > container_of magic. Also since in the future we'll have multiple power > >> > wells each with its own power_well struct it makes sense to remove the > >> > field from there where it'd be just redundancy. > >> > > >> > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com> > >> > >> My original idea was to just move it from i915_power_well to > >> i915_power_domains, so hsw_pwr (which is the new external static > >> thing) would still have a pointer to our driver. This way we wouldn't > >> need the container_of magic. But your solution works too, and saves > >> 4/8 bytes :) > >> > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > First 3 patches merged, thanks. > > I just realized that at some point we accidentally killed the > i915.disable_power_well option... I see the i915_disable_power_well > variable is not being used anywhere. Imre, can you please investigate > that? Yep, that got removed by mistake in commit 6efdf354ddb186c6604d1692075421e8d2c740e9 Author: Imre Deak <imre.deak@intel.com> Date: Wed Oct 16 17:25:52 2013 +0300 drm/i915: enable only the needed power domains during modeset I'll follow up with a fix. --Imre
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3565db2..2731fbb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -911,7 +911,6 @@ struct intel_ilk_power_mgmt { /* Power well structure for haswell */ struct i915_power_well { - struct drm_device *device; /* power well enable/disable usage count */ int count; }; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4c38e28..4f84a4b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5608,17 +5608,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable) } } -static void __intel_power_well_get(struct i915_power_well *power_well) +static void __intel_power_well_get(struct drm_device *dev, + struct i915_power_well *power_well) { if (!power_well->count++) - __intel_set_power_well(power_well->device, true); + __intel_set_power_well(dev, true); } -static void __intel_power_well_put(struct i915_power_well *power_well) +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) - __intel_set_power_well(power_well->device, false); + __intel_set_power_well(dev, false); } void intel_display_power_get(struct drm_device *dev, @@ -5636,7 +5638,7 @@ void intel_display_power_get(struct drm_device *dev, power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); - __intel_power_well_get(&power_domains->power_wells[0]); + __intel_power_well_get(dev, &power_domains->power_wells[0]); mutex_unlock(&power_domains->lock); } @@ -5655,7 +5657,7 @@ void intel_display_power_put(struct drm_device *dev, power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); - __intel_power_well_put(&power_domains->power_wells[0]); + __intel_power_well_put(dev, &power_domains->power_wells[0]); mutex_unlock(&power_domains->lock); } @@ -5664,11 +5666,16 @@ static struct i915_power_domains *hsw_pwr; /* Display audio driver power well request */ void i915_request_power_well(void) { + struct drm_i915_private *dev_priv; + if (WARN_ON(!hsw_pwr)) return; + dev_priv = container_of(hsw_pwr, struct drm_i915_private, + power_domains); + mutex_lock(&hsw_pwr->lock); - __intel_power_well_get(&hsw_pwr->power_wells[0]); + __intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]); mutex_unlock(&hsw_pwr->lock); } EXPORT_SYMBOL_GPL(i915_request_power_well); @@ -5676,11 +5683,16 @@ EXPORT_SYMBOL_GPL(i915_request_power_well); /* Display audio driver power well release */ void i915_release_power_well(void) { + struct drm_i915_private *dev_priv; + if (WARN_ON(!hsw_pwr)) return; + dev_priv = container_of(hsw_pwr, struct drm_i915_private, + power_domains); + mutex_lock(&hsw_pwr->lock); - __intel_power_well_put(&hsw_pwr->power_wells[0]); + __intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]); mutex_unlock(&hsw_pwr->lock); } EXPORT_SYMBOL_GPL(i915_release_power_well); @@ -5695,7 +5707,6 @@ int i915_init_power_well(struct drm_device *dev) hsw_pwr = power_domains; power_well = &power_domains->power_wells[0]; - power_well->device = dev; power_well->count = 0; return 0;
The only real need for this field was in i915_{request,release}_power_well, but there we can get at it by a container_of magic. Also since in the future we'll have multiple power wells each with its own power_well struct it makes sense to remove the field from there where it'd be just redundancy. Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-)