new engine discovery interface

The gem_engine_topology library is a set of functions that interface with the query and getparam/setparam ioctls.

The library's three main access points are:

- get_active_engines()
- gem_set_context_get_engines()
- igt_require_gem_engine_list()

The first two functions are similar, with the difference that the 'gem_set_context_get_engines()' sets the engine before returning the engine list in a 'intel_execution_engine2' type array. Another improtant difference is that the first function works only with the getparam/setparam and query ioctls are implemented and it return 'NULL' in the other case.

'gem_set_context_get_engines()' function, at the first call generates the array of active engines (either physical or virtual) called 'intel_active_engines2'.
If the driver cannot be queried, the 'intel_active_engines2' array points to the exisiting 'intel_execution_engines2' which has been previously defined.

The value of the 'intel_active_engines2' will be used in further calls to check whether the above mentioned ioctls are implemented without the need to call getparam().

The 'igt_require_gem_engine_list()' causes the test to fail in case the required ioctls are not implemented.

Signed-off-by Andi Shyti <[email protected]>

---
 lib/Makefile.sources           |   2 +
 lib/i915/gem_engine_topology.c | 199 +++++++++++++++++++++++++++++++++
 lib/i915/gem_engine_topology.h |  39 +++++++
 lib/igt_gt.c                   |   2 +-
 lib/igt_gt.h                   |   2 +-
 lib/meson.build                |   1 +
 6 files changed, 243 insertions(+), 2 deletions(-)
 create mode 100644 lib/i915/gem_engine_topology.c
 create mode 100644 lib/i915/gem_engine_topology.h

lib/Makefile.sources +2 -0

index cf2720981707..757bd7a17ebe 100644
@@ -13,6 +13,8 @@ lib_source_list =             \
        i915/gem_ring.c \
        i915/gem_mman.c \
        i915/gem_mman.h \
+       i915/gem_engine_topology.c      \
+       i915/gem_engine_topology.h      \
        i915_3d.h               \
        i915_reg.h              \
        i915_pciids.h           \

lib/i915/gem_engine_topology.c new file +73 -0

new file mode 100644
index 000000000000..9376b4792441
@@ -0,0 +1,199 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "drmtest.h"
+#include "igt_gt.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_engine_topology.h"
+
+#define SIZEOF_CTX_PARAM       offsetof(struct i915_context_param_engines, \
+                                       class_instance[I915_EXEC_RING_MASK + 1])
+#define SIZEOF_QUERY           offsetof(struct drm_i915_query_engine_info, \
+                                       engines[I915_EXEC_RING_MASK + 1])
+
+static struct intel_execution_engine2 *intel_active_engines2;
+
+int __gem_query(int fd, struct drm_i915_query *q)
+{
+       int err = 0;
+
+       if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
+               err = -errno;
+
+       errno = 0;
+       return err;
+}
+
+void gem_query(int fd, struct drm_i915_query *q)
+{
+       igt_assert_eq(__gem_query(fd, q), 0);
+}
+
+static void query_engines(int fd,
+                         struct drm_i915_query_engine_info *query_engines)
+{
+       struct drm_i915_query_item item = { };
+       struct drm_i915_query query = { };
+
+       item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+       query.items_ptr = to_user_pointer(&item);
+       query.num_items = 1;
+       item.length = SIZEOF_QUERY;
+
+       item.data_ptr = to_user_pointer(query_engines);
+
+       gem_query(fd, &query);
+}
+
+bool gem_has_engine_topology(void)
+{
+       return intel_active_engines2 != intel_execution_engines2;
+}
+

Quoted 4 lines

lib/i915/gem_engine_topology.c new file +4 -0

+static void set_ctx_param_engines(int fd, uint32_t ctx_id)
+{
+       struct i915_context_param_engines *ctx_engine;
+       struct drm_i915_gem_context_param ctx_param;

lib/i915/gem_engine_topology.c line 83

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+       const struct intel_execution_engine2 *e2;
+       uint8_t buff[SIZEOF_CTX_PARAM] = { };
+       int i;
+
+       if (!gem_has_engine_topology())
+               return;

Why would different fd have the same topology?

The gem_engine_topology library is a set of functions that interface with the query and getparam/setparam ioctls.

The library's three main access points are:

- get_active_engines()
- gem_set_context_get_engines()
- igt_require_gem_engine_list()

The first two functions are similar, with the difference that the 'gem_set_context_get_engines()' sets the engine before returning the engine list in a 'intel_execution_engine2' type array. Another improtant difference is that the first function works only with the getparam/setparam and query ioctls are implemented and it return 'NULL' in the other case.

'gem_set_context_get_engines()' function, at the first call generates the array of active engines (either physical or virtual) called 'intel_active_engines2'.
If the driver cannot be queried, the 'intel_active_engines2' array points to the exisiting 'intel_execution_engines2' which has been previously defined.

The value of the 'intel_active_engines2' will be used in further calls to check whether the above mentioned ioctls are implemented without the need to call getparam().

The 'igt_require_gem_engine_list()' causes the test to fail in case the required ioctls are not implemented.

Signed-off-by Andi Shyti <[email protected]>

---
  lib/Makefile.sources           |   2 +
  lib/i915/gem_engine_topology.c | 199 +++++++++++++++++++++++++++++++++
  lib/i915/gem_engine_topology.h |  39 +++++++
  lib/igt_gt.c                   |   2 +-
  lib/igt_gt.h                   |   2 +-
  lib/meson.build                |   1 +
  6 files changed, 243 insertions(+), 2 deletions(-)

create mode 100644 lib/i915/gem_engine_topology.c create mode 100644 lib/i915/gem_engine_topology.h

lib/Makefile.sources +2 -0

index cf2720981707..757bd7a17ebe 100644
@@ -13,6 +13,8 @@ lib_source_list =	 	\
  	i915/gem_ring.c	\
  	i915/gem_mman.c	\
  	i915/gem_mman.h	\
+	i915/gem_engine_topology.c	\
+	i915/gem_engine_topology.h	\
  	i915_3d.h		\
  	i915_reg.h		\
  	i915_pciids.h		\

lib/i915/gem_engine_topology.c new file +23 -0

new file mode 100644
index 000000000000..9376b4792441
@@ -0,0 +1,199 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+

lib/i915/gem_engine_topology.c new file +8 -0

+
+static struct intel_execution_engine2 *intel_active_engines2;
+
+int __gem_query(int fd, struct drm_i915_query *q)
+{
+	int err = 0;
+
+	if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))

lib/i915/gem_engine_topology.c new file +39 -0

+		ctx_engine->class_instance[i].engine_class = e2->class;
+		ctx_engine->class_instance[i].engine_instance = e2->instance;
+	}
+
+	ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i + 1]);
+	ctx_param.value = to_user_pointer(ctx_engine);
+
+	gem_context_set_param(fd, &ctx_param);
+}
+
+/*
+ * Initializes the list of engines.
+ *
+ * Returns:
+ *
+ *  - 0 in case of success and the get/set_param ioctls are implemented
+ *  - -ENODEV in case of success but I915_CONTEXT_PARAM_ENGINES is not
+ *    implemented in DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl.
+ */
+static int init_engine_list(int fd)
+{
+	const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
+	struct drm_i915_query_engine_info *query_engine;
+	unsigned char query_buffer[SIZEOF_QUERY] = { };
+	struct drm_i915_gem_context_param ctx_param = {
+		.param = I915_CONTEXT_PARAM_ENGINES,
+	};
+	int i;
+
+	/* the list is already initialized */
+	if (intel_active_engines2)
+		return gem_has_engine_topology() ? 0 : -ENODEV;
+
+	/*
+	 * We check first whether the I915_CONTEXT_PARAM_ENGINES parameter is
+	 * supported by the running kernel. If not, __gem_context_get_param()
+	 * will return -EINVAL which, at this point, is not necessarily a
+	 * failure but it means that we need to fall beck to polling the engines
+	 * directly from intel_execution_engines2[].

lib/i915/gem_engine_topology.c new file +6 -0

+			igt_assert(asprintf(&name, "unk-%d:%d",
+					    class, instance) > 0);
+		else
+			igt_assert(asprintf(&name, "%s%d",
+					    class_names[class], instance) > 0);
+

lib/i915/gem_engine_topology.c new file +8 -0

+
+	return intel_active_engines2;
+}
+
+void igt_require_gem_engine_list(int fd)
+{
+	igt_require(!init_engine_list(fd));
+}

lib/i915/gem_engine_topology.h new file +15 -0

new file mode 100644
index 000000000000..be5e81481dba
@@ -0,0 +1,39 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

lib/i915/gem_engine_topology.h new file +14 -0

+
+int __gem_query(int fd, struct drm_i915_query *q);
+void gem_query(int fd, struct drm_i915_query *q);
+
+bool gem_has_engine_topology(void);
+struct intel_execution_engine2 *get_active_engines(int fd);
+struct intel_execution_engine2 *gem_set_context_get_engines(int fd,
+							    uint32_t ctx_id);
+
+void igt_require_gem_engine_list(int fd);
+
+#define gem_get_engines() gem_context_get_engines(0, 0)
+
+#endif /* GEM_QUEY_H */

lib/igt_gt.c +0 -0

index c98a7553b7fe..54c71e5e7186 100644

lib/igt_gt.c +0 -0

  	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
  	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
  	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },

lib/igt_gt.h +1 -1

index 54e95da98084..f4bd6c22a81a 100644
@@ -91,7 +91,7 @@ bool gem_ring_has_physical_engine(int fd, unsigned int ring);
 
  bool gem_can_store_dword(int fd, unsigned int engine);
 
-extern const struct intel_execution_engine2 {
+extern struct intel_execution_engine2 {
  	const char *name;
  	int class;
  	int instance;

lib/meson.build +0 -0

index 0eb5585d72b9..3cc52f97c8bf 100644

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+#include "drmtest.h"
+#include "igt_gt.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_engine_topology.h"

lib/i915/gem_engine_topology.c line 33

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+
+#define SIZEOF_CTX_PARAM	offsetof(struct i915_context_param_engines, \
+					class_instance[I915_EXEC_RING_MASK + 1])
+#define SIZEOF_QUERY		offsetof(struct drm_i915_query_engine_info, \
+					engines[I915_EXEC_RING_MASK + 1])

There is no relationship between I915_EXEC_RING_MASK and the number of engines we can query. I'll suggest how I think it should work a bit later.

Quoted 12 lines

lib/i915/gem_engine_topology.c new file +12 -0

+
+static struct intel_execution_engine2 *intel_active_engines2;
+
+int __gem_query(int fd, struct drm_i915_query *q)
+{
+	int err = 0;
+
+	if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
+		err = -errno;
+
+	errno = 0;
+	return err;

lib/i915/gem_engine_topology.c line 51

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+}
+
+void gem_query(int fd, struct drm_i915_query *q)
+{
+	igt_assert_eq(__gem_query(fd, q), 0);
+}

I would keep these two static if there are no external callers for now.

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+
+static void query_engines(int fd,
+			  struct drm_i915_query_engine_info *query_engines)
+{
+	struct drm_i915_query_item item = { };
+	struct drm_i915_query query = { };

lib/i915/gem_engine_topology.c line 62

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+
+	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+	query.items_ptr = to_user_pointer(&item);
+	query.num_items = 1;
+	item.length = SIZEOF_QUERY;

I think making this function take buffer size and return the queried size would be simplest.

lib/i915/gem_engine_topology.c line 71

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+	item.data_ptr = to_user_pointer(query_engines);
+
+	gem_query(fd, &query);
+}
+
+bool gem_has_engine_topology(void)
+{
+	return intel_active_engines2 != intel_execution_engines2;

A problem for the exported API if init_engine_list hasn't been called yet? Maybe:

	if (!intel_active_engines2)
		init_engine_list(fd);

	return intel_active_engines2 != intel_execution_engines2;

?

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+}
+
+static void set_ctx_param_engines(int fd, uint32_t ctx_id)
+{
+	struct i915_context_param_engines *ctx_engine;
+	struct drm_i915_gem_context_param ctx_param;

lib/i915/gem_engine_topology.c line 83

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+	const struct intel_execution_engine2 *e2;
+	uint8_t buff[SIZEOF_CTX_PARAM] = { };
+	int i;
+
+	if (!gem_has_engine_topology())
+		return;

AFAICS this can be assert here.

lib/i915/gem_engine_topology.c line 91

Quoted 8 lines

lib/i915/gem_engine_topology.c new file +8 -0

+
+	ctx_engine = (struct i915_context_param_engines *) buff;
+
+	ctx_param.ctx_id = ctx_id;
+	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+
+	ctx_engine->extensions = 0;
+	for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {

Here I think is the place where we check that the total number of engines does not exceed the engine map execbuf index.

	igt_require(i <= I915_EXEC_RING_MASK);

?

Quoted 39 lines

lib/i915/gem_engine_topology.c new file +39 -0

+		ctx_engine->class_instance[i].engine_class = e2->class;
+		ctx_engine->class_instance[i].engine_instance = e2->instance;
+	}
+
+	ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i + 1]);
+	ctx_param.value = to_user_pointer(ctx_engine);
+
+	gem_context_set_param(fd, &ctx_param);
+}
+
+/*
+ * Initializes the list of engines.
+ *
+ * Returns:
+ *
+ *  - 0 in case of success and the get/set_param ioctls are implemented
+ *  - -ENODEV in case of success but I915_CONTEXT_PARAM_ENGINES is not
+ *    implemented in DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl.
+ */
+static int init_engine_list(int fd)
+{
+	const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
+	struct drm_i915_query_engine_info *query_engine;
+	unsigned char query_buffer[SIZEOF_QUERY] = { };
+	struct drm_i915_gem_context_param ctx_param = {
+		.param = I915_CONTEXT_PARAM_ENGINES,
+	};
+	int i;
+
+	/* the list is already initialized */
+	if (intel_active_engines2)
+		return gem_has_engine_topology() ? 0 : -ENODEV;
+
+	/*
+	 * We check first whether the I915_CONTEXT_PARAM_ENGINES parameter is
+	 * supported by the running kernel. If not, __gem_context_get_param()
+	 * will return -EINVAL which, at this point, is not necessarily a
+	 * failure but it means that we need to fall beck to polling the engines
+	 * directly from intel_execution_engines2[].

lib/i915/gem_engine_topology.c line 140

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+	 *
+	 * We will return -ENODEV with the meaning of missing interface
+	 */
+	if (__gem_context_get_param(fd, &ctx_param)) {
+		intel_active_engines2 = intel_execution_engines2;
+		return -ENODEV;
+	}
+
+	query_engine = (struct drm_i915_query_engine_info *) query_buffer;
+	query_engines(fd, query_engine);

Do a two stage query here - first ask for required buffer size, then allocate it and query:

	size = query_engines(fd, NULL, 0);
	query_engine = malloc(size);
	query_engines(fd, query_engine, size);

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	igt_assert(query_engine->num_engines < I915_EXEC_RING_MASK + 1);

Remove this since we don't need to limit the size of the array during the query. It is only the engine map that has the limitation.

lib/i915/gem_engine_topology.c line 151

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+	intel_active_engines2 = malloc((query_engine->num_engines + 1) *
+				       sizeof(*intel_active_engines2));
+	igt_assert(intel_active_engines2);
+
+	for (i = 0; i < query_engine->num_engines; i++) {
+		char *name;
+		int class = query_engine->engines[i].engine_class;
+		int instance = query_engine->engines[i].engine_instance;

Could use __u16 to match uapi.

lib/i915/gem_engine_topology.c line 157

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+
+		intel_active_engines2[i].class = class;
+		intel_active_engines2[i].instance = instance;
+
+		/* if we don't recognise the class, then we mark it as "unk" */
+		if (class >= ARRAY_SIZE(class_names) || !class_names[class])

Second condition seems to be not needed at the moment.

Quoted 1 line

+struct intel_execution_engine2 *get_active_engines(int fd)

Unexport this one?

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+{
+	if (init_engine_list(fd)) {
+		igt_info("using pre-allocated engine list\n");
+		return NULL;
+	}
+

lib/i915/gem_engine_topology.c line 184

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+	return intel_active_engines2;
+}
+
+struct intel_execution_engine2 *gem_set_context_get_engines(int fd,
+							    uint32_t ctx_id)

I find the name of this function really confusing (set get what?). Can I suggest __gem_prepare_context_engines or something like that?

lib/i915/gem_engine_topology.c line 189

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+{
+	struct intel_execution_engine2 *active_engines = get_active_engines(fd);
+
+	if (!active_engines)
+		return intel_execution_engines2;

So without engine discovery the function will not configure the engine map. How will the code inside for_each_engine2 be able to work then?

I guess it's okay since we don't expect to see a kernel with ctx engine map support and no engine discovery but it feels a bit unnatural.

But I think it effectively means you should have igt_require_gem_engine_list in here since it is otherwise not usable from the for_each_engine2 iterator.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	set_ctx_param_engines(fd, ctx_id);

You need to handle the use case where the context already has a map configured instead of overriding it. Should be easy.

lib/i915/gem_engine_topology.h line 25

Quoted 2 lines

lib/i915/gem_engine_topology.h new file +2 -0

+#ifndef GEM_QUERY_H
+#define GEM_QUERY_H

GEM_ENGINE_TOPOLOGY_H

lib/igt_gt.c bool gem_can_store_dword(int fd, unsigned int engine) · line 580

Quoted 2 lines

lib/igt_gt.c +1 -1

-const struct intel_execution_engine2 intel_execution_engines2[] = {
+struct intel_execution_engine2 intel_execution_engines2[] = {

This one can't stay const?

Hi Tvrtko,

Quoted 5 lines

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+#include "drmtest.h"
+#include "igt_gt.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_engine_topology.h"

Quoted 8 lines

lib/i915/gem_engine_topology.c line 33

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+
+#define SIZEOF_CTX_PARAM	offsetof(struct i915_context_param_engines, \
+					class_instance[I915_EXEC_RING_MASK + 1])
+#define SIZEOF_QUERY		offsetof(struct drm_i915_query_engine_info, \
+					engines[I915_EXEC_RING_MASK + 1])

There is no relationship between I915_EXEC_RING_MASK and the number of engines we can query. I'll suggest how I think it should work a bit later.

I915_EXEC_RING_MASK is the upper limit checked in the driver, that's why I used that one. I could have hardcoded it to 64, but I wanted to keep it consistent to the driver.

Quoted 12 lines

Quoted 12 lines

lib/i915/gem_engine_topology.c new file +12 -0

+
+static struct intel_execution_engine2 *intel_active_engines2;
+
+int __gem_query(int fd, struct drm_i915_query *q)
+{
+	int err = 0;
+
+	if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
+		err = -errno;
+
+	errno = 0;
+	return err;

Quoted 8 lines

lib/i915/gem_engine_topology.c line 51

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+}
+
+void gem_query(int fd, struct drm_i915_query *q)
+{
+	igt_assert_eq(__gem_query(fd, q), 0);
+}

I would keep these two static if there are no external callers for now.

Yes, I can make them static, I thought someone might need them in the future.

Quoted 15 lines

lib/i915/gem_engine_topology.c line 62

Quoted 12 lines

lib/i915/gem_engine_topology.c new file +11 -0

+
+static void query_engines(int fd,
+			  struct drm_i915_query_engine_info *query_engines)
+{
+	struct drm_i915_query_item item = { };
+	struct drm_i915_query query = { };
+
+	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+	query.items_ptr = to_user_pointer(&item);
+	query.num_items = 1;
+	item.length = SIZEOF_QUERY;

I think making this function take buffer size and return the queried size would be simplest.

I haven't really understood, do you mean doing something like:

static int query_engines(...., int size)

{
    [...]
    return size;
}

or calling the gem_query twice, first time for getting the size and second time for allocating the necessary resources.

Because in the first case, we have the size in query_engines, is that right?

While if you mean the second case, this is how I did it at the very beginning, but then with Chris we agreed that 64 (I915_EXEC_RING_MASK) is "big enough". If you are strong with it, I can revert it back.

Quoted 13 lines

lib/i915/gem_engine_topology.c line 71

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+	item.data_ptr = to_user_pointer(query_engines);
+
+	gem_query(fd, &query);
+}
+
+bool gem_has_engine_topology(void)
+{
+	return intel_active_engines2 != intel_execution_engines2;

A problem for the exported API if init_engine_list hasn't been called yet? Maybe:

Quoted 6 lines

	if (!intel_active_engines2)
		init_engine_list(fd);

	return intel_active_engines2 != intel_execution_engines2;

?

Yes, it's indeed prone to be misused, I would need to make a more generic way, because the two lines

    if (!intel_active_engines2)
           init_engine_list(fd);

are called so many times that I'm having an overdose of them :)

Quoted 15 lines

lib/i915/gem_engine_topology.c line 83

Quoted 13 lines

lib/i915/gem_engine_topology.c new file +12 -0

+}
+
+static void set_ctx_param_engines(int fd, uint32_t ctx_id)
+{
+	struct i915_context_param_engines *ctx_engine;
+	struct drm_i915_gem_context_param ctx_param;
+	const struct intel_execution_engine2 *e2;
+	uint8_t buff[SIZEOF_CTX_PARAM] = { };
+	int i;
+
+	if (!gem_has_engine_topology())
+		return;

AFAICS this can be assert here.

I think this check is redundant as it's never going to happen. This function is 'static' and the calling function checks that already. I will remove it.

Quoted 13 lines

lib/i915/gem_engine_topology.c line 91

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +8 -0

+
+	ctx_engine = (struct i915_context_param_engines *) buff;
+
+	ctx_param.ctx_id = ctx_id;
+	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+
+	ctx_engine->extensions = 0;
+	for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {

Here I think is the place where we check that the total number of engines does not exceed the engine map execbuf index.

Quoted 3 lines

	igt_require(i <= I915_EXEC_RING_MASK);

?

I can add that, but it would be redundant as the list would have never been created otherwise... anyway, better be safe than sorry :)

BTW, given my natural confusion between 'require' and 'assert', you meant 'assert', right?

Quoted 53 lines

Quoted 39 lines

lib/i915/gem_engine_topology.c new file +39 -0

+		ctx_engine->class_instance[i].engine_class = e2->class;
+		ctx_engine->class_instance[i].engine_instance = e2->instance;
+	}
+
+	ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i + 1]);
+	ctx_param.value = to_user_pointer(ctx_engine);
+
+	gem_context_set_param(fd, &ctx_param);
+}
+
+/*
+ * Initializes the list of engines.
+ *
+ * Returns:
+ *
+ *  - 0 in case of success and the get/set_param ioctls are implemented
+ *  - -ENODEV in case of success but I915_CONTEXT_PARAM_ENGINES is not
+ *    implemented in DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl.
+ */
+static int init_engine_list(int fd)
+{
+	const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
+	struct drm_i915_query_engine_info *query_engine;
+	unsigned char query_buffer[SIZEOF_QUERY] = { };
+	struct drm_i915_gem_context_param ctx_param = {
+		.param = I915_CONTEXT_PARAM_ENGINES,
+	};
+	int i;
+
+	/* the list is already initialized */
+	if (intel_active_engines2)
+		return gem_has_engine_topology() ? 0 : -ENODEV;
+
+	/*
+	 * We check first whether the I915_CONTEXT_PARAM_ENGINES parameter is
+	 * supported by the running kernel. If not, __gem_context_get_param()
+	 * will return -EINVAL which, at this point, is not necessarily a
+	 * failure but it means that we need to fall beck to polling the engines
+	 * directly from intel_execution_engines2[].

lib/i915/gem_engine_topology.c line 140

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+	 *
+	 * We will return -ENODEV with the meaning of missing interface
+	 */
+	if (__gem_context_get_param(fd, &ctx_param)) {
+		intel_active_engines2 = intel_execution_engines2;
+		return -ENODEV;
+	}
+
+	query_engine = (struct drm_i915_query_engine_info *) query_buffer;
+	query_engines(fd, query_engine);

Do a two stage query here - first ask for required buffer size, then allocate it and query:

Quoted 3 lines

	size = query_engines(fd, NULL, 0);
	query_engine = malloc(size);
	query_engines(fd, query_engine, size);

As above :) this is how I did it at the beginning and with Chris we agreed that 64 is big enough, and then we had some review rounds on agreeing on buffer size, name, calculation and so on.

Shall I move back? Honestly I like the double call, as well.

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	igt_assert(query_engine->num_engines < I915_EXEC_RING_MASK + 1);

Remove this since we don't need to limit the size of the array during the query. It is only the engine map that has the limitation.

This came after one of the 10 rounds of review as well :) It's indeed a bit redundant, It's mainly for the malloc, to avoid crazy allocation in case something goes wrong.

Quoted 11 lines

lib/i915/gem_engine_topology.c line 151

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+	intel_active_engines2 = malloc((query_engine->num_engines + 1) *
+				       sizeof(*intel_active_engines2));
+	igt_assert(intel_active_engines2);
+
+	for (i = 0; i < query_engine->num_engines; i++) {
+		char *name;
+		int class = query_engine->engines[i].engine_class;
+		int instance = query_engine->engines[i].engine_instance;

Could use __u16 to match uapi.

Sure!

Quoted 8 lines

lib/i915/gem_engine_topology.c line 157

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+
+		intel_active_engines2[i].class = class;
+		intel_active_engines2[i].instance = instance;
+
+		/* if we don't recognise the class, then we mark it as "unk" */
+		if (class >= ARRAY_SIZE(class_names) || !class_names[class])

Second condition seems to be not needed at the moment.

Yes, of course, it must be a leftover.

Quoted 3 lines

Quoted 1 line

+struct intel_execution_engine2 *get_active_engines(int fd)

Unexport this one?

I thought someone might need it, I will do it static and rework it with the bool gem_has_engine_topology() as they would become very similar.

Quoted 6 lines

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+{
+	if (init_engine_list(fd)) {
+		igt_info("using pre-allocated engine list\n");
+		return NULL;
+	}
+

Quoted 8 lines

lib/i915/gem_engine_topology.c line 184

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+	return intel_active_engines2;
+}
+
+struct intel_execution_engine2 *gem_set_context_get_engines(int fd,
+							    uint32_t ctx_id)

I find the name of this function really confusing (set get what?). Can I suggest __gem_prepare_context_engines or something like that?

The name means "set context and get engines", as it's a "get_" kind of function. But as it came out, I have a very limited imagination for names choice. I will call it __gem_prepare_context_engines(), then.

Quoted 8 lines

lib/i915/gem_engine_topology.c line 189

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+{
+	struct intel_execution_engine2 *active_engines = get_active_engines(fd);
+
+	if (!active_engines)
+		return intel_execution_engines2;

So without engine discovery the function will not configure the engine map. How will the code inside for_each_engine2 be able to work then?

It's this part of the for_each_engine2 that would make it work:

  for_if (gem_has_engine_topology() || \
          gem_has_engine(fd, e2__->class, e2__->instance))

We had quite some discussions about backward compatibility and the whole thing is designed to be backward compatible, otherwise, I would have removed quite some code from the above.

Quoted 11 lines

I guess it's okay since we don't expect to see a kernel with ctx engine map support and no engine discovery but it feels a bit unnatural.

But I think it effectively means you should have igt_require_gem_engine_list in here since it is otherwise not usable from the for_each_engine2 iterator.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	set_ctx_param_engines(fd, ctx_id);

You need to handle the use case where the context already has a map configured instead of overriding it. Should be easy.

Yes, this is relevant to your comment above, about the igt_require(<new_api>). This part is checked in 'get_active_engines(fd)', that returns NULL if we do not have the new uapi and therefore it returns the old list of engines.

adding the igt_require() (asset?) would be redundant. Am I missing any use case?

Quoted 4 lines

lib/i915/gem_engine_topology.h line 25

Quoted 2 lines

lib/i915/gem_engine_topology.h new file +2 -0

+#ifndef GEM_QUERY_H
+#define GEM_QUERY_H

GEM_ENGINE_TOPOLOGY_H

Yes :) This library was affected by an extensive name changing, due to my lack of imagination :)

Quoted 4 lines

lib/igt_gt.c bool gem_can_store_dword(int fd, unsigned int engine) · line 580

Quoted 2 lines

lib/igt_gt.c +1 -1

-const struct intel_execution_engine2 intel_execution_engines2[] = {
+struct intel_execution_engine2 intel_execution_engines2[] = {

This one can't stay const?

Yes, I hate this bit as well! I removed here the const, because I wanted active_engines2 to point to execution_engine2.

active_engines2 cannot be const, so that I cannot make it point to this one, therefore I have to remove the const from here.

But, indeed, because now I am using the "get_engine...()" and because I am going to rework the 'bool has_engine_topology()' function, approach, I might not need anymore to refer to this one.

What will happen, though, is that we would then need, at some point, to patch all the tests that refer to intel_execution_engines2.

Quoted 16 lines

Hi Tvrtko,

Quoted 13 lines

lib/i915/gem_engine_topology.c line 33

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+#include "drmtest.h"
+#include "igt_gt.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_engine_topology.h"
+
+#define SIZEOF_CTX_PARAM   offsetof(struct i915_context_param_engines, \
+                                   class_instance[I915_EXEC_RING_MASK + 1])
+#define SIZEOF_QUERY               offsetof(struct drm_i915_query_engine_info, \
+                                   engines[I915_EXEC_RING_MASK + 1])

There is no relationship between I915_EXEC_RING_MASK and the number of engines we can query. I'll suggest how I think it should work a bit later.

Quoted 3 lines

I915_EXEC_RING_MASK is the upper limit checked in the driver, that's why I used that one. I could have hardcoded it to 64, but I wanted to keep it consistent to the driver.

I'll argue with Chris to remove this limit from the engine map (come eb3 might not be relevant any more) - nevertheless the query does not have this limit so I think it would be best to decouple the code from the start.

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

lib/i915/gem_engine_topology.c new file +12 -0

+
+static struct intel_execution_engine2 *intel_active_engines2;
+
+int __gem_query(int fd, struct drm_i915_query *q)
+{
+   int err = 0;
+
+   if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
+           err = -errno;
+
+   errno = 0;
+   return err;

Quoted 8 lines

lib/i915/gem_engine_topology.c line 51

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+}
+
+void gem_query(int fd, struct drm_i915_query *q)
+{
+   igt_assert_eq(__gem_query(fd, q), 0);
+}

I would keep these two static if there are no external callers for now.

Quoted 2 lines

Yes, I can make them static, I thought someone might need them in the future.

Could be but it will be easy to export them at that point. I wanted to keep the patch smallest possible for now so I can focus on reviewing the important bits.

Quoted 30 lines

Quoted 15 lines

lib/i915/gem_engine_topology.c line 62

Quoted 12 lines

lib/i915/gem_engine_topology.c new file +11 -0

+
+static void query_engines(int fd,
+                     struct drm_i915_query_engine_info *query_engines)
+{
+   struct drm_i915_query_item item = { };
+   struct drm_i915_query query = { };
+
+   item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+   query.items_ptr = to_user_pointer(&item);
+   query.num_items = 1;
+   item.length = SIZEOF_QUERY;

I think making this function take buffer size and return the queried size would be simplest.

I haven't really understood, do you mean doing something like:

static int query_engines(...., int size)

{
    [...]
    return size;
}

or calling the gem_query twice, first time for getting the size and second time for allocating the necessary resources.

Because in the first case, we have the size in query_engines, is that right?

Quoted 4 lines

While if you mean the second case, this is how I did it at the very beginning, but then with Chris we agreed that 64 (I915_EXEC_RING_MASK) is "big enough". If you are strong with it, I can revert it back.

Hm.. what happened to the old Chris!? :)) I am not objecting that strongly. Especially if you already changed it once then just leave it. It can be changed later if someone gets the desire to tidy..

Quoted 24 lines

Quoted 14 lines

lib/i915/gem_engine_topology.c line 71

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+   item.data_ptr = to_user_pointer(query_engines);
+
+   gem_query(fd, &query);
+}
+
+bool gem_has_engine_topology(void)
+{
+   return intel_active_engines2 != intel_execution_engines2;

A problem for the exported API if init_engine_list hasn't been called yet? Maybe:

Quoted 6 lines

     if (!intel_active_engines2)
             init_engine_list(fd);

     return intel_active_engines2 != intel_execution_engines2;

?

Yes, it's indeed prone to be misused, I would need to make a more generic way, because the two lines

Quoted 4 lines

    if (!intel_active_engines2)
           init_engine_list(fd);

are called so many times that I'm having an overdose of them :)

Yes the call chains and responsibilities do feel a bit unobvious.

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

lib/i915/gem_engine_topology.c new file +2 -0

+}
+

lib/i915/gem_engine_topology.c line 83

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+static void set_ctx_param_engines(int fd, uint32_t ctx_id)
+{
+   struct i915_context_param_engines *ctx_engine;
+   struct drm_i915_gem_context_param ctx_param;
+   const struct intel_execution_engine2 *e2;
+   uint8_t buff[SIZEOF_CTX_PARAM] = { };
+   int i;
+
+   if (!gem_has_engine_topology())
+           return;

AFAICS this can be assert here.

Quoted 3 lines

I think this check is redundant as it's never going to happen. This function is 'static' and the calling function checks that already. I will remove it.

Better to have it as assert but as you wish. I guess even without an assert it would segfault when it tries to iterate intel_active_engines2.

Quoted 13 lines

Quoted 13 lines

lib/i915/gem_engine_topology.c line 91

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +8 -0

+
+   ctx_engine = (struct i915_context_param_engines *) buff;
+
+   ctx_param.ctx_id = ctx_id;
+   ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+
+   ctx_engine->extensions = 0;
+   for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {

Here I think is the place where we check that the total number of engines does not exceed the engine map execbuf index.

Quoted 6 lines

Quoted 3 lines

     igt_require(i <= I915_EXEC_RING_MASK);

?

I can add that, but it would be redundant as the list would have never been created otherwise... anyway, better be safe than sorry :)

Yes I know, I wrote that because I was suggesting to remove the 64 engine limit from the query and just have it in the engine map.

But as said if you already discussed that point then leave it.

Quoted 2 lines

BTW, given my natural confusion between 'require' and 'assert', you meant 'assert', right?

I meant require. But I don't know either, it's so hard to choose sometimes. :)

Quoted 57 lines

Quoted 53 lines

Quoted 39 lines

lib/i915/gem_engine_topology.c new file +39 -0

+           ctx_engine->class_instance[i].engine_class = e2->class;
+           ctx_engine->class_instance[i].engine_instance = e2->instance;
+   }
+
+   ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i + 1]);
+   ctx_param.value = to_user_pointer(ctx_engine);
+
+   gem_context_set_param(fd, &ctx_param);
+}
+
+/*
+ * Initializes the list of engines.
+ *
+ * Returns:
+ *
+ *  - 0 in case of success and the get/set_param ioctls are implemented
+ *  - -ENODEV in case of success but I915_CONTEXT_PARAM_ENGINES is not
+ *    implemented in DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl.
+ */
+static int init_engine_list(int fd)
+{
+   const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
+   struct drm_i915_query_engine_info *query_engine;
+   unsigned char query_buffer[SIZEOF_QUERY] = { };
+   struct drm_i915_gem_context_param ctx_param = {
+           .param = I915_CONTEXT_PARAM_ENGINES,
+   };
+   int i;
+
+   /* the list is already initialized */
+   if (intel_active_engines2)
+           return gem_has_engine_topology() ? 0 : -ENODEV;
+
+   /*
+    * We check first whether the I915_CONTEXT_PARAM_ENGINES parameter is
+    * supported by the running kernel. If not, __gem_context_get_param()
+    * will return -EINVAL which, at this point, is not necessarily a
+    * failure but it means that we need to fall beck to polling the engines
+    * directly from intel_execution_engines2[].

lib/i915/gem_engine_topology.c line 140

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+    *
+    * We will return -ENODEV with the meaning of missing interface
+    */
+   if (__gem_context_get_param(fd, &ctx_param)) {
+           intel_active_engines2 = intel_execution_engines2;
+           return -ENODEV;
+   }
+
+   query_engine = (struct drm_i915_query_engine_info *) query_buffer;
+   query_engines(fd, query_engine);

Do a two stage query here - first ask for required buffer size, then allocate it and query:

Quoted 3 lines

     size = query_engines(fd, NULL, 0);
     query_engine = malloc(size);
     query_engines(fd, query_engine, size);

Quoted 6 lines

As above :) this is how I did it at the beginning and with Chris we agreed that 64 is big enough, and then we had some review rounds on agreeing on buffer size, name, calculation and so on.

Shall I move back? Honestly I like the double call, as well.

Leave it at least for now then.

Quoted 72 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+   igt_assert(query_engine->num_engines < I915_EXEC_RING_MASK + 1);

Remove this since we don't need to limit the size of the array during the query. It is only the engine map that has the limitation.

This came after one of the 10 rounds of review as well :) It's indeed a bit redundant, It's mainly for the malloc, to avoid crazy allocation in case something goes wrong.

Quoted 11 lines

lib/i915/gem_engine_topology.c line 151

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+   intel_active_engines2 = malloc((query_engine->num_engines + 1) *
+                                  sizeof(*intel_active_engines2));
+   igt_assert(intel_active_engines2);
+
+   for (i = 0; i < query_engine->num_engines; i++) {
+           char *name;
+           int class = query_engine->engines[i].engine_class;
+           int instance = query_engine->engines[i].engine_instance;

Could use __u16 to match uapi.

Sure!

Quoted 8 lines

lib/i915/gem_engine_topology.c line 157

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+
+           intel_active_engines2[i].class = class;
+           intel_active_engines2[i].instance = instance;
+
+           /* if we don't recognise the class, then we mark it as "unk" */
+           if (class >= ARRAY_SIZE(class_names) || !class_names[class])

Second condition seems to be not needed at the moment.

Yes, of course, it must be a leftover.

Quoted 3 lines

Quoted 1 line

+struct intel_execution_engine2 *get_active_engines(int fd)

Unexport this one?

I thought someone might need it, I will do it static and rework it with the bool gem_has_engine_topology() as they would become very similar.

Quoted 6 lines

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+{
+   if (init_engine_list(fd)) {
+           igt_info("using pre-allocated engine list\n");
+           return NULL;
+   }
+

Quoted 8 lines

lib/i915/gem_engine_topology.c line 184

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+   return intel_active_engines2;
+}
+
+struct intel_execution_engine2 *gem_set_context_get_engines(int fd,
+                                                       uint32_t ctx_id)

I find the name of this function really confusing (set get what?). Can I suggest __gem_prepare_context_engines or something like that?

The name means "set context and get engines", as it's a "get_" kind of function. But as it came out, I have a very limited imagination for names choice. I will call it __gem_prepare_context_engines(), then.

Quoted 8 lines

lib/i915/gem_engine_topology.c line 189

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+{
+   struct intel_execution_engine2 *active_engines = get_active_engines(fd);
+
+   if (!active_engines)
+           return intel_execution_engines2;

So without engine discovery the function will not configure the engine map. How will the code inside for_each_engine2 be able to work then?

Quoted 8 lines

It's this part of the for_each_engine2 that would make it work:

  for_if (gem_has_engine_topology() || \
          gem_has_engine(fd, e2__->class, e2__->instance))

We had quite some discussions about backward compatibility and the whole thing is designed to be backward compatible, otherwise, I would have removed quite some code from the above.

Yes, I comment on that in the other reply.

Quoted 17 lines

Quoted 11 lines

I guess it's okay since we don't expect to see a kernel with ctx engine map support and no engine discovery but it feels a bit unnatural.

But I think it effectively means you should have igt_require_gem_engine_list in here since it is otherwise not usable from the for_each_engine2 iterator.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+   set_ctx_param_engines(fd, ctx_id);

You need to handle the use case where the context already has a map configured instead of overriding it. Should be easy.

Yes, this is relevant to your comment above, about the igt_require(<new_api>). This part is checked in 'get_active_engines(fd)', that returns NULL if we do not have the new uapi and therefore it returns the old list of engines.

Quoted 2 lines

adding the igt_require() (asset?) would be redundant. Am I missing any use case?

AFAIR Chris wanted it to support the use case where he configures the engine map and passes the context to for_each_engine2. Maybe he changed his mind?

It would be tricky to do since then there is no intel_active_engine array to walk.. Could only define the iterator that the pointer to engine is sometimes not valid (must be checked) and added index used to submit. Not sure, depends if Chris still wants this feature.

Quoting Tvrtko Ursulin (2019-03-07 14:16:13)

Quoted 22 lines · Andi Shyti

Quoted 19 lines

Hi Tvrtko,

Quoted 5 lines

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+#include "drmtest.h"
+#include "igt_gt.h"
+#include "ioctl_wrappers.h"
+
+#include "i915/gem_engine_topology.h"

Quoted 8 lines

lib/i915/gem_engine_topology.c line 33

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+
+#define SIZEOF_CTX_PARAM   offsetof(struct i915_context_param_engines, \
+                                   class_instance[I915_EXEC_RING_MASK + 1])
+#define SIZEOF_QUERY               offsetof(struct drm_i915_query_engine_info, \
+                                   engines[I915_EXEC_RING_MASK + 1])

There is no relationship between I915_EXEC_RING_MASK and the number of engines we can query. I'll suggest how I think it should work a bit later.

I915_EXEC_RING_MASK is the upper limit checked in the driver, that's why I used that one. I could have hardcoded it to 64, but I wanted to keep it consistent to the driver.

Quoted 3 lines

I'll argue with Chris to remove this limit from the engine map (come eb3 might not be relevant any more) - nevertheless the query does not have this limit so I think it would be best to decouple the code from the start.

At the moment it fits in a u64 and I don't have to worry about flexible bitmaps. It's a bridge to cross later, we've plenty of space in the enum api to version a new interface.

Quoted 59 lines

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

lib/i915/gem_engine_topology.c new file +12 -0

+
+static struct intel_execution_engine2 *intel_active_engines2;
+
+int __gem_query(int fd, struct drm_i915_query *q)
+{
+   int err = 0;
+
+   if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
+           err = -errno;
+
+   errno = 0;
+   return err;

Quoted 8 lines

lib/i915/gem_engine_topology.c line 51

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+}
+
+void gem_query(int fd, struct drm_i915_query *q)
+{
+   igt_assert_eq(__gem_query(fd, q), 0);
+}

I would keep these two static if there are no external callers for now.

Quoted 2 lines

Yes, I can make them static, I thought someone might need them in the future.

Could be but it will be easy to export them at that point. I wanted to keep the patch smallest possible for now so I can focus on reviewing the important bits.

Quoted 30 lines

Quoted 15 lines

lib/i915/gem_engine_topology.c line 62

Quoted 12 lines

lib/i915/gem_engine_topology.c new file +11 -0

+
+static void query_engines(int fd,
+                     struct drm_i915_query_engine_info *query_engines)
+{
+   struct drm_i915_query_item item = { };
+   struct drm_i915_query query = { };
+
+   item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+   query.items_ptr = to_user_pointer(&item);
+   query.num_items = 1;
+   item.length = SIZEOF_QUERY;

I think making this function take buffer size and return the queried size would be simplest.

I haven't really understood, do you mean doing something like:

static int query_engines(...., int size)

{
     [...]
     return size;
}

or calling the gem_query twice, first time for getting the size and second time for allocating the necessary resources.

Because in the first case, we have the size in query_engines, is that right?

Quoted 8 lines

Quoted 4 lines

While if you mean the second case, this is how I did it at the very beginning, but then with Chris we agreed that 64 (I915_EXEC_RING_MASK) is "big enough". If you are strong with it, I can revert it back.

Hm.. what happened to the old Chris!? :)) I am not objecting that strongly. Especially if you already changed it once then just leave it. It can be changed later if someone gets the desire to tidy..

If I recall, my position is that you can have a fixed stack and then fallback to a heap. But for these interfaces where we have to return a heap pointer, we might as well use heaps and be flexible.

Quoted 27 lines

Quoted 27 lines

Quoted 14 lines

lib/i915/gem_engine_topology.c line 71

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+   item.data_ptr = to_user_pointer(query_engines);
+
+   gem_query(fd, &query);
+}
+
+bool gem_has_engine_topology(void)
+{
+   return intel_active_engines2 != intel_execution_engines2;

A problem for the exported API if init_engine_list hasn't been called yet? Maybe:

Quoted 6 lines

      if (!intel_active_engines2)
              init_engine_list(fd);

      return intel_active_engines2 != intel_execution_engines2;

?

Yes, it's indeed prone to be misused, I would need to make a more generic way, because the two lines

     if (!intel_active_engines2)
            init_engine_list(fd);

Quoted 3 lines

Quoted 1 line

are called so many times that I'm having an overdose of them :)

Yes the call chains and responsibilities do feel a bit unobvious.

I really don't want init_engine_list(fd) at all :) Ideally the query interface is cheap enough that it doesn't matter (other than debug noise in strace) and if we can't make it that cheap, it is cached in sensible scopes.

That may be an igt_device class (i915_device subclass).

Quoted 221 lines

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

lib/i915/gem_engine_topology.c new file +2 -0

+}
+

lib/i915/gem_engine_topology.c line 83

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+static void set_ctx_param_engines(int fd, uint32_t ctx_id)
+{
+   struct i915_context_param_engines *ctx_engine;
+   struct drm_i915_gem_context_param ctx_param;
+   const struct intel_execution_engine2 *e2;
+   uint8_t buff[SIZEOF_CTX_PARAM] = { };
+   int i;
+
+   if (!gem_has_engine_topology())
+           return;

AFAICS this can be assert here.

Quoted 3 lines

I think this check is redundant as it's never going to happen. This function is 'static' and the calling function checks that already. I will remove it.

Better to have it as assert but as you wish. I guess even without an assert it would segfault when it tries to iterate intel_active_engines2.

Quoted 13 lines

Quoted 13 lines

lib/i915/gem_engine_topology.c line 91

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +8 -0

+
+   ctx_engine = (struct i915_context_param_engines *) buff;
+
+   ctx_param.ctx_id = ctx_id;
+   ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+
+   ctx_engine->extensions = 0;
+   for (i = 0, e2 = intel_active_engines2; e2->name; i++, e2++) {

Here I think is the place where we check that the total number of engines does not exceed the engine map execbuf index.

Quoted 6 lines

Quoted 3 lines

      igt_require(i <= I915_EXEC_RING_MASK);

?

I can add that, but it would be redundant as the list would have never been created otherwise... anyway, better be safe than sorry :)

Yes I know, I wrote that because I was suggesting to remove the 64 engine limit from the query and just have it in the engine map.

But as said if you already discussed that point then leave it.

Quoted 2 lines

BTW, given my natural confusion between 'require' and 'assert', you meant 'assert', right?

I meant require. But I don't know either, it's so hard to choose sometimes. :)

Quoted 57 lines

Quoted 53 lines

Quoted 39 lines

lib/i915/gem_engine_topology.c new file +39 -0

+           ctx_engine->class_instance[i].engine_class = e2->class;
+           ctx_engine->class_instance[i].engine_instance = e2->instance;
+   }
+
+   ctx_param.size = offsetof(typeof(*ctx_engine), class_instance[i + 1]);
+   ctx_param.value = to_user_pointer(ctx_engine);
+
+   gem_context_set_param(fd, &ctx_param);
+}
+
+/*
+ * Initializes the list of engines.
+ *
+ * Returns:
+ *
+ *  - 0 in case of success and the get/set_param ioctls are implemented
+ *  - -ENODEV in case of success but I915_CONTEXT_PARAM_ENGINES is not
+ *    implemented in DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl.
+ */
+static int init_engine_list(int fd)
+{
+   const char *class_names[] = { "rcs", "bcs", "vcs", "vecs" };
+   struct drm_i915_query_engine_info *query_engine;
+   unsigned char query_buffer[SIZEOF_QUERY] = { };
+   struct drm_i915_gem_context_param ctx_param = {
+           .param = I915_CONTEXT_PARAM_ENGINES,
+   };
+   int i;
+
+   /* the list is already initialized */
+   if (intel_active_engines2)
+           return gem_has_engine_topology() ? 0 : -ENODEV;
+
+   /*
+    * We check first whether the I915_CONTEXT_PARAM_ENGINES parameter is
+    * supported by the running kernel. If not, __gem_context_get_param()
+    * will return -EINVAL which, at this point, is not necessarily a
+    * failure but it means that we need to fall beck to polling the engines
+    * directly from intel_execution_engines2[].

lib/i915/gem_engine_topology.c line 140

Quoted 10 lines

lib/i915/gem_engine_topology.c new file +10 -0

+    *
+    * We will return -ENODEV with the meaning of missing interface
+    */
+   if (__gem_context_get_param(fd, &ctx_param)) {
+           intel_active_engines2 = intel_execution_engines2;
+           return -ENODEV;
+   }
+
+   query_engine = (struct drm_i915_query_engine_info *) query_buffer;
+   query_engines(fd, query_engine);

Do a two stage query here - first ask for required buffer size, then allocate it and query:

Quoted 3 lines

      size = query_engines(fd, NULL, 0);
      query_engine = malloc(size);
      query_engines(fd, query_engine, size);

Quoted 6 lines

As above :) this is how I did it at the beginning and with Chris we agreed that 64 is big enough, and then we had some review rounds on agreeing on buffer size, name, calculation and so on.

Shall I move back? Honestly I like the double call, as well.

Leave it at least for now then.

Quoted 72 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+   igt_assert(query_engine->num_engines < I915_EXEC_RING_MASK + 1);

Remove this since we don't need to limit the size of the array during the query. It is only the engine map that has the limitation.

This came after one of the 10 rounds of review as well :) It's indeed a bit redundant, It's mainly for the malloc, to avoid crazy allocation in case something goes wrong.

Quoted 11 lines

lib/i915/gem_engine_topology.c line 151

Quoted 9 lines

lib/i915/gem_engine_topology.c new file +9 -0

+
+   intel_active_engines2 = malloc((query_engine->num_engines + 1) *
+                                  sizeof(*intel_active_engines2));
+   igt_assert(intel_active_engines2);
+
+   for (i = 0; i < query_engine->num_engines; i++) {
+           char *name;
+           int class = query_engine->engines[i].engine_class;
+           int instance = query_engine->engines[i].engine_instance;

Could use __u16 to match uapi.

Sure!

Quoted 8 lines

lib/i915/gem_engine_topology.c line 157

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+
+           intel_active_engines2[i].class = class;
+           intel_active_engines2[i].instance = instance;
+
+           /* if we don't recognise the class, then we mark it as "unk" */
+           if (class >= ARRAY_SIZE(class_names) || !class_names[class])

Second condition seems to be not needed at the moment.

Yes, of course, it must be a leftover.

Quoted 3 lines

Quoted 1 line

+struct intel_execution_engine2 *get_active_engines(int fd)

Unexport this one?

I thought someone might need it, I will do it static and rework it with the bool gem_has_engine_topology() as they would become very similar.

Quoted 6 lines

Quoted 6 lines

lib/i915/gem_engine_topology.c new file +6 -0

+{
+   if (init_engine_list(fd)) {
+           igt_info("using pre-allocated engine list\n");
+           return NULL;
+   }
+

Quoted 8 lines

lib/i915/gem_engine_topology.c line 184

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+   return intel_active_engines2;
+}
+
+struct intel_execution_engine2 *gem_set_context_get_engines(int fd,
+                                                       uint32_t ctx_id)

I find the name of this function really confusing (set get what?). Can I suggest __gem_prepare_context_engines or something like that?

The name means "set context and get engines", as it's a "get_" kind of function. But as it came out, I have a very limited imagination for names choice. I will call it __gem_prepare_context_engines(), then.

Quoted 8 lines

lib/i915/gem_engine_topology.c line 189

Quoted 5 lines

lib/i915/gem_engine_topology.c new file +5 -0

+{
+   struct intel_execution_engine2 *active_engines = get_active_engines(fd);
+
+   if (!active_engines)
+           return intel_execution_engines2;

So without engine discovery the function will not configure the engine map. How will the code inside for_each_engine2 be able to work then?

Quoted 8 lines

It's this part of the for_each_engine2 that would make it work:

   for_if (gem_has_engine_topology() || \
           gem_has_engine(fd, e2__->class, e2__->instance))

We had quite some discussions about backward compatibility and the whole thing is designed to be backward compatible, otherwise, I would have removed quite some code from the above.

Yes, I comment on that in the other reply.

Quoted 17 lines

Quoted 11 lines

I guess it's okay since we don't expect to see a kernel with ctx engine map support and no engine discovery but it feels a bit unnatural.

But I think it effectively means you should have igt_require_gem_engine_list in here since it is otherwise not usable from the for_each_engine2 iterator.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+   set_ctx_param_engines(fd, ctx_id);

You need to handle the use case where the context already has a map configured instead of overriding it. Should be easy.

Yes, this is relevant to your comment above, about the igt_require(<new_api>). This part is checked in 'get_active_engines(fd)', that returns NULL if we do not have the new uapi and therefore it returns the old list of engines.

Quoted 6 lines

Quoted 2 lines

adding the igt_require() (asset?) would be redundant. Am I missing any use case?

AFAIR Chris wanted it to support the use case where he configures the engine map and passes the context to for_each_engine2. Maybe he changed his mind?

I want to change engine maps on the fly, have multiple contexts with different engines, and have multiple devices, and have it all work.

Quoted 4 lines

It would be tricky to do since then there is no intel_active_engine array to walk.. Could only define the iterator that the pointer to engine is sometimes not valid (must be checked) and added index used to submit. Not sure, depends if Chris still wants this feature.

Just construct the engine array in the iterator. DO NOT USE A STATIC MAP! :)

'for_each_engine2()' defines a loop through the gpu engines.

It can work with both active and pre-defined engines.

In case we are looping through active engines (i.e. the running kernel has the query and get/setparam ioctls), the intel_active_engines2 points to an array that contains only the list engines dynamically allocated after having interrogated the driver.

While, if we are looping through pre-defined engines, intel_active_engines2 points to the 'intel_execution_engines2' array and works exactly as for_each_engine() but using the new 'struct intel_execution_engine2' data type.

Signed-off-by Andi Shyti <[email protected]>

---
 lib/igt_gt.h | 8 ++++++++
 1 file changed, 8 insertions(+)

lib/igt_gt.h +2 -0

index f4bd6c22a81a..f8bdaa62899f 100644
@@ -30,6 +30,8 @@
 
 #include "i915_drm.h"
 
+#include "i915/gem_engine_topology.h"
+
 void igt_require_hang_ring(int fd, int ring);
 
 typedef struct igt_hang {

'for_each_engine2()' defines a loop through the gpu engines.

It can work with both active and pre-defined engines.

In case we are looping through active engines (i.e. the running kernel has the query and get/setparam ioctls), the intel_active_engines2 points to an array that contains only the list engines dynamically allocated after having interrogated the driver.

While, if we are looping through pre-defined engines, intel_active_engines2 points to the 'intel_execution_engines2' array and works exactly as for_each_engine() but using the new 'struct intel_execution_engine2' data type.

Signed-off-by Andi Shyti <[email protected]>

---
  lib/igt_gt.h | 8 ++++++++
  1 file changed, 8 insertions(+)

lib/igt_gt.h +2 -0

index f4bd6c22a81a..f8bdaa62899f 100644
@@ -30,6 +30,8 @@
 
  #include "i915_drm.h"
 
+#include "i915/gem_engine_topology.h"
+
  void igt_require_hang_ring(int fd, int ring);
 
  typedef struct igt_hang {

Quoted 31 lines · Andi Shyti

'for_each_engine2()' defines a loop through the gpu engines.

It can work with both active and pre-defined engines.

In case we are looping through active engines (i.e. the running kernel has the query and get/setparam ioctls), the intel_active_engines2 points to an array that contains only the list engines dynamically allocated after having interrogated the driver.

While, if we are looping through pre-defined engines, intel_active_engines2 points to the 'intel_execution_engines2' array and works exactly as for_each_engine() but using the new 'struct intel_execution_engine2' data type.

Signed-off-by Andi Shyti <[email protected]>

---
  lib/igt_gt.h | 8 ++++++++

  1 file changed, 8 insertions(+)

lib/igt_gt.h +2 -0

index f4bd6c22a81a..f8bdaa62899f 100644
@@ -30,6 +30,8 @@
   #include "i915_drm.h"
+#include "i915/gem_engine_topology.h"
+
   void igt_require_hang_ring(int fd, int ring);
   typedef struct igt_hang {

lib/igt_gt.h extern const struct intel_execution_engine { · line 94

Quoted 7 lines

lib/igt_gt.h +5 -0

@@ -86,6 +88,12 @@ extern const struct intel_execution_engine {
            e__++) \
           for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
+#define for_each_engine2(fd, ctx) \
+        for (struct intel_execution_engine2 *e2__ = \
+             gem_set_context_get_engines(fd, ctx); e2__->name; e2__++) \
+            for_if (gem_has_engine_topology() || \
+                gem_has_engine(fd, e2__->class, e2__->instance))

gem_has_engine is a legacy hack which shouldn't be used and can hopefully be eliminated by the end of this work.

I think this iterator should assume ctx engine map has been configured with only available engines so neiher gem_has_engine_topology or gem_has_engine should be needed. Unless I am missing something?

lib/igt_gt.h extern const struct intel_execution_engine { · line 99

Quoted 5 lines

lib/igt_gt.h +1 -0

+
   bool gem_ring_is_physical_engine(int fd, unsigned int ring);
   bool gem_ring_has_physical_engine(int fd, unsigned int ring);
 

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Quoted 6 lines

Quoted 5 lines

+#define for_each_engine2(fd, ctx) \
+        for (struct intel_execution_engine2 *e2__ = \
+             gem_set_context_get_engines(fd, ctx); e2__->name; e2__++) \
+            for_if (gem_has_engine_topology() || \
+                gem_has_engine(fd, e2__->class, e2__->instance))

Quoted 6 lines

gem_has_engine is a legacy hack which shouldn't be used and can hopefully be eliminated by the end of this work.

I think this iterator should assume ctx engine map has been configured with only available engines so neiher gem_has_engine_topology or gem_has_engine should be needed. Unless I am missing something?

I forgot about the desire to run on old kernels.. blah..

But it still doesn't work since whatever is in the loop will use the index and the context may not have one.

Would a helper like gem_get_ctx_engine_flags(fd, ctx, e2, i) solve this?

I think something along those lines was in discussion some time back.

bool gem_get_ctx_engine_flags(...)

{
	if (ctx.has_map, or maybe, if gem_has_engine_topology)
		return i;
	else
		return e2->eb_flags;

}

It would need a comeback of index param to the iterator though.

'for_each_engine2()' defines a loop through the gpu engines.

It can work with both active and pre-defined engines.

In case we are looping through active engines (i.e. the running kernel has the query and get/setparam ioctls), the intel_active_engines2 points to an array that contains only the list engines dynamically allocated after having interrogated the driver.

While, if we are looping through pre-defined engines, intel_active_engines2 points to the 'intel_execution_engines2' array and works exactly as for_each_engine() but using the new 'struct intel_execution_engine2' data type.

Signed-off-by Andi Shyti <[email protected]>

---
  lib/igt_gt.h | 8 ++++++++
  1 file changed, 8 insertions(+)

lib/igt_gt.h +2 -0

index f4bd6c22a81a..f8bdaa62899f 100644
@@ -30,6 +30,8 @@
 
  #include "i915_drm.h"
 
+#include "i915/gem_engine_topology.h"
+
  void igt_require_hang_ring(int fd, int ring);
 
  typedef struct igt_hang {

@@ -86,6 +88,12 @@ extern const struct intel_execution_engine {
  	     e__++) \
  		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_context_has_engine()' that requires the index that the engine is mapped within the driver.

The previous 'gem_has_ring()' function becomes a wrapper to the new 'gem_context_has_engine()'.

Signed-off-by Andi Shyti <[email protected]>

---
 lib/ioctl_wrappers.c | 4 +++-
 lib/ioctl_wrappers.h | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

lib/ioctl_wrappers.c +0 -0

index 39920f8707d2..a2597e282704 100644

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_context_has_engine()' that requires the index that the engine is mapped within the driver.

The previous 'gem_has_ring()' function becomes a wrapper to the new 'gem_context_has_engine()'.

Signed-off-by Andi Shyti <[email protected]>

---
  lib/ioctl_wrappers.c | 4 +++-
  lib/ioctl_wrappers.h | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)

lib/ioctl_wrappers.c +3 -1

index 39920f8707d2..a2597e282704 100644
@@ -1252,7 +1252,7 @@ void igt_require_gem(int fd)
  	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
  }
 
-bool gem_has_ring(int fd, unsigned ring)
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx)
  {
  	struct drm_i915_gem_execbuffer2 execbuf;
  	struct drm_i915_gem_exec_object2 exec;

@@ -1268,6 +1268,8 @@ bool gem_has_ring(int fd, unsigned ring)
  	execbuf.buffers_ptr = to_user_pointer(&exec);
  	execbuf.buffer_count = 1;
  	execbuf.flags = ring;
+	execbuf.rsvd1 = ctx;
+
  	return __gem_execbuf(fd, &execbuf) == -ENOENT;
  }
 

lib/ioctl_wrappers.h +0 -1

index f0be26080da6..446e973b7449 100644
@@ -142,11 +142,13 @@ bool gem_has_exec_fence(int fd);
 
  /* check functions which auto-skip tests by calling igt_skip() */
  void gem_require_caching(int fd);
-bool gem_has_ring(int fd, unsigned ring);

Quoted 48 lines

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_context_has_engine()' that requires the index that the engine is mapped within the driver.

The previous 'gem_has_ring()' function becomes a wrapper to the new 'gem_context_has_engine()'.

Signed-off-by Andi Shyti <[email protected]>

---
  lib/ioctl_wrappers.c | 4 +++-
  lib/ioctl_wrappers.h | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)

lib/ioctl_wrappers.c +3 -1

index 39920f8707d2..a2597e282704 100644
@@ -1252,7 +1252,7 @@ void igt_require_gem(int fd)
  	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
  }
-bool gem_has_ring(int fd, unsigned ring)
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx)
  {
  	struct drm_i915_gem_execbuffer2 execbuf;
  	struct drm_i915_gem_exec_object2 exec;

@@ -1268,6 +1268,8 @@ bool gem_has_ring(int fd, unsigned ring)
  	execbuf.buffers_ptr = to_user_pointer(&exec);
  	execbuf.buffer_count = 1;
  	execbuf.flags = ring;
+	execbuf.rsvd1 = ctx;
+
  	return __gem_execbuf(fd, &execbuf) == -ENOENT;
  }

lib/ioctl_wrappers.h +2 -1

index f0be26080da6..446e973b7449 100644
@@ -142,11 +142,13 @@ bool gem_has_exec_fence(int fd);
  /* check functions which auto-skip tests by calling igt_skip() */
  void gem_require_caching(int fd);
-bool gem_has_ring(int fd, unsigned ring);
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx);
  void gem_require_ring(int fd, unsigned ring);
  bool gem_has_mocs_registers(int fd);
  void gem_require_mocs_registers(int fd);
+#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0)

lib/ioctl_wrappers.h bool gem_has_exec_fence(int fd);

Quoted 5 lines

lib/ioctl_wrappers.h +1 -0

+
  /* prime */
  struct local_dma_buf_sync {
  	uint64_t flags;
 

I don't understand why this. All current callers of gem_has_ring pass in eb flags and not an index so how it can work?

Hi Tvrtko,

Quoted 48 lines

Quoted 48 lines

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_context_has_engine()' that requires the index that the engine is mapped within the driver.

The previous 'gem_has_ring()' function becomes a wrapper to the new 'gem_context_has_engine()'.

Signed-off-by Andi Shyti <[email protected]>

---
   lib/ioctl_wrappers.c | 4 +++-
   lib/ioctl_wrappers.h | 4 +++-
   2 files changed, 6 insertions(+), 2 deletions(-)

lib/ioctl_wrappers.c +3 -1

index 39920f8707d2..a2597e282704 100644
@@ -1252,7 +1252,7 @@ void igt_require_gem(int fd)
   	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
   }
-bool gem_has_ring(int fd, unsigned ring)
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx)
   {
   	struct drm_i915_gem_execbuffer2 execbuf;
   	struct drm_i915_gem_exec_object2 exec;

@@ -1268,6 +1268,8 @@ bool gem_has_ring(int fd, unsigned ring)
   	execbuf.buffers_ptr = to_user_pointer(&exec);
   	execbuf.buffer_count = 1;
   	execbuf.flags = ring;
+	execbuf.rsvd1 = ctx;
+
   	return __gem_execbuf(fd, &execbuf) == -ENOENT;
   }

lib/ioctl_wrappers.h +2 -1

index f0be26080da6..446e973b7449 100644
@@ -142,11 +142,13 @@ bool gem_has_exec_fence(int fd);
   /* check functions which auto-skip tests by calling igt_skip() */
   void gem_require_caching(int fd);
-bool gem_has_ring(int fd, unsigned ring);
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx);
   void gem_require_ring(int fd, unsigned ring);
   bool gem_has_mocs_registers(int fd);
   void gem_require_mocs_registers(int fd);
+#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0)

Quoted 8 lines

lib/ioctl_wrappers.h bool gem_has_exec_fence(int fd);

Quoted 5 lines

lib/ioctl_wrappers.h +1 -0

+
   /* prime */
   struct local_dma_buf_sync {
   	uint64_t flags;
 

I don't understand why this. All current callers of gem_has_ring pass in eb flags and not an index so how it can work?

This is because of patch 3/6 this makes the for_each_engine2() able to work with new and old api.

Have I messed up the patch order?

Andi

Quoted 57 lines

Quoted 56 lines

Quoted 43 lines

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_context_has_engine()' that requires the index that the engine is mapped within the driver.

The previous 'gem_has_ring()' function becomes a wrapper to the new 'gem_context_has_engine()'.

Signed-off-by Andi Shyti <[email protected]>

---
   lib/ioctl_wrappers.c | 4 +++-
   lib/ioctl_wrappers.h | 4 +++-
   2 files changed, 6 insertions(+), 2 deletions(-)

lib/ioctl_wrappers.c +3 -1

index 39920f8707d2..a2597e282704 100644
@@ -1252,7 +1252,7 @@ void igt_require_gem(int fd)
   	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
   }
-bool gem_has_ring(int fd, unsigned ring)
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx)
   {
   	struct drm_i915_gem_execbuffer2 execbuf;
   	struct drm_i915_gem_exec_object2 exec;

@@ -1268,6 +1268,8 @@ bool gem_has_ring(int fd, unsigned ring)
   	execbuf.buffers_ptr = to_user_pointer(&exec);
   	execbuf.buffer_count = 1;
   	execbuf.flags = ring;
+	execbuf.rsvd1 = ctx;
+
   	return __gem_execbuf(fd, &execbuf) == -ENOENT;
   }

lib/ioctl_wrappers.h +0 -1

index f0be26080da6..446e973b7449 100644
@@ -142,11 +142,13 @@ bool gem_has_exec_fence(int fd);
   /* check functions which auto-skip tests by calling igt_skip() */
   void gem_require_caching(int fd);
-bool gem_has_ring(int fd, unsigned ring);

lib/ioctl_wrappers.h bool gem_has_exec_fence(int fd); · line 153

Quoted 10 lines

lib/ioctl_wrappers.h +3 -0

+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx);
   void gem_require_ring(int fd, unsigned ring);
   bool gem_has_mocs_registers(int fd);
   void gem_require_mocs_registers(int fd);
+#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0)
+
   /* prime */
   struct local_dma_buf_sync {
   	uint64_t flags;
 

I don't understand why this. All current callers of gem_has_ring pass in eb flags and not an index so how it can work?

Quoted 2 lines

This is because of patch 3/6 this makes the for_each_engine2() able to work with new and old api.

How does it do that? Maybe I am extra slow today..

How is the for_each_engine2 without a helper to get engine flags, or the index variable, supposed to be used if one wants to submit a batch to all engines:

for_each_engine2(...)

	eb(engine=??)

Both in legacy and engine discovery mode.

Regards,

Tvrtko

Quoted 4 lines

Have I messed up the patch order?

Andi

#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0) (*)

Hi Tvrtko,

yes, this patchset has grown and changed many times over the review iterations and unfortunately, it's not very obvius.

Quoted 57 lines

Quoted 57 lines

Quoted 56 lines

Quoted 43 lines

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_context_has_engine()' that requires the index that the engine is mapped within the driver.

The previous 'gem_has_ring()' function becomes a wrapper to the new 'gem_context_has_engine()'.

Signed-off-by Andi Shyti <[email protected]>

---
    lib/ioctl_wrappers.c | 4 +++-
    lib/ioctl_wrappers.h | 4 +++-
    2 files changed, 6 insertions(+), 2 deletions(-)

lib/ioctl_wrappers.c +3 -1

index 39920f8707d2..a2597e282704 100644
@@ -1252,7 +1252,7 @@ void igt_require_gem(int fd)
    	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
    }
-bool gem_has_ring(int fd, unsigned ring)
+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx)
    {
    	struct drm_i915_gem_execbuffer2 execbuf;
    	struct drm_i915_gem_exec_object2 exec;

@@ -1268,6 +1268,8 @@ bool gem_has_ring(int fd, unsigned ring)
    	execbuf.buffers_ptr = to_user_pointer(&exec);
    	execbuf.buffer_count = 1;
    	execbuf.flags = ring;
+	execbuf.rsvd1 = ctx;
+
    	return __gem_execbuf(fd, &execbuf) == -ENOENT;
    }

lib/ioctl_wrappers.h +0 -1

index f0be26080da6..446e973b7449 100644
@@ -142,11 +142,13 @@ bool gem_has_exec_fence(int fd);
    /* check functions which auto-skip tests by calling igt_skip() */
    void gem_require_caching(int fd);
-bool gem_has_ring(int fd, unsigned ring);

lib/ioctl_wrappers.h bool gem_has_exec_fence(int fd); · line 153

Quoted 10 lines

lib/ioctl_wrappers.h +3 -0

+bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx);
    void gem_require_ring(int fd, unsigned ring);
    bool gem_has_mocs_registers(int fd);
    void gem_require_mocs_registers(int fd);
+#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0)
+
    /* prime */
    struct local_dma_buf_sync {
    	uint64_t flags;
 

I don't understand why this. All current callers of gem_has_ring pass in eb flags and not an index so how it can work?

Quoted 4 lines

Quoted 2 lines

This is because of patch 3/6 this makes the for_each_engine2() able to work with new and old api.

How does it do that? Maybe I am extra slow today..

We assume we have the old api (i.e. we use the current intel_execution_engines2[] array):

#define for_each_engine2(fd, ctx) \
             ...
                    |---- the for_if() inverts the logic ----------|
                    V                                              V
                  for_if (gem_has_engine_topology() || \    <--- false
                          gem_has_engine(fd, e2__->class, e2__->instance)) <--- true

gem_has_engine() is the bit that will be called, which translates class/instance to eb flag and calls gem_has_ring(fd, flags), which now is

#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0) (*)

gem_context_has_engine() would work exactly as before and assign ring to execbuf.flags and '0' to execbuf.rsvd1, nothing changes, although the logic is a bit twisted (we still have discussion on names with Chris :) ).

At the same time the "gem_set_context_get_engines()" (which means set context and get engines) has returned intel_execution_engines2[] and we iterate through the preallocated engines.

It works exactly like for_each_engine(...), but using the new "struct intel_execution_engine2" instead of the old "struct intel_execution_engine".

If we have the new uapi, then we don't care, because gem_has_engine_topology is true and we move forward:

#define for_each_engine2(fd, ctx) \
             ...
                  for_if (gem_has_engine_topology() || \    <--- true
                          gem_has_engine(fd, e2__->class, e2__->instance)) <--- does not matter

"gem_set_context_get_engines()" has returned the "intel_active_engines2[]" array that we created by querying the driver.

On the other hand, if you see the subtest "exec-ctx" (patch 6/6), we call exactly the same function without goint through the definition(*) and gem_context_has_engine(...) would work by using the new api:

    gem_context_has_engine(fd, ++index_map, ctx_id));