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

[Tvrtko Ursulin]

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