[Intel-gfx] [PATCH v2 7/8] drm/i915: add a default always-on power well
Paulo Zanoni
przanoni at gmail.com
Fri Nov 22 17:24:47 CET 2013
2013/11/14 Imre Deak <imre.deak at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list