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

[Andi Shyti]

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

Your first suggestion is somehow cleaner :)

> > > > +	} 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!

I will report when I find something, but in this case I had some
patches on top and in the middle that were messing things up, a
clean install fixed it :)

Andi