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

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

On 22/03/2019 09:51, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> +static void init_engine(struct intel_execution_engine2 *e2, const char *name,
>>> +			uint16_t class, uint16_t instance, uint64_t flags)
>>
>> You are keeping name for the future? (It is unused at the moment.)
> 
> I simply forgot to remove it :)
> 
>>> +{
>>> +	static const char *unknown_name = "unknown",
>>> +			  *virtual_name = "virtual";
>>
>> Unusual style but it is actually readable so I think I like it.
> 
> like the function below, it doesn't happen everyday to declare
> variables/functions where the longest part of the line is the
> type.
> 
> Need to come out with something :)
> 
>>> +
>>> +	e2->class    = class;
>>> +	e2->instance = instance;
>>> +	e2->flags    = flags;
>>> +
>>> +	if (class < 0 && instance < 0) {
>>> +		e2->name = virtual_name;
>>> +	} else {
>>> +		const struct intel_execution_engine2 *__e2;
>>> +
>>> +		__for_each_static_engine(__e2)
>>> +			if (__e2->class == class && __e2->instance == instance)
>>> +				break;
>>> +
>>> +		e2->name = __e2->name ? __e2->name : unknown_name;
>>
>> I've now started to worry about how will CI/buglog handle us forgetting to
>> expand the static list. (More than one subtest of a same name for
>> "test-$engine_name" ones?) Do we want and igt_warn on unknown engines to
>> make it more visible? Or even just crash?
> 
> Right! I guess just a warning would be nice, we can gather more
> information from the logs about unkown engines... I guess.
> 
>>> +	if (nengines > I915_EXEC_RING_MASK + 1) {
>>> +		engine_data.error = ret ? ret : -EINVAL;
>>> +		return engine_data;
>>> +	}
>>
>> If we one day allow more engines in the map than the current limit?
> 
> for now this is a driver limitation and that's what igt checks. I
> guess the right approach would be to update igt according to the
> driver, right?

Yes, I think old IGT on newer kernels is not something we need to 
support. Old kernel with new IGT we sometimes, like in this series do.

>> It looks this would make the iterator not work. Was that the intention? What
>> is the point of continuing then rather than just asserting?
> 
> yes, the iterator wouldn't loop and would provide a 0 size list
> of engines. Asserting makes more sense.

I think so, or igt_warn. But there is also the query which will fail if 
this limit is surpassed, due limited passed in buffer size.

Maybe assert in the query on -EINVAL and assert here instead of the 
error complication?

>>> +struct intel_execution_engine2
>>> +	*intel_get_current_engine(struct intel_engine_data *ed)
>>
>> Unusual coding style, we use:
>>
>> type
>> func(params)
>>
>> or:
>>
>> type func(params)
> 
> yes, I've seen it around, I personally don't like it, but I will
> do it as the style is.

Yep, have to comply with our style.

>>> +	return (ed->n < ed->nengines) && !ed->error ?
>>> +		&ed->engines[ed->n] :
>>> +		NULL;
>>
>> So could store the pointer to current engine in the iterator?
> 
> I still don't see the use of it, but of course, I can add it. It
> might make more sense now that I have a "_current" and a "_next"
> function.

That exactly why I mentioned it again, but it is of a minor importance 
as long as we are going with the explicit engine pointer in the for loop 
iterator. Do as you prefer.

>>> @@ -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_context_engine(gem_fd, 0, e_) {
>>
>> Make perf_pmu use for_each_physical_engine, apart from the test enumeration.
> 
> the reason I didn't replace it right away is that still to many
> functions are using the current implementation of
> for_each_physical_engine and I wanted to avoid, in this patchset,
> touching too many files. I'm already changing more than I
> wished.

It doesn't seem like a lot to me. And you have to change something so we 
know it works. :)

> that's why I called it:
> 
>     #define __for_each_physical_engine__(fd__, e__)
> 
> at the beginning I wanted to call it
> 
>     #define __DO_NOT_USE_for_each_physical_engine_YET__(fd__, e__) \
> 
> Besides, none of all those functions is using the new e2
> structure.
> 
> If it's OK with you, I would swap everything to use either
> __for_each_static_engine or the new for_each_physical_engine
> right after this patch goes in.

Oh I missed it is __for_each_physical_engine in this patch.

I don't like the intermediate step of perf_pmu using the context 
iterator. Make it use __for_each_physical_engine then for now? It's just 
a trivial find and replace.

Regards,

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