Re: [PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 27, 2013 at 08:09:04PM +0200, Imre Deak wrote:
> Atm we call intel_display_power_enabled() from
> i915_capture_error_state() in IRQ context and then take a mutex. To fix
> this add a new intel_display_power_enabled_sw() which returns the domain
> state based on software tracking as opposed to reading the actual HW
> state.
> 
> Since we use domain_use_count for this without locking on the reader
> side make sure we increase the counter only after enabling all required
> power wells and decrease it before disabling any of these power wells.
> 
> Regression introduced in
> commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Tue Sep 24 16:17:09 2013 +0300
> 
>     drm/i915: support for multiple power wells
> 
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 --
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++++++++++++--------
>  4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 47b8fd1..d17a62a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -966,9 +966,7 @@ struct i915_power_domains {
>  	int power_well_count;
>  
>  	struct mutex lock;
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
>  	int domain_use_count[POWER_DOMAIN_NUM];
> -#endif
>  	struct i915_power_well *power_wells;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fab7d35..5b5d831 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11374,7 +11374,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>  
>  	for_each_pipe(i) {
> -		if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i)))
> +		if (!intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i)))
>  			continue;
>  
>  		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
> @@ -11410,7 +11410,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  	for (i = 0; i < error->num_transcoders; i++) {
>  		enum transcoder cpu_transcoder = transcoders[i];
>  
> -		if (!intel_display_power_enabled(dev,
> +		if (!intel_display_power_enabled_sw(dev,
>  				POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
>  			continue;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b2d2cc1..fb3cfc5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -830,6 +830,8 @@ int intel_power_domains_init(struct drm_device *dev);
>  void intel_power_domains_remove(struct drm_device *dev);
>  bool intel_display_power_enabled(struct drm_device *dev,
>  				 enum intel_display_power_domain domain);
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> +				    enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_device *dev,
>  			     enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a1a54ab..b9a900d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5633,6 +5633,21 @@ static bool hsw_power_well_enabled(struct drm_device *dev,
>  		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>  }
>  
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> +				    enum intel_display_power_domain domain)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_power_domains *power_domains;
> +	bool enabled;
> +
> +	power_domains = &dev_priv->power_domains;
> +
> +	enabled = power_domains->domain_use_count[domain];
> +	smp_rmb();

I'm not sure the barriers really provide extra value. Some other
thread could still come along just after this and disable the power
wells.

> +
> +	return enabled;
> +}
> +
>  bool intel_display_power_enabled(struct drm_device *dev,
>  				 enum intel_display_power_domain domain)
>  {
> @@ -5753,12 +5768,16 @@ void intel_display_power_get(struct drm_device *dev,
>  
>  	mutex_lock(&power_domains->lock);
>  
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> -	power_domains->domain_use_count[domain]++;
> -#endif
>  	for_each_power_well(i, power_well, BIT(domain), power_domains)
>  		__intel_power_well_get(dev, power_well);
>  
> +	/*
> +	 * Don't reorder the adjustment of domain_use_count with whatever
> +	 * __intel_power_well_get does.
> +	 */
> +	smp_wmb();
> +	power_domains->domain_use_count[domain]++;
> +
>  	mutex_unlock(&power_domains->lock);
>  }
>  
> @@ -5774,13 +5793,16 @@ void intel_display_power_put(struct drm_device *dev,
>  
>  	mutex_lock(&power_domains->lock);
>  
> -	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> -		__intel_power_well_put(dev, power_well);
> -
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
>  	WARN_ON(!power_domains->domain_use_count[domain]);
>  	power_domains->domain_use_count[domain]--;
> -#endif
> +	/*
> +	 * Don't reorder the adjustment of domain_use_count with whatever
> +	 * __intel_power_well_put does.
> +	 */
> +	smp_wmb();
> +
> +	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> +		__intel_power_well_put(dev, power_well);
>  
>  	mutex_unlock(&power_domains->lock);
>  }
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC