[igt-dev] [PATCH v3.5 2/3] lib: implement new engine discovery interface

[Tvrtko Ursulin]

On 14/12/2018 15:34, Andi Shyti wrote:
> Hi Tvrtko,
> 
> would the following patch work? the "for_each_engine_ctx()" gets
> an argument more that is a double pointer to a "struct
> intel_execution_engine2".
> 
> The __gem_setup_ctx_engines() function allocates an array of
> engines where it stores the engines present in the GPU.
> 
> The new iterator can be used either:
> 
>    ...
>    struct intel_execution_engine2 *engines;
> 
>    for_each_engine_ctx(fd, ctx_id, r, &engines) {
>      printf("engine %d is %s\n", r, engines[r].name);
>    }
> 
> or, if the user is not interested at getting the structure, can
> send NULL instead and get nothing.
> 
>    for_each_engine_ctx(fd, ctx_id, r, NULL) {
>      ...
>    }
> 
> Is this what you meant to get from the kernel?

I was thinking that either on the first call to the new iterator, or 
somewhere from drm fd open, we would create this dynamic array, 
replacing the existing static array in lib/igt_gt.c.

So we wouldn't have this extra parameter to the iterator.

Another note - see to convert for_each_engine_class_instance to the new 
scheme and also all places which use PMU to probe for engine existence. 
(Could be only perf_pmu is upstream which does it.)

Basically all code which uses gem_class_instance_to_eb_flags needs to be 
converted to the engine map approach and that helper removed.

Can be a second stage together with for_each_physical_engine conversion.

Regards,

Tvrtko

> 
> Thanks,
> Andi
> 
> ---
>   lib/igt_gt.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_gt.h |  6 ++++
>   2 files changed, 105 insertions(+)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a20619246296..8616dba44f13 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -650,3 +650,102 @@ 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 store_engine(struct intel_execution_engine2 *to,
> +			 struct drm_i915_engine_info *from)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(intel_execution_engines2); i++)
> +		if (from->class == intel_execution_engines2[i].class &&
> +		    from->instance == intel_execution_engines2[i].instance) {
> +			to->name = intel_execution_engines2[i].name;
> +			to->instance = intel_execution_engines2[i].instance;
> +			to->class = intel_execution_engines2[i].class;
> +		}
> +}
> +
> +static void set_param(int fd, uint32_t ctx_id,
> +		      struct drm_i915_query_engine_info *query_engine,
> +		      struct intel_execution_engine2 *engines)
> +{
> +	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;
> +
> +		if(engines)
> +			store_engine(&engines[i], &query_engine->engines[i]);
> +	}
> +
> +	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 intel_execution_engine2 **engines)
> +{
> +	struct drm_i915_query_engine_info *query_engine;
> +	int n;
> +
> +	query_engine = query_engines(fd);
> +
> +	n = query_engine->num_engines;
> +
> +	if (engines)
> +		igt_assert(*engines = malloc(n * sizeof(**engines)));
> +
> +	set_param(fd, ctx_id, query_engine, engines ? *engines : NULL);
> +
> +	free(query_engine);
> +
> +	return n;
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da98084..cb51082b1bed 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,6 +86,10 @@ 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, engines) \
> +		for (int __m = __gem_setup_ctx_engines(fd, ctx, engines), __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);
>   
> @@ -97,6 +101,8 @@ extern const struct intel_execution_engine2 {
>   	int instance;
>   } intel_execution_engines2[];
>   
> +int __gem_setup_ctx_engines(int fd, uint32_t ctx_id, struct intel_execution_engine2 **engines);
> +
>   unsigned int
>   gem_class_instance_to_eb_flags(int gem_fd,
>   			       enum drm_i915_gem_engine_class class,
>