| Message ID | 20190312152551.26868-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Stop needlessly acquiring wakeref for debugfs/drop_caches_set | expand |
The part of i915_drop_caches_set() is Reviewed-by: Yokoyama, Caz <caz.yokoyama@intel.com> While I don't know whether we don't really need wakeref, calling intel_runtime_pm_get() makes GPU active state from suspended state for example. It makes i915_pm_rpm@gem-execbuf-stress-extra-wait test difficult to make it faster. Thank you. -caz On Tue, 2019-03-12 at 15:25 +0000, Chris Wilson wrote: > We only need to acquire a wakeref for ourselves for a few operations, > as > most either already acquire their own wakeref or imply a wakeref. In > particular, it is i915_gem_set_wedged() that needed us to present it > with a wakeref, which is incongruous with its "use anywhere" ability. > > Suggested-by: Yokoyama, Caz <caz.yokoyama@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Yokoyama, Caz <caz.yokoyama@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++-------- > drivers/gpu/drm/i915/i915_reset.c | 4 +++- > 2 files changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 6a90558de213..08683dca7775 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -3888,12 +3888,9 @@ static int > i915_drop_caches_set(void *data, u64 val) > { > struct drm_i915_private *i915 = data; > - intel_wakeref_t wakeref; > - int ret = 0; > > DRM_DEBUG("Dropping caches: 0x%08llx [0x%08llx]\n", > val, val & DROP_ALL); > - wakeref = intel_runtime_pm_get(i915); > > if (val & DROP_RESET_ACTIVE && > wait_for(intel_engines_are_idle(i915), > I915_IDLE_ENGINES_TIMEOUT)) > @@ -3902,9 +3899,11 @@ i915_drop_caches_set(void *data, u64 val) > /* No need to check and wait for gpu resets, only libdrm auto- > restarts > * on ioctls on -EAGAIN. */ > if (val & (DROP_ACTIVE | DROP_RETIRE | DROP_RESET_SEQNO)) { > + int ret; > + > ret = mutex_lock_interruptible(&i915- > >drm.struct_mutex); > if (ret) > - goto out; > + return ret; > > if (val & DROP_ACTIVE) > ret = i915_gem_wait_for_idle(i915, > @@ -3943,10 +3942,7 @@ i915_drop_caches_set(void *data, u64 val) > if (val & DROP_FREED) > i915_gem_drain_freed_objects(i915); > > -out: > - intel_runtime_pm_put(i915, wakeref); > - > - return ret; > + return 0; > } > > DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops, > diff --git a/drivers/gpu/drm/i915/i915_reset.c > b/drivers/gpu/drm/i915/i915_reset.c > index 3c08e08837d0..955c22b8dfc7 100644 > --- a/drivers/gpu/drm/i915/i915_reset.c > +++ b/drivers/gpu/drm/i915/i915_reset.c > @@ -861,9 +861,11 @@ static void __i915_gem_set_wedged(struct > drm_i915_private *i915) > void i915_gem_set_wedged(struct drm_i915_private *i915) > { > struct i915_gpu_error *error = &i915->gpu_error; > + intel_wakeref_t wakeref; > > mutex_lock(&error->wedge_mutex); > - __i915_gem_set_wedged(i915); > + with_intel_runtime_pm(i915, wakeref) > + __i915_gem_set_wedged(i915); > mutex_unlock(&error->wedge_mutex); > } >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 6a90558de213..08683dca7775 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3888,12 +3888,9 @@ static int i915_drop_caches_set(void *data, u64 val) { struct drm_i915_private *i915 = data; - intel_wakeref_t wakeref; - int ret = 0; DRM_DEBUG("Dropping caches: 0x%08llx [0x%08llx]\n", val, val & DROP_ALL); - wakeref = intel_runtime_pm_get(i915); if (val & DROP_RESET_ACTIVE && wait_for(intel_engines_are_idle(i915), I915_IDLE_ENGINES_TIMEOUT)) @@ -3902,9 +3899,11 @@ i915_drop_caches_set(void *data, u64 val) /* No need to check and wait for gpu resets, only libdrm auto-restarts * on ioctls on -EAGAIN. */ if (val & (DROP_ACTIVE | DROP_RETIRE | DROP_RESET_SEQNO)) { + int ret; + ret = mutex_lock_interruptible(&i915->drm.struct_mutex); if (ret) - goto out; + return ret; if (val & DROP_ACTIVE) ret = i915_gem_wait_for_idle(i915, @@ -3943,10 +3942,7 @@ i915_drop_caches_set(void *data, u64 val) if (val & DROP_FREED) i915_gem_drain_freed_objects(i915); -out: - intel_runtime_pm_put(i915, wakeref); - - return ret; + return 0; } DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops, diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c index 3c08e08837d0..955c22b8dfc7 100644 --- a/drivers/gpu/drm/i915/i915_reset.c +++ b/drivers/gpu/drm/i915/i915_reset.c @@ -861,9 +861,11 @@ static void __i915_gem_set_wedged(struct drm_i915_private *i915) void i915_gem_set_wedged(struct drm_i915_private *i915) { struct i915_gpu_error *error = &i915->gpu_error; + intel_wakeref_t wakeref; mutex_lock(&error->wedge_mutex); - __i915_gem_set_wedged(i915); + with_intel_runtime_pm(i915, wakeref) + __i915_gem_set_wedged(i915); mutex_unlock(&error->wedge_mutex); }
We only need to acquire a wakeref for ourselves for a few operations, as most either already acquire their own wakeref or imply a wakeref. In particular, it is i915_gem_set_wedged() that needed us to present it with a wakeref, which is incongruous with its "use anywhere" ability. Suggested-by: Yokoyama, Caz <caz.yokoyama@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Yokoyama, Caz <caz.yokoyama@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++-------- drivers/gpu/drm/i915/i915_reset.c | 4 +++- 2 files changed, 7 insertions(+), 9 deletions(-)