2013/10/25 Imre Deak <imre.deak@xxxxxxxxx>:
> Currently we make sure that all power domains are enabled during driver
> init and turn off unneded ones only after the first modeset. Similarly
> during suspend we enable all power domains, which will remain on through
> the following resume until the first modeset.
>
> This logic is supported by intel_set_power_well() in the power domain
> framework. It would be nice to simplify the API, so that we only have
> get/put functions and make it more explicit on the higher level how this
> "power well on during init" logic works. This will make it also easier
> if in the future we want to shorten the time the power wells are on.
>
> For this add a new device private flag tracking whether we have the
> power wells on because of init/suspend and use only
> intel_display_power_get()/put(). As nothing else uses
> intel_set_power_well() we can remove it.
>
> This also fixes
>
> commit 6efdf354ddb186c6604d1692075421e8d2c740e9
> Author: Imre Deak <imre.deak@xxxxxxxxx>
> Date: Wed Oct 16 17:25:52 2013 +0300
>
> drm/i915: enable only the needed power domains during modeset
>
> where removing intel_set_power_well() resulted in not releasing the
> reference on the power well that was taken during init and thus leaving
> the power well on all the time. Regression reported by Paulo.
>
> v2:
> - move the init_power_on flag to the power_domains struct (Daniel)
>
> v3:
> - add note about this being a regression fix too (Paulo)
I didn't closely follow the first 5 patches, but this one makes sense.
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 8 +++++++-
> drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 37 +-----------------------------------
> 6 files changed, 28 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index fd848ef..b722b35 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1710,7 +1710,7 @@ int i915_driver_unload(struct drm_device *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_set_power_well(dev, true);
> + intel_display_set_init_power(dev, true);
> i915_remove_power_well(dev);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 85ae0dc..770c9f8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -477,7 +477,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> /* We do a lot of poking in a lot of registers, make sure they work
> * properly. */
> hsw_disable_package_c8(dev_priv);
> - intel_set_power_well(dev, true);
> + intel_display_set_init_power(dev, true);
>
> drm_kms_helper_poll_disable(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7315095..3565db2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -100,6 +100,7 @@ enum intel_display_power_domain {
> POWER_DOMAIN_TRANSCODER_C,
> POWER_DOMAIN_TRANSCODER_EDP,
> POWER_DOMAIN_VGA,
> + POWER_DOMAIN_INIT,
>
> POWER_DOMAIN_NUM,
> };
> @@ -913,12 +914,17 @@ struct i915_power_well {
> struct drm_device *device;
> /* power well enable/disable usage count */
> int count;
> - int i915_request;
> };
>
> #define I915_MAX_POWER_WELLS 1
>
> struct i915_power_domains {
> + /*
> + * Power wells needed for initialization at driver init and suspend
> + * time are on. They are kept on until after the first modeset.
> + */
> + bool init_power_on;
> +
> struct mutex lock;
> struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> };
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e79a2a..0c2e83c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6573,6 +6573,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev,
> return mask;
> }
>
> +void intel_display_set_init_power(struct drm_device *dev, bool enable)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (dev_priv->power_domains.init_power_on == enable)
> + return;
> +
> + if (enable)
> + intel_display_power_get(dev, POWER_DOMAIN_INIT);
> + else
> + intel_display_power_put(dev, POWER_DOMAIN_INIT);
> +
> + dev_priv->power_domains.init_power_on = enable;
> +}
> +
> static void modeset_update_power_wells(struct drm_device *dev)
> {
> unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> @@ -6604,6 +6619,8 @@ static void modeset_update_power_wells(struct drm_device *dev)
>
> crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> }
> +
> + intel_display_set_init_power(dev, false);
> }
>
> static void haswell_modeset_global_resources(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index af1553c..bf4394a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -692,6 +692,7 @@ bool intel_crtc_active(struct drm_crtc *crtc);
> void i915_disable_vga_mem(struct drm_device *dev);
> void hsw_enable_ips(struct intel_crtc *crtc);
> void hsw_disable_ips(struct intel_crtc *crtc);
> +void intel_display_set_init_power(struct drm_device *dev, bool enable);
>
>
> /* intel_dp.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 09ca6f9..4c38e28 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5706,41 +5706,6 @@ void i915_remove_power_well(struct drm_device *dev)
> hsw_pwr = NULL;
> }
>
> -void intel_set_power_well(struct drm_device *dev, bool enable)
> -{
> - 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;
> -
> - if (!HAS_POWER_WELL(dev))
> - return;
> -
> - if (!i915_disable_power_well && !enable)
> - return;
> -
> - 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
> - * is what tracks whether we have or have not
> - * added the one to the reference count.
> - */
> - if (power_well->i915_request == enable)
> - goto out;
> -
> - power_well->i915_request = enable;
> -
> - if (enable)
> - __intel_power_well_get(power_well);
> - else
> - __intel_power_well_put(power_well);
> -
> - out:
> - mutex_unlock(&power_domains->lock);
> -}
> -
> static void intel_resume_power_well(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5772,7 +5737,7 @@ void intel_init_power_well(struct drm_device *dev)
> return;
>
> /* For now, we need the power well to be always enabled. */
> - intel_set_power_well(dev, true);
> + intel_display_set_init_power(dev, true);
> intel_resume_power_well(dev);
>
> /* We're taking over the BIOS, so clear any requests made by it since
> --
> 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
