| Message ID | 20221107170917.3566758-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tgl+: Enable DC power states on all eDP ports | expand |
On Mon, 07 Nov 2022, Imre Deak <imre.deak@intel.com> wrote: > Since the intel_display_power_domain_set struct, currently its current > size close 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 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. > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_crtc.c | 4 + > .../drm/i915/display/intel_display_power.c | 95 +++++++++++++++---- > .../drm/i915/display/intel_display_power.h | 2 +- > 3 files changed, 79 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..2c8d564e73182 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -205,6 +205,10 @@ static void intel_crtc_destroy(struct drm_crtc *_crtc) > cpu_latency_qos_remove_request(&crtc->vblank_pm_qos); > > drm_crtc_cleanup(&crtc->base); > + > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) > + drm_WARN_ON(crtc->base.dev, crtc->enabled_power_domains.wakerefs); > +#endif Can we please not add this kind of asserts without abstractions? The #ifdef is ugly, looking at crtc->enabled_power_domains.wakerefs directly is ugly. Maybe add an assert_something_or_other(crtc) that does the right thing? Similar to the other assert_*() functions we have? Does it even need to depend on the config? If it's worth having, maybe worth having unconditionally? BR, Jani. > 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..db235b79c9629 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; > + > + 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,17 +934,10 @@ 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); > } > } > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h > index 1e77e52c87fec..662123d260a7a 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 > };
On Tue, Nov 08, 2022 at 10:54:21AM +0200, Jani Nikula wrote: > On Mon, 07 Nov 2022, Imre Deak <imre.deak@intel.com> wrote: > > Since the intel_display_power_domain_set struct, currently its current > > size close 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 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. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_crtc.c | 4 + > > .../drm/i915/display/intel_display_power.c | 95 +++++++++++++++---- > > .../drm/i915/display/intel_display_power.h | 2 +- > > 3 files changed, 79 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..2c8d564e73182 100644 > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > > @@ -205,6 +205,10 @@ static void intel_crtc_destroy(struct drm_crtc *_crtc) > > cpu_latency_qos_remove_request(&crtc->vblank_pm_qos); > > > > drm_crtc_cleanup(&crtc->base); > > + > > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) > > + drm_WARN_ON(crtc->base.dev, crtc->enabled_power_domains.wakerefs); > > +#endif > > Can we please not add this kind of asserts without abstractions? The > #ifdef is ugly, looking at crtc->enabled_power_domains.wakerefs directly > is ugly. > > Maybe add an assert_something_or_other(crtc) that does the right thing? > Similar to the other assert_*() functions we have? Yes, can add assert_power_domains_disabled(crtc) to this file. > Does it even need to depend on the config? If it's worth having, maybe > worth having unconditionally? The wakerefs array exists only when the config is enabled, but the non-debug crtc->enabled_power_domains.mask should be checked as well. > BR, > Jani. > > > 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..db235b79c9629 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; > > + > > + 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,17 +934,10 @@ 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); > > } > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h > > index 1e77e52c87fec..662123d260a7a 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 > > }; > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index 037fc140b585c..2c8d564e73182 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -205,6 +205,10 @@ static void intel_crtc_destroy(struct drm_crtc *_crtc) cpu_latency_qos_remove_request(&crtc->vblank_pm_qos); drm_crtc_cleanup(&crtc->base); + +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) + drm_WARN_ON(crtc->base.dev, crtc->enabled_power_domains.wakerefs); +#endif 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..db235b79c9629 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; + + 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,17 +934,10 @@ 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); } } diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h index 1e77e52c87fec..662123d260a7a 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 };
Since the intel_display_power_domain_set struct, currently its current size close 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 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. Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_crtc.c | 4 + .../drm/i915/display/intel_display_power.c | 95 +++++++++++++++---- .../drm/i915/display/intel_display_power.h | 2 +- 3 files changed, 79 insertions(+), 22 deletions(-)