Re: [igt-dev] [RFC v2 2/3] lib: implement new engine discovery interface

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

On 26/11/2018 09:07, Andi Shyti wrote:
> Hi Tvrtko,
> 
> just few things that came to my mind while going through your
> review.
> 
>>> +	query_engines = malloc(size);
>>> +	if (!query_engines)
>>> +		return NULL;
>>
>> We probably want to igt_assert on this since there is no point going
>> further.
> 
> isn't igt_assert used for the test outcome? What I mean is that
> the test outcome would be to query and set the engines, but if we
> fail in malloc we do not necessarily fail the test, but we have a
> different kind of system failure.
> 
> (this is not important, just for me to understand better :) )

We use it for both since we haven't agreed on any mechanism to 
differentiate. I think it was mentioned a few times but I don't remember 
what were the different opinions.

>>> +int get_engines(int fd, uint32_t ctx_id)
>>
>> setup_ctx_engines?
>>
>> Does it need to be exported or it can be static?
> 
> How do we reach it from outside if we set it static? This
> function is called in the for_each_engine_ctx macro that is used
> outside from igt_gt.c

Of course! But then it needs a gem prefix and probably double underscore 
to signify it shouldn't be called directly.

>>> +#define for_each_engine_ctx(fd, ctx, e) \
>>
>> High level design question: Do we want 'e' to be an integer or a struct
>> describing each engine?
> 
> Do you mean that you would you prefer iterating with a
> 'i915_context_param_engines' or a 'intel_execution_engine' struct
> instead of an 'e' integer? This way it would also be different
> from how the current 'for_each_engine' works.
> 
> I could do it in a next patch, so that in this one we keep it as
> close as possible to the current way of doing things.

I forgot in current code struct intel_execution_engine is a hidden 
variable in the iterator, unlike the i915 version.

I think struct intel_execution_engine available during iteration is 
preferable (we often need easy access to engine name, class/instance and 
such), but I don't have an opinion on whether it should be explicit or 
still hidden variable. Staying with hidden makes the churn smaller. 
Maybe Chris has an opinion.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev