[igt-dev] [PATCH v15 4/5] lib/i915: add gem_engine_topology library and for_each loop definition
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 22 10:10:33 UTC 2019
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
More information about the igt-dev
mailing list