Re: [PATCH v2 02/11] drm/i915: Force printing wakeref tacking during pm_cleanup

Chris Wilson
2019-05-09T08:05:15Z

Quoting Imre Deak (2019-05-09 07:19:45)
> Make sure we print and drop the wakeref tracking info during pm_cleanup
> even if there are wakeref holders (either raw-wakeref or wakelock
> holders). Dropping the wakeref tracking means that a late put on the ref
> will WARN since the wakeref will be unknown, but that is rightly so,
> since the put is late and we want to catch that case.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 75 ++++++++++++++++++-------
> 1 file changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 84a18d8b942c..dc964c8608f1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -233,31 +233,60 @@ __print_intel_runtime_pm_wakeref(struct drm_printer *p,
> }
>
> static noinline void
> -__intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
> +__untrack_all_wakerefs(struct intel_runtime_pm_debug *debug,
> + struct intel_runtime_pm_debug *saved)
> +{
> + *saved = *debug;
> +
> + debug->owners = NULL;
> + debug->count = 0;
> + debug->last_release = __save_depot_stack();
> +}
> +
> +static void
> +dump_and_free_wakeref_tracking(struct intel_runtime_pm_debug *debug)
> {
> - struct i915_runtime_pm *rpm = &i915->runtime_pm;
> - struct intel_runtime_pm_debug dbg = {};
> struct drm_printer p;
> - unsigned long flags;
>
> - if (atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
> - &rpm->debug.lock,
> - flags)) {
> - dbg = rpm->debug;
> -
> - rpm->debug.owners = NULL;
> - rpm->debug.count = 0;
> - rpm->debug.last_release = __save_depot_stack();
> -
> - spin_unlock_irqrestore(&rpm->debug.lock, flags);
> - }
> - if (!dbg.count)
> + if (!debug->count)
> return;
>
> p = drm_debug_printer("i915");
> - __print_intel_runtime_pm_wakeref(&p, &dbg);
> + __print_intel_runtime_pm_wakeref(&p, debug);
>
> - kfree(dbg.owners);
> + kfree(debug->owners);
> +}
> +
> +static noinline void
> +__intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
> +{
> + struct i915_runtime_pm *rpm = &i915->runtime_pm;
> + struct intel_runtime_pm_debug dbg = {};
> + unsigned long flags;
> +
> + if (!atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
> + &rpm->debug.lock,
> + flags))
> + return;
> +
> + __untrack_all_wakerefs(&rpm->debug, &dbg);
> + spin_unlock_irqrestore(&rpm->debug.lock, flags);
> +
> + dump_and_free_wakeref_tracking(&dbg);
> +}
> +
> +static noinline void
> +untrack_all_intel_runtime_pm_wakerefs(struct drm_i915_private *i915)
> +{
> + struct i915_runtime_pm *rpm = &i915->runtime_pm;
> + struct intel_runtime_pm_debug dbg = {};
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rpm->debug.lock, flags);
> + __untrack_all_wakerefs(&rpm->debug, &dbg);
> + spin_unlock_irqrestore(&rpm->debug.lock, flags);
> +
> + dump_and_free_wakeref_tracking(&dbg);
> }
>
> void print_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
> @@ -321,6 +350,11 @@ __intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
> atomic_dec(&i915->runtime_pm.wakeref_count);
> }
>
> +static void
> +untrack_all_intel_runtime_pm_wakerefs(struct drm_i915_private *i915)
> +{
> +}
> +
> #endif
>
> static void
> @@ -4838,15 +4872,14 @@ void intel_runtime_pm_disable(struct drm_i915_private *i915)
> void intel_runtime_pm_cleanup(struct drm_i915_private *i915)
> {
> struct i915_runtime_pm *rpm = &i915->runtime_pm;
> - int count;
> + int count = atomic_read(&rpm->wakeref_count);
>
> - count = atomic_fetch_inc(&rpm->wakeref_count); /* balance untrack */
> WARN(count,
> "i915 raw-wakerefs=%d wakelocks=%d on cleanup\n",
> intel_rpm_raw_wakeref_count(count),
> intel_rpm_wakelock_count(count));
>
> - intel_runtime_pm_release(i915, false);
> + untrack_all_intel_runtime_pm_wakerefs(i915);
> }

That looks much better, thanks!

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris