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

[Lionel Landwerlin]

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?

>
> 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
>