Re: [Intel-gfx] [PATCH 12/46] drm/i915/gem: Track the rpm wakerefs
On 1/9/2019 03:16, Mika Kuoppala wrote:
Chris Wilson writes:
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c
b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 16693dd4d019..bc230e43b98f 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -154,6 +154,7 @@ i915_gem_shrink(struct drm_i915_private *i915,
{ >mm.bound_list, I915_SHRINK_BOUND },
{ NULL, 0 },
}, *phase;
+ intel_wakeref_t wakeref = 0;
unsigned long count = 0;
unsigned long scanned = 0;
bool unlock;
@@ -183,9 +184,11 @@ i915_gem_shrink(struct drm_i915_private *i915,
* device just to recover a little memory. If absolutely necessary,
* we will force the wake during oom-notifier.
*/
- if ((flags & I915_SHRINK_BOUND) &&
- !intel_runtime_pm_get_if_in_use(i915))
- flags &= ~I915_SHRINK_BOUND;
+ if (flags & I915_SHRINK_BOUND) {
+ wakeref = intel_runtime_pm_get_if_in_use(i915);
+ if (!wakeref)
+ flags &= ~I915_SHRINK_BOUND;
+ }
/*
* As we may completely rewrite the (un)bound list whilst unbinding
@@ -266,7 +269,7 @@ i915_gem_shrink(struct drm_i915_private *i915,
}
if (flags & I915_SHRINK_BOUND)
- intel_runtime_pm_put_unchecked(i915);
+ intel_runtime_pm_put(i915, wakeref);
This is ok but raises a question that did we have
GEM_BUG_ON(wakeref == 0) on pm_put? Perhaps not needed
per se as we do find that we don't have ref for 0.
Reviewed-by: Mika Kuoppala
There is a WARN not a BUG if pm_put() is called with a zero wakeref (in
the cancel_ function after the search fails to find a match with zero).
However, the flag checks mean that it can't happen from here.
John.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/46] drm/i915/gem: Track the rpm wakerefs
Chris Wilson writes:
> Keep track of the temporary rpm wakerefs used for user access to the
> device, so that we can cancel them upon release and clearly identify any
> leaks.
>
> Signed-off-by: Chris Wilson
> Cc: Jani Nikula
> ---
> drivers/gpu/drm/i915/i915_gem.c| 47 +-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++-
> drivers/gpu/drm/i915/i915_gem_fence_reg.c | 6 ++-
> drivers/gpu/drm/i915/i915_gem_gtt.c| 22 ++
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 32 +--
> drivers/gpu/drm/i915/intel_engine_cs.c | 12 --
> drivers/gpu/drm/i915/intel_uncore.c| 5 ++-
> 7 files changed, 81 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 27f207cbabd9..e04dadeca879 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -786,6 +786,8 @@ fb_write_origin(struct drm_i915_gem_object *obj, unsigned
> int domain)
>
> void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
> {
> + intel_wakeref_t wakeref;
> +
> /*
>* No actual flushing is required for the GTT write domain for reads
>* from the GTT domain. Writes to it "immediately" go to main memory
> @@ -812,13 +814,13 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private
> *dev_priv)
>
> i915_gem_chipset_flush(dev_priv);
>
> - intel_runtime_pm_get(dev_priv);
> + wakeref = intel_runtime_pm_get(dev_priv);
> spin_lock_irq(_priv->uncore.lock);
>
> POSTING_READ_FW(RING_HEAD(RENDER_RING_BASE));
>
> spin_unlock_irq(_priv->uncore.lock);
> - intel_runtime_pm_put_unchecked(dev_priv);
> + intel_runtime_pm_put(dev_priv, wakeref);
> }
>
> static void
> @@ -1070,6 +1072,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
> {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> struct i915_ggtt *ggtt = >ggtt;
> + intel_wakeref_t wakeref;
> struct drm_mm_node node;
> struct i915_vma *vma;
> void __user *user_data;
> @@ -1080,7 +1083,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
> if (ret)
> return ret;
>
> - intel_runtime_pm_get(i915);
> + wakeref = intel_runtime_pm_get(i915);
> vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> PIN_MAPPABLE |
> PIN_NONFAULT |
> @@ -1153,7 +1156,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
> i915_vma_unpin(vma);
> }
> out_unlock:
> - intel_runtime_pm_put_unchecked(i915);
> + intel_runtime_pm_put(i915, wakeref);
> mutex_unlock(>drm.struct_mutex);
>
> return ret;
> @@ -1254,6 +1257,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object
> *obj,
> {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> struct i915_ggtt *ggtt = >ggtt;
> + intel_wakeref_t wakeref;
> struct drm_mm_node node;
> struct i915_vma *vma;
> u64 remain, offset;
> @@ -1272,13 +1276,14 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object
> *obj,
>* This easily dwarfs any performance advantage from
>* using the cache bypass of indirect GGTT access.
>*/
> - if (!intel_runtime_pm_get_if_in_use(i915)) {
> + wakeref = intel_runtime_pm_get_if_in_use(i915);
> + if (!wakeref) {
> ret = -EFAULT;
> goto out_unlock;
> }
> } else {
> /* No backing pages, no fallback, we must force GGTT access */
> - intel_runtime_pm_get(i915);
> + wakeref = intel_runtime_pm_get(i915);
> }
>
> vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> @@ -1360,7 +1365,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object
> *obj,
> i915_vma_unpin(vma);
> }
> out_rpm:
> - intel_runtime_pm_put_unchecked(i915);
> + intel_runtime_pm_put(i915, wakeref);
> out_unlock:
> mutex_unlock(>drm.struct_mutex);
> return ret;
> @@ -1865,6 +1870,7 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct i915_ggtt *ggtt = _priv->ggtt;
> bool write = area->vm_flags & VM_WRITE;
> + intel_wakeref_t wakeref;
> struct i915_vma *vma;
> pgoff_t page_offset;
> int ret;
> @@ -1894,7 +1900,7 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> if (ret)
> goto err;
>
> - intel_runtime_pm_get(dev_priv);
> + wakeref = intel_runtime_pm_get(dev_priv);
>
> ret = i915_mutex_lock_interruptible(dev);
> if (ret)
> @@ -1972,7 +1978,7 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> err_unlock:
> mutex_unlock(>struct_mutex);
> err_rpm:
> - intel_runtime_pm_put_unchecked(dev_priv);
> +
[Intel-gfx] [PATCH 12/46] drm/i915/gem: Track the rpm wakerefs
Keep track of the temporary rpm wakerefs used for user access to the
device, so that we can cancel them upon release and clearly identify any
leaks.
Signed-off-by: Chris Wilson
Cc: Jani Nikula
---
drivers/gpu/drm/i915/i915_gem.c| 47 +-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++-
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 6 ++-
drivers/gpu/drm/i915/i915_gem_gtt.c| 22 ++
drivers/gpu/drm/i915/i915_gem_shrinker.c | 32 +--
drivers/gpu/drm/i915/intel_engine_cs.c | 12 --
drivers/gpu/drm/i915/intel_uncore.c| 5 ++-
7 files changed, 81 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 27f207cbabd9..e04dadeca879 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -786,6 +786,8 @@ fb_write_origin(struct drm_i915_gem_object *obj, unsigned
int domain)
void i915_gem_flush_ggtt_writes(struct drm_i915_private *dev_priv)
{
+ intel_wakeref_t wakeref;
+
/*
* No actual flushing is required for the GTT write domain for reads
* from the GTT domain. Writes to it "immediately" go to main memory
@@ -812,13 +814,13 @@ void i915_gem_flush_ggtt_writes(struct drm_i915_private
*dev_priv)
i915_gem_chipset_flush(dev_priv);
- intel_runtime_pm_get(dev_priv);
+ wakeref = intel_runtime_pm_get(dev_priv);
spin_lock_irq(_priv->uncore.lock);
POSTING_READ_FW(RING_HEAD(RENDER_RING_BASE));
spin_unlock_irq(_priv->uncore.lock);
- intel_runtime_pm_put_unchecked(dev_priv);
+ intel_runtime_pm_put(dev_priv, wakeref);
}
static void
@@ -1070,6 +1072,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
{
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct i915_ggtt *ggtt = >ggtt;
+ intel_wakeref_t wakeref;
struct drm_mm_node node;
struct i915_vma *vma;
void __user *user_data;
@@ -1080,7 +1083,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
if (ret)
return ret;
- intel_runtime_pm_get(i915);
+ wakeref = intel_runtime_pm_get(i915);
vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
PIN_MAPPABLE |
PIN_NONFAULT |
@@ -1153,7 +1156,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
i915_vma_unpin(vma);
}
out_unlock:
- intel_runtime_pm_put_unchecked(i915);
+ intel_runtime_pm_put(i915, wakeref);
mutex_unlock(>drm.struct_mutex);
return ret;
@@ -1254,6 +1257,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
{
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct i915_ggtt *ggtt = >ggtt;
+ intel_wakeref_t wakeref;
struct drm_mm_node node;
struct i915_vma *vma;
u64 remain, offset;
@@ -1272,13 +1276,14 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object
*obj,
* This easily dwarfs any performance advantage from
* using the cache bypass of indirect GGTT access.
*/
- if (!intel_runtime_pm_get_if_in_use(i915)) {
+ wakeref = intel_runtime_pm_get_if_in_use(i915);
+ if (!wakeref) {
ret = -EFAULT;
goto out_unlock;
}
} else {
/* No backing pages, no fallback, we must force GGTT access */
- intel_runtime_pm_get(i915);
+ wakeref = intel_runtime_pm_get(i915);
}
vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
@@ -1360,7 +1365,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
i915_vma_unpin(vma);
}
out_rpm:
- intel_runtime_pm_put_unchecked(i915);
+ intel_runtime_pm_put(i915, wakeref);
out_unlock:
mutex_unlock(>drm.struct_mutex);
return ret;
@@ -1865,6 +1870,7 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
struct drm_i915_private *dev_priv = to_i915(dev);
struct i915_ggtt *ggtt = _priv->ggtt;
bool write = area->vm_flags & VM_WRITE;
+ intel_wakeref_t wakeref;
struct i915_vma *vma;
pgoff_t page_offset;
int ret;
@@ -1894,7 +1900,7 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
if (ret)
goto err;
- intel_runtime_pm_get(dev_priv);
+ wakeref = intel_runtime_pm_get(dev_priv);
ret = i915_mutex_lock_interruptible(dev);
if (ret)
@@ -1972,7 +1978,7 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
err_unlock:
mutex_unlock(>struct_mutex);
err_rpm:
- intel_runtime_pm_put_unchecked(dev_priv);
+ intel_runtime_pm_put(dev_priv, wakeref);
i915_gem_object_unpin_pages(obj);
err:
switch (ret) {
@@ -2045,6 +2051,7 @@ void
