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

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-10 08:23:20)
> On 10/08/17 00:13, Chris Wilson wrote:
> > 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.
> 
> We serialize the requests to make sure that our modifications to the 
> context image have been applied.
> So that when we return the perf stream fd, all work that was submitted 
> before the context image was modified has retired.
> 
> When new contexts are created, there is a need to set the OA parameters 
> in their image. But that has to read from the currently opened stream.
> So there must be some kind of synchronization there.
> 
> I can make the access to exclusive_stream go through an srcu. Does that 
> sound like the right way to do it?

To put it simply any sleeping lock within the reset path is a nuisance.
We may very well want to perform the reset from interrupt/softirq
context (for handling watchdogs).

All you need to do is to stage the change, and only apply it once you
have serialized the request stream. That's your barrier.
-Chris