[igt-dev] [PATCH v12 5/7] lib/i915: add gem_engine_topology library

[Andi Shyti]

Hi Tvrtko,

I agree with everything... few comments.

> > +	if (__gem_context_get_param(fd, &ctx_param)) {
> > +		const struct intel_execution_engine2 *e2;
> > +
> > +		igt_debug("using pre-allocated engine list\n");
> > +
> > +		__for_each_engine_class_instance(e2) {
> > +			if (!gem_has_engine(fd, e2->class, e2->instance))
> > +				continue;
> > +
> > +			engine_data.engines[engine_data.nengines].name =
> > +								e2->name;
> > +			engine_data.engines[engine_data.nengines].instance =
> > +								e2->instance;
> > +			engine_data.engines[engine_data.nengines].class =
> > +								e2->class;
> 
> Also could keep a pointer to engine_data.engines[<current>] for more
> readable assignments.
> 
> > +			engine_data.nengines++;
> > +		}
> > +
> > +		return engine_data;
> > +	}
> > +
> > +	init_engine_list(&engine_data);
> 
> As far as I can see you missed the use case where we want to iterate over
> engines already defined in the context engine map.
> 
> So I think if the above __gem_context_get_param succeeds with non-zero size
> returned, you need to build engine_data based on those engines.

Don't I get the engine list anyway? Do you man that
DRM_I915_QUERY_ENGINE_INFO might have a different list from
I915_CONTEXT_PARAM_ENGINES?

> > +	if (__gem_context_get_param(fd, &ctx_param)) {
> > +		eb->flags |= (I915_EXEC_RING_MASK & engine);
> > +		eb->rsvd1 = ctx;
> > +	} else {
> > +		eb->flags |= gem_class_instance_to_eb_flags(fd, e2.class,
> > +							    e2.instance);
> > +	}
> > +}
> 
> Store flags in struct intel_execution_engine2 while building the list and
> then just eb->flags |= e2->eb_flags in tests?

You are recommending to extend the 'intel_execution_engine2' (that
would make my life so very easy), I've been asked not to touch
that structure.

Then, I will add the flags value in the 'intel_execution_engine2'
definition.

> > +struct intel_engine_data {
> > +	int fd;
> > +	uint32_t ctx;
> > +
> > +	uint32_t nengines;
> > +	uint32_t n;
> 
> This is the current engine index? Could it come handy to also have a pointer
> to current engine in the iterator? Will see in later patches..

Actually not, in earlier version I had the pointer, but never
used, so that I removed it.

> Regards,

Thanks a lot,
Andi

> Tvrtko