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

[Ville Syrjälä]

On Thu, Nov 10, 2022 at 09:55:55PM +0200, Imre Deak wrote:
> On Thu, Nov 10, 2022 at 09:11:20PM +0200, Ville Syrjälä wrote:
> > 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 at intel.com>
> > > Signed-off-by: Imre Deak <imre.deak at 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?
> 
> Yes, along with the alloc failure which is also logged. I assumed this
> is enough to explain why wakeref tracking doesn't work afterwards, but I
> suppose the wakeref could be untracked here in this case.

I think a more clear message what is going on would be good.
And probably preventing the spam from the wakerefs would
also be good to make sure the whole thing doesn't get
misdiagnosed as a real power ref leak.

-- 
Ville Syrjälä
Intel