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@xxxxxxxxxxxxxxxxxx>
> >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> >>> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> >>> ---
> >>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
