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

Submitted by Andi Shyti on Nov. 21, 2018, 4:10 p.m.

Details

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

Commit Message

Andi Shyti Nov. 21, 2018, 4:10 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 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 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_gt.h |  8 +++++
 2 files changed, 91 insertions(+)

Patch hide | download patch | download mbox

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a2061924..4a7b8671 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -650,3 +650,86 @@  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 __set_param(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 get_engines(int fd, uint32_t ctx_id)
+{
+	struct drm_i915_query_engine_info *query_engine;
+	int n;
+
+	query_engine = __query_engines(fd);
+	if (!query_engine)
+		return -1;
+
+	n = __set_param(fd, ctx_id, query_engine);
+	if (n < 0)
+		return n;
+
+	n = query_engine->num_engines;
+	free(query_engine);
+
+	return n;
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 54e95da9..7e299c31 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -86,8 +86,16 @@  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, __e = 1; (__m = get_engines(fd, ctx)); ) \
+			if (__e > __m) \
+				break; \
+			else \
+				for(e = __e ; __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 get_engines(int fd, uint32_t ctx_id);
 
 bool gem_can_store_dword(int fd, unsigned int engine);
 

Comments

Tvrtko Ursulin Nov. 22, 2018, 12:14 p.m.
On 21/11/2018 16:10, 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

typo in 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_gt.h |  8 +++++
>   2 files changed, 91 insertions(+)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a2061924..4a7b8671 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -650,3 +650,86 @@ 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);

You'll have to change this in the final version to dynamically probe the 
amount of space needed.

If you send one DRM_IOCTL_I915_QUERY with the length set to zero it will 
return you the amount of space required to return everything.

> +	int err;
> +
> +	query_engines = malloc(size);
> +	if (!query_engines)
> +		return NULL;

We probably want to igt_assert on this since there is no point going 
further.

> +
> +	memset(&query, 0, sizeof(query));
> +	memset(&item, 0, sizeof(item));

For these two you can save some lines by adding a static initializer "= 
{ };" to the declaration, but do as you prefer.

> +	memset(query_engines, 0, sizeof(*query_engines));
> +
> +	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> +	item.length = size;
> +	item.flags = 0;

Can skip since it is already zero.

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

I am not sure about this one, but am thinking that here we should 
probably use ioctl(3) directly. I don't think we need the auto-retry on 
EINTR and EAGAIN for this one.

> +	if (err) {

Should we perhaps igt_require ioctl works? So new IGT skips on old kernels.

> +		free(query_engines);
> +		return NUL > +	}
> +
> +	return query_engines;
> +}
> +
> +static int __set_param(int fd, unsigned ctx_id,

Use uint32_t for ctx_id for consistency.

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

Again probably better to just assert.

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

Same idea that we should perhaps skip here as with the query.

> +	free(ctx_engine);
> +
> +	return ret;
> +}
> +
> +int get_engines(int fd, uint32_t ctx_id)

setup_ctx_engines?

Does it need to be exported or it can be static?

> +{
> +	struct drm_i915_query_engine_info *query_engine;
> +	int n;
> +
> +	query_engine = __query_engines(fd);
> +	if (!query_engine)
> +		return -1;
> +
> +	n = __set_param(fd, ctx_id, query_engine);
> +	if (n < 0)
> +		return n;
> +
> +	n = query_engine->num_engines;
> +	free(query_engine);
> +
> +	return n;
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da9..7e299c31 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,8 +86,16 @@ 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) \

High level design question: Do we want 'e' to be an integer or a struct 
describing each engine?

> +		for (int __m, __e = 1; (__m = get_engines(fd, ctx)); ) \
> +			if (__e > __m) \
> +				break; \
> +			else \
> +				for(e = __e ; __e <= __m; e = ++__e)

I am struggling to figure this out, but it is probably just me. Can it 
cause some compiler warnings for misleading braces, indent or whatever 
the things are it could warn here?

Would something simpler not work?

#define for_each_engine_ctx(fd, ctx, e) \
    for (int __e = 0, __m = get_engines(fd, ctx);
	(e) = __e, __e < __m;
	__e++)

I am probably missing something elementary...


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

Regards,

Tvrtko
Andi Shyti Nov. 22, 2018, 1:28 p.m.
Hi Tvrtko,

> > +		for (int __m, __e = 1; (__m = get_engines(fd, ctx)); ) \
> > +			if (__e > __m) \
> > +				break; \
> > +			else \
> > +				for(e = __e ; __e <= __m; e = ++__e)
> 
> I am struggling to figure this out, but it is probably just me. Can it cause
> some compiler warnings for misleading braces, indent or whatever the things
> are it could warn here?
> 
> Would something simpler not work?
> 
> #define for_each_engine_ctx(fd, ctx, e) \
>    for (int __e = 0, __m = get_engines(fd, ctx);
> 	(e) = __e, __e < __m;
> 	__e++)
> 
> I am probably missing something elementary...

It doesn't complain, but I already simplified this macro.
I just used Chris's way just for the RFC (also because it ends up
calling calling get_engines() twice).

I agree with all your comments from this and the other RFC. I
will post directly the patch next, with few more improvements.

Important for me was not to go off track with this.

Thanks a lot, Tvrtko,
Andi
Andi Shyti Nov. 26, 2018, 9:07 a.m.
Hi Tvrtko,

just few things that came to my mind while going through your
review.

> > +	query_engines = malloc(size);
> > +	if (!query_engines)
> > +		return NULL;
> 
> We probably want to igt_assert on this since there is no point going
> further.

isn't igt_assert used for the test outcome? What I mean is that
the test outcome would be to query and set the engines, but if we
fail in malloc we do not necessarily fail the test, but we have a
different kind of system failure.

(this is not important, just for me to understand better :) )

> > +int get_engines(int fd, uint32_t ctx_id)
> 
> setup_ctx_engines?
> 
> Does it need to be exported or it can be static?

How do we reach it from outside if we set it static? This
function is called in the for_each_engine_ctx macro that is used
outside from igt_gt.c

> > +#define for_each_engine_ctx(fd, ctx, e) \
> 
> High level design question: Do we want 'e' to be an integer or a struct
> describing each engine?

Do you mean that you would you prefer iterating with a
'i915_context_param_engines' or a 'intel_execution_engine' struct
instead of an 'e' integer? This way it would also be different
from how the current 'for_each_engine' works.

I could do it in a next patch, so that in this one we keep it as
close as possible to the current way of doing things.

Andi
Tvrtko Ursulin Nov. 26, 2018, 9:27 a.m.
On 26/11/2018 09:07, Andi Shyti wrote:
> Hi Tvrtko,
> 
> just few things that came to my mind while going through your
> review.
> 
>>> +	query_engines = malloc(size);
>>> +	if (!query_engines)
>>> +		return NULL;
>>
>> We probably want to igt_assert on this since there is no point going
>> further.
> 
> isn't igt_assert used for the test outcome? What I mean is that
> the test outcome would be to query and set the engines, but if we
> fail in malloc we do not necessarily fail the test, but we have a
> different kind of system failure.
> 
> (this is not important, just for me to understand better :) )

We use it for both since we haven't agreed on any mechanism to 
differentiate. I think it was mentioned a few times but I don't remember 
what were the different opinions.

>>> +int get_engines(int fd, uint32_t ctx_id)
>>
>> setup_ctx_engines?
>>
>> Does it need to be exported or it can be static?
> 
> How do we reach it from outside if we set it static? This
> function is called in the for_each_engine_ctx macro that is used
> outside from igt_gt.c

Of course! But then it needs a gem prefix and probably double underscore 
to signify it shouldn't be called directly.

>>> +#define for_each_engine_ctx(fd, ctx, e) \
>>
>> High level design question: Do we want 'e' to be an integer or a struct
>> describing each engine?
> 
> Do you mean that you would you prefer iterating with a
> 'i915_context_param_engines' or a 'intel_execution_engine' struct
> instead of an 'e' integer? This way it would also be different
> from how the current 'for_each_engine' works.
> 
> I could do it in a next patch, so that in this one we keep it as
> close as possible to the current way of doing things.

I forgot in current code struct intel_execution_engine is a hidden 
variable in the iterator, unlike the i915 version.

I think struct intel_execution_engine available during iteration is 
preferable (we often need easy access to engine name, class/instance and 
such), but I don't have an opinion on whether it should be explicit or 
still hidden variable. Staying with hidden makes the churn smaller. 
Maybe Chris has an opinion.

Regards,

Tvrtko