Re: [igt-dev] [RFC 2/2] lib: implement new engine discovery interface

[Andi Shyti <andi@etezian.org>]

Hi Tvrtko,

thanks for your inputs!

> > Kernel commits:
> > 
> > [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> > [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> > 
> > from [*] repository, implement a new way uapi for engine
> > discovery that consist in first coupling context with engine [2]
> > and then query the engines through a specific structure [1].
> > 
> > In igt the classic way for discovering engines is done trhough
> > the for_each_physical_engine() macro, that is replaced by the
> > new for_each_engine_ctx().
> 
> My idea was that we migrate for_each_physical_engine to this new scheme.
> 
> As an easy starting point I was thinking to keep the current
> for_each_physical_engine as is, and just add new
> for_each_physical_engine_new and migrate one test to it as a demo.
> 
> Then when this part is reviewed, as a second step we convert the rest and
> rename the macro stripping the _new suffix and nuking the old one.

The for_each_physical_engine is left as it is.

I have indeed posted only the changes in the libraries, I can
send a demo test as well.

> With regards to implementation details I was thinking along these lines:
> 
> On first invocation for_each_physical_engine_new initializes some hidden
> data stored in one or more globals (which will live in igt_gt.c).
> 
> This would query the engines and create a context with all engines mapped.
> 
> We also add a helper to get this internal ctx id to use within
> for_each_physical_engine_new loops.
> 
> That should make it easy to convert simple tests over like:
> 
>   igt_subtest {
>   	for_each_physical_engine_new(fd, engine) {
>   		...
>   		execbuf.rsvd1 = gem_physical_engine_ctx();
>   		execbuf.flags = gem_physical_engine_idx(engine);
>   		gem_execbuf(fd, execbuf);
>   	}
>   }
> 
> We also need to think about what kind of helpers would be needed for tests
> which use for_each_physical_engine and their own contexts. If such exist,
> and I haven't checked, a helper like
> gem_setup_context_for_each_physical_engine or something?
> 
> eg:
> 
> 	igt_subtest {
> 		unit32_t test_ctx;
> 
> 		...
> 		gem_init_context_for_each_physical_engine(fd, test_ctx);
> 
> 		for_each_physical_engine_new(fd, engine) {
> 	  		...
> 	  		execbuf.rsvd1 = test_ctx;
> 	  		execbuf.flags = gem_physical_engine_idx(engine);
> 	  		gem_execbuf(fd, execbuf);
> 		}
> 	}			

I tried to keep the "for_each_physical_engine_new" (which I
called "for_each_engine_ctx" because it looks more meaningful)
similar to what it was by adding a few arguments more and a
"bind" and an "unbind" functions before and after in order to
create and remove the structures and contexts I am creating.

I think it's not that different from what you are suggesting and
what we discussed offline. But perhaps a demo test would make
it more clear.

Thanks again,
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev