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

Submitted by Imre Deak on May 9, 2019, 6:19 a.m.

Details

Message ID 20190509061954.10379-3-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Add support for asynchronous display power disabling" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak May 9, 2019, 6:19 a.m.
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(-)

Patch hide | download patch | download mbox

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);
 }
 
 void intel_runtime_pm_init_early(struct drm_i915_private *i915)

Comments

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