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

[Andi Shyti]

Hi Chris,

thanks for looking into this!

> > @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >  
> >         fd = __search_and_open(base, offset, chipset);
> >         if (fd != -1)
> > -               return fd;
> > +               goto set_engines_and_return;
> >  
> >         pthread_mutex_lock(&mutex);
> >         for (const struct module *m = modules; m->module; m++) {
> > @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >         }
> >         pthread_mutex_unlock(&mutex);
> >  
> > -       return __search_and_open(base, offset, chipset);
> > +       fd = __search_and_open(base, offset, chipset);
> > +       if (fd < 0)
> > +               return fd;
> > +
> > +set_engines_and_return:
> > +       if (is_i915_device(fd))
> > +               gem_init_engine_list(fd);
> 
> Do we really want more implicit actions on opening an fd?
> 
> We already have igt_require_gem() which would make an interesting
> starting point, for that we may want to use fd not from
> drm_open_driver(). However, there seems to be no issue with creating the
> names on the fly (and ask for the complementary getter for engines[] so
> that an index could be translated back to class:instance).
> 
> Certainly having drmtest presume GEM (and be subject to all of the extra
> rules) given i915 seems a bit rude.

OK, I can try it out in igt_require_gem().

> > +       /*
> > +        * The first ioctl is sent with item.length = 0
> > +        * which asks to the driver to store in length the
> > +        * memory needed for the engines. In the driver, length
> > +        * is equal to
> > +        *
> > +        *   len = sizeof(struct drm_i915_query_engine_info) +
> > +        *                   INTEL_INFO(i915)->num_rings *
> > +        *                   sizeof(struct drm_i915_engine_info);
> 
> Nah, do not imply you are tied to implementation details - that is the
> whole point of querying the length first. Do note that you can over
> allocate (say use a small bit of stack) and do the query in one shot,
> only allocating from heap if we need more room.

Sure!

> > +        */
> > +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> 
> Bad news for old kernels.

No bit of this patch would work on old kernels. I thought this
was the reason to create a second for_each_physical_engine2
(which I called for_each_engine_ctx()).

Maybe in a next patch I can think of reworking things a bit (in
the igt_gt.h) so that we choose the right "for_each..." based on
the kernel's API.

> > +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> > +{
> > +       int i;
> > +       unsigned char n;
> > +       struct drm_i915_gem_context_param ctx_param;
> > +       struct i915_context_param_engines *ctx_engine;
> > +       size_t size;
> > +
> > +       for (n = 0; intel_execution_engines2[n].name; n++);
> 
> Close your eyes and tell me where the ';' is.

:)

> > +       size = sizeof(struct i915_context_param_engines) +
> > +                     (n + 1) * sizeof(*ctx_engine->class_instance);
> 
> Should be small enough for a stack allocations of say 64 engines (the
> limit of the current execbuf uabi).

I will over-allocate here as well, so that I remove the ';' that
you don't like.

> > +       igt_assert((ctx_engine = malloc(size)));
> > +
> > +       ctx_engine->extensions = 0;
> > +       for (i = 0; i <= n; i++) {
> > +               ctx_engine->class_instance[i].class =
> > +                                       intel_execution_engines2[i].class;
> > +               ctx_engine->class_instance[i].instance =
> > +                                       intel_execution_engines2[i].instance;
> > +       }
> > +
> > +       ctx_param.ctx_id = ctx_id;
> > +       ctx_param.size = size;
> > +       ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> > +       ctx_param.value = to_user_pointer(ctx_engine);
> > +
> > +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> 
> Do you really want to risk that this won't be interrupted by a signal at
> any point in the future?

Sorry, I don't understand, everything can be interrupted by
signals. What are you suggesting?

Thanks,
Andi