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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-11-20 10:00:25)
> 
> On 19/11/2018 20:38, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-11-19 19:59:19)
> >>
> >> On 19/11/2018 15:55, Andi Shyti wrote:
> >>> 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.
> >>
> >> 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);
> >>          }
> >>     }
> > 
> > Granted that we have a lot of tests that just use the default ctx, I
> > don't think the iterator interface we create should enforce that.
> > 
> > for_each_physical_engine_new(fd, ctx, engine) {
> >       execbuf.rsvd1 = ctx;
> >       execbuf.flags = engine;
> >       gem_execbuf(fd, execbuf);
> > }
> 
> To be clear, you think we should convert all such tests to use a 
> non-default context? My idea was to minimize the churn, but I am also 
> okay with this plan.

No, perfectly fine to use ctx=0 and even to have the common iterator
default to ctx=0. My opinion is that we don't restrict the iterator to
only work on a pre-defined context, but accept that using the iterator
will adjust the ctx->engines[] to suit (and so allow the iterator to
work on any context).

> > where ctx is provided, and engine the iterator. Off the top of my head,
> > I have a silly idea like
> > 
> > for (int __max_engine__; (__max_engine__ = igt_physical_engine_iter_init(fd, ctx)); )
> >       for (engine = 1; engine <= __max_engine__; engine++)
> > 
> > where igt_physical_engine_iter_init(int fd, uint32_t ctx) {
> >       if (ctx_getparam(fd, ctx, ENGINES).count > 0) {
> >               ctx_setparam(fd, ctx, ENGINES, NULL);
> >               return 0;
> >       }
> > 
> >       // query and set engine array
> >       return count;
> > }
> 
> That works and avoids the global on the face of it. However with engine 
> iterator a simple int, we will probably still need some to enable 
> querying iterated engine properties like class, instance and name for 
> subtest enumeration and similar.

True worst case would be

class_instance_get_name(magic_ctx_getparam(fd, ctx, ENGINES)[idx]);

Passable for the one-off uses, but I guess we want

	ctx_get_engine_names(fd, ctx, name_array, name_count);

Or a
	struct engine_attribute {
		u16 class, instance;
		const char *name;
		... other common details ...
	};
-Chris