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

Tvrtko Ursulin
2018-11-27T12:00:42Z

On 26/11/2018 20:43, 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 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().
>
> [*] git://people.freedesktop.org/~tursulin/drm-intel
>
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
> lib/igt_gt.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> lib/igt_gt.h | 5 ++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a2061924..e5543b27 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -650,3 +650,77 @@ 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);
> + */
> + igt_require(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> +
> + 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_require(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> +
> + return query_engines;
> +}
> +
> +static void set_param(int fd, uint32_t ctx_id,
> + struct drm_i915_query_engine_info *query_engine)
> +{
> + int i;
> + struct drm_i915_gem_context_param ctx_param;
> + struct i915_context_param_engines *ctx_engine;
> + size_t size = sizeof(struct i915_context_param_engines) +
> + query_engine->num_engines *
> + sizeof(*ctx_engine->class_instance);
> +
> + igt_assert((ctx_engine = malloc(size)));
> +
> + ctx_engine->extensions = 0;
> + for (i = 0; i < query_engine->num_engines; i++) {
> + ctx_engine->class_instance[i].class = query_engine->engines[i].class;
> + ctx_engine->class_instance[i].instance = query_engine->engines[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);
> +
> + /* check whether we free the engines */
> + igt_require(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> +
> + free(ctx_engine);

Leaks on skip, not that it matters a lot, but the comments makes me
think you wanted to handle it.

> +}
> +
> +int __gem_setup_ctx_engines(int fd, uint32_t ctx_id)
> +{
> + struct drm_i915_query_engine_info *query_engine;
> + int n;
> +
> + query_engine = query_engines(fd);
> + set_param(fd, ctx_id, query_engine);
> +
> + n = query_engine->num_engines;
> + free(query_engine);
> +
> + return n;
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da9..512f958d 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,8 +86,13 @@ extern const struct intel_execution_engine {
> e__++) \
> for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
>
> +#define for_each_engine_ctx(fd, ctx, e) \
> + for (int __m = __gem_setup_ctx_engines(fd, ctx), __e = e = 1; \
> + __e <= __m; e = ++__e)
> +

Is __e needed? Could just use passed in e?

> bool gem_ring_is_physical_engine(int fd, unsigned int ring);
> bool gem_ring_has_physical_engine(int fd, unsigned int ring);
> +int __gem_setup_ctx_engines(int fd, uint32_t ctx_id);
>
> bool gem_can_store_dword(int fd, unsigned int engine);
>
>

Looks okay. But we need to decide whether we want the iterator to be a
struct sooner rather than later now.

I think if you go and convert one of the tests which uses
for_each_physical_engine to enumerate subtests and so, it will become
clearer what approach work better. (struct iterator, or helpers to get
data from engine index.)

Regards,

Tvrtko