Re: [Intel-gfx] [PATCH 03/46] drm/i915: Track all held rpm wakerefs
Quoting Mika Kuoppala (2019-01-07 13:14:00)
> Chris Wilson writes:
> > @@ -2011,6 +2011,8 @@ static int i915_drm_suspend_late(struct drm_device
> > *dev, bool hibernation)
> >
> > out:
> > enable_rpm_wakeref_asserts(dev_priv);
> > + if (!dev_priv->uncore.user_forcewake.count)
> > + intel_runtime_pm_cleanup(dev_priv);
> >
>
> Why would we have forcewake active in here?
Why would the user suspend while holding
open("/debug/dri/0/i915_user_forcewake")?
Because they can.
> Are you planning on extending the intel_runtime_pm_cleanup?
> Atleast in the callsite 'intel_runtime_assert_no_wakerefs' would
> make more sense.
Oh yes, yes, yes. The challenge is that we take the rpm wakeref with
such frequency and variety of lifetimes is that we end up with so much
tracked that it makes finding the leak very hard (and we cannot report
an underflow elsewhere at the moment for similar reasons). So this first
wave was to catch the easy stuff and report a leak on module unload, then
once everyone is tracking their own wakeref, we can do the must_check
annotation and WARN on underflow.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/46] drm/i915: Track all held rpm wakerefs
Chris Wilson writes:
> 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
> Cc: Jani Nikula
> ---
> drivers/gpu/drm/i915/Kconfig.debug| 2 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 6 +
> drivers/gpu/drm/i915/i915_drv.c | 8 +-
> drivers/gpu/drm/i915/i915_drv.h | 7 +
> drivers/gpu/drm/i915/intel_drv.h | 44 ++-
> drivers/gpu/drm/i915/intel_runtime_pm.c | 267 --
> .../gpu/drm/i915/selftests/mock_gem_device.c | 8 +-
> 7 files changed, 292 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug
> b/drivers/gpu/drm/i915/Kconfig.debug
> index 9e36ffb5eb7c..a97929c47466 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -21,11 +21,11 @@ config DRM_I915_DEBUG
> select DEBUG_FS
> select PREEMPT_COUNT
> select I2C_CHARDEV
> +select STACKDEPOT
> select DRM_DP_AUX_CHARDEV
> select X86_MSR # used by igt/pm_rpm
> select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
> select DRM_DEBUG_MM if DRM=y
> -select STACKDEPOT if DRM=y # for DRM_DEBUG_MM
> select DRM_DEBUG_SELFTEST
> select SW_SYNC # signaling validation framework (igt/syncobj*)
> select DRM_I915_SW_FENCE_DEBUG_OBJECTS
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c77326a7d058..3a369245d7e6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2702,6 +2702,12 @@ static int i915_runtime_pm_status(struct seq_file *m,
> void *unused)
> pci_power_name(pdev->current_state),
> pdev->current_state);
>
> + if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)) {
> + struct drm_printer p = drm_seq_file_printer(m);
> +
> + print_intel_runtime_pm_wakeref(dev_priv, );
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 17fca3ba343e..e2f4753ca21f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -906,6 +906,7 @@ static int i915_driver_init_early(struct drm_i915_private
> *dev_priv)
> mutex_init(_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)
> @@ -1808,8 +1809,7 @@ void i915_driver_unload(struct drm_device *dev)
> i915_driver_cleanup_mmio(dev_priv);
>
> enable_rpm_wakeref_asserts(dev_priv);
> -
> - WARN_ON(atomic_read(_priv->runtime_pm.wakeref_count));
> + intel_runtime_pm_cleanup(dev_priv);
> }
>
> static void i915_driver_release(struct drm_device *dev)
> @@ -2011,6 +2011,8 @@ static int i915_drm_suspend_late(struct drm_device
> *dev, bool hibernation)
>
> out:
> enable_rpm_wakeref_asserts(dev_priv);
> + if (!dev_priv->uncore.user_forcewake.count)
> + intel_runtime_pm_cleanup(dev_priv);
>
Why would we have forcewake active in here?
Are you planning on extending the intel_runtime_pm_cleanup?
Atleast in the callsite 'intel_runtime_assert_no_wakerefs' would
make more sense.
> return ret;
> }
> @@ -2966,7 +2968,7 @@ static int intel_runtime_suspend(struct device *kdev)
> }
>
> enable_rpm_wakeref_asserts(dev_priv);
> - WARN_ON_ONCE(atomic_read(_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 17a017645c5d..60b98103aba3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -45,6 +45,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1156,6 +1157,12 @@ struct i915_runtime_pm {
> atomic_t wakeref_count;
> bool suspended;
> bool irqs_enabled;
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> + 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 1a11c2beb7f3..ac513fd70315 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -41,6 +41,8 @@
> #include
> #include
>
> +struct drm_printer;
> +
> /**
> * __wait_for - magic wait macro
> *
> @@ -2084,6 +2086,7 @@ bool
[Intel-gfx] [PATCH 03/46] drm/i915: Track all held rpm wakerefs
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
Cc: Jani Nikula
---
drivers/gpu/drm/i915/Kconfig.debug| 2 +-
drivers/gpu/drm/i915/i915_debugfs.c | 6 +
drivers/gpu/drm/i915/i915_drv.c | 8 +-
drivers/gpu/drm/i915/i915_drv.h | 7 +
drivers/gpu/drm/i915/intel_drv.h | 44 ++-
drivers/gpu/drm/i915/intel_runtime_pm.c | 267 --
.../gpu/drm/i915/selftests/mock_gem_device.c | 8 +-
7 files changed, 292 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/Kconfig.debug
b/drivers/gpu/drm/i915/Kconfig.debug
index 9e36ffb5eb7c..a97929c47466 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -21,11 +21,11 @@ config DRM_I915_DEBUG
select DEBUG_FS
select PREEMPT_COUNT
select I2C_CHARDEV
+select STACKDEPOT
select DRM_DP_AUX_CHARDEV
select X86_MSR # used by igt/pm_rpm
select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
select DRM_DEBUG_MM if DRM=y
-select STACKDEPOT if DRM=y # for DRM_DEBUG_MM
select DRM_DEBUG_SELFTEST
select SW_SYNC # signaling validation framework (igt/syncobj*)
select DRM_I915_SW_FENCE_DEBUG_OBJECTS
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index c77326a7d058..3a369245d7e6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2702,6 +2702,12 @@ static int i915_runtime_pm_status(struct seq_file *m,
void *unused)
pci_power_name(pdev->current_state),
pdev->current_state);
+ if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)) {
+ struct drm_printer p = drm_seq_file_printer(m);
+
+ print_intel_runtime_pm_wakeref(dev_priv, );
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 17fca3ba343e..e2f4753ca21f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -906,6 +906,7 @@ static int i915_driver_init_early(struct drm_i915_private
*dev_priv)
mutex_init(_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)
@@ -1808,8 +1809,7 @@ void i915_driver_unload(struct drm_device *dev)
i915_driver_cleanup_mmio(dev_priv);
enable_rpm_wakeref_asserts(dev_priv);
-
- WARN_ON(atomic_read(_priv->runtime_pm.wakeref_count));
+ intel_runtime_pm_cleanup(dev_priv);
}
static void i915_driver_release(struct drm_device *dev)
@@ -2011,6 +2011,8 @@ static int i915_drm_suspend_late(struct drm_device *dev,
bool hibernation)
out:
enable_rpm_wakeref_asserts(dev_priv);
+ if (!dev_priv->uncore.user_forcewake.count)
+ intel_runtime_pm_cleanup(dev_priv);
return ret;
}
@@ -2966,7 +2968,7 @@ static int intel_runtime_suspend(struct device *kdev)
}
enable_rpm_wakeref_asserts(dev_priv);
- WARN_ON_ONCE(atomic_read(_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 17a017645c5d..60b98103aba3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -45,6 +45,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1156,6 +1157,12 @@ struct i915_runtime_pm {
atomic_t wakeref_count;
bool suspended;
bool irqs_enabled;
+
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+ 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 1a11c2beb7f3..ac513fd70315 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -41,6 +41,8 @@
#include
#include
+struct drm_printer;
+
/**
* __wait_for - magic wait macro
*
@@ -2084,6 +2086,7 @@ bool intel_psr_enabled(struct intel_dp *intel_dp);
void intel_init_quirks(struct drm_i915_private *dev_priv);
/* 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);
@@ -2106,6
