Re: [PATCH 6/6] drm/i915: use power get/put instead of set for power on after init

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 16 Oct 2013 17:25:53 +0300
Imre Deak <imre.deak@intel.com> wrote:

> 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.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  7 ++++++-
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 35 +----------------------------------
>  6 files changed, 27 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b8bc887..dd7f1f6 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1708,7 +1708,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 59649c0..7299a96 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -469,7 +469,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 e4354dd..0557c6b 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,7 +914,6 @@ struct i915_power_well {
>  	struct mutex lock;
>  	/* power well enable/disable usage count */
>  	int count;
> -	int i915_request;
>  };
>  
>  struct i915_dri1_state {
> @@ -1369,6 +1369,11 @@ typedef struct drm_i915_private {
>  	 * mchdev_lock in intel_pm.c */
>  	struct intel_ilk_power_mgmt ips;
>  
> +	/*
> +	 * 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;
>  	/* Haswell power well */
>  	struct i915_power_well power_well;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e2a4f3b..e30db91 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6581,6 +6581,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->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->init_power_on = enable;
> +}
> +
>  static void modeset_update_power_wells(struct drm_device *dev)
>  {
>  	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> @@ -6612,6 +6627,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 63a5bfd..e6306f5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -680,6 +680,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 f7363a8..9291a2e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5542,39 +5542,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_well *power_well = &dev_priv->power_well;
> -
> -	if (!HAS_POWER_WELL(dev))
> -		return;
> -
> -	if (!i915_disable_power_well && !enable)
> -		return;
> -
> -	mutex_lock(&power_well->lock);
> -
> -	/*
> -	 * 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_well->lock);
> -}
> -
>  static void intel_resume_power_well(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5602,7 +5569,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

And there it goes, yay! :)  Now we just have to get rid of the _INIT
domain and actually shut things down before the first mode set if
nothing is in use (maybe with a delayed shutdown to avoid toggling too
fast).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center