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

[Tvrtko Ursulin]

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);
> 
>