| Message ID | 1384434656-3248-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/11/14 Imre Deak <imre.deak@intel.com>: > So far we distinguished platforms without a dynamic power well with > the HAS_POWER_WELL macro and for such platforms we didn't call any power > domain functions. Instead of doing this check we can add an always-on > power well for these platforms and call the power domain functions > unconditionally. For always-on power wells we only increase/decrease > their refcounts, otherwise they are nop. > > This makes high level driver code more readable and as a bonus provides > some idea of the current power domains state for all platforms (once > the relevant debugfs entry is added). I took a similar approach with the PC8 code - platforms that don't support PC8 would never reach zero refcount - but Chris didn't like it and then sent a patch that added lots of "if (!HAS_PC8(dev))" checks everywhere. I personally don't have any strong opinions: I'm fine either with or without this patch. If it helps us catch unbalanced refcounts somehow, then I'm all in favor of it :) > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 18 +++++++----------- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++------------------- > 3 files changed, 16 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 00d74f8..95dd3db 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1639,8 +1639,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > goto out_gem_unload; > } > > - if (HAS_POWER_WELL(dev)) > - intel_power_domains_init(dev); > + intel_power_domains_init(dev); > > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > ret = i915_load_modeset_init(dev); > @@ -1667,8 +1666,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > return 0; > > out_power_well: > - if (HAS_POWER_WELL(dev)) > - intel_power_domains_remove(dev); > + intel_power_domains_remove(dev); > drm_vblank_cleanup(dev); > out_gem_unload: > if (dev_priv->mm.inactive_shrinker.scan_objects) > @@ -1706,13 +1704,11 @@ int i915_driver_unload(struct drm_device *dev) > > intel_gpu_ips_teardown(); > > - if (HAS_POWER_WELL(dev)) { > - /* The i915.ko module is still not prepared to be loaded when > - * the power well is not enabled, so just enable it in case > - * we're going to unload/reload. */ > - intel_display_set_init_power(dev, true); > - intel_power_domains_remove(dev); > - } > + /* The i915.ko module is still not prepared to be loaded when > + * the power well is not enabled, so just enable it in case > + * we're going to unload/reload. */ > + intel_display_set_init_power(dev, true); > + intel_power_domains_remove(dev); > > i915_teardown_sysfs(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ff3314d..06f47bf 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1834,7 +1834,6 @@ struct drm_i915_file_private { > #define HAS_IPS(dev) (IS_ULT(dev) || IS_BROADWELL(dev)) > > #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi) > -#define HAS_POWER_WELL(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) > #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) > #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index d5eacd8..d252453 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5642,9 +5642,6 @@ bool intel_display_power_enabled(struct drm_device *dev, > bool is_enabled; > int i; > > - if (!HAS_POWER_WELL(dev)) > - return true; > - > power_domains = &dev_priv->power_domains; > > is_enabled = true; > @@ -5752,9 +5749,6 @@ void intel_display_power_get(struct drm_device *dev, > struct i915_power_well *power_well; > int i; > > - if (!HAS_POWER_WELL(dev)) > - return; > - > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > @@ -5771,9 +5765,6 @@ void intel_display_power_put(struct drm_device *dev, > struct i915_power_well *power_well; > int i; > > - if (!HAS_POWER_WELL(dev)) > - return; > - > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > @@ -5812,6 +5803,14 @@ void i915_release_power_well(void) > } > EXPORT_SYMBOL_GPL(i915_release_power_well); > > +static struct i915_power_well intel_power_wells[] = { I would rename this to something like "fake_power_wells", "legacy_power_wells", "old_power_wells", "gen2_power_wells" or something else. With or without the rename: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > + { > + .name = "always-on", > + .always_on = 1, > + .domains = POWER_DOMAIN_MASK, > + }, > +}; > + > static struct i915_power_well hsw_power_wells[] = { > { > .name = "always-on", > @@ -5850,9 +5849,6 @@ 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; > > - if (!HAS_POWER_WELL(dev)) > - return 0; > - > mutex_init(&power_domains->lock); > > /* > @@ -5866,7 +5862,7 @@ int intel_power_domains_init(struct drm_device *dev) > set_power_wells(power_domains, bdw_power_wells); > hsw_pwr = power_domains; > } else { > - WARN_ON(1); > + set_power_wells(power_domains, intel_power_wells); > } > > return 0; > @@ -5884,9 +5880,6 @@ static void intel_power_domains_resume(struct drm_device *dev) > struct i915_power_well *power_well; > int i; > > - if (!HAS_POWER_WELL(dev)) > - return; > - > mutex_lock(&power_domains->lock); > for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) { > if (power_well->set) > @@ -5905,9 +5898,6 @@ void intel_power_domains_init_hw(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - if (!HAS_POWER_WELL(dev)) > - return; > - > /* For now, we need the power well to be always enabled. */ > intel_display_set_init_power(dev, true); > intel_power_domains_resume(dev); > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 00d74f8..95dd3db 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1639,8 +1639,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) goto out_gem_unload; } - if (HAS_POWER_WELL(dev)) - intel_power_domains_init(dev); + intel_power_domains_init(dev); if (drm_core_check_feature(dev, DRIVER_MODESET)) { ret = i915_load_modeset_init(dev); @@ -1667,8 +1666,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) return 0; out_power_well: - if (HAS_POWER_WELL(dev)) - intel_power_domains_remove(dev); + intel_power_domains_remove(dev); drm_vblank_cleanup(dev); out_gem_unload: if (dev_priv->mm.inactive_shrinker.scan_objects) @@ -1706,13 +1704,11 @@ int i915_driver_unload(struct drm_device *dev) intel_gpu_ips_teardown(); - if (HAS_POWER_WELL(dev)) { - /* The i915.ko module is still not prepared to be loaded when - * the power well is not enabled, so just enable it in case - * we're going to unload/reload. */ - intel_display_set_init_power(dev, true); - intel_power_domains_remove(dev); - } + /* The i915.ko module is still not prepared to be loaded when + * the power well is not enabled, so just enable it in case + * we're going to unload/reload. */ + intel_display_set_init_power(dev, true); + intel_power_domains_remove(dev); i915_teardown_sysfs(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ff3314d..06f47bf 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1834,7 +1834,6 @@ struct drm_i915_file_private { #define HAS_IPS(dev) (IS_ULT(dev) || IS_BROADWELL(dev)) #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi) -#define HAS_POWER_WELL(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev)) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d5eacd8..d252453 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5642,9 +5642,6 @@ bool intel_display_power_enabled(struct drm_device *dev, bool is_enabled; int i; - if (!HAS_POWER_WELL(dev)) - return true; - power_domains = &dev_priv->power_domains; is_enabled = true; @@ -5752,9 +5749,6 @@ void intel_display_power_get(struct drm_device *dev, struct i915_power_well *power_well; int i; - if (!HAS_POWER_WELL(dev)) - return; - power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); @@ -5771,9 +5765,6 @@ void intel_display_power_put(struct drm_device *dev, struct i915_power_well *power_well; int i; - if (!HAS_POWER_WELL(dev)) - return; - power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); @@ -5812,6 +5803,14 @@ void i915_release_power_well(void) } EXPORT_SYMBOL_GPL(i915_release_power_well); +static struct i915_power_well intel_power_wells[] = { + { + .name = "always-on", + .always_on = 1, + .domains = POWER_DOMAIN_MASK, + }, +}; + static struct i915_power_well hsw_power_wells[] = { { .name = "always-on", @@ -5850,9 +5849,6 @@ 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; - if (!HAS_POWER_WELL(dev)) - return 0; - mutex_init(&power_domains->lock); /* @@ -5866,7 +5862,7 @@ int intel_power_domains_init(struct drm_device *dev) set_power_wells(power_domains, bdw_power_wells); hsw_pwr = power_domains; } else { - WARN_ON(1); + set_power_wells(power_domains, intel_power_wells); } return 0; @@ -5884,9 +5880,6 @@ static void intel_power_domains_resume(struct drm_device *dev) struct i915_power_well *power_well; int i; - if (!HAS_POWER_WELL(dev)) - return; - mutex_lock(&power_domains->lock); for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) { if (power_well->set) @@ -5905,9 +5898,6 @@ void intel_power_domains_init_hw(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - if (!HAS_POWER_WELL(dev)) - return; - /* For now, we need the power well to be always enabled. */ intel_display_set_init_power(dev, true); intel_power_domains_resume(dev);
So far we distinguished platforms without a dynamic power well with the HAS_POWER_WELL macro and for such platforms we didn't call any power domain functions. Instead of doing this check we can add an always-on power well for these platforms and call the power domain functions unconditionally. For always-on power wells we only increase/decrease their refcounts, otherwise they are nop. This makes high level driver code more readable and as a bonus provides some idea of the current power domains state for all platforms (once the relevant debugfs entry is added). Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 18 +++++++----------- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++------------------- 3 files changed, 16 insertions(+), 31 deletions(-)