Everytime we take a wakeref, record the stack trace of where it was
taken; clearing the set if we ever drop back to no owners. For debugging
a rpm leak, we can look at all the current wakerefs and check if they
have a matching rpm_put.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/Kconfig.debug      |  13 ++
 drivers/gpu/drm/i915/i915_drv.c         |   8 +-
 drivers/gpu/drm/i915/i915_drv.h         |   7 +
 drivers/gpu/drm/i915/intel_drv.h        |  32 ++--
 drivers/gpu/drm/i915/intel_runtime_pm.c | 217 +++++++++++++++++++++---
 5 files changed, 233 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug 
b/drivers/gpu/drm/i915/Kconfig.debug
index 459f8f88a34c..ed572a454e01 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -39,6 +39,19 @@ config DRM_I915_DEBUG
 
           If in doubt, say "N".
 
+config DRM_I915_DEBUG_RPM
+        bool "Insert extra checks into the runtime pm internals"
+        depends on DRM_I915
+        default n
+        select STACKDEPOT
+        help
+          Enable extra sanity checks (including BUGs) along the runtime pm
+          paths that may slow the system down and if hit hang the machine.
+
+          Recommended for driver developers only.
+
+          If in doubt, say "N".
+
 config DRM_I915_DEBUG_GEM
         bool "Insert extra checks into the GEM internals"
         default n
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 62ef105a241d..b6e83043ac9d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -908,6 +908,7 @@ static int i915_driver_init_early(struct drm_i915_private 
*dev_priv,
        mutex_init(&dev_priv->pps_mutex);
 
        i915_memcpy_init_early(dev_priv);
+       intel_runtime_pm_init_early(dev_priv);
 
        ret = i915_workqueues_init(dev_priv);
        if (ret < 0)
@@ -1329,6 +1330,8 @@ static void i915_welcome_messages(struct drm_i915_private 
*dev_priv)
                DRM_INFO("DRM_I915_DEBUG enabled\n");
        if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
                DRM_INFO("DRM_I915_DEBUG_GEM enabled\n");
+       if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_RPM))
+               DRM_INFO("DRM_I915_DEBUG_RPM enabled\n");
 }
 
 /**
@@ -1477,7 +1480,7 @@ void i915_driver_unload(struct drm_device *dev)
 
        intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
-       WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count));
+       intel_runtime_pm_cleanup(dev_priv);
 }
 
 static void i915_driver_release(struct drm_device *dev)
@@ -1685,6 +1688,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, 
bool hibernation)
 
 out:
        enable_rpm_wakeref_asserts(dev_priv);
+       intel_runtime_pm_cleanup(dev_priv);
 
        return ret;
 }
@@ -2644,7 +2648,7 @@ static int intel_runtime_suspend(struct device *kdev)
        }
 
        enable_rpm_wakeref_asserts(dev_priv);
-       WARN_ON_ONCE(atomic_read(&dev_priv->runtime_pm.wakeref_count));
+       intel_runtime_pm_cleanup(dev_priv);
 
        if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv))
                DRM_ERROR("Unclaimed access detected prior to suspending\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0b10a30b7d96..c8d5b896b155 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -45,6 +45,7 @@
 #include <linux/pm_qos.h>
 #include <linux/reservation.h>
 #include <linux/shmem_fs.h>
+#include <linux/stackdepot.h>
 
 #include <drm/drmP.h>
 #include <drm/intel-gtt.h>
@@ -1278,6 +1279,12 @@ struct i915_runtime_pm {
        atomic_t wakeref_count;
        bool suspended;
        bool irqs_enabled;
+
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RPM)
+       spinlock_t debug_lock;
+       depot_stack_handle_t *debug_owners;
+       unsigned long debug_count;
+#endif
 };
 
 enum intel_pipe_crc_source {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dc6c0cec9b36..823db4ed47c8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1947,6 +1947,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp);
 int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state);
 
 /* intel_runtime_pm.c */
+void intel_runtime_pm_init_early(struct drm_i915_private *dev_priv);
 int intel_power_domains_init(struct drm_i915_private *);
 void intel_power_domains_cleanup(struct drm_i915_private *dev_priv);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool 
resume);
@@ -1957,6 +1958,7 @@ void bxt_display_core_init(struct drm_i915_private 
*dev_priv, bool resume);
 void bxt_display_core_uninit(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_disable(struct drm_i915_private *dev_priv);
+void intel_runtime_pm_cleanup(struct drm_i915_private *dev_priv);
 const char *
 intel_display_power_domain_str(enum intel_display_power_domain domain);
 
@@ -1974,23 +1976,23 @@ void icl_dbuf_slices_update(struct drm_i915_private 
*dev_priv,
                            u8 req_slices);
 
 static inline void
-assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
+assert_rpm_device_not_suspended(struct drm_i915_private *i915)
 {
-       WARN_ONCE(dev_priv->runtime_pm.suspended,
+       WARN_ONCE(i915->runtime_pm.suspended,
                  "Device suspended during HW access\n");
 }
 
 static inline void
-assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
+assert_rpm_wakelock_held(struct drm_i915_private *i915)
 {
-       assert_rpm_device_not_suspended(dev_priv);
-       WARN_ONCE(!atomic_read(&dev_priv->runtime_pm.wakeref_count),
+       assert_rpm_device_not_suspended(i915);
+       WARN_ONCE(!atomic_read(&i915->runtime_pm.wakeref_count),
                  "RPM wakelock ref not held during HW access");
 }
 
 /**
  * disable_rpm_wakeref_asserts - disable the RPM assert checks
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function disable asserts that check if we hold an RPM wakelock
  * reference, while keeping the device-not-suspended checks still enabled.
@@ -2007,14 +2009,14 @@ assert_rpm_wakelock_held(struct drm_i915_private 
*dev_priv)
  * enable_rpm_wakeref_asserts().
  */
 static inline void
-disable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
+disable_rpm_wakeref_asserts(struct drm_i915_private *i915)
 {
-       atomic_inc(&dev_priv->runtime_pm.wakeref_count);
+       atomic_inc(&i915->runtime_pm.wakeref_count);
 }
 
 /**
  * enable_rpm_wakeref_asserts - re-enable the RPM assert checks
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function re-enables the RPM assert checks after disabling them with
  * disable_rpm_wakeref_asserts. It's meant to be used only in special
@@ -2024,15 +2026,15 @@ disable_rpm_wakeref_asserts(struct drm_i915_private 
*dev_priv)
  * disable_rpm_wakeref_asserts().
  */
 static inline void
-enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
+enable_rpm_wakeref_asserts(struct drm_i915_private *i915)
 {
-       atomic_dec(&dev_priv->runtime_pm.wakeref_count);
+       atomic_dec(&i915->runtime_pm.wakeref_count);
 }
 
-void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
-bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
-void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
-void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
+void intel_runtime_pm_get(struct drm_i915_private *i915);
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915);
+void intel_runtime_pm_get_noresume(struct drm_i915_private *i915);
+void intel_runtime_pm_put(struct drm_i915_private *i915);
 
 void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b78c3b48aa62..8cc57eab862a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,6 +49,143 @@
  * present for a given platform.
  */
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RPM)
+
+#include <linux/sort.h>
+
+#define STACKDEPTH 8
+
+static void init_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+       spin_lock_init(&i915->runtime_pm.debug_lock);
+}
+
+static noinline void
+track_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+       struct i915_runtime_pm *rpm = &i915->runtime_pm;
+       unsigned long entries[STACKDEPTH];
+       struct stack_trace trace = {
+               .entries = entries,
+               .max_entries = ARRAY_SIZE(entries),
+               .skip = 1
+       };
+       unsigned long flags;
+       depot_stack_handle_t stack, *stacks;
+
+       if (!HAS_RUNTIME_PM(i915))
+               return;
+
+       save_stack_trace(&trace);
+       if (trace.nr_entries &&
+           trace.entries[trace.nr_entries - 1] == ULONG_MAX)
+               trace.nr_entries--;
+
+       stack = depot_save_stack(&trace, GFP_NOWAIT | __GFP_NOWARN);
+       if (!stack)
+               return;
+
+       spin_lock_irqsave(&rpm->debug_lock, flags);
+       stacks = krealloc(rpm->debug_owners,
+                         (rpm->debug_count + 1) * sizeof(*stacks),
+                         GFP_NOWAIT | __GFP_NOWARN);
+       if (stacks) {
+               stacks[rpm->debug_count++] = stack;
+               rpm->debug_owners = stacks;
+       }
+       spin_unlock_irqrestore(&rpm->debug_lock, flags);
+}
+
+static void untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+       struct i915_runtime_pm *rpm = &i915->runtime_pm;
+       depot_stack_handle_t *stacks;
+       unsigned long flags;
+
+       spin_lock_irqsave(&rpm->debug_lock, flags);
+       stacks = fetch_and_zero(&rpm->debug_owners);
+       rpm->debug_count = 0;
+       spin_unlock_irqrestore(&rpm->debug_lock, flags);
+
+       kfree(stacks);
+}
+
+static int cmphandle(const void *_a, const void *_b)
+{
+       const depot_stack_handle_t * const a = _a, * const b = _b;
+
+       if (*a < *b)
+               return -1;
+       else if (*a > *b)
+               return 1;
+       else
+               return 0;
+}
+
+static void show_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+       struct i915_runtime_pm *rpm = &i915->runtime_pm;
+       unsigned long entries[STACKDEPTH];
+       depot_stack_handle_t *stacks;
+       unsigned long flags, count, i;
+       char *buf;
+
+       spin_lock_irqsave(&rpm->debug_lock, flags);
+       stacks = fetch_and_zero(&rpm->debug_owners);
+       count = fetch_and_zero(&rpm->debug_count);
+       spin_unlock_irqrestore(&rpm->debug_lock, flags);
+       if (!count)
+               return;
+
+       DRM_DEBUG_DRIVER("leaked %lu wakerefs\n", count);
+
+       buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+       if (!buf)
+               goto out_stacks;
+
+       sort(stacks, count, sizeof(*stacks), cmphandle, NULL);
+
+       for (i = 0; i < count; i++) {
+               struct stack_trace trace = {
+                       .entries = entries,
+                       .max_entries = ARRAY_SIZE(entries),
+               };
+               depot_stack_handle_t stack = stacks[i];
+               unsigned long rep;
+
+               rep = 1;
+               while (i + 1 < count && stacks[i + 1] == stack)
+                       rep++, i++;
+               depot_fetch_stack(stack, &trace);
+               snprint_stack_trace(buf, PAGE_SIZE, &trace, 0);
+               DRM_DEBUG_DRIVER("wakeref x%lu at\n%s", rep, buf);
+       }
+
+       kfree(buf);
+out_stacks:
+       kfree(stacks);
+}
+
+#else
+
+static void init_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+}
+
+static void track_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+}
+
+static void untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+}
+
+static void show_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+{
+}
+
+#endif
+
 bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
                                         enum i915_power_well_id power_well_id);
 
@@ -3920,7 +4057,7 @@ void intel_power_domains_verify_state(struct 
drm_i915_private *dev_priv)
 
 /**
  * intel_runtime_pm_get - grab a runtime pm reference
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function grabs a device-level runtime pm reference (mostly used for GEM
  * code to ensure the GTT or GT is on) and ensures that it is powered up.
@@ -3928,22 +4065,24 @@ void intel_power_domains_verify_state(struct 
drm_i915_private *dev_priv)
  * Any runtime pm reference obtained by this function must have a symmetric
  * call to intel_runtime_pm_put() to release the reference again.
  */
-void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
+void intel_runtime_pm_get(struct drm_i915_private *i915)
 {
-       struct pci_dev *pdev = dev_priv->drm.pdev;
+       struct pci_dev *pdev = i915->drm.pdev;
        struct device *kdev = &pdev->dev;
        int ret;
 
        ret = pm_runtime_get_sync(kdev);
        WARN_ONCE(ret < 0, "pm_runtime_get_sync() failed: %d\n", ret);
 
-       atomic_inc(&dev_priv->runtime_pm.wakeref_count);
-       assert_rpm_wakelock_held(dev_priv);
+       atomic_inc(&i915->runtime_pm.wakeref_count);
+       assert_rpm_wakelock_held(i915);
+
+       track_intel_runtime_pm_wakeref(i915);
 }
 
 /**
  * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in 
use
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function grabs a device-level runtime pm reference if the device is
  * already in use and ensures that it is powered up. It is illegal to try
@@ -3954,10 +4093,10 @@ void intel_runtime_pm_get(struct drm_i915_private 
*dev_priv)
  *
  * Returns: True if the wakeref was acquired, or False otherwise.
  */
-bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915)
 {
        if (IS_ENABLED(CONFIG_PM)) {
-               struct pci_dev *pdev = dev_priv->drm.pdev;
+               struct pci_dev *pdev = i915->drm.pdev;
                struct device *kdev = &pdev->dev;
 
                /*
@@ -3970,15 +4109,17 @@ bool intel_runtime_pm_get_if_in_use(struct 
drm_i915_private *dev_priv)
                        return false;
        }
 
-       atomic_inc(&dev_priv->runtime_pm.wakeref_count);
-       assert_rpm_wakelock_held(dev_priv);
+       atomic_inc(&i915->runtime_pm.wakeref_count);
+       assert_rpm_wakelock_held(i915);
+
+       track_intel_runtime_pm_wakeref(i915);
 
        return true;
 }
 
 /**
  * intel_runtime_pm_get_noresume - grab a runtime pm reference
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function grabs a device-level runtime pm reference (mostly used for GEM
  * code to ensure the GTT or GT is on).
@@ -3993,32 +4134,35 @@ bool intel_runtime_pm_get_if_in_use(struct 
drm_i915_private *dev_priv)
  * Any runtime pm reference obtained by this function must have a symmetric
  * call to intel_runtime_pm_put() to release the reference again.
  */
-void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
+void intel_runtime_pm_get_noresume(struct drm_i915_private *i915)
 {
-       struct pci_dev *pdev = dev_priv->drm.pdev;
+       struct pci_dev *pdev = i915->drm.pdev;
        struct device *kdev = &pdev->dev;
 
-       assert_rpm_wakelock_held(dev_priv);
+       assert_rpm_wakelock_held(i915);
        pm_runtime_get_noresume(kdev);
 
-       atomic_inc(&dev_priv->runtime_pm.wakeref_count);
+       atomic_inc(&i915->runtime_pm.wakeref_count);
+
+       track_intel_runtime_pm_wakeref(i915);
 }
 
 /**
  * intel_runtime_pm_put - release a runtime pm reference
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function drops the device-level runtime pm reference obtained by
  * intel_runtime_pm_get() and might power down the corresponding
  * hardware block right away if this is the last reference.
  */
-void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
+void intel_runtime_pm_put(struct drm_i915_private *i915)
 {
-       struct pci_dev *pdev = dev_priv->drm.pdev;
+       struct pci_dev *pdev = i915->drm.pdev;
        struct device *kdev = &pdev->dev;
 
-       assert_rpm_wakelock_held(dev_priv);
-       atomic_dec(&dev_priv->runtime_pm.wakeref_count);
+       assert_rpm_wakelock_held(i915);
+       if (atomic_dec_and_test(&i915->runtime_pm.wakeref_count))
+               untrack_intel_runtime_pm_wakeref(i915);
 
        pm_runtime_mark_last_busy(kdev);
        pm_runtime_put_autosuspend(kdev);
@@ -4026,7 +4170,7 @@ void intel_runtime_pm_put(struct drm_i915_private 
*dev_priv)
 
 /**
  * intel_runtime_pm_enable - enable runtime pm
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
  *
  * This function enables runtime pm at the end of the driver load sequence.
  *
@@ -4034,9 +4178,9 @@ void intel_runtime_pm_put(struct drm_i915_private 
*dev_priv)
  * subordinate display power domains. That is only done on the first modeset
  * using intel_display_set_init_power().
  */
-void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
+void intel_runtime_pm_enable(struct drm_i915_private *i915)
 {
-       struct pci_dev *pdev = dev_priv->drm.pdev;
+       struct pci_dev *pdev = i915->drm.pdev;
        struct device *kdev = &pdev->dev;
 
        pm_runtime_set_autosuspend_delay(kdev, 10000); /* 10s */
@@ -4048,7 +4192,7 @@ void intel_runtime_pm_enable(struct drm_i915_private 
*dev_priv)
         * so the driver's own RPM reference tracking asserts also work on
         * platforms without RPM support.
         */
-       if (!HAS_RUNTIME_PM(dev_priv)) {
+       if (!HAS_RUNTIME_PM(i915)) {
                int ret;
 
                pm_runtime_dont_use_autosuspend(kdev);
@@ -4066,9 +4210,9 @@ void intel_runtime_pm_enable(struct drm_i915_private 
*dev_priv)
        pm_runtime_put_autosuspend(kdev);
 }
 
-void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
+void intel_runtime_pm_disable(struct drm_i915_private *i915)
 {
-       struct pci_dev *pdev = dev_priv->drm.pdev;
+       struct pci_dev *pdev = i915->drm.pdev;
        struct device *kdev = &pdev->dev;
 
        pm_runtime_dont_use_autosuspend(kdev);
@@ -4077,6 +4221,25 @@ void intel_runtime_pm_disable(struct drm_i915_private 
*dev_priv)
         * Remove the refcount we took in intel_runtime_pm_enable() in case
         * the platform doesn't support runtime PM.
         */
-       if (!HAS_RUNTIME_PM(dev_priv))
+       if (!HAS_RUNTIME_PM(i915))
                pm_runtime_put(kdev);
 }
+
+void intel_runtime_pm_cleanup(struct drm_i915_private *i915)
+{
+       struct i915_runtime_pm *rpm = &i915->runtime_pm;
+
+       if (WARN(atomic_read(&rpm->wakeref_count),
+                "i915->runtime_pm.wakeref_count=%d on cleanup\n",
+                atomic_read(&rpm->wakeref_count))) {
+               show_intel_runtime_pm_wakeref(i915);
+               atomic_set(&rpm->wakeref_count, 0);
+       }
+
+       untrack_intel_runtime_pm_wakeref(i915);
+}
+
+void intel_runtime_pm_init_early(struct drm_i915_private *i915)
+{
+       init_intel_runtime_pm_wakeref(i915);
+}
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to