[v3,2/3] lib: implement new engine discovery interface

Submitted by Andi Shyti on Nov. 26, 2018, 8:43 p.m.

Details

Message ID 20181126204349.6739-3-andi.shyti@intel.com
State New
Series "new engine discovery interface"
Headers show

Commit Message

Andi Shyti Nov. 26, 2018, 8:43 p.m.
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(+)

Patch hide | download patch | download mbox

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

Comments

Tvrtko Ursulin Nov. 27, 2018, noon
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
Andi Shyti Nov. 27, 2018, 8:33 p.m.
Hi Tvrtko,

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

The comment is a leftover from the cleanup. I need to fix/remove
it.

While as for freeing ctx_engine in case of failure, some ugly code
would be required. I think it's cleaner to leave it as it is,
anyway 'igt_require' exits in case of failure.

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

I cannot mix declarations and "already declared variables"
initializations in the first expression in 'for'. If I do
something like:

  for (int __m = __gem_setup_ctx_engines(), e = 1; ... )

I would re-define 'e' and it would be a different variable
outside the loop.

So that either I declare '__m' outside, or I initialize 'e'
outside, or I use the double 'for' loop as it was done
previously, or do some ugly tricks.

It looked simplier to define an '__e'.

Am I missing anything?

> >   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.)

All right, I'll try it out and I will post something as a reply to this
patch.

Thanks a lot, Tvrtko!
Andi
Tvrtko Ursulin Nov. 28, 2018, 8:29 a.m.
On 27/11/2018 20:33, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> +	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.
> 
> The comment is a leftover from the cleanup. I need to fix/remove
> it.
> 
> While as for freeing ctx_engine in case of failure, some ugly code
> would be required. I think it's cleaner to leave it as it is,
> anyway 'igt_require' exits in case of failure.

Yeah, ok.

>>> +#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?
> 
> I cannot mix declarations and "already declared variables"
> initializations in the first expression in 'for'. If I do
> something like:
> 
>    for (int __m = __gem_setup_ctx_engines(), e = 1; ... )
> 
> I would re-define 'e' and it would be a different variable
> outside the loop.
> 
> So that either I declare '__m' outside, or I initialize 'e'
> outside, or I use the double 'for' loop as it was done
> previously, or do some ugly tricks.
> 
> It looked simplier to define an '__e'.
> 
> Am I missing anything?

No, looks like you're right, makes sense.

>>>    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.)
> 
> All right, I'll try it out and I will post something as a reply to this
> patch.

Ack.

Regards,

Tvrtko