[Intel-gfx] [PATCH 1/6] drm/i915: Get runtime pm ref on i915_drop_caches_set

[Daniel Vetter]

On Tue, Dec 15, 2015 at 01:36:07PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris at 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 at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch