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

[Chris Wilson]

Quoting Andi Shyti (2019-01-15 12:35:10)
> 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 uapi for engine discovery
> that consist in first querying the driver about the engines in
> the gpu [1] and then binding a context to the set of engines that
> it can access [2].
> 
> In igt the classic way for discovering engines is done through
> the for_each_physical_engine() macro, that would be replaced by
> the new for_each_engine_ctx().
> 
> A new function is added gem_init_engine_list() that is called
> during device open which creates the list of engines. That list
> is stored in the intel_execution_engines2 that replaces the
> current array which has more a reference meaning. Now the
> intel_execution_engines2 stores the engines currently present in
> the GPU.
> 
> [*] git://people.freedesktop.org/~tursulin/drm-intel
> 
> Signed-off-by: Andi Shyti <andi.shyti at intel.com>
> ---
>  lib/drmtest.c | 12 +++++--
>  lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  lib/igt_gt.h  | 10 +++++-
>  3 files changed, 110 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 7c124ac666ec..2d155eea8a13 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -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.

> +
> +       return fd;
>  }
>  
>  /**
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a20619246296..ab73d4b45087 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -577,14 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>         return true;
>  }
>  
> -const struct intel_execution_engine2 intel_execution_engines2[] = {
> -       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> -       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> -       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> -       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> -       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> -       { }
> -};
> +struct intel_execution_engine2 intel_execution_engines2[32] = { };
>  
>  unsigned int
>  gem_class_instance_to_eb_flags(int gem_fd,
> @@ -650,3 +643,93 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>  
>         return gem_has_ring(fd, ring);
>  }
> +
> +static struct drm_i915_query_engine_info *query_engines(int fd)
> +{
> +       struct drm_i915_query query = { };
> +       struct drm_i915_query_item item = { };
> +       struct drm_i915_query_engine_info *query_engines;
> +
> +       item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> +       query.items_ptr = to_user_pointer(&item);
> +       query.num_items = 1;
> +
> +       /*
> +        * 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.

> +        */
> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

> +       igt_assert((query_engines = calloc(item.length, 1)));
> +       item.data_ptr = to_user_pointer(query_engines);
> +
> +       /* The second ioctl stores the engines in query_engines */
> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> +
> +       return query_engines;
> +}
> +
> +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).

> +       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?

> +
> +       free(ctx_engine);
> +}