Re: [igt-dev] [PATCH] lib/i915: gem_engine_topology: get eb flags from engine's coordiantes

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

On 20/06/2019 08:52, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> +	struct intel_execution_engine2 *e;
>>> +
>>> +	for_each_context_engine(fd, ctx_id, e)
>>> +		if (class == e->class && instance == e->instance)
>>> +			return e->flags;
>>
>> And most important difference, I wouldn't configure the context from this helper. Instead I think all we need is:
>>
>> int gem_context_get_eb_flags(int fd, uint32_t ctx, struct i915_engine_class_instance ci)
>> {
>> 	DEFINE_CONTEXT_ENGINES_PARAM(engines, param, ctx, GEM_MAX_ENGINES);
>> 	int ret
>>
>> 	ret = gem_topology_get_param(fd, &param);
>> 	if (ret) {
>> 		const struct intel_execution_engine2 *e;
>>
>> 		/* Legacy kernels. */
>> 		__for_each_static_engine(e) {
>> 			if (e->class == ci.engine_class &&
>> 			    e->instance == ci.engine_instance)
>> 				return e->flags;
>> 		}
>>
>> 		return -EINVAL;
>> 	}
>>
>> 	/* Engine map with no engines. */
>> 	if (!param.size)
>> 		return -EINVAL;
> 
> this means that tests have responsibility to alway create a
> context and do the mapping before (we haven't always assumed it
> in other tests).

This particular check? This should be map with zero engines, a very 
special case we currently do not configure in scope of engine topology code.

>> 	/* Engine map lookup. */
>> 	for (unsigned int i = 0; i < param.size; i++) {
>> 		if (engines.engines[i].engine_class == ci.engine_class &&
>> 		    engines.engines[i].engine_instance == ci.engine_instance)
>> 			return i;
>> 	}
>>
>> 	return -EINVAL;
>> }
> 
> This version, a the end, becomes a slim version of
> 'intel_init_engine_list', with the only difference that it
> wouldn't do any mapping (which I agree would be too much but we
> did it in other tests), that's why, at the end I preferred the
> short version I sent.
> 
> OK, we can definitely do it this way, too

I think it would be wrong to configure the context from this helper. The 
name of the function translates to "give me correct eb.flags for this 
context to submit to this class:instance". All function needs to do is 
to answer this question both on legacy and new kernels.

Also, it is okay to refactor the library code if there is enough 
commonality for extracting or something.

Regards,

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