[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
Headers show
Series "new engine discovery interface" ( rev: 3 ) in IGT

Not browsing as part of any series.

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

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