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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/05/2019 12:39, Andi Shyti wrote:
>>> +	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?

igt_assert(e) I think would be a better fit.
>> 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

Yeah, I got fooled by the same trick.

Regards,

Tvrtko

.
> 
> [...]
> 
>> And please put a follow up to add API docs on your TODO list shortly after
>> we merge this.
> 
> OK.
> 
> Andi
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev