Re: [PATCH 1/6] drm/i915: Get runtime pm ref on i915_drop_caches_set

[Imre Deak <imre.deak@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx