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

[Tvrtko Ursulin]

On 19/03/2019 10:44, Andi Shyti wrote:
> 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?

Yes, context may be configured to a subset of physical engines. That's 
Chris' use case of preparing his own context and then being able to 
iterate over it.

>>> +	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.

If you have explicit ask from Chris on how to do it, then follow his 
idea. That's usually the quickest way upstream. :)

> 
>>> +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.

I guess it depends on whether pointer to current engine is a parameter 
in the for loop iterator and tests have to declare it, or we allow tests 
to not bother and use the implicit iterator object to access it.

For me it is 50-50 which way we go.

Regards,

Tvrtko