[Intel-gfx] [PATCH v2 1/9] drm/i915/perf: store the associated engine of a stream

[Lionel Landwerlin]

On 09/10/2019 17:10, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-10-09 14:48:42)
>> On 09/10/2019 16:40, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2019-10-09 14:34:49)
>>>> On 09/10/2019 15:45, Chris Wilson wrote:
>>>>> Quoting Lionel Landwerlin (2019-10-09 13:43:32)
>>>>>> Do you have branch somewhere with this series?
>>>>> https://cgit.freedesktop.org/~ickle/linux-2.6/log/?h=wip-perf
>>>>> -Chris
>>>>>
>>>> Cheers,
>>>>
>>>>
>>>> I've modified the top patch to set the nopreempt flag for as long as the
>>>> context has been flagged (as long at the perf stream is opened) :
>>>> https://github.com/djdeath/linux/commit/d3327b30c6141fac98a3d46f3398c87fe70976aa
>>> That means you are not passing in the ext_perf_config to every batch that
>>> is using it, right? The oa_config tracking also hinges on that you do.
>>> -Chris
>>>
>> Like I mentioned, there are empty batch to drain the context that we
>> emit without OA reconfiguration.
>>
>> There could also be a sequence such as :
>>
>>       - batch0 (includes perf query config=42)
>>
>>       - batch1 (no perf query, includes timestamp or pipeline query)
> But this execbuf is not using the oa_config, so why should it?


I was just working about the problem I guess.

With the patch you sent earlier to not merge request with different 
flags, that's probably useless.


>   The
> direction you've suggested is that we should attach the perf state to
> the context.
>
> eb_oa_config():
> 	struct i915_perf_stream *stream;
>
> 	/* attached/removed by perf_ioctl */
> 	stream = rcu_get(eb->gem_context->perf);
> 	if (!stream)
> 		return;
>
> 	if (stream->oa_config != stream->perf->oa_config) {
> 		mutex_lock(&perf->lock);
> 		...
> 		mutex_unlock(&perf->lock);
> 	}
>
> 	if (stream->hold_preemption)
> 		eb->request->flags |= NOPREEMPT;
> 	
> 	i915_perf_stream_put(stream);
>
> Then instead of an execbuf extension it would either be a perf ioctl to
> update the oa_config on the attached specified context, or a context
> param. Using the perf ioctl does not seem amiss.
>
>>       - batch2 (includes perf query config=42)
>>
>>       - batch3 (includes perf query config=43)
>>
>>
>> It sounds reasonable to ensure that all the requests are flagged with
>> nopreempt to ensure we don't preempt one because we don't have
>> visibility on what's completed when reloading the execlists ports.
>>
>> This entire sequence above would be surrounded by open/close of the perf
>> stream. Once close() returns, then any new request won't be flagged with
>> nopreempt but it's the application's responsability to have collected
>> all the queries' results before closing the stream.
> Are you happy with associating the i915_perf_stream with the
> specific_ctx and controlling all the parameters via perf-ioctl?
> -Chris
>

Yeah sounds like it should work, I would like to test the whole setup.

If you can share the patches changing the config through a perf stream 
ioctl, I'll update my driver and test.


Thanks a lot,


-Lionel