[igt-dev] [PATCH v14 4/5] lib/i915: add gem_engine_topology library and for_each loop definition

[Tvrtko Ursulin]

On 21/03/2019 11:23, Andi Shyti wrote:
>>> +static void init_engine(struct intel_execution_engine2 *e2, const char *name,
>>> +			uint16_t class, uint16_t instance, uint8_t flags)
>>
>> I'd probably use u64 for flags to match the structure.
> 
> yes, flags it's u64, I used u8 because the flags we use is never
> supposed to be higher than than 3f. But sure, can make it u64.
> 
>>> +{
>>> +	static const char *unk_name = "unk";
>>> +
>>> +	e2->class    = class;
>>> +	e2->instance = instance;
>>> +	e2->flags    = flags;
>>> +
>>> +	if (name) {
>>> +		e2->name = name;
>>
>> This path is used only for the legacy fall back mode so I am contemplating
>> whether is is even worth having the name passed in.
> 
> yes, just wanted to be consistent. At the biginning the
> dup_engine had a bigger role, but then I demoted it to just doing
> this.
> 
>> The if you find a virtual engine in the list (
>> I915_ENGINE_CLASS_INVALID/I915_ENGINE_CLASS_INVALID_VIRTUAL) you could set
>> the name to "virtual" or something.
> 
> do we really need a name of the type "virtual-<engine>"?

Probably not. Plain "virtual" sounds ok.

> 
>> Now listen to this.. how about we export the engine names via the query API?
>> Primarily I was thinking to distinguish difference instance of virtual, but
>> then it would also lessen the reliance on the static map. Thoughts?
> 
> Do you mean that the name would be provided by the driver?
> 
> Other than improving the debug information, is the name
> formatting giving any advantage if we can distinguish by
> class/instance/flags?
> 
> We can't use it anyway for test creation.

Yeah, just need to skip it during for_each_physical_engine.

> 
> [...]
> 
>>> +		uint8_t nengines = (param.size -
>>> +				sizeof(struct i915_context_param_engines)) /
>>> +				sizeof(engines->class_instance[0]);
>>
>> I'd probably just use unsigned int.
> 
> Oh... I have set u32 in the intel_engine_data, I didn't reliase,
> I assume that nengines would never be higher than 64 (if that
> happens we can't handle it here).
> 
> But Chris is considering the case we will have more tha 64
> engines, I can set it to u32, of course.
> 
> [...]
> 
>>> +#define for_each_engine_class_instance(fd__, ctx__, e__) \
>>> +	for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
>>> +		((e__) = (i__.n < i__.nengines) ? &i__.engines[i__.n] : NULL); \
>>> +			i__.n++)
>>
>> Do we want a context parameter in this helper, or even this helper at all? I
>> thought we can end up with only two, for_each_physical_engine and
>> for_each_context_engine - but I guess it is open for discussion.
> 
> I don't know of possible use cases that do or don't need ctx
> outside the for_each...().
> 
> If you don't see any use of the context index outside the
> for_each I can create the context inside the init_list function.
> 
> But, I have a little concern about the destraction of that
> context. If the for_each... gets interrupted in the middle of the
> loop, we lose the context.

I am not following how we lose the context?

I was just discussing of our desired end game in therms of number and 
signature for for_each_.. iterators.

For me for_each_physical_engine doesn't need the context since it is 
about physical engines - not engine from the engine map. Maybe that one 
should even have some asserts then to make sure someone hasn't 
re-configured the default context.

And we have __for_each_physical_engine which uses the static table, for 
subtest enumeration.

Then for_each_context_engine is the fully featured one, which has the 
context id in parameters.

Can we solve all use cases with those three or we need mode?

Chris?

> 
> [...]
> 
>>> @@ -434,7 +434,7 @@ busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
>>>    	i = 0;
>>>    	fd[0] = -1;
>>> -	for_each_engine_class_instance(gem_fd, e_) {
>>> +	for_each_engine_class_instance(gem_fd, 0, e_) {
>>>    		if (e == e_)
>>>    			busy_idx = i;
>>> @@ -497,7 +497,7 @@ most_busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
>>>    	unsigned int idle_idx, i;
>>>    	i = 0;
>>> -	for_each_engine_class_instance(gem_fd, e_) {
>>> +	for_each_engine_class_instance(gem_fd, 0, e_) {
>>>    		if (e == e_)
>>>    			idle_idx = i;
>>>    		else if (spin)
>>> @@ -554,7 +554,7 @@ all_busy_check_all(int gem_fd, const unsigned int num_engines,
>>>    	unsigned int i;
>>>    	i = 0;
>>> -	for_each_engine_class_instance(gem_fd, e) {
>>> +	for_each_engine_class_instance(gem_fd, 0, e) {
>>>    		if (spin)
>>>    			__submit_spin_batch(gem_fd, spin, e, 64);
>>>    		else
>>> @@ -1683,7 +1683,7 @@ igt_main
>>>    		igt_require_gem(fd);
>>>    		igt_require(i915_type_id() > 0);
>>> -		for_each_engine_class_instance(fd, e)
>>> +		for_each_engine_class_instance(fd, 0, e)
>>>    			num_engines++;
>>>    	}
>>>
>>
>> Looks like this would work. Just the question of virtual engine, set of
>> chosen iterators, and maybe some nits.
> 
> Yes, as we discussed, right after this patchset I will do the
> for_each_physical.
> 
> What are the nits? I love nits :)

Just things like types and coding style. :) And some more details from 
Chris' review.

Regards,

Tvrtko