[Intel-gfx] [PATCH 2/2] drm/i915/perf: Drop lockdep assert for i915_oa_init_reg_state()

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-09 23:47:20)
> On 09/08/17 16:38, Chris Wilson wrote:
> > This is called from execlist context init which we need to be unlocked.
> > Commit f89823c21224 ("drm/i915/perf: Implement
> > I915_PERF_ADD/REMOVE_CONFIG interface") added a lockdep assert to this
> > path for unclear reasons, remove it again!
> >
> > Fixes: 701f8231a2fe ("drm/i915/perf: prune OA configs")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 1be355d14e8a..3bdf53faae24 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2173,8 +2173,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
> >       struct drm_i915_private *dev_priv = engine->i915;
> >       struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
> 
> I was trying to avoid adding a new lock for exclusive_stream.
> If we can't rely on dev_priv->drm.struct_mutex to update 
> exclusive_stream, I believe we need to add a new lock.
> Or maybe some other mechanism?

Doesn't changing the exclusive_stream require serialization against the
requests? Therefore we would be safe here for reset as the existence of
the incomplete request that we are altering is the barrier.

Wait... You don't have that barrier anymore? So you never change the
context image for the exclusive stream on setting and force the reload
of the image? I expected to see something like
gen8_configure_all_contexts(), but only needing to change the old/new
exclusive_streams.

At any rate, just wrapping exclusive_stream = bah inside struct_mutex is
not the barrier you intended, or at least not the barrier I think you need
wrt to request construction and concurrent execution thereof.

Longer term, I don't plan for the context allocation/initialisation to
be under the struct_mutex, but we can still expect it to be part of the
request construction, so would still be serialized by a future semaphore
between oa and execbuf.
-Chris