[igt-dev] [PATCH v24 02/14] lib/i915: add gem_engine_topology library and for_each loop definition

[Andi Shyti]

> > +	p->size = (p->size - sizeof(struct i915_context_param_engines)) /
> > +		  (offsetof(struct i915_context_param_engines,
> > +			    engines[1]) -
> > +		  sizeof(struct i915_context_param_engines));
> 
> So param.size starts with number of bytes, but then becomes number of
> engines? It's a bit evil and non-obvious, because a line below confused me:

this was a review from Chris and later on I use indeed size as
engine count.

I understand it's a bit unclear given that in the kernel it has a
slightly different meaning.

> > +	igt_assert_f(p->size <= GEM_MAX_ENGINES, "unsupported engine count\n");

[...]

> > +	if (!param.size) {
> > +		query_engine_list(fd, &engine_data);
> > +		ctx_map_engines(fd, &engine_data, &param);
> > +	} else {
> > +		for (i = 0; i < param.size; i++)
> 
> This one. It is an apparent mismatch between indices and bytes.
> 
> Put a comment with this block saying in what case we get here and the trick
> with param.size you play.

I will add a few comments to make it more clear.

> > +int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
> > +			      struct intel_execution_engine2 *e)
> > +{
> > +	DEFINE_CONTEXT_PARAM(engines, param, ctx_id, GEM_MAX_ENGINES);
> > +
> > +	if (!e || gem_topology_get_param(fd, &param) || !param.size)

> You expect a NULL e here and to what purpose?

just to be a bit paranoic, besides I wouldn't like a segfault in
my function :)

> Best to just disallow it I think.

isn't 'return -EINVAL' enough? igt_assert?

> Also param.size == 0? It can't be possible due how you define the structure
> a line above it.

yes, but before I call 'gem_topology_get_param()' where size
might get overridden.

[...]

> And please put a follow up to add API docs on your TODO list shortly after
> we merge this.

OK.

Andi