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

[Chris Wilson]

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