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 09:23:59PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 11, 2022 at 08:56:16PM +0200, Imre Deak wrote:
> > On Fri, Nov 11, 2022 at 08:30:41PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 11, 2022 at 05:47:05PM +0200, Imre Deak wrote:
> > > > 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.
> > > 
> > > I mean just allocate eg. during driver init.
> > 
> > Ok, it'd work for this case if it's per-crtc. We could add a
> > 'readout_hw_domains' power_domain_set struct to intel_crtc and simply
> > use that in hsw_get_pipe_config() instead of this patch, would that be
> > ok?
> 
> That seems like an easier fix for now at least. I think we could
> live with it. But if you can make the internal alloc handle the
> failures a bit more gracefully I think that would be ok too.
> I'll let you pick ;)

Ok. The simpler approach looks better, making it more generic would make
sense only with more users for it.

> > > > > 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
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel