| Message ID | 1450113268-12183-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 14, 2015 at 07:14:23PM +0200, Mika Kuoppala wrote: > When we drop caches, this debugfs entry does hardware access later in > the chain, when fences are updated, so it needs a runtime pm ref. > > Dropping caches is used by some igt/bat tests, so this fixes > some unclaimed register access traces. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 24318b7..bd8ba7e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4839,6 +4839,8 @@ i915_drop_caches_set(void *data, u64 val) > if (ret) > return ret; > > + intel_runtime_pm_get(dev_priv); The current idea of the very coarse granularity of rpm_get() is to do it before struct_mutex (since rpm_get resume may try to acquire the mutex iirc). Ok, fixing that may be bolting the stable door after the horse bolted, but we should nevertheless. In my opinion, it would be more productive to work with Imre on making rpm fine grained so that we don't so many and can actually place the wakelock around the hardware access itself, not every single path that *may* touch hardware. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Mon, Dec 14, 2015 at 07:14:23PM +0200, Mika Kuoppala wrote: >> When we drop caches, this debugfs entry does hardware access later in >> the chain, when fences are updated, so it needs a runtime pm ref. >> >> Dropping caches is used by some igt/bat tests, so this fixes >> some unclaimed register access traces. >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 24318b7..bd8ba7e 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -4839,6 +4839,8 @@ i915_drop_caches_set(void *data, u64 val) >> if (ret) >> return ret; >> >> + intel_runtime_pm_get(dev_priv); > > The current idea of the very coarse granularity of rpm_get() is to do it > before struct_mutex (since rpm_get resume may try to acquire the mutex > iirc). > > Ok, fixing that may be bolting the stable door after the horse bolted, > but we should nevertheless. In my opinion, it would be more productive > to work with Imre on making rpm fine grained so that we don't so many > and can actually place the wakelock around the hardware access itself, > not every single path that *may* touch hardware. Please consider 1/6 v2 as it is needed to avoid random unclaimed accesses during igt/bat if the drop caches is used in wrong spot. We can forget the rest. -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Tue, Dec 15, 2015 at 01:36:07PM +0200, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Mon, Dec 14, 2015 at 07:14:23PM +0200, Mika Kuoppala wrote: > >> When we drop caches, this debugfs entry does hardware access later in > >> the chain, when fences are updated, so it needs a runtime pm ref. > >> > >> Dropping caches is used by some igt/bat tests, so this fixes > >> some unclaimed register access traces. > >> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index 24318b7..bd8ba7e 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -4839,6 +4839,8 @@ i915_drop_caches_set(void *data, u64 val) > >> if (ret) > >> return ret; > >> > >> + intel_runtime_pm_get(dev_priv); > > > > The current idea of the very coarse granularity of rpm_get() is to do it > > before struct_mutex (since rpm_get resume may try to acquire the mutex > > iirc). > > > > Ok, fixing that may be bolting the stable door after the horse bolted, > > but we should nevertheless. In my opinion, it would be more productive > > to work with Imre on making rpm fine grained so that we don't so many > > and can actually place the wakelock around the hardware access itself, > > not every single path that *may* touch hardware. > > Please consider 1/6 v2 as it is needed to avoid random unclaimed > accesses during igt/bat if the drop caches is used in wrong spot. Yeah wakelocks work like locks, except lockdep doesn't check them because they're not really locks in all aspects. But anyway, we need to obey ordering constraints. Hm ... maybe it would be possible to annotate get/put_rpm with a lockdep context (and trylock for the others), and then also acquire that context around the actual resume/suspend functions? Might be worth a shot as a tech demo, perhaps even adding that to the pm core and pinging Rafael for feedback. Since auditing correctness here is way too much pain. -Daniel
On ke, 2015-12-16 at 11:42 +0100, Daniel Vetter wrote: > On Tue, Dec 15, 2015 at 01:36:07PM +0200, Mika Kuoppala wrote: > > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > > > On Mon, Dec 14, 2015 at 07:14:23PM +0200, Mika Kuoppala wrote: > > > > When we drop caches, this debugfs entry does hardware access > > > > later in > > > > the chain, when fences are updated, so it needs a runtime pm > > > > ref. > > > > > > > > Dropping caches is used by some igt/bat tests, so this fixes > > > > some unclaimed register access traces. > > > > > > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > > index 24318b7..bd8ba7e 100644 > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > @@ -4839,6 +4839,8 @@ i915_drop_caches_set(void *data, u64 val) > > > > if (ret) > > > > return ret; > > > > > > > > + intel_runtime_pm_get(dev_priv); > > > > > > The current idea of the very coarse granularity of rpm_get() is > > > to do it > > > before struct_mutex (since rpm_get resume may try to acquire the > > > mutex > > > iirc). > > > > > > Ok, fixing that may be bolting the stable door after the horse > > > bolted, > > > but we should nevertheless. In my opinion, it would be more > > > productive > > > to work with Imre on making rpm fine grained so that we don't so > > > many > > > and can actually place the wakelock around the hardware access > > > itself, > > > not every single path that *may* touch hardware. > > > > Please consider 1/6 v2 as it is needed to avoid random unclaimed > > accesses during igt/bat if the drop caches is used in wrong spot. > > Yeah wakelocks work like locks, except lockdep doesn't check them > because > they're not really locks in all aspects. But anyway, we need to obey > ordering constraints. > > Hm ... maybe it would be possible to annotate get/put_rpm with a > lockdep > context (and trylock for the others), and then also acquire that > context > around the actual resume/suspend functions? Might be worth a shot as > a > tech demo, perhaps even adding that to the pm core and pinging Rafael > for > feedback. Since auditing correctness here is way too much pain. Yea, this idea came up already: http://lists.freedesktop.org/archives/intel-gfx/2015-November/079807.html and I have a preliminary patch at: https://github.com/ideak/linux/commit/8f63aaaef27f3c56a1996ea4ac6a8393e0af4e44 --Imre
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 24318b7..bd8ba7e 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -4839,6 +4839,8 @@ i915_drop_caches_set(void *data, u64 val) if (ret) return ret; + intel_runtime_pm_get(dev_priv); + if (val & DROP_ACTIVE) { ret = i915_gpu_idle(dev); if (ret) @@ -4855,6 +4857,7 @@ i915_drop_caches_set(void *data, u64 val) i915_gem_shrink(dev_priv, LONG_MAX, I915_SHRINK_UNBOUND); unlock: + intel_runtime_pm_put(dev_priv); mutex_unlock(&dev->struct_mutex); return ret;
When we drop caches, this debugfs entry does hardware access later in the chain, when fences are updated, so it needs a runtime pm ref. Dropping caches is used by some igt/bat tests, so this fixes some unclaimed register access traces. Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ 1 file changed, 3 insertions(+)