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@xxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > 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.
> > +
> > + 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
