Re: [Intel-gfx] [PATCH v3 1/9] drm/i915: Allocate power domain set wakerefs dynamically

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

On Tue, Nov 08, 2022 at 05:18:23PM +0200, Imre Deak wrote:
> Since the intel_display_power_domain_set struct, currently its current
> size close to 1kB, can be allocated on the stack, it's better to
> allocate the per-domain wakeref pointer array - used for debugging -
> within the struct dynamically, so do this.
> 
> The memory freeing is guaranteed by the fact that the acquired domain
> references tracked by the struct can't be leaked either.
> 
> v2:
> - Don't use fetch_and_zero() when freeing the wakerefs array. (Jani)
> - Simplify intel_display_power_get/put_in_set(). (Jani)
> - Check in intel_crtc_destroy() that the wakerefs array has been freed.
> v3:
> - Add intel_display_power_set_disabled() and a separate assert
>   function instead of open coding these. (Jani)
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c     |  11 ++
>  .../drm/i915/display/intel_display_power.c    | 109 ++++++++++++++----
>  .../drm/i915/display/intel_display_power.h    |   6 +-
>  3 files changed, 104 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 037fc140b585c..c18d98bfe1a7c 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -21,6 +21,7 @@
>  #include "intel_crtc.h"
>  #include "intel_cursor.h"
>  #include "intel_display_debugfs.h"
> +#include "intel_display_power.h"
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
>  #include "intel_drrs.h"
> @@ -37,6 +38,14 @@ static void assert_vblank_disabled(struct drm_crtc *crtc)
>  		drm_crtc_vblank_put(crtc);
>  }
>  
> +static void assert_power_domains_disabled(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +
> +	drm_WARN_ON(&i915->drm,
> +		    !intel_display_power_set_disabled(i915, &crtc->enabled_power_domains));
> +}
> +
>  struct intel_crtc *intel_first_crtc(struct drm_i915_private *i915)
>  {
>  	return to_intel_crtc(drm_crtc_from_index(&i915->drm, 0));
> @@ -204,6 +213,8 @@ static void intel_crtc_destroy(struct drm_crtc *_crtc)
>  
>  	cpu_latency_qos_remove_request(&crtc->vblank_pm_qos);
>  
> +	assert_power_domains_disabled(crtc);
> +
>  	drm_crtc_cleanup(&crtc->base);
>  	kfree(crtc);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 4c1de91e56ff9..ca63b4f1af41b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -830,20 +830,85 @@ void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> +static void
> +add_domain_to_set(struct drm_i915_private *i915,
> +		  struct intel_display_power_domain_set *power_domain_set,
> +		  enum intel_display_power_domain domain,
> +		  intel_wakeref_t wf)
> +{
> +	drm_WARN_ON(&i915->drm, test_bit(domain, power_domain_set->mask.bits));
> +
> +	if (!power_domain_set->wakerefs)
> +		power_domain_set->wakerefs = kcalloc(POWER_DOMAIN_NUM,
> +						     sizeof(*power_domain_set->wakerefs),
> +						     GFP_KERNEL);
> +
> +	if (power_domain_set->wakerefs)
> +		power_domain_set->wakerefs[domain] = wf;

So if the kcalloc() fails is it going to look like
we're leaking power wakerefs?

> +
> +	set_bit(domain, power_domain_set->mask.bits);
> +}
> +
> +static intel_wakeref_t
> +remove_domain_from_set(struct drm_i915_private *i915,
> +		       struct intel_display_power_domain_set *power_domain_set,
> +		       enum intel_display_power_domain domain)
> +{
> +	intel_wakeref_t wf;
> +
> +	drm_WARN_ON(&i915->drm, !test_bit(domain, power_domain_set->mask.bits));
> +
> +	clear_bit(domain, power_domain_set->mask.bits);
> +
> +	if (!power_domain_set->wakerefs)
> +		return -1;
> +
> +	wf = fetch_and_zero(&power_domain_set->wakerefs[domain]);
> +
> +	if (bitmap_empty(power_domain_set->mask.bits, POWER_DOMAIN_NUM)) {
> +		kfree(power_domain_set->wakerefs);
> +		power_domain_set->wakerefs = NULL;
> +	}
> +
> +	return wf;
> +
> +}
> +#else
> +static void
> +add_domain_to_set(struct drm_i915_private *i915,
> +		  struct intel_display_power_domain_set *power_domain_set,
> +		  enum intel_display_power_domain domain,
> +		  intel_wakeref_t wf)
> +{
> +	drm_WARN_ON(&i915->drm, test_bit(domain, power_domain_set->mask.bits));
> +
> +	set_bit(domain, power_domain_set->mask.bits);
> +}
> +
> +static intel_wakeref_t
> +remove_domain_from_set(struct drm_i915_private *i915,
> +		       struct intel_display_power_domain_set *power_domain_set,
> +		       enum intel_display_power_domain domain)
> +{
> +	drm_WARN_ON(&i915->drm, !test_bit(domain, power_domain_set->mask.bits));
> +
> +	clear_bit(domain, power_domain_set->mask.bits);
> +
> +	return -1;
> +}
> +#endif
> +
>  void
>  intel_display_power_get_in_set(struct drm_i915_private *i915,
>  			       struct intel_display_power_domain_set *power_domain_set,
>  			       enum intel_display_power_domain domain)
>  {
> -	intel_wakeref_t __maybe_unused wf;
> -
> -	drm_WARN_ON(&i915->drm, test_bit(domain, power_domain_set->mask.bits));
> +	intel_wakeref_t wf;
>  
>  	wf = intel_display_power_get(i915, domain);
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> -	power_domain_set->wakerefs[domain] = wf;
> -#endif
> -	set_bit(domain, power_domain_set->mask.bits);
> +
> +	add_domain_to_set(i915, power_domain_set, domain, wf);
>  }
>  
>  bool
> @@ -853,16 +918,11 @@ intel_display_power_get_in_set_if_enabled(struct drm_i915_private *i915,
>  {
>  	intel_wakeref_t wf;
>  
> -	drm_WARN_ON(&i915->drm, test_bit(domain, power_domain_set->mask.bits));
> -
>  	wf = intel_display_power_get_if_enabled(i915, domain);
>  	if (!wf)
>  		return false;
>  
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> -	power_domain_set->wakerefs[domain] = wf;
> -#endif
> -	set_bit(domain, power_domain_set->mask.bits);
> +	add_domain_to_set(i915, power_domain_set, domain, wf);
>  
>  	return true;
>  }
> @@ -874,20 +934,27 @@ intel_display_power_put_mask_in_set(struct drm_i915_private *i915,
>  {
>  	enum intel_display_power_domain domain;
>  
> -	drm_WARN_ON(&i915->drm,
> -		    !bitmap_subset(mask->bits, power_domain_set->mask.bits, POWER_DOMAIN_NUM));
> -
>  	for_each_power_domain(domain, mask) {
> -		intel_wakeref_t __maybe_unused wf = -1;
> +		intel_wakeref_t wf = remove_domain_from_set(i915, power_domain_set, domain);
>  
> -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> -		wf = fetch_and_zero(&power_domain_set->wakerefs[domain]);
> -#endif
>  		intel_display_power_put(i915, domain, wf);
> -		clear_bit(domain, power_domain_set->mask.bits);
>  	}
>  }
>  
> +bool
> +intel_display_power_set_disabled(struct drm_i915_private *i915,
> +				 struct intel_display_power_domain_set *power_domain_set)
> +{
> +	if (!bitmap_empty(power_domain_set->mask.bits, POWER_DOMAIN_NUM))
> +		return false;
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> +	drm_WARN_ON(&i915->drm, power_domain_set->wakerefs);
> +#endif
> +
> +	return true;
> +}
> +
>  static int
>  sanitize_disable_power_well_option(const struct drm_i915_private *dev_priv,
>  				   int disable_power_well)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 1e77e52c87fec..31b0e9ae863c3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -147,7 +147,7 @@ struct i915_power_domains {
>  struct intel_display_power_domain_set {
>  	struct intel_power_domain_mask mask;
>  #ifdef CONFIG_DRM_I915_DEBUG_RUNTIME_PM
> -	intel_wakeref_t wakerefs[POWER_DOMAIN_NUM];
> +	intel_wakeref_t *wakerefs;
>  #endif
>  };
>  
> @@ -243,6 +243,10 @@ intel_display_power_put_all_in_set(struct drm_i915_private *i915,
>  	intel_display_power_put_mask_in_set(i915, power_domain_set, &power_domain_set->mask);
>  }
>  
> +bool
> +intel_display_power_set_disabled(struct drm_i915_private *i915,
> +				 struct intel_display_power_domain_set *power_domain_set);
> +
>  void intel_display_power_debug(struct drm_i915_private *i915, struct seq_file *m);
>  
>  enum intel_display_power_domain
> -- 
> 2.37.1

-- 
Ville Syrjälä
Intel