[igt-dev] [PATCH v11 2/6] lib/i915: add gem_engine_topology library

[Andi Shyti]

Hi Tvrtko,

> > +static void set_ctx_param_engines(int fd, uint32_t ctx_id)
> > +{
> > +	struct i915_context_param_engines *ctx_engine;
> > +	struct drm_i915_gem_context_param ctx_param;
> > +	const struct intel_execution_engine2 *e2;
> > +	uint8_t buff[SIZEOF_CTX_PARAM] = { };
> > +	int i;
> > +
> > +	if (ctx_is_set)
> > +		return;
> 
> A global ctx_is_set bool doesn't work with passed in ctx_id I think!?
> 
> But more importantly.. could you try and make the iterator work of the local
> ctx.get_engines() query? Only configuring the ctx map to include all engines
> if not already set.
> 
> Chris had a suggestion in the previous posting to use an iterator struct
> which I think has merit since it hides the explicit index var/param.
> (155179296327.27405.14019468668411244512 at skylake-alporthouse-com)
> 
> And then I'd drop the for_each_engine2 and just convert
> for_each_engine_class_instance straight away. Only one test uses it so
> shouldn't be too hard (perf_pmu).
> 
> This will I think require adding the gem_set_eb_engine helper and possibly
> adding some helpers to allow usage with spin batches.
> 
> We want to get a better view of usability in future IGT code and I think the
> series needs to be complete in this respect.

Yes, I understand the advantage of Chris proposal and the
ugliness of a globa ctx_is_set, but I thought we were going
throught this path in a second phase, given that for now "future
proof" (at least for me) is something with a different dimension
(I also wanted to keep the changes as smaller as possible, but it
turned out to be the wrong time to do so :) ).

Anyway, since you are pushing for this, I take this path.

Because I am now going to drastically change the logic, I would
also remove the 'init_engine_list()' and query the engine at any
'intel_engine_iter()' call with the double query (one for size
and one for the query itself).

... and it would be a completely different patchset :)

Andi