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

[Tvrtko Ursulin]

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);
   	}
   }

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);
		}
	}			

Regards,

Tvrtko

> The new engine discovery macro requires, though, to first bind a
> context to the engines and the unbind it:
> 
> Here's an example of usage:
> 
>    int main(void)
>    {
>       int fd;
>       uint32_t n, ctx_id, e;
> 
>       fd = drm_open_driver(DRIVER_INTEL);
> 
>       if(bind_ctx_to_engine(fd, &n, &ctx_id))
>          goto out;
> 
>       for_each_engine_ctx(fd, n, ctx_id, e)
>          printf("%d OK:\n", e+1);
> 
>       unbind_ctx_from_engine(fd, ctx_id);
> 
>    out:
>       close(fd);
> 
>       return 0;
>    }
> 
> [*] git://people.freedesktop.org/~tursulin/drm-intel
> 
> Signed-off-by: Andi Shyti <andi.shyti at intel.com>
> ---
>   lib/igt_gt.c         | 89 ++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_gt.h         |  6 +++
>   lib/ioctl_wrappers.c | 25 +++++++++++++
>   lib/ioctl_wrappers.h |  1 +
>   4 files changed, 121 insertions(+)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a2061924..bafe1f86 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -650,3 +650,92 @@ 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;
> +	ssize_t size = sizeof(*query_engines) + 10 * sizeof(*query_engines->engines);
> +	int err;
> +
> +	query_engines = malloc(size);
> +	if (!query_engines)
> +		return NULL;
> +
> +	memset(&query, 0, sizeof(query));
> +	memset(&item, 0, sizeof(item));
> +	memset(query_engines, 0, sizeof(*query_engines));
> +
> +	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> +	item.length = size;
> +	item.flags = 0;
> +	item.data_ptr = to_user_pointer(query_engines);
> +
> +	query.items_ptr = to_user_pointer(&item);
> +	query.num_items = 1;
> +
> +	err = igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query);
> +	if (err) {
> +		free(query_engines);
> +		return NULL;
> +	}
> +
> +	return query_engines;
> +}
> +
> +static int __bind_ctx_to_engine(int fd, unsigned ctx_id,
> +		struct drm_i915_query_engine_info *query_engine)
> +{
> +	int i, ret;
> +	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);
> +
> +	ctx_engine = malloc(size);
> +	if (!ctx_engine)
> +		return errno;
> +
> +	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);
> +
> +	ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param);
> +	free(ctx_engine);
> +
> +	return ret;
> +}
> +
> +int bind_ctx_to_engine(int fd, uint32_t *n, uint32_t *ctx_id)
> +{
> +	struct drm_i915_query_engine_info *query_engine;
> +	int ret;
> +
> +	query_engine = __query_engines(fd);
> +	if (!query_engine)
> +		return -1;
> +
> +	*ctx_id = gem_context_create(fd);
> +	ret = __bind_ctx_to_engine(fd, *ctx_id, query_engine);
> +	if (ret)
> +		return ret;
> +
> +	*n = query_engine->num_engines;
> +	free(query_engine);
> +
> +	return 0;
> +}
> +
> +void unbind_ctx_from_engine(int fd, uint32_t ctx_id)
> +{
> +	gem_context_destroy(fd, ctx_id);
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da9..d8937318 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,8 +86,14 @@ 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, n, id, e) \
> +	for (e = 0; e < n; e++) \
> +		for_if(gem_has_ring_ctx(fd, e + 1, id))
> +
>   bool gem_ring_is_physical_engine(int fd, unsigned int ring);
>   bool gem_ring_has_physical_engine(int fd, unsigned int ring);
> +int bind_ctx_to_engine(int fd, uint32_t *n, uint32_t *ctx_id);
> +void unbind_ctx_from_engine(int fd, uint32_t ctx_id);
>   
>   bool gem_can_store_dword(int fd, unsigned int engine);
>   
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..2115bad1 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1472,6 +1472,31 @@ bool gem_has_ring(int fd, unsigned ring)
>   	return __gem_execbuf(fd, &execbuf) == -ENOENT;
>   }
>   
> +bool gem_has_ring_ctx(int fd, unsigned ring, unsigned ctx_id)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec;
> +	uint32_t bbend = MI_BATCH_BUFFER_END;
> +	int ret;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	memset(&exec, 0, sizeof(exec));
> +
> +	exec.handle = gem_create(fd, 4096);
> +	gem_write(fd, exec.handle, 0, &bbend, sizeof(bbend));
> +
> +	execbuf.buffers_ptr = to_user_pointer(&exec);
> +	execbuf.buffer_count = 1;
> +	execbuf.flags = ring;
> +	i915_execbuffer2_set_context_id(execbuf, ctx_id);
> +
> +	ret = __gem_execbuf(fd, &execbuf);
> +
> +	gem_close(fd, exec.handle);
> +
> +	return !ret;
> +}
> +
>   /**
>    * gem_require_ring:
>    * @fd: open i915 drm file descriptor
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b22b36b0..d4adf912 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -165,6 +165,7 @@ bool gem_has_exec_fence(int fd);
>   /* check functions which auto-skip tests by calling igt_skip() */
>   void gem_require_caching(int fd);
>   bool gem_has_ring(int fd, unsigned ring);
> +bool gem_has_ring_ctx(int fd, unsigned ring, unsigned ctx_id);
>   void gem_require_ring(int fd, unsigned ring);
>   bool gem_has_mocs_registers(int fd);
>   void gem_require_mocs_registers(int fd);
>