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.
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.
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 images. But that has to read configuration from the currently
opened stream.
So there must be some kind of synchronization there.
I can make the access to exclusive_stream go through a srcu. Do that
sound right?
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
