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

[Imre Deak <imre.deak@intel.com>]

On Fri, Nov 11, 2022 at 03:52:08PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 11, 2022 at 03:43:54PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 11, 2022 at 02:37:13PM +0200, Imre Deak wrote:
> > > On Thu, Nov 10, 2022 at 11:49:19PM +0200, Ville Syrjälä wrote:
> > > > 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@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?
> > > > > 
> > > > > 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.
> > > 
> > > Ok, I can add a debug print about the failure and untrack the wakeref.
> > 
> > The other idea that came to mind was to just preallocate this
> > somehow. Looks like the only place where this even matters
> > currently is hsw_get_pipe_config(). The other instance of
> > this structure is already embedded in a kmalloced thing
> > (struct intel_crtc).

Can't think of a good way to allocate the whole struct, as the domain
mask within it is still needed if the allocation failed. The API is
simpler imo if the allocation happens internally, but yes not a big
change if there is a good way to allocate only the array part.

> Hmm. I wonder if this wakeref tracking is even really useful
> in the readout path. We just do a put_all_in_set() in the end,
> so can it even leak?

If the function would incorrectly do an early return, the domains
wouldn't be put. I think it makes sense to keep wakerefs working for
this case as well.

> -- 
> Ville Syrjälä
> Intel