Re: [PATCH 3/6] drm/i915: change power_well->lock to be mutex

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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

> There is no hard need for this to be a spin lock, as we don't take these
> locks in irq context from anywhere. An upcoming patch will add calls to
> punit read/write functions from within regions protected by this lock
> and those functions need a mutex in turn. As a solution for that convert
> the spin lock to be a mutex.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++-------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca05f3a..e4354dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt {
>  /* Power well structure for haswell */
>  struct i915_power_well {
>  	struct drm_device *device;
> -	spinlock_t lock;
> +	struct mutex lock;
>  	/* power well enable/disable usage count */
>  	int count;
>  	int i915_request;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 57d08a2..f7363a8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev,
>  	if (is_always_on_power_domain(dev, domain))
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  	__intel_power_well_get(power_well);
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  void intel_display_power_put(struct drm_device *dev,
> @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev,
>  	if (is_always_on_power_domain(dev, domain))
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  	__intel_power_well_put(power_well);
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  static struct i915_power_well *hsw_pwr;
> @@ -5506,9 +5506,9 @@ void i915_request_power_well(void)
>  	if (WARN_ON(!hsw_pwr))
>  		return;
>  
> -	spin_lock_irq(&hsw_pwr->lock);
> +	mutex_lock(&hsw_pwr->lock);
>  	__intel_power_well_get(hsw_pwr);
> -	spin_unlock_irq(&hsw_pwr->lock);
> +	mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
>  
> @@ -5518,9 +5518,9 @@ void i915_release_power_well(void)
>  	if (WARN_ON(!hsw_pwr))
>  		return;
>  
> -	spin_lock_irq(&hsw_pwr->lock);
> +	mutex_lock(&hsw_pwr->lock);
>  	__intel_power_well_put(hsw_pwr);
> -	spin_unlock_irq(&hsw_pwr->lock);
> +	mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>  
> @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev)
>  	hsw_pwr = &dev_priv->power_well;
>  
>  	hsw_pwr->device = dev;
> -	spin_lock_init(&hsw_pwr->lock);
> +	mutex_init(&hsw_pwr->lock);
>  	hsw_pwr->count = 0;
>  
>  	return 0;
> @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>  	if (!i915_disable_power_well && !enable)
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  
>  	/*
>  	 * This function will only ever contribute one
> @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>  		__intel_power_well_put(power_well);
>  
>   out:
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  static void intel_resume_power_well(struct drm_device *dev)
> @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev)
>  	if (!HAS_POWER_WELL(dev))
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  	__intel_set_power_well(dev, power_well->count > 0);
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  /*

Are there ordering requirements we should document?  E.g. always take
this after the mode config lock or something?

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

-- 
Jesse Barnes, Intel Open Source Technology Center