Re: [igt-dev] [PATCH v21 5/6] lib: igt_dummyload: use for_each_context_engine()

Tvrtko Ursulin
2019-04-17T16:14:55Z

On 17/04/2019 16:42, Daniele Ceraolo Spurio wrote:
> <snip>
>
>> @@ -94,17 +95,17 @@ emit_recursive_batch(igt_spin_t *spin,
>>       nengine = 0;
>>       if (opts->engine == ALL_ENGINES) {
>> -        unsigned int engine;
>> +        struct intel_execution_engine2 *engine;
>> -        for_each_physical_engine(fd, engine) {
>> +        for_each_context_engine(fd, opts->ctx, engine) {
>
> On a kernel that has the new I915_CONTEXT_PARAM_ENGINES, wouldn't this
> implicitly update opts->ctx to use it (via the ctx_map_engines in
> intel_init_engine_list)? What if the caller then tries to submit with
> that ctx using an execbuf flag?only an issue until all the callers are
> updated I guess.

I think you are right. And I think it is not only until all tests are
updated that we need dual paths but maybe even longer.

Something like:

if (ctx_has_map(ctx)) {
for_each_context_engine()
...
} else {
for_each_physical_engine
...
}

Problem though is the end game of replacing for_each_physical_engine
with the new implementation. It is okay to have for_each_physical_engine
configure the context when called directly in a test, but a bit less
okay if done deep down in some function like this one.

So what to do to preserve compatibility with eb.flags and not mess up
the context in here.. use the static iterator for contexts wo/ maps and
legacy eb? But what does ALL_ENGINES mean then? Phase out ALL_ENGINES
support in spin batch? Needs an audit of how many call sites we have and
how do they look.

Regards,

Tvrtko

>
> Thanks,
> Daniele
>
>>               if (opts->flags & IGT_SPIN_POLL_RUN &&
>> -                !gem_can_store_dword(fd, engine))
>> +                !gem_class_can_store_dword(fd, engine->class))
>>                   continue;
>> -            engines[nengine++] = engine;
>> +            flags[nengine++] = engine->flags;
>>           }
>>       } else {
>> -        engines[nengine++] = opts->engine;
>> +        flags[nengine++] = opts->engine;
>>       }
>>       igt_require(nengine);
>> @@ -234,7 +235,7 @@ emit_recursive_batch(igt_spin_t *spin,
>>       for (i = 0; i < nengine; i++) {
>>           execbuf->flags &= ~ENGINE_MASK;
>> -        execbuf->flags |= engines[i];
>> +        execbuf->flags |= flags[i];
>>           gem_execbuf_wr(fd, execbuf);
>> @@ -309,9 +310,19 @@ igt_spin_batch_factory(int fd, const struct
>> igt_spin_factory *opts)
>>       igt_require_gem(fd);
>>       if (opts->engine != ALL_ENGINES) {
>> -        gem_require_ring(fd, opts->engine);
>> +        struct intel_execution_engine2 e;
>> +        int class;
>> +
>> +        if (!gem_context_lookup_engine(fd, opts->engine,
>> +                           opts->ctx, &e)) {
>> +            class = e.class;
>> +        } else {
>> +            gem_require_ring(fd, opts->engine);
>> +            class = gem_execbuf_flags_to_engine_class(opts->engine);
>> +        }
>> +
>>           if (opts->flags & IGT_SPIN_POLL_RUN)
>> -            igt_require(gem_can_store_dword(fd, opts->engine));
>> +            igt_require(gem_class_can_store_dword(fd, class));
>>       }
>>       spin = spin_batch_create(fd, opts);
>
>