[igt-dev] [PATCH v13 6/9] lib/i915: add gem_engine_topology library

[Tvrtko Ursulin]

On 20/03/2019 10:49, Andi Shyti wrote:
>>> +{
>>> +	const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
>>> +	char *__name;
>>> +
>>> +	/* if we don't recognise the class, then we mark it as "unk" */
>>> +	if (name) {
>>> +		e2->name = name;
>>> +	} else {
>>> +		if (class >= ARRAY_SIZE(class_names))
>>> +			igt_assert(asprintf(&__name, "unk-%d:%d",
>>> +					    class, instance) > 0);
>>> +		else
>>> +			igt_assert(asprintf(&__name, "%s%d",
>>> +					    class_names[class], instance) > 0);
>>> +
>>> +		e2->name = __name;
>>
>> We discussed on the IRC how names can be borrowed from the static engine
>> table, which we have to keep maintaining due CI limitations anyway. That way
>> all leaks are solved.
>>
>> So you would just replace the above with a name lookup against the static
>> table and use that pointer.
> 
> Yes, this is the tax we need to pay for having counts name inside
> intel_execution_engine2.
> 
> I can take the pointer from our intel_exectuion_engines2 array,
> and then we wouldn't have "unk<class>:<instance>".
> 
> As I said the best thing here would be to remove the "const" and
> be able to use all the string functions.

Or, as long as we are depending on the static array being a superset, 
you could store a list of pointers to static array entries in the 
dynamic engine_data list, instead of copying over elements?

>>> +	} else if (cparam.size == sizeof(struct i915_context_param_engines)) {
>>
>> This should be "== 0" since Chris has special plans for the other.
>>
>>> +		/* else if context doesn't have mapped engines */
>>> +		query_engine_list(&engine_data);
>>> +		map_engine_context(&engine_data, &cparam);
>>> +
>>> +	} else {
>>> +		/* context has a list of mapped engines */
>>> +
>>> +		uint8_t nengines = (cparam.size -
>>> +				sizeof(struct i915_context_param_engines)) /
>>> +				sizeof(cengine->class_instance[0]);
>>> +
>>> +		for (i = 0; i < nengines - 1; i++)
>>
>> nengines - 1 is not right I think, just nengines.
>>
>>> +			dup_engine(&engine_data.engines[i], NULL,
>>> +				   cengine->class_instance[i].engine_class,
>>> +				   cengine->class_instance[i].engine_instance,
>>> +				   i + 1);
>>
>> Just "i" since index zero is not special any longer.
> 
> I was running on a messed up kernel that and was receiving weird
> values :/

If you are using my media branch and it is broken in some respect please 
report the details!

Regards,

Tvrtko