[igt-dev] [RFC PATCH v4 0/3] new engine discovery interface

[igt-dev] [RFC PATCH v4 0/3] new engine discovery interface

[Andi Shyti]

Hi,

In this patchset I propose an alternative way of engine discovery
thanks to the new interfaces developed by Tvrtko and Chris[4].

Thanks Chris and Tvrtko for your comments in the RFC.

V1 --> V2
=========
RFC v1: [1]
- added a demo test that simply queries the driver about the
  engines and executes a buffer (thanks Tvrtko)
- refactored the for_each_engine_ctx() macro so that what in the
  previous version was done by the "bind" function, now it's done
  in the first iteration. (Thanks Crhis)
- removed the "gem_has_ring_ctx()" because it was out of the
  scope.
- rename functions to more meaningful names

V2 --> V3
=========
RFC v2: [2]
- removed a standalone gem_query_engines_demo test and added the
  exec-ctx subtest inside gem_exec_basic (thanks Tvrtko).
- fixed most of Tvrtko's comments in [5], which consist in
  putting the mallocs igt_assert and ictls in igt_require and few
  refactoring (thanks Tvrtko).

V3 --> V4
=========
PATCH v3: [3]
- re-architectured the discovery mechanism based on Tvrtko's
  sugestion and reviews.. In this version the discovery is done
  during the device opening and stored in a NULL terminated
  array, which replaces the existing intel_execution_engines2
  that is mainly used as a reference.

Andi

[1] RFC v1: https://lists.freedesktop.org/archives/igt-dev/2018-November/007025.html
[2] RFC v2: https://lists.freedesktop.org/archives/igt-dev/2018-November/007079.html
[3] PATCH v3: https://lists.freedesktop.org/archives/igt-dev/2018-November/007148.html
[4] git://people.freedesktop.org/~tursulin/drm-intel
[5] https://lists.freedesktop.org/archives/igt-dev/2018-November/007100.html

Andi Shyti (3):
  include/drm-uapi: import i915_drm.h header file
  lib: implement new engine discovery interface
  tests: gem_exec_basic: add "exec-ctx" buffer execution demo test

 include/drm-uapi/i915_drm.h | 209 +++++++++++++++++++++++++++++++++++-
 lib/drmtest.c               |  12 ++-
 lib/igt_gt.c                |  99 +++++++++++++++--
 lib/igt_gt.h                |  10 +-
 tests/i915/gem_exec_basic.c |  33 ++++++
 5 files changed, 350 insertions(+), 13 deletions(-)

-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [RFC PATCH v4 1/3] include/drm-uapi: import i915_drm.h header file

[Andi Shyti]

This header file is imported in order to include the two new
ioctls DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM and
DRM_IOCTL_I915_QUERY. They are not based on a latest version of
the branch, but based on the

git://people.freedesktop.org/~tursulin/drm-intel

tree, "media" branch. In this RFC it's just to give a meaning to
the next patch.

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
 include/drm-uapi/i915_drm.h | 209 +++++++++++++++++++++++++++++++++++-
 1 file changed, 207 insertions(+), 2 deletions(-)

diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
index e39b26d4bb3d..b14ca9695f1e 100644
--- a/include/drm-uapi/i915_drm.h
+++ b/include/drm-uapi/i915_drm.h
@@ -62,6 +62,26 @@ extern "C" {
 #define I915_ERROR_UEVENT		"ERROR"
 #define I915_RESET_UEVENT		"RESET"
 
+/*
+ * i915_user_extension: Base class for defining a chain of extensions
+ *
+ * Many interfaces need to grow over time. In most cases we can simply
+ * extend the struct and have userspace pass in more data. Another option,
+ * as demonstrated by Vulkan's approach to providing extensions for forward
+ * and backward compatibility, is to use a list of optional structs to
+ * provide those extra details.
+ *
+ * The key advantage to using an extension chain is that it allows us to
+ * redefine the interface more easily than an ever growing struct of
+ * increasing complexity, and for large parts of that interface to be
+ * entirely optional. The downside is more pointer chasing; chasing across
+ * the boundary with pointers encapsulated inside u64.
+ */
+struct i915_user_extension {
+	__u64 next_extension;
+	__u64 name;
+};
+
 /*
  * MOCS indexes used for GPU surfaces, defining the cacheability of the
  * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
@@ -367,6 +387,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
 #define DRM_IOCTL_I915_GEM_WAIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait)
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
+#define DRM_IOCTL_I915_GEM_CONTEXT_CREATE_v2	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create_v2)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
 #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
 #define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
@@ -559,6 +580,13 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_MMAP_GTT_COHERENT	52
 
+/*
+ * Query whether DRM_I915_GEM_EXECBUFFER2 supports coordination of parallel
+ * execution through use of explicit fence support.
+ * See I915_EXEC_FENCE_OUT and I915_EXEC_FENCE_SUBMIT.
+ */
+#define I915_PARAM_HAS_EXEC_SUBMIT_FENCE 53
+
 typedef struct drm_i915_getparam {
 	__s32 param;
 	/*
@@ -972,7 +1000,7 @@ struct drm_i915_gem_execbuffer2 {
 	 * struct drm_i915_gem_exec_fence *fences.
 	 */
 	__u64 cliprects_ptr;
-#define I915_EXEC_RING_MASK              (7<<0)
+#define I915_EXEC_RING_MASK              (0x3f)
 #define I915_EXEC_DEFAULT                (0<<0)
 #define I915_EXEC_RENDER                 (1<<0)
 #define I915_EXEC_BSD                    (2<<0)
@@ -1078,7 +1106,16 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_FENCE_ARRAY   (1<<19)
 
-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_ARRAY<<1))
+/*
+ * Setting I915_EXEC_FENCE_SUBMIT implies that lower_32_bits(rsvd2) represent
+ * a sync_file fd to wait upon (in a nonblocking manner) prior to executing
+ * the batch.
+ *
+ * Returns -EINVAL if the sync_file fd cannot be found.
+ */
+#define I915_EXEC_FENCE_SUBMIT		(1<<20)
+
+#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_SUBMIT<<1))
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
@@ -1417,6 +1454,16 @@ struct drm_i915_gem_context_create {
 	__u32 pad;
 };
 
+struct drm_i915_gem_context_create_v2 {
+	/*  output: id of new context*/
+	__u32 ctx_id;
+	__u32 flags;
+#define I915_GEM_CONTEXT_SHARE_GTT		0x1
+#define I915_GEM_CONTEXT_SINGLE_TIMELINE	0x2
+	__u32 share_ctx;
+	__u32 pad;
+};
+
 struct drm_i915_gem_context_destroy {
 	__u32 ctx_id;
 	__u32 pad;
@@ -1486,9 +1533,122 @@ struct drm_i915_gem_context_param {
 #define   I915_CONTEXT_MAX_USER_PRIORITY	1023 /* inclusive */
 #define   I915_CONTEXT_DEFAULT_PRIORITY		0
 #define   I915_CONTEXT_MIN_USER_PRIORITY	-1023 /* inclusive */
+
+/*
+ * I915_CONTEXT_PARAM_ENGINES:
+ *
+ * Bind this context to operate on this subset of available engines. Henceforth,
+ * the I915_EXEC_RING selector for DRM_IOCTL_I915_GEM_EXECBUFFER2 operates as
+ * an index into this array of engines; I915_EXEC_DEFAULT selecting engine[0]
+ * and upwards. The array created is offset by 1, such that by default
+ * I915_EXEC_DEFAULT is left empty, to be filled in as directed. Slots 1...N
+ * are then filled in using the specified (class, instance).
+ *
+ * Setting the number of engines bound to the context will revert back to
+ * default settings.
+ *
+ * See struct i915_context_param_engines.
+ *
+ * Extensions:
+ *   i915_context_engines_load_balance (I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE)
+ *   i915_context_engines_bond (I915_CONTEXT_ENGINES_EXT_BOND)
+ */
+#define I915_CONTEXT_PARAM_ENGINES	0x7
+
+/*
+ * When using the following param, value should be a pointer to
+ * drm_i915_gem_context_param_sseu.
+ */
+#define I915_CONTEXT_PARAM_SSEU		0x8
+
 	__u64 value;
 };
 
+/*
+ * i915_context_engines_load_balance:
+ *
+ * Enable load balancing across this set of engines.
+ *
+ * Into the I915_EXEC_DEFAULT slot, a virtual engine is created that when
+ * used will proxy the execbuffer request onto one of the set of engines
+ * in such a way as to distribute the load evenly across the set.
+ *
+ * The set of engines must be compatible (e.g. the same HW class) as they
+ * will share the same logical GPU context and ring.
+ *
+ * The context must be defined to use a single timeline for all engines.
+ */
+struct i915_context_engines_load_balance {
+	struct i915_user_extension base;
+
+	__u64 flags; /* all undefined flags must be zero */
+	__u64 engines_mask;
+
+	__u64 mbz[4]; /* reserved for future use; must be zero */
+};
+
+/*
+ * i915_context_engines_bond:
+ *
+ */
+struct i915_context_engines_bond {
+	struct i915_user_extension base;
+
+	__u16 master_class;
+	__u16 master_instance;
+	__u32 flags; /* all undefined flags must be zero */
+	__u64 sibling_mask;
+};
+
+struct i915_context_param_engines {
+	__u64 extensions;
+#define I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE 0
+#define I915_CONTEXT_ENGINES_EXT_BOND 1
+
+	struct {
+		__u16 class; /* see enum drm_i915_gem_engine_class */
+		__u16 instance;
+	} class_instance[0];
+};
+
+struct drm_i915_gem_context_param_sseu {
+	/*
+	 * Engine class & instance to be configured or queried.
+	 */
+	__u16 class;
+	__u16 instance;
+
+	/*
+	 * Unused for now. Must be cleared to zero.
+	 */
+	__u32 rsvd1;
+
+	/*
+	 * Mask of slices to enable for the context. Valid values are a subset
+	 * of the bitmask value returned for I915_PARAM_SLICE_MASK.
+	 */
+	__u64 slice_mask;
+
+	/*
+	 * Mask of subslices to enable for the context. Valid values are a
+	 * subset of the bitmask value return by I915_PARAM_SUBSLICE_MASK.
+	 */
+	__u64 subslice_mask;
+
+	/*
+	 * Minimum/Maximum number of EUs to enable per subslice for the
+	 * context. min_eus_per_subslice must be inferior or equal to
+	 * max_eus_per_subslice.
+	 */
+	__u16 min_eus_per_subslice;
+	__u16 max_eus_per_subslice;
+
+	/*
+	 * Unused for now. Must be cleared to zero.
+	 */
+	__u32 rsvd2;
+};
+
 enum drm_i915_oa_format {
 	I915_OA_FORMAT_A13 = 1,	    /* HSW only */
 	I915_OA_FORMAT_A29,	    /* HSW only */
@@ -1650,6 +1810,7 @@ struct drm_i915_perf_oa_config {
 struct drm_i915_query_item {
 	__u64 query_id;
 #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
 	/*
 	 * When set to zero by userspace, this is filled with the size of the
@@ -1747,6 +1908,50 @@ struct drm_i915_query_topology_info {
 	__u8 data[];
 };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine and it's capabilities as known to the driver.
+ */
+struct drm_i915_engine_info {
+	/** Engine class as in enum drm_i915_gem_engine_class. */
+	__u16 class;
+
+	/** Engine instance number. */
+	__u16 instance;
+
+	/** Reserved field. */
+	__u32 rsvd0;
+
+	/** Engine flags. */
+	__u64 flags;
+
+	/** Capabilities of this engine. */
+	__u64 capabilities;
+#define I915_VIDEO_CLASS_CAPABILITY_HEVC		(1 << 0)
+#define I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC	(1 << 1)
+
+	/** Reserved fields. */
+	__u64 rsvd1[4];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+	/** Number of struct drm_i915_engine_info structs following. */
+	__u32 num_engines;
+
+	/** MBZ */
+	__u32 rsvd[3];
+
+	/** Marker for drm_i915_engine_info structures. */
+	struct drm_i915_engine_info engines[];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Andi Shyti]

Kernel commits:

[1] ae8f4544dd8f ("drm/i915: Engine discovery query")
[2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")

from [*] repository, implement a new uapi for engine discovery
that consist in first querying the driver about the engines in
the gpu [1] and then binding a context to the set of engines that
it can access [2].

In igt the classic way for discovering engines is done through
the for_each_physical_engine() macro, that would be replaced by
the new for_each_engine_ctx().

A new function is added gem_init_engine_list() that is called
during device open which creates the list of engines. That list
is stored in the intel_execution_engines2 that replaces the
current array which has more a reference meaning. Now the
intel_execution_engines2 stores the engines currently present in
the GPU.

[*] git://people.freedesktop.org/~tursulin/drm-intel

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
 lib/drmtest.c | 12 +++++--
 lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
 lib/igt_gt.h  | 10 +++++-
 3 files changed, 110 insertions(+), 11 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 7c124ac666ec..2d155eea8a13 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 
 	fd = __search_and_open(base, offset, chipset);
 	if (fd != -1)
-		return fd;
+		goto set_engines_and_return;
 
 	pthread_mutex_lock(&mutex);
 	for (const struct module *m = modules; m->module; m++) {
@@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
 	}
 	pthread_mutex_unlock(&mutex);
 
-	return __search_and_open(base, offset, chipset);
+	fd = __search_and_open(base, offset, chipset);
+	if (fd < 0)
+		return fd;
+
+set_engines_and_return:
+	if (is_i915_device(fd))
+		gem_init_engine_list(fd);
+
+	return fd;
 }
 
 /**
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a20619246296..ab73d4b45087 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -577,14 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
 	return true;
 }
 
-const struct intel_execution_engine2 intel_execution_engines2[] = {
-	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
-	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
-	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
-	{ "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
-	{ "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
-	{ }
-};
+struct intel_execution_engine2 intel_execution_engines2[32] = { };
 
 unsigned int
 gem_class_instance_to_eb_flags(int gem_fd,
@@ -650,3 +643,93 @@ 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_assert(!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_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
+
+	return query_engines;
+}
+
+void __set_ctx_engine_map(int fd, uint32_t ctx_id)
+{
+	int i;
+	unsigned char n;
+	struct drm_i915_gem_context_param ctx_param;
+	struct i915_context_param_engines *ctx_engine;
+	size_t size;
+
+	for (n = 0; intel_execution_engines2[n].name; n++);
+
+	size = sizeof(struct i915_context_param_engines) +
+		      (n + 1) * sizeof(*ctx_engine->class_instance);
+
+	igt_assert((ctx_engine = malloc(size)));
+
+	ctx_engine->extensions = 0;
+	for (i = 0; i <= n; i++) {
+		ctx_engine->class_instance[i].class =
+					intel_execution_engines2[i].class;
+		ctx_engine->class_instance[i].instance =
+					intel_execution_engines2[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);
+
+	igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
+
+	free(ctx_engine);
+}
+
+void gem_init_engine_list(int fd)
+{
+	int i;
+	struct drm_i915_query_engine_info *query_engine = query_engines(fd);
+	const char *engine_names[] = { "rcs", "bcs", "vcs", "vecs" };
+
+	for (i = 0; i < query_engine->num_engines; i++) {
+		char *name;
+		int class = query_engine->engines[i].class;
+		int instance = query_engine->engines[i].instance;
+
+		igt_require(class < ARRAY_SIZE(engine_names) && class >= 0);
+		igt_require(engine_names[class]);
+
+		intel_execution_engines2[i].class = class;
+		intel_execution_engines2[i].instance = instance;
+		intel_execution_engines2[i].map_index = i + 1;
+
+		igt_assert(asprintf(&name, "%s%d",
+				    engine_names[class], instance) > 0);
+		intel_execution_engines2[i].name = name;
+	}
+
+	free(query_engine);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 54e95da98084..555c04ea98c3 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -86,17 +86,25 @@ extern const struct intel_execution_engine {
 	     e__++) \
 		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
 
+#define for_each_engine_ctx(fd, ctx, e) \
+		for (__set_ctx_engine_map(fd, ctx_id), \
+				e = intel_execution_engines2; e->name; e++)
+
 bool gem_ring_is_physical_engine(int fd, unsigned int ring);
 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;
+	int map_index;
 } intel_execution_engines2[];
 
+void gem_init_engine_list(int fd);
+void __set_ctx_engine_map(int fd, uint32_t ctx_id);
+
 unsigned int
 gem_class_instance_to_eb_flags(int gem_fd,
 			       enum drm_i915_gem_engine_class class,
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [RFC PATCH v4 3/3] tests: gem_exec_basic: add "exec-ctx" buffer execution demo test

[Andi Shyti]

The "exec-ctx" is a demo subtest inserted in the gem_exec_basic
test. The main scope is to demonstrate an alternative way for
querying engines as implemented in the new API in commit 87079e04
("lib: implement new engine discovery interface").

The "exec-ctx" subtest simply gets the list of engines, binds
them to a context and executes a buffer. This is done through a
new "for_each_engine_ctx" loop which iterates through the
engines.

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
 tests/i915/gem_exec_basic.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
index dcb83864b1c1..2b09f8cd79b7 100644
--- a/tests/i915/gem_exec_basic.c
+++ b/tests/i915/gem_exec_basic.c
@@ -114,6 +114,22 @@ static void gtt(int fd, unsigned ring)
 	munmap(execbuf, 4096);
 }
 
+static void execbuf_in_ctx(int fd, struct intel_execution_engine2 *engine,
+			   uint32_t ctx, uint32_t handle)
+{
+	struct drm_i915_gem_exec_object2 exec = {
+		.handle = handle,
+	};
+	struct drm_i915_gem_execbuffer2 execbuf = {
+		.buffers_ptr = to_user_pointer(&exec),
+		.buffer_count = 1,
+		.flags = engine->map_index,
+		.rsvd1 = ctx,
+	};
+
+	gem_execbuf(fd, &execbuf);
+}
+
 igt_main
 {
 	const struct intel_execution_engine *e;
@@ -135,6 +151,23 @@ igt_main
 			gtt(fd, e->exec_id | e->flags);
 	}
 
+	igt_subtest("exec-ctx") {
+		uint32_t ctx_id;
+		uint32_t handle;
+		uint32_t bbend = MI_BATCH_BUFFER_END;
+		struct intel_execution_engine2 *e2;
+
+		ctx_id = gem_context_create(fd);
+		handle = gem_create(fd, 4096);
+		gem_write(fd, handle, 0, &bbend, sizeof(bbend));
+
+		for_each_engine_ctx(fd, ctx_id, e2)
+			execbuf_in_ctx(fd, e2, ctx_id, handle);
+
+		gem_close(fd, handle);
+		gem_context_destroy(fd, ctx_id);
+	}
+
 	igt_fixture {
 		igt_stop_hang_detector();
 		close(fd);
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for new engine discovery interface (rev5)

[Patchwork]

== Series Details ==

Series: new engine discovery interface (rev5)
URL   : https://patchwork.freedesktop.org/series/52699/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
951e2b1a016b750544d0f42459b13b9c70631c68 intel-ci: Drop gem_ctx_switch/heavy from BAT

239/263 testcase check: gen3_render_linear_blits  OK       0.16 s
240/263 testcase check: gen3_render_mixed_blits  OK       0.13 s
241/263 testcase check: gen3_render_tiledx_blits  OK       0.16 s
242/263 testcase check: gen3_render_tiledy_blits  OK       0.16 s
243/263 testcase check: gem_eio                 OK       0.14 s
244/263 testcase check: gem_mocs_settings       OK       0.12 s
245/263 testcase check: perf_pmu                OK       0.15 s
246/263 testcase check: testdisplay             OK       0.17 s
247/263 testcase check: amdgpu/amd_abm          OK       0.12 s
248/263 testcase check: amdgpu/amd_basic        OK       0.11 s
249/263 testcase check: amdgpu/amd_cs_nop       OK       0.12 s
250/263 testcase check: amdgpu/amd_prime        OK       0.13 s
251/263 runner                                  OK       1.36 s
252/263 runner_json                             OK       0.09 s
253/263 assembler: test/mov                     OK       0.08 s
254/263 assembler: test/frc                     OK       0.07 s
255/263 assembler: test/regtype                 OK       0.08 s
256/263 assembler: test/rndd                    OK       0.09 s
257/263 assembler: test/rndu                    OK       0.08 s
258/263 assembler: test/rnde                    OK       0.08 s
259/263 assembler: test/rnde-intsrc             OK       0.07 s
260/263 assembler: test/rndz                    OK       0.06 s
261/263 assembler: test/lzd                     OK       0.06 s
262/263 assembler: test/not                     OK       0.03 s
263/263 assembler: test/immediate               OK       0.06 s

OK:       262
FAIL:       1
SKIP:       0
TIMEOUT:    0


The output from the failed tests:

137/263 testcase check: gem_ctx_isolation       FAIL     0.22 s

--- command ---
/home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_ctx_isolation
--- stdout ---
tests/gem_ctx_isolation:
  Checking invalid option handling...
  Checking valid option handling...
  Checking subtest enumeration...
FAIL: tests/gem_ctx_isolation
-------

Full log written to /home/cidrm/igt-gpu-tools/build/meson-logs/testlog.txt
FAILED: meson-test 
/usr/bin/python3 -u /usr/bin/meson test --no-rebuild --print-errorlogs
ninja: build stopped: subcommand failed.

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

Quoting Andi Shyti (2019-01-15 12:35:10)
> Kernel commits:
> 
> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> 
> from [*] repository, implement a new uapi for engine discovery
> that consist in first querying the driver about the engines in
> the gpu [1] and then binding a context to the set of engines that
> it can access [2].
> 
> In igt the classic way for discovering engines is done through
> the for_each_physical_engine() macro, that would be replaced by
> the new for_each_engine_ctx().
> 
> A new function is added gem_init_engine_list() that is called
> during device open which creates the list of engines. That list
> is stored in the intel_execution_engines2 that replaces the
> current array which has more a reference meaning. Now the
> intel_execution_engines2 stores the engines currently present in
> the GPU.
> 
> [*] git://people.freedesktop.org/~tursulin/drm-intel
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>  lib/drmtest.c | 12 +++++--
>  lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  lib/igt_gt.h  | 10 +++++-
>  3 files changed, 110 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 7c124ac666ec..2d155eea8a13 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>  
>         fd = __search_and_open(base, offset, chipset);
>         if (fd != -1)
> -               return fd;
> +               goto set_engines_and_return;
>  
>         pthread_mutex_lock(&mutex);
>         for (const struct module *m = modules; m->module; m++) {
> @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>         }
>         pthread_mutex_unlock(&mutex);
>  
> -       return __search_and_open(base, offset, chipset);
> +       fd = __search_and_open(base, offset, chipset);
> +       if (fd < 0)
> +               return fd;
> +
> +set_engines_and_return:
> +       if (is_i915_device(fd))
> +               gem_init_engine_list(fd);

Do we really want more implicit actions on opening an fd?

We already have igt_require_gem() which would make an interesting
starting point, for that we may want to use fd not from
drm_open_driver(). However, there seems to be no issue with creating the
names on the fly (and ask for the complementary getter for engines[] so
that an index could be translated back to class:instance).

Certainly having drmtest presume GEM (and be subject to all of the extra
rules) given i915 seems a bit rude.

> +
> +       return fd;
>  }
>  
>  /**
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a20619246296..ab73d4b45087 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -577,14 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>         return true;
>  }
>  
> -const struct intel_execution_engine2 intel_execution_engines2[] = {
> -       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> -       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> -       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> -       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> -       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> -       { }
> -};
> +struct intel_execution_engine2 intel_execution_engines2[32] = { };
>  
>  unsigned int
>  gem_class_instance_to_eb_flags(int gem_fd,
> @@ -650,3 +643,93 @@ 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);

Nah, do not imply you are tied to implementation details - that is the
whole point of querying the length first. Do note that you can over
allocate (say use a small bit of stack) and do the query in one shot,
only allocating from heap if we need more room.

> +        */
> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

Bad news for old kernels.

> +       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_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> +
> +       return query_engines;
> +}
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> +{
> +       int i;
> +       unsigned char n;
> +       struct drm_i915_gem_context_param ctx_param;
> +       struct i915_context_param_engines *ctx_engine;
> +       size_t size;
> +
> +       for (n = 0; intel_execution_engines2[n].name; n++);

Close your eyes and tell me where the ';' is.

> +       size = sizeof(struct i915_context_param_engines) +
> +                     (n + 1) * sizeof(*ctx_engine->class_instance);

Should be small enough for a stack allocations of say 64 engines (the
limit of the current execbuf uabi).

> +       igt_assert((ctx_engine = malloc(size)));
> +
> +       ctx_engine->extensions = 0;
> +       for (i = 0; i <= n; i++) {
> +               ctx_engine->class_instance[i].class =
> +                                       intel_execution_engines2[i].class;
> +               ctx_engine->class_instance[i].instance =
> +                                       intel_execution_engines2[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);
> +
> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));

Do you really want to risk that this won't be interrupted by a signal at
any point in the future?

> +
> +       free(ctx_engine);
> +}
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [RFC PATCH v4 3/3] tests: gem_exec_basic: add "exec-ctx" buffer execution demo test

[Chris Wilson]

Quoting Andi Shyti (2019-01-15 12:35:11)
> The "exec-ctx" is a demo subtest inserted in the gem_exec_basic
> test. The main scope is to demonstrate an alternative way for
> querying engines as implemented in the new API in commit 87079e04
> ("lib: implement new engine discovery interface").
> 
> The "exec-ctx" subtest simply gets the list of engines, binds
> them to a context and executes a buffer. This is done through a
> new "for_each_engine_ctx" loop which iterates through the
> engines.
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>  tests/i915/gem_exec_basic.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
> index dcb83864b1c1..2b09f8cd79b7 100644
> --- a/tests/i915/gem_exec_basic.c
> +++ b/tests/i915/gem_exec_basic.c
> @@ -114,6 +114,22 @@ static void gtt(int fd, unsigned ring)
>         munmap(execbuf, 4096);
>  }
>  
> +static void execbuf_in_ctx(int fd, struct intel_execution_engine2 *engine,
> +                          uint32_t ctx, uint32_t handle)
> +{
> +       struct drm_i915_gem_exec_object2 exec = {
> +               .handle = handle,
> +       };
> +       struct drm_i915_gem_execbuffer2 execbuf = {
> +               .buffers_ptr = to_user_pointer(&exec),
> +               .buffer_count = 1,
> +               .flags = engine->map_index,

No. Do not store engine->map_index in the global array, as that is
per-context and I would like to iterate over different arrays of engines.

> +               .rsvd1 = ctx,
> +       };
> +
> +       gem_execbuf(fd, &execbuf);

This doesn't actually prove anything other than no -EINVAL. How many
different ways might we have accidentally caused success. Could we feed
in a known failure execbuf and then look for specific failures? (Hint,
what happens if handle = 0.)
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Tvrtko Ursulin]

On 15/01/2019 13:00, Chris Wilson wrote:
> Quoting Andi Shyti (2019-01-15 12:35:10)
>> Kernel commits:
>>
>> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
>> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
>>
>> from [*] repository, implement a new uapi for engine discovery
>> that consist in first querying the driver about the engines in
>> the gpu [1] and then binding a context to the set of engines that
>> it can access [2].
>>
>> In igt the classic way for discovering engines is done through
>> the for_each_physical_engine() macro, that would be replaced by
>> the new for_each_engine_ctx().
>>
>> A new function is added gem_init_engine_list() that is called
>> during device open which creates the list of engines. That list
>> is stored in the intel_execution_engines2 that replaces the
>> current array which has more a reference meaning. Now the
>> intel_execution_engines2 stores the engines currently present in
>> the GPU.
>>
>> [*] git://people.freedesktop.org/~tursulin/drm-intel
>>
>> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
>> ---
>>   lib/drmtest.c | 12 +++++--
>>   lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>   lib/igt_gt.h  | 10 +++++-
>>   3 files changed, 110 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/drmtest.c b/lib/drmtest.c
>> index 7c124ac666ec..2d155eea8a13 100644
>> --- a/lib/drmtest.c
>> +++ b/lib/drmtest.c
>> @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>>   
>>          fd = __search_and_open(base, offset, chipset);
>>          if (fd != -1)
>> -               return fd;
>> +               goto set_engines_and_return;
>>   
>>          pthread_mutex_lock(&mutex);
>>          for (const struct module *m = modules; m->module; m++) {
>> @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>>          }
>>          pthread_mutex_unlock(&mutex);
>>   
>> -       return __search_and_open(base, offset, chipset);
>> +       fd = __search_and_open(base, offset, chipset);
>> +       if (fd < 0)
>> +               return fd;
>> +
>> +set_engines_and_return:
>> +       if (is_i915_device(fd))
>> +               gem_init_engine_list(fd);
> 
> Do we really want more implicit actions on opening an fd?
> 
> We already have igt_require_gem() which would make an interesting
> starting point, for that we may want to use fd not from
> drm_open_driver(). However, there seems to be no issue with creating the
> names on the fly (and ask for the complementary getter for engines[] so
> that an index could be translated back to class:instance).
> 
> Certainly having drmtest presume GEM (and be subject to all of the extra
> rules) given i915 seems a bit rude.

My suggestion was to co-site with existing "is i915" code in 
drm_open_driver and drm_open_driver_render. But igt_require_gem also 
sounds okay.

I didn't understand the bit about the complementary getter.

> 
>> +
>> +       return fd;
>>   }
>>   
>>   /**
>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>> index a20619246296..ab73d4b45087 100644
>> --- a/lib/igt_gt.c
>> +++ b/lib/igt_gt.c
>> @@ -577,14 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>>          return true;
>>   }
>>   
>> -const struct intel_execution_engine2 intel_execution_engines2[] = {
>> -       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>> -       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>> -       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
>> -       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
>> -       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
>> -       { }
>> -};
>> +struct intel_execution_engine2 intel_execution_engines2[32] = { };

I'd just make it a pointer, both size and explicit static initialization 
to zero are not needed.

>>   
>>   unsigned int
>>   gem_class_instance_to_eb_flags(int gem_fd,
>> @@ -650,3 +643,93 @@ 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);
> 
> Nah, do not imply you are tied to implementation details - that is the
> whole point of querying the length first. Do note that you can over
> allocate (say use a small bit of stack) and do the query in one shot,
> only allocating from heap if we need more room.
> 
>> +        */
>> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> 
> Bad news for old kernels.

Do we care about old kernels? I thought for IGT we did not.

Regards,

Tvrtko

> 
>> +       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_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
>> +
>> +       return query_engines;
>> +}
>> +
>> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
>> +{
>> +       int i;
>> +       unsigned char n;
>> +       struct drm_i915_gem_context_param ctx_param;
>> +       struct i915_context_param_engines *ctx_engine;
>> +       size_t size;
>> +
>> +       for (n = 0; intel_execution_engines2[n].name; n++);
> 
> Close your eyes and tell me where the ';' is.
> 
>> +       size = sizeof(struct i915_context_param_engines) +
>> +                     (n + 1) * sizeof(*ctx_engine->class_instance);
> 
> Should be small enough for a stack allocations of say 64 engines (the
> limit of the current execbuf uabi).
> 
>> +       igt_assert((ctx_engine = malloc(size)));
>> +
>> +       ctx_engine->extensions = 0;
>> +       for (i = 0; i <= n; i++) {
>> +               ctx_engine->class_instance[i].class =
>> +                                       intel_execution_engines2[i].class;
>> +               ctx_engine->class_instance[i].instance =
>> +                                       intel_execution_engines2[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);
>> +
>> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> 
> Do you really want to risk that this won't be interrupted by a signal at
> any point in the future?
> 
>> +
>> +       free(ctx_engine);
>> +}
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 13:14:10)
> 
> On 15/01/2019 13:00, Chris Wilson wrote:
> > Quoting Andi Shyti (2019-01-15 12:35:10)
> >> Kernel commits:
> >>
> >> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> >> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> >>
> >> from [*] repository, implement a new uapi for engine discovery
> >> that consist in first querying the driver about the engines in
> >> the gpu [1] and then binding a context to the set of engines that
> >> it can access [2].
> >>
> >> In igt the classic way for discovering engines is done through
> >> the for_each_physical_engine() macro, that would be replaced by
> >> the new for_each_engine_ctx().
> >>
> >> A new function is added gem_init_engine_list() that is called
> >> during device open which creates the list of engines. That list
> >> is stored in the intel_execution_engines2 that replaces the
> >> current array which has more a reference meaning. Now the
> >> intel_execution_engines2 stores the engines currently present in
> >> the GPU.
> >>
> >> [*] git://people.freedesktop.org/~tursulin/drm-intel
> >>
> >> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> >> ---
> >>   lib/drmtest.c | 12 +++++--
> >>   lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >>   lib/igt_gt.h  | 10 +++++-
> >>   3 files changed, 110 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/lib/drmtest.c b/lib/drmtest.c
> >> index 7c124ac666ec..2d155eea8a13 100644
> >> --- a/lib/drmtest.c
> >> +++ b/lib/drmtest.c
> >> @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >>   
> >>          fd = __search_and_open(base, offset, chipset);
> >>          if (fd != -1)
> >> -               return fd;
> >> +               goto set_engines_and_return;
> >>   
> >>          pthread_mutex_lock(&mutex);
> >>          for (const struct module *m = modules; m->module; m++) {
> >> @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >>          }
> >>          pthread_mutex_unlock(&mutex);
> >>   
> >> -       return __search_and_open(base, offset, chipset);
> >> +       fd = __search_and_open(base, offset, chipset);
> >> +       if (fd < 0)
> >> +               return fd;
> >> +
> >> +set_engines_and_return:
> >> +       if (is_i915_device(fd))
> >> +               gem_init_engine_list(fd);
> > 
> > Do we really want more implicit actions on opening an fd?
> > 
> > We already have igt_require_gem() which would make an interesting
> > starting point, for that we may want to use fd not from
> > drm_open_driver(). However, there seems to be no issue with creating the
> > names on the fly (and ask for the complementary getter for engines[] so
> > that an index could be translated back to class:instance).
> > 
> > Certainly having drmtest presume GEM (and be subject to all of the extra
> > rules) given i915 seems a bit rude.
> 
> My suggestion was to co-site with existing "is i915" code in 
> drm_open_driver and drm_open_driver_render. But igt_require_gem also 
> sounds okay.
> 
> I didn't understand the bit about the complementary getter.

Just we're missing the GETPARAM engines interface, which I think could be
used to retrieve the information required for pretty printing at runtime,
and so avoid global tables that may not match my context.

> >> +       /*
> >> +        * 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);
> > 
> > Nah, do not imply you are tied to implementation details - that is the
> > whole point of querying the length first. Do note that you can over
> > allocate (say use a small bit of stack) and do the query in one shot,
> > only allocating from heap if we need more room.
> > 
> >> +        */
> >> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> > 
> > Bad news for old kernels.
> 
> Do we care about old kernels? I thought for IGT we did not.

We supposedly do! :)

We definitely do care about being able to bisect kernels from an arm's
length ago. Beyond an arm's length, no one will notice. Certainly we
don't intend to make things harder than we need to. :)
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Andi Shyti]

Hi Chris,

thanks for looking into this!

> > @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >  
> >         fd = __search_and_open(base, offset, chipset);
> >         if (fd != -1)
> > -               return fd;
> > +               goto set_engines_and_return;
> >  
> >         pthread_mutex_lock(&mutex);
> >         for (const struct module *m = modules; m->module; m++) {
> > @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >         }
> >         pthread_mutex_unlock(&mutex);
> >  
> > -       return __search_and_open(base, offset, chipset);
> > +       fd = __search_and_open(base, offset, chipset);
> > +       if (fd < 0)
> > +               return fd;
> > +
> > +set_engines_and_return:
> > +       if (is_i915_device(fd))
> > +               gem_init_engine_list(fd);
> 
> Do we really want more implicit actions on opening an fd?
> 
> We already have igt_require_gem() which would make an interesting
> starting point, for that we may want to use fd not from
> drm_open_driver(). However, there seems to be no issue with creating the
> names on the fly (and ask for the complementary getter for engines[] so
> that an index could be translated back to class:instance).
> 
> Certainly having drmtest presume GEM (and be subject to all of the extra
> rules) given i915 seems a bit rude.

OK, I can try it out in igt_require_gem().

> > +       /*
> > +        * 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);
> 
> Nah, do not imply you are tied to implementation details - that is the
> whole point of querying the length first. Do note that you can over
> allocate (say use a small bit of stack) and do the query in one shot,
> only allocating from heap if we need more room.

Sure!

> > +        */
> > +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> 
> Bad news for old kernels.

No bit of this patch would work on old kernels. I thought this
was the reason to create a second for_each_physical_engine2
(which I called for_each_engine_ctx()).

Maybe in a next patch I can think of reworking things a bit (in
the igt_gt.h) so that we choose the right "for_each..." based on
the kernel's API.

> > +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> > +{
> > +       int i;
> > +       unsigned char n;
> > +       struct drm_i915_gem_context_param ctx_param;
> > +       struct i915_context_param_engines *ctx_engine;
> > +       size_t size;
> > +
> > +       for (n = 0; intel_execution_engines2[n].name; n++);
> 
> Close your eyes and tell me where the ';' is.

:)

> > +       size = sizeof(struct i915_context_param_engines) +
> > +                     (n + 1) * sizeof(*ctx_engine->class_instance);
> 
> Should be small enough for a stack allocations of say 64 engines (the
> limit of the current execbuf uabi).

I will over-allocate here as well, so that I remove the ';' that
you don't like.

> > +       igt_assert((ctx_engine = malloc(size)));
> > +
> > +       ctx_engine->extensions = 0;
> > +       for (i = 0; i <= n; i++) {
> > +               ctx_engine->class_instance[i].class =
> > +                                       intel_execution_engines2[i].class;
> > +               ctx_engine->class_instance[i].instance =
> > +                                       intel_execution_engines2[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);
> > +
> > +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> 
> Do you really want to risk that this won't be interrupted by a signal at
> any point in the future?

Sorry, I don't understand, everything can be interrupted by
signals. What are you suggesting?

Thanks,
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Tvrtko Ursulin]

On 15/01/2019 13:19, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-15 13:14:10)
>>
>> On 15/01/2019 13:00, Chris Wilson wrote:
>>> Quoting Andi Shyti (2019-01-15 12:35:10)
>>>> Kernel commits:
>>>>
>>>> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
>>>> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
>>>>
>>>> from [*] repository, implement a new uapi for engine discovery
>>>> that consist in first querying the driver about the engines in
>>>> the gpu [1] and then binding a context to the set of engines that
>>>> it can access [2].
>>>>
>>>> In igt the classic way for discovering engines is done through
>>>> the for_each_physical_engine() macro, that would be replaced by
>>>> the new for_each_engine_ctx().
>>>>
>>>> A new function is added gem_init_engine_list() that is called
>>>> during device open which creates the list of engines. That list
>>>> is stored in the intel_execution_engines2 that replaces the
>>>> current array which has more a reference meaning. Now the
>>>> intel_execution_engines2 stores the engines currently present in
>>>> the GPU.
>>>>
>>>> [*] git://people.freedesktop.org/~tursulin/drm-intel
>>>>
>>>> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
>>>> ---
>>>>    lib/drmtest.c | 12 +++++--
>>>>    lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>>    lib/igt_gt.h  | 10 +++++-
>>>>    3 files changed, 110 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/lib/drmtest.c b/lib/drmtest.c
>>>> index 7c124ac666ec..2d155eea8a13 100644
>>>> --- a/lib/drmtest.c
>>>> +++ b/lib/drmtest.c
>>>> @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>>>>    
>>>>           fd = __search_and_open(base, offset, chipset);
>>>>           if (fd != -1)
>>>> -               return fd;
>>>> +               goto set_engines_and_return;
>>>>    
>>>>           pthread_mutex_lock(&mutex);
>>>>           for (const struct module *m = modules; m->module; m++) {
>>>> @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>>>>           }
>>>>           pthread_mutex_unlock(&mutex);
>>>>    
>>>> -       return __search_and_open(base, offset, chipset);
>>>> +       fd = __search_and_open(base, offset, chipset);
>>>> +       if (fd < 0)
>>>> +               return fd;
>>>> +
>>>> +set_engines_and_return:
>>>> +       if (is_i915_device(fd))
>>>> +               gem_init_engine_list(fd);
>>>
>>> Do we really want more implicit actions on opening an fd?
>>>
>>> We already have igt_require_gem() which would make an interesting
>>> starting point, for that we may want to use fd not from
>>> drm_open_driver(). However, there seems to be no issue with creating the
>>> names on the fly (and ask for the complementary getter for engines[] so
>>> that an index could be translated back to class:instance).
>>>
>>> Certainly having drmtest presume GEM (and be subject to all of the extra
>>> rules) given i915 seems a bit rude.
>>
>> My suggestion was to co-site with existing "is i915" code in
>> drm_open_driver and drm_open_driver_render. But igt_require_gem also
>> sounds okay.
>>
>> I didn't understand the bit about the complementary getter.
> 
> Just we're missing the GETPARAM engines interface, which I think could be
> used to retrieve the information required for pretty printing at runtime,
> and so avoid global tables that may not match my context.

Are you imagining for_each_engine_ctx to query if the context already 
has engine map set up and in that case iterate over those engines? And 
if there is no engine map to set up one containing all engines?

Or actually, maybe the former as the only behaviour of 
for_each_engine_ctx, and only for_each_physical_engine auto-configuring 
the map?

>>>> +       /*
>>>> +        * 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);
>>>
>>> Nah, do not imply you are tied to implementation details - that is the
>>> whole point of querying the length first. Do note that you can over
>>> allocate (say use a small bit of stack) and do the query in one shot,
>>> only allocating from heap if we need more room.
>>>
>>>> +        */
>>>> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
>>>
>>> Bad news for old kernels.
>>
>> Do we care about old kernels? I thought for IGT we did not.
> 
> We supposedly do! :)
> 
> We definitely do care about being able to bisect kernels from an arm's
> length ago. Beyond an arm's length, no one will notice. Certainly we
> don't intend to make things harder than we need to. :)

Question is how does the complete new IGT world look. If everything 
depends on engine map interface (everything as all 
for_each_physical_engine tests), then we can have it fall back to a 
static table as today but lose some coverage. (Since we don't have a way 
of probing or accessing all engines without the new uapi, we can only 
have a subset of engines in the static fall back table.)

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Andi Shyti]

Hi Tvrtko,

> > > -const struct intel_execution_engine2 intel_execution_engines2[] = {
> > > -       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> > > -       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> > > -       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> > > -       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> > > -       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > > -       { }
> > > -};
> > > +struct intel_execution_engine2 intel_execution_engines2[32] = { };
> 
> I'd just make it a pointer, both size and explicit static initialization to
> zero are not needed.

So that you want to allocate *intel_execution_engines2 somewhere
in gem_init_engine_list(), is this what you are recommending?

Thanks,
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-15 13:32:39)
> 
> On 15/01/2019 13:19, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-15 13:14:10)
> >>
> >> On 15/01/2019 13:00, Chris Wilson wrote:
> >>> Quoting Andi Shyti (2019-01-15 12:35:10)
> >>>> Kernel commits:
> >>>>
> >>>> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> >>>> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> >>>>
> >>>> from [*] repository, implement a new uapi for engine discovery
> >>>> that consist in first querying the driver about the engines in
> >>>> the gpu [1] and then binding a context to the set of engines that
> >>>> it can access [2].
> >>>>
> >>>> In igt the classic way for discovering engines is done through
> >>>> the for_each_physical_engine() macro, that would be replaced by
> >>>> the new for_each_engine_ctx().
> >>>>
> >>>> A new function is added gem_init_engine_list() that is called
> >>>> during device open which creates the list of engines. That list
> >>>> is stored in the intel_execution_engines2 that replaces the
> >>>> current array which has more a reference meaning. Now the
> >>>> intel_execution_engines2 stores the engines currently present in
> >>>> the GPU.
> >>>>
> >>>> [*] git://people.freedesktop.org/~tursulin/drm-intel
> >>>>
> >>>> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> >>>> ---
> >>>>    lib/drmtest.c | 12 +++++--
> >>>>    lib/igt_gt.c  | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >>>>    lib/igt_gt.h  | 10 +++++-
> >>>>    3 files changed, 110 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/lib/drmtest.c b/lib/drmtest.c
> >>>> index 7c124ac666ec..2d155eea8a13 100644
> >>>> --- a/lib/drmtest.c
> >>>> +++ b/lib/drmtest.c
> >>>> @@ -301,7 +301,7 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >>>>    
> >>>>           fd = __search_and_open(base, offset, chipset);
> >>>>           if (fd != -1)
> >>>> -               return fd;
> >>>> +               goto set_engines_and_return;
> >>>>    
> >>>>           pthread_mutex_lock(&mutex);
> >>>>           for (const struct module *m = modules; m->module; m++) {
> >>>> @@ -314,7 +314,15 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> >>>>           }
> >>>>           pthread_mutex_unlock(&mutex);
> >>>>    
> >>>> -       return __search_and_open(base, offset, chipset);
> >>>> +       fd = __search_and_open(base, offset, chipset);
> >>>> +       if (fd < 0)
> >>>> +               return fd;
> >>>> +
> >>>> +set_engines_and_return:
> >>>> +       if (is_i915_device(fd))
> >>>> +               gem_init_engine_list(fd);
> >>>
> >>> Do we really want more implicit actions on opening an fd?
> >>>
> >>> We already have igt_require_gem() which would make an interesting
> >>> starting point, for that we may want to use fd not from
> >>> drm_open_driver(). However, there seems to be no issue with creating the
> >>> names on the fly (and ask for the complementary getter for engines[] so
> >>> that an index could be translated back to class:instance).
> >>>
> >>> Certainly having drmtest presume GEM (and be subject to all of the extra
> >>> rules) given i915 seems a bit rude.
> >>
> >> My suggestion was to co-site with existing "is i915" code in
> >> drm_open_driver and drm_open_driver_render. But igt_require_gem also
> >> sounds okay.
> >>
> >> I didn't understand the bit about the complementary getter.
> > 
> > Just we're missing the GETPARAM engines interface, which I think could be
> > used to retrieve the information required for pretty printing at runtime,
> > and so avoid global tables that may not match my context.
> 
> Are you imagining for_each_engine_ctx to query if the context already 
> has engine map set up and in that case iterate over those engines? And 
> if there is no engine map to set up one containing all engines?
> 
> Or actually, maybe the former as the only behaviour of 
> for_each_engine_ctx, and only for_each_physical_engine auto-configuring 
> the map?

Yes, I would like at least one version of for_each_engine() that
iterated over a preconfigured context. Then the default version would
setup the context to iterate over all engines, before calling the low
level version.

Then I also imagine that the pretty printer might pull out the engine
details from the context at the point of need, just to reduce the amount
of state we need to pass around the for loop (and into functions).

Pros and cons.

> >>>> +       /*
> >>>> +        * 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);
> >>>
> >>> Nah, do not imply you are tied to implementation details - that is the
> >>> whole point of querying the length first. Do note that you can over
> >>> allocate (say use a small bit of stack) and do the query in one shot,
> >>> only allocating from heap if we need more room.
> >>>
> >>>> +        */
> >>>> +       igt_assert(!ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> >>>
> >>> Bad news for old kernels.
> >>
> >> Do we care about old kernels? I thought for IGT we did not.
> > 
> > We supposedly do! :)
> > 
> > We definitely do care about being able to bisect kernels from an arm's
> > length ago. Beyond an arm's length, no one will notice. Certainly we
> > don't intend to make things harder than we need to. :)
> 
> Question is how does the complete new IGT world look. If everything 
> depends on engine map interface (everything as all 
> for_each_physical_engine tests), then we can have it fall back to a 
> static table as today but lose some coverage. (Since we don't have a way 
> of probing or accessing all engines without the new uapi, we can only 
> have a subset of engines in the static fall back table.)

Yup, if GETPARAM engines[] fails, the index would be reduced to
I915_EXEC_RING and no fancy VCS.

However, I don't think that's a problem. There is nothing wrong with
testing both ABI (and I think I've said that we should always have
coverage of both ;), with having more tests for newer hardware on the new
ABI and just leaving the old ABI in the attic (i.e. move BAT/full over
to the new interface). For that I was thinking that being able to query
engines[] from the test routines would be a quick and easy way to
identify either the static legacy name or the new dynamic name.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [RFC PATCH v4 3/3] tests: gem_exec_basic: add "exec-ctx" buffer execution demo test

[Andi Shyti]

Hi Chris,

> > +       struct drm_i915_gem_execbuffer2 execbuf = {
> > +               .buffers_ptr = to_user_pointer(&exec),
> > +               .buffer_count = 1,
> > +               .flags = engine->map_index,
> 
> No. Do not store engine->map_index in the global array, as that is
> per-context and I would like to iterate over different arrays of engines.

OK.

> > +               .rsvd1 = ctx,
> > +       };
> > +
> > +       gem_execbuf(fd, &execbuf);
> 
> This doesn't actually prove anything other than no -EINVAL. How many
> different ways might we have accidentally caused success. Could we feed
> in a known failure execbuf and then look for specific failures? (Hint,
> what happens if handle = 0.)

Of course, thanks!

Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Tvrtko Ursulin]

On 15/01/2019 13:41, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>>> -const struct intel_execution_engine2 intel_execution_engines2[] = {
>>>> -       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>>>> -       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>>>> -       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
>>>> -       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
>>>> -       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
>>>> -       { }
>>>> -};
>>>> +struct intel_execution_engine2 intel_execution_engines2[32] = { };
>>
>> I'd just make it a pointer, both size and explicit static initialization to
>> zero are not needed.
> 
> So that you want to allocate *intel_execution_engines2 somewhere
> in gem_init_engine_list(), is this what you are recommending?

Yep. Do you see an advantage of putting it in the data segment? I don't 
think failure to allocate this on heap at startup is a concern.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Andi Shyti]

> > > > > -const struct intel_execution_engine2 intel_execution_engines2[] = {
> > > > > -       { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> > > > > -       { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> > > > > -       { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> > > > > -       { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> > > > > -       { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > > > > -       { }
> > > > > -};
> > > > > +struct intel_execution_engine2 intel_execution_engines2[32] = { };
> > > 
> > > I'd just make it a pointer, both size and explicit static initialization to
> > > zero are not needed.
> > 
> > So that you want to allocate *intel_execution_engines2 somewhere
> > in gem_init_engine_list(), is this what you are recommending?
> 
> Yep. Do you see an advantage of putting it in the data segment? I don't
> think failure to allocate this on heap at startup is a concern.

All right! :)

Thanks,
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev