[igt-dev] [RFC PATCH v6 0/4] new engine discovery interface

[igt-dev] [RFC PATCH v6 0/4] 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].

This rfc is still temporary as it waits for the kernel patches to
be applied first and a final version of i915_drm.h to be used.

Thanks Chris and Tvrtko for your comments in the previous RFCs.

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.

V4 --> V5
=========
RFC v4: [6]
- the engine list is now built in 'igt_require_gem()' instead of
  '__open_driver()' so that we keep this discovery method specific
  to the i915 driver (thanks Chris).
- All the query/setparam structures dynamic allocation based on
  the number of engines, now are politically allocated 64 times,
  to avoid extra ioctl calls that retrieve the engine number
  (thanks Chris)
- use igt_ioctl instead of ioctl (thanks Chris)
- allocate intel_execution_engines2 dynamically instead of
  statically (thanks Tvrtko)
- simplify the test in 'gem_exec_basic()' so that simply checks
  the presence of the engine instead of executing a buffer
  (thank Chris)
- a new patch has been added (patch 3) that extends the
  'gem_has_ring()' boolean function. The new version sets the
  index as it's mapped in the kernel.The previous function is now
  a wrapper to the new function.

V5 --> V6
=========
RFC v5: [7]
- Chris implemented the getparam ioctl which allows to the test
  to figure otu whether the new interface has been implemented.
  This way the for_each_engine_ctx() is able to work with new and
  old kernel uapi (thanks Chris)

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
[6] https://lists.freedesktop.org/archives/igt-dev/2019-January/008029.html
[7] https://lists.freedesktop.org/archives/igt-dev/2019-January/008165.html

Andi Shyti (4):
  include/drm-uapi: import i915_drm.h header file
  lib: implement new engine discovery interface
  lib: ioctl_wrappers: reach engines by index as well
  tests: gem_exec_basic: add "exec-ctx" buffer execution demo test

 include/drm-uapi/i915_drm.h | 292 +++++++++++++++++++++++++++---------
 lib/igt_gt.c                | 110 +++++++++++++-
 lib/igt_gt.h                |  14 +-
 lib/ioctl_wrappers.c        |  13 +-
 lib/ioctl_wrappers.h        |   4 +-
 tests/i915/gem_exec_basic.c |  14 ++
 6 files changed, 368 insertions(+), 79 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 v6 1/4] 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,
DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM and DRM_IOCTL_I915_QUERY.

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

diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
index d2792ab3640b..a9f6ebfba4af 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.
@@ -102,6 +122,8 @@ enum drm_i915_gem_engine_class {
 	I915_ENGINE_CLASS_INVALID	= -1
 };
 
+#define I915_ENGINE_CLASS_INVALID_NONE -1
+
 /**
  * DOC: perf_events exposed by i915 through /sys/bus/event_sources/drivers/i915
  *
@@ -319,6 +341,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_PERF_ADD_CONFIG	0x37
 #define DRM_I915_PERF_REMOVE_CONFIG	0x38
 #define DRM_I915_QUERY			0x39
+#define DRM_I915_GEM_VM_CREATE		0x3a
+#define DRM_I915_GEM_VM_DESTROY		0x3b
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -367,6 +391,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_EXT	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create_ext)
 #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)
@@ -377,6 +402,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_PERF_ADD_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
 #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
 #define DRM_IOCTL_I915_QUERY			DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)
+#define DRM_IOCTL_I915_GEM_VM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control)
+#define DRM_IOCTL_I915_GEM_VM_DESTROY	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -476,6 +503,8 @@ typedef struct drm_i915_irq_wait {
 #define   I915_SCHEDULER_CAP_ENABLED	(1ul << 0)
 #define   I915_SCHEDULER_CAP_PRIORITY	(1ul << 1)
 #define   I915_SCHEDULER_CAP_PREEMPTION	(1ul << 2)
+#define   I915_SCHEDULER_CAP_PMU	(1ul << 3)
+#define   I915_SCHEDULER_CAP_SEMAPHORES	(1ul << 4)
 
 #define I915_PARAM_HUC_STATUS		 42
 
@@ -972,7 +1001,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)
@@ -1120,32 +1149,34 @@ struct drm_i915_gem_busy {
 	 * as busy may become idle before the ioctl is completed.
 	 *
 	 * Furthermore, if the object is busy, which engine is busy is only
-	 * provided as a guide. There are race conditions which prevent the
-	 * report of which engines are busy from being always accurate.
-	 * However, the converse is not true. If the object is idle, the
-	 * result of the ioctl, that all engines are idle, is accurate.
+	 * provided as a guide and only indirectly by reporting its class
+	 * (there may be more than one engine in each class). There are race
+	 * conditions which prevent the report of which engines are busy from
+	 * being always accurate.  However, the converse is not true. If the
+	 * object is idle, the result of the ioctl, that all engines are idle,
+	 * is accurate.
 	 *
 	 * The returned dword is split into two fields to indicate both
-	 * the engines on which the object is being read, and the
-	 * engine on which it is currently being written (if any).
+	 * the engine classess on which the object is being read, and the
+	 * engine class on which it is currently being written (if any).
 	 *
 	 * The low word (bits 0:15) indicate if the object is being written
 	 * to by any engine (there can only be one, as the GEM implicit
 	 * synchronisation rules force writes to be serialised). Only the
-	 * engine for the last write is reported.
+	 * engine class (offset by 1, I915_ENGINE_CLASS_RENDER is reported as
+	 * 1 not 0 etc) for the last write is reported.
 	 *
-	 * The high word (bits 16:31) are a bitmask of which engines are
-	 * currently reading from the object. Multiple engines may be
+	 * The high word (bits 16:31) are a bitmask of which engines classes
+	 * are currently reading from the object. Multiple engines may be
 	 * reading from the object simultaneously.
 	 *
-	 * The value of each engine is the same as specified in the
-	 * EXECBUFFER2 ioctl, i.e. I915_EXEC_RENDER, I915_EXEC_BSD etc.
-	 * Note I915_EXEC_DEFAULT is a symbolic value and is mapped to
-	 * the I915_EXEC_RENDER engine for execution, and so it is never
+	 * The value of each engine class is the same as specified in the
+	 * I915_CONTEXT_SET_ENGINES parameter and via perf, i.e.
+	 * I915_ENGINE_CLASS_RENDER, I915_ENGINE_CLASS_COPY, etc.
 	 * reported as active itself. Some hardware may have parallel
 	 * execution engines, e.g. multiple media engines, which are
-	 * mapped to the same identifier in the EXECBUFFER2 ioctl and
-	 * so are not separately reported for busyness.
+	 * mapped to the same class identifier and so are not separately
+	 * reported for busyness.
 	 *
 	 * Caveat emptor:
 	 * Only the boolean result of this query is reliable; that is whether
@@ -1412,65 +1443,15 @@ struct drm_i915_gem_wait {
 };
 
 struct drm_i915_gem_context_create {
-	/*  output: id of new context*/
-	__u32 ctx_id;
+	__u32 ctx_id; /* output: id of new context*/
 	__u32 pad;
 };
 
-struct drm_i915_gem_context_destroy {
-	__u32 ctx_id;
-	__u32 pad;
-};
-
-struct drm_i915_reg_read {
-	/*
-	 * Register offset.
-	 * For 64bit wide registers where the upper 32bits don't immediately
-	 * follow the lower 32bits, the offset of the lower 32bits must
-	 * be specified
-	 */
-	__u64 offset;
-#define I915_REG_READ_8B_WA (1ul << 0)
-
-	__u64 val; /* Return value */
-};
-/* Known registers:
- *
- * Render engine timestamp - 0x2358 + 64bit - gen7+
- * - Note this register returns an invalid value if using the default
- *   single instruction 8byte read, in order to workaround that pass
- *   flag I915_REG_READ_8B_WA in offset field.
- *
- */
-
-struct drm_i915_reset_stats {
-	__u32 ctx_id;
+struct drm_i915_gem_context_create_ext {
+	__u32 ctx_id; /* output: id of new context*/
 	__u32 flags;
-
-	/* All resets since boot/module reload, for all contexts */
-	__u32 reset_count;
-
-	/* Number of batches lost when active in GPU, for this context */
-	__u32 batch_active;
-
-	/* Number of batches lost pending for execution, for this context */
-	__u32 batch_pending;
-
-	__u32 pad;
-};
-
-struct drm_i915_gem_userptr {
-	__u64 user_ptr;
-	__u64 user_size;
-	__u32 flags;
-#define I915_USERPTR_READ_ONLY 0x1
-#define I915_USERPTR_UNSYNCHRONIZED 0x80000000
-	/**
-	 * Returned handle for the object.
-	 *
-	 * Object handles are nonzero.
-	 */
-	__u32 handle;
+#define I915_GEM_CONTEXT_SINGLE_TIMELINE	0x1
+	__u64 extensions;
 };
 
 struct drm_i915_gem_context_param {
@@ -1491,6 +1472,38 @@ struct drm_i915_gem_context_param {
 	 * drm_i915_gem_context_param_sseu.
 	 */
 #define I915_CONTEXT_PARAM_SSEU		0x7
+
+	/*
+	 * The id of the associated virtual memory address space (ppGTT) of
+	 * this context. Can be retreived and passed to another context
+	 * (on the same fd) for both to use the same ppGTT and so share
+	 * address layouts, and avoid reloading the page tables on context
+	 * switches between themselves.
+	 *
+	 * See DRM_I915_GEM_VM_CREATE and DRM_I915_GEM_VM_DESTROY.
+	 */
+#define I915_CONTEXT_PARAM_VM		0x8
+
+/*
+ * 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. Slots 0...N are filled in using the specified (class, instance).
+ * Use
+ *	engine_class: I915_ENGINE_CLASS_INVALID,
+ *	engine_instance: I915_ENGINE_CLASS_INVALID_NONE
+ * to specify a gap in the array that can be filled in later, e.g. by a
+ * virtual engine used for load balancing.
+ *
+ * Setting the number of engines bound to the context to 0, by passing a zero
+ * sized argument, will revert back to default settings.
+ *
+ * See struct i915_context_param_engines.
+ */
+#define I915_CONTEXT_PARAM_ENGINES	0x9
+
 	__u64 value;
 };
 
@@ -1553,6 +1566,98 @@ struct drm_i915_gem_context_param_sseu {
 	__u32 rsvd;
 };
 
+struct i915_context_param_engines {
+	__u64 extensions; /* linked chain of extension blocks, 0 terminates */
+
+	struct {
+		__u16 engine_class; /* see enum drm_i915_gem_engine_class */
+		__u16 engine_instance;
+	} class_instance[0];
+};
+
+struct drm_i915_gem_context_create_ext_setparam {
+#define I915_CONTEXT_CREATE_EXT_SETPARAM 0
+	struct i915_user_extension base;
+	struct drm_i915_gem_context_param setparam;
+};
+
+struct drm_i915_gem_context_destroy {
+	__u32 ctx_id;
+	__u32 pad;
+};
+
+/*
+ * DRM_I915_GEM_VM_CREATE -
+ *
+ * Create a new virtual memory address space (ppGTT) for use within a context
+ * on the same file. Extensions can be provided to configure exactly how the
+ * address space is setup upon creation.
+ *
+ * The id of new VM (bound to the fd) for use with I915_CONTEXT_PARAM_VM is
+ * returned.
+ *
+ * DRM_I915_GEM_VM_DESTROY -
+ *
+ * Destroys a previously created VM id.
+ */
+struct drm_i915_gem_vm_control {
+	__u64 extensions;
+	__u32 flags;
+	__u32 id;
+};
+
+struct drm_i915_reg_read {
+	/*
+	 * Register offset.
+	 * For 64bit wide registers where the upper 32bits don't immediately
+	 * follow the lower 32bits, the offset of the lower 32bits must
+	 * be specified
+	 */
+	__u64 offset;
+#define I915_REG_READ_8B_WA (1ul << 0)
+
+	__u64 val; /* Return value */
+};
+
+/* Known registers:
+ *
+ * Render engine timestamp - 0x2358 + 64bit - gen7+
+ * - Note this register returns an invalid value if using the default
+ *   single instruction 8byte read, in order to workaround that pass
+ *   flag I915_REG_READ_8B_WA in offset field.
+ *
+ */
+
+struct drm_i915_reset_stats {
+	__u32 ctx_id;
+	__u32 flags;
+
+	/* All resets since boot/module reload, for all contexts */
+	__u32 reset_count;
+
+	/* Number of batches lost when active in GPU, for this context */
+	__u32 batch_active;
+
+	/* Number of batches lost pending for execution, for this context */
+	__u32 batch_pending;
+
+	__u32 pad;
+};
+
+struct drm_i915_gem_userptr {
+	__u64 user_ptr;
+	__u64 user_size;
+	__u32 flags;
+#define I915_USERPTR_READ_ONLY 0x1
+#define I915_USERPTR_UNSYNCHRONIZED 0x80000000
+	/**
+	 * Returned handle for the object.
+	 *
+	 * Object handles are nonzero.
+	 */
+	__u32 handle;
+};
+
 enum drm_i915_oa_format {
 	I915_OA_FORMAT_A13 = 1,	    /* HSW only */
 	I915_OA_FORMAT_A29,	    /* HSW only */
@@ -1714,6 +1819,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
@@ -1811,6 +1917,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 v6 2/4] 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")

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 gem_init_engine_list() is addedthat is called
in igt_require_gem() to set the i915 requirement. A list of
existing engines is created and 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.

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
 lib/igt_gt.c         | 110 ++++++++++++++++++++++++++++++++++++++++++-
 lib/igt_gt.h         |  14 +++++-
 lib/ioctl_wrappers.c |   3 ++
 3 files changed, 124 insertions(+), 3 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 646696727ee4..7d933e64ec59 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -577,7 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
 	return true;
 }
 
-const struct intel_execution_engine2 intel_execution_engines2[] = {
+static struct intel_execution_engine2 intel_exec_engines2_reflist[] = {
 	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
 	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
 	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
@@ -586,6 +586,8 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
 	{ }
 };
 
+struct intel_execution_engine2 *intel_execution_engines2;
+
 unsigned int
 gem_class_instance_to_eb_flags(int gem_fd,
 			       enum drm_i915_gem_engine_class class,
@@ -650,3 +652,109 @@ 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;
+	item.length = sizeof(*query_engines) +
+		      64 * sizeof(struct drm_i915_engine_info);
+
+	igt_assert((query_engines = calloc(1, item.length)));
+	item.data_ptr = to_user_pointer(query_engines);
+
+	igt_assert(!igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
+
+	return query_engines;
+}
+
+bool gem_is_new_get_set_param(void)
+{
+	return intel_execution_engines2 != intel_exec_engines2_reflist;
+}
+
+void __set_ctx_engine_map(int fd, uint32_t ctx_id)
+{
+	int i;
+	struct intel_execution_engine2 *e2;
+	struct drm_i915_gem_context_param ctx_param;
+	struct i915_context_param_engines *ctx_engine;
+
+	if (!gem_is_new_get_set_param())
+		return;
+
+	ctx_param.ctx_id = ctx_id;
+	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+	ctx_param.size = sizeof(*ctx_engine) +
+			 (I915_EXEC_RING_MASK - 1) *
+			 sizeof(*ctx_engine->class_instance);
+
+	igt_assert((ctx_engine = calloc(1, ctx_param.size)));
+
+	ctx_engine->extensions = 0;
+	for (i = 0, e2 = intel_execution_engines2; e2->name; i++, e2++) {
+		ctx_engine->class_instance[i].engine_class = e2->class;
+		ctx_engine->class_instance[i].engine_instance = e2->instance;
+	}
+
+	ctx_param.value = to_user_pointer(ctx_engine);
+
+	igt_assert(!igt_ioctl(fd,
+			      DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
+
+	free(ctx_engine);
+}
+
+void gem_init_engine_list(int fd)
+{
+	int i, ret;
+	struct drm_i915_query_engine_info *query_engine = query_engines(fd);
+	const char *engine_names[] = { "rcs", "bcs", "vcs", "vecs" };
+	struct drm_i915_gem_context_param ctx_param = {
+		.param = I915_CONTEXT_PARAM_ENGINES,
+	};
+
+	if (intel_execution_engines2)
+		return;
+
+	/*
+	 * we check first whether the new engine discovery uapi
+	 * is in the current kernel, if not, the
+	 * DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM will fail with
+	 * errno = EINVAL. In this case, we fall back to using
+	 * the previous engine discovery way
+	 */
+	ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &ctx_param);
+	if (errno == EINVAL) {
+		intel_execution_engines2 = intel_exec_engines2_reflist;
+		return;
+	}
+
+	igt_require_f(!ret, "failed DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl");
+
+	igt_assert((intel_execution_engines2 =
+		    calloc(64, sizeof(*intel_execution_engines2))));
+
+	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;
+
+		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..fde67a6eb0ee 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -86,16 +86,26 @@ 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++) \
+			for_if (gem_is_new_get_set_param() || \
+				gem_has_engine(fd, e->class, e->instance))
+
 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;
-} intel_execution_engines2[];
+} *intel_execution_engines2;
+
+bool gem_is_new_get_set_param(void);
+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,
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 404c2fbf9355..57ed8c51bea5 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -55,6 +55,7 @@
 #include "igt_debugfs.h"
 #include "igt_sysfs.h"
 #include "config.h"
+#include "igt_gt.h"
 
 #ifdef HAVE_VALGRIND
 #include <valgrind/valgrind.h>
@@ -1459,6 +1460,8 @@ void igt_require_gem(int fd)
 	err = 0;
 	if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE))
 		err = -errno;
+	else
+		gem_init_engine_list(fd);
 
 	close(fd);
 
-- 
2.20.1

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

[igt-dev] [RFC PATCH v6 3/4] lib: ioctl_wrappers: reach engines by index as well

[Andi Shyti]

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

The 'gem_has_ring()' becomes 'gem_has_ring_by_idx()' that
requires the index that the engine is mapped with in the driver.

The previous function becomes a wrapper to the new
'gem_has_ring_by_idx()'.
---
 lib/ioctl_wrappers.c | 10 ++++++----
 lib/ioctl_wrappers.h |  4 +++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 57ed8c51bea5..5c908f4a35a5 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1468,14 +1468,14 @@ 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_has_ring_by_idx(int fd, unsigned flags, unsigned rsvd1)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
 	struct drm_i915_gem_exec_object2 exec;
 
 	/* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */
-	if ((ring & ~(3<<13)) == I915_EXEC_BSD) {
-		if (ring & (3 << 13) && !gem_has_bsd2(fd))
+	if ((flags & ~(3<<13)) == I915_EXEC_BSD) {
+		if (flags & (3 << 13) && !gem_has_bsd2(fd))
 			return false;
 	}
 
@@ -1483,7 +1483,9 @@ bool gem_has_ring(int fd, unsigned ring)
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(&exec);
 	execbuf.buffer_count = 1;
-	execbuf.flags = ring;
+	execbuf.flags = flags;
+	execbuf.rsvd1 = rsvd1;
+
 	return __gem_execbuf(fd, &execbuf) == -ENOENT;
 }
 
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index b22b36b0b2dd..19abcdb9ad1d 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -164,11 +164,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_has_ring_by_idx(int fd, unsigned flags, unsigned rsvd1);
 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_has_ring_by_idx(fd, ring, 0)
+
 /* prime */
 struct local_dma_buf_sync {
 	uint64_t flags;
-- 
2.20.1

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

[igt-dev] [RFC PATCH v6 4/4] 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 goal of the test is to get to the engines by using
the new API as implemented in 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
index dcb83864b1c1..b68829f664db 100644
--- a/tests/i915/gem_exec_basic.c
+++ b/tests/i915/gem_exec_basic.c
@@ -135,6 +135,20 @@ igt_main
 			gtt(fd, e->exec_id | e->flags);
 	}
 
+	igt_subtest("exec-ctx") {
+		uint32_t ctx_id;
+		struct intel_execution_engine2 *e2;
+		int index_map = 0;
+
+		ctx_id = gem_context_create(fd);
+
+		for_each_engine_ctx(fd, ctx_id, e2)
+			igt_assert(gem_has_ring_by_idx(fd, ++index_map,
+						       ctx_id));
+
+		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 (rev7)

[Patchwork]

== Series Details ==

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

== Summary ==

IGT patchset build failed on latest successful build
592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code



The output from the failed tests:

138/266 testcase check: gem_ctx_isolation       FAIL     0.36 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
--- stderr ---
Received signal SIGSEGV.
Stack trace: 
 #0 [fatal_sig_handler+0xd5]
 #1 [killpg+0x40]
 #2 [__real_main687+0x810]
 #3 [main+0x44]
 #4 [__libc_start_main+0xe7]
 #5 [_start+0x2a]
-------

248/266 testcase check: perf_pmu                FAIL     0.21 s

--- command ---
/home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu
--- stdout ---
tests/perf_pmu:
  Checking invalid option handling...
  Checking valid option handling...
  Checking subtest enumeration...
FAIL: tests/perf_pmu
--- stderr ---
Received signal SIGSEGV.
Stack trace: 
 #0 [fatal_sig_handler+0xd5]
 #1 [killpg+0x40]
 #2 [__real_main1671+0xf1f]
 #3 [main+0x44]
 #4 [__libc_start_main+0xe7]
 #5 [_start+0x2a]
-------

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 v6 2/4] lib: implement new engine discovery interface

[Chris Wilson]

Quoting Andi Shyti (2019-02-06 09:18:48)
> Kernel commits:
> 
> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> 
> 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 gem_init_engine_list() is addedthat is called
> in igt_require_gem() to set the i915 requirement. A list of
> existing engines is created and 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.
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>  lib/igt_gt.c         | 110 ++++++++++++++++++++++++++++++++++++++++++-
>  lib/igt_gt.h         |  14 +++++-
>  lib/ioctl_wrappers.c |   3 ++
>  3 files changed, 124 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 646696727ee4..7d933e64ec59 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -577,7 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>         return true;
>  }
>  
> -const struct intel_execution_engine2 intel_execution_engines2[] = {
> +static struct intel_execution_engine2 intel_exec_engines2_reflist[] = {
>         { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>         { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>         { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> @@ -586,6 +586,8 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
>         { }
>  };
>  
> +struct intel_execution_engine2 *intel_execution_engines2;
> +
>  unsigned int
>  gem_class_instance_to_eb_flags(int gem_fd,
>                                enum drm_i915_gem_engine_class class,
> @@ -650,3 +652,109 @@ 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;
> +       item.length = sizeof(*query_engines) +
> +                     64 * sizeof(struct drm_i915_engine_info);
> +
> +       igt_assert((query_engines = calloc(1, item.length)));
> +       item.data_ptr = to_user_pointer(query_engines);
> +
> +       igt_assert(!igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query));

These ioctls should all be wrapped, in gem_query(),
gem_context_set_param() etc.

You never ever want to have to debug an error from
igt_assert(!igt_ioctl(foo)), it is painful to the eyes and utterly
opaque.

> +
> +       return query_engines;
> +}
> +
> +bool gem_is_new_get_set_param(void)
> +{
> +       return intel_execution_engines2 != intel_exec_engines2_reflist;
> +}
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> +{
> +       int i;
> +       struct intel_execution_engine2 *e2;
> +       struct drm_i915_gem_context_param ctx_param;
> +       struct i915_context_param_engines *ctx_engine;
> +
> +       if (!gem_is_new_get_set_param())
> +               return;
> +
> +       ctx_param.ctx_id = ctx_id;
> +       ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> +       ctx_param.size = sizeof(*ctx_engine) +
> +                        (I915_EXEC_RING_MASK - 1) *
> +                        sizeof(*ctx_engine->class_instance);
> +
> +       igt_assert((ctx_engine = calloc(1, ctx_param.size)));
> +
> +       ctx_engine->extensions = 0;
> +       for (i = 0, e2 = intel_execution_engines2; e2->name; i++, e2++) {
> +               ctx_engine->class_instance[i].engine_class = e2->class;
> +               ctx_engine->class_instance[i].engine_instance = e2->instance;
> +       }
> +
> +       ctx_param.value = to_user_pointer(ctx_engine);
> +
> +       igt_assert(!igt_ioctl(fd,
> +                             DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> +
> +       free(ctx_engine);
> +}
> +
> +void gem_init_engine_list(int fd)
> +{
> +       int i, ret;
> +       struct drm_i915_query_engine_info *query_engine = query_engines(fd);
> +       const char *engine_names[] = { "rcs", "bcs", "vcs", "vecs" };
> +       struct drm_i915_gem_context_param ctx_param = {
> +               .param = I915_CONTEXT_PARAM_ENGINES,
> +       };
> +
> +       if (intel_execution_engines2)
> +               return;
> +
> +       /*
> +        * we check first whether the new engine discovery uapi
> +        * is in the current kernel, if not, the
> +        * DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM will fail with
> +        * errno = EINVAL. In this case, we fall back to using
> +        * the previous engine discovery way
> +        */
> +       ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &ctx_param);
> +       if (errno == EINVAL) {
> +               intel_execution_engines2 = intel_exec_engines2_reflist;
> +               return;
> +       }
> +
> +       igt_require_f(!ret, "failed DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl");

Please use English. This is worse than
igt_require(__gem_context_set_param() == 0).

> +
> +       igt_assert((intel_execution_engines2 =
> +                   calloc(64, sizeof(*intel_execution_engines2))));
> +
> +       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]);

require here?

You can't just use "unknown[%d]%d" for future proofing?

> +               intel_execution_engines2[i].class = class;
> +               intel_execution_engines2[i].instance = instance;
> +
> +               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..fde67a6eb0ee 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,16 +86,26 @@ 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++) \
> +                       for_if (gem_is_new_get_set_param() || \
> +                               gem_has_engine(fd, e->class, e->instance))
> +
>  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;
> -} intel_execution_engines2[];
> +} *intel_execution_engines2;
> +
> +bool gem_is_new_get_set_param(void);
> +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,
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 404c2fbf9355..57ed8c51bea5 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -55,6 +55,7 @@
>  #include "igt_debugfs.h"
>  #include "igt_sysfs.h"
>  #include "config.h"
> +#include "igt_gt.h"
>  
>  #ifdef HAVE_VALGRIND
>  #include <valgrind/valgrind.h>
> @@ -1459,6 +1460,8 @@ void igt_require_gem(int fd)
>         err = 0;
>         if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE))
>                 err = -errno;
> +       else
> +               gem_init_engine_list(fd);

igt_require_gem() maybe called multiple times per subtest.

Just gem_require_engine_list() ? Gives a good indicator which tests are
converted and which not, and a reminder that we still need to test
legacy ABIs in conjunction with the new era.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [RFC PATCH v6 3/4] lib: ioctl_wrappers: reach engines by index as well

[Chris Wilson]

Quoting Andi Shyti (2019-02-06 09:18:49)
> With the new engine query method engines are reachable through
> an index and context they are combined with.
> 
> The 'gem_has_ring()' becomes 'gem_has_ring_by_idx()' that
> requires the index that the engine is mapped with in the driver.
> 
> The previous function becomes a wrapper to the new
> 'gem_has_ring_by_idx()'.
> ---
>  lib/ioctl_wrappers.c | 10 ++++++----
>  lib/ioctl_wrappers.h |  4 +++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 57ed8c51bea5..5c908f4a35a5 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1468,14 +1468,14 @@ 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_has_ring_by_idx(int fd, unsigned flags, unsigned rsvd1)

Don't conflate the ABIs? We either use index or the mismash.
by_index *does* not use the flags...
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Antonio Argenziano]

On 06/02/19 01:18, Andi Shyti wrote:
> Kernel commits:
> 
> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> 
> 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 gem_init_engine_list() is addedthat is called

nit: missing white space ^^^^^^^^^^^^^^^^^^^^^

> in igt_require_gem() to set the i915 requirement. A list of
> existing engines is created and 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.
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>   lib/igt_gt.c         | 110 ++++++++++++++++++++++++++++++++++++++++++-
>   lib/igt_gt.h         |  14 +++++-
>   lib/ioctl_wrappers.c |   3 ++
>   3 files changed, 124 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 646696727ee4..7d933e64ec59 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -577,7 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>   	return true;
>   }
>   
> -const struct intel_execution_engine2 intel_execution_engines2[] = {
> +static struct intel_execution_engine2 intel_exec_engines2_reflist[] = {
>   	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>   	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>   	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> @@ -586,6 +586,8 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
>   	{ }
>   };
>   
> +struct intel_execution_engine2 *intel_execution_engines2;
> +
>   unsigned int
>   gem_class_instance_to_eb_flags(int gem_fd,
>   			       enum drm_i915_gem_engine_class class,
> @@ -650,3 +652,109 @@ 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;
> +	item.length = sizeof(*query_engines) +
> +		      64 * sizeof(struct drm_i915_engine_info);
> +
> +	igt_assert((query_engines = calloc(1, item.length)));
> +	item.data_ptr = to_user_pointer(query_engines);
> +
> +	igt_assert(!igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> +
> +	return query_engines;
> +}
> +
> +bool gem_is_new_get_set_param(void)
> +{
> +	return intel_execution_engines2 != intel_exec_engines2_reflist;
> +}
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> +{
> +	int i;
> +	struct intel_execution_engine2 *e2;
> +	struct drm_i915_gem_context_param ctx_param;
> +	struct i915_context_param_engines *ctx_engine;
> +
> +	if (!gem_is_new_get_set_param())
> +		return;
> +
> +	ctx_param.ctx_id = ctx_id;
> +	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> +	ctx_param.size = sizeof(*ctx_engine) +
> +			 (I915_EXEC_RING_MASK - 1) *
> +			 sizeof(*ctx_engine->class_instance);
> +
> +	igt_assert((ctx_engine = calloc(1, ctx_param.size)));
> +
> +	ctx_engine->extensions = 0;
> +	for (i = 0, e2 = intel_execution_engines2; e2->name; i++, e2++) {
> +		ctx_engine->class_instance[i].engine_class = e2->class;
> +		ctx_engine->class_instance[i].engine_instance = e2->instance;
> +	}
> +
> +	ctx_param.value = to_user_pointer(ctx_engine);
> +
> +	igt_assert(!igt_ioctl(fd,
> +			      DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));

It would be good to have a wrapper for assigning the context to an 
engine instance and then another wrapper (or maybe a macro) for 
assigning the context to all engines, so not to hide the intent of the 
IOCLT underneath. I assume tests will want to have contexts assigned 
only to some engines and not others.

> +
> +	free(ctx_engine);
> +}
> +
> +void gem_init_engine_list(int fd)
> +{
> +	int i, ret;
> +	struct drm_i915_query_engine_info *query_engine = query_engines(fd);
> +	const char *engine_names[] = { "rcs", "bcs", "vcs", "vecs" };
> +	struct drm_i915_gem_context_param ctx_param = {
> +		.param = I915_CONTEXT_PARAM_ENGINES,
> +	};
> +
> +	if (intel_execution_engines2)
> +		return;
> +
> +	/*
> +	 * we check first whether the new engine discovery uapi
> +	 * is in the current kernel, if not, the
> +	 * DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM will fail with
> +	 * errno = EINVAL. In this case, we fall back to using
> +	 * the previous engine discovery way
> +	 */
> +	ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &ctx_param);
> +	if (errno == EINVAL) {
> +		intel_execution_engines2 = intel_exec_engines2_reflist;
> +		return;
> +	}
> +
> +	igt_require_f(!ret, "failed DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl");
> +
> +	igt_assert((intel_execution_engines2 =
> +		    calloc(64, sizeof(*intel_execution_engines2))));

If you require !ret, I think you should require ^. Also, don't you want 
to request 'query_engine->num_engines' elements?

Antonio

> +
> +	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;
> +
> +		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..fde67a6eb0ee 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,16 +86,26 @@ 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++) \
> +			for_if (gem_is_new_get_set_param() || \
> +				gem_has_engine(fd, e->class, e->instance))
> +
>   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;
> -} intel_execution_engines2[];
> +} *intel_execution_engines2;
> +
> +bool gem_is_new_get_set_param(void);
> +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,
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 404c2fbf9355..57ed8c51bea5 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -55,6 +55,7 @@
>   #include "igt_debugfs.h"
>   #include "igt_sysfs.h"
>   #include "config.h"
> +#include "igt_gt.h"
>   
>   #ifdef HAVE_VALGRIND
>   #include <valgrind/valgrind.h>
> @@ -1459,6 +1460,8 @@ void igt_require_gem(int fd)
>   	err = 0;
>   	if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE))
>   		err = -errno;
> +	else
> +		gem_init_engine_list(fd);
>   
>   	close(fd);
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Petri Latvala]

On Wed, Feb 06, 2019 at 09:25:38AM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: new engine discovery interface (rev7)
> URL   : https://patchwork.freedesktop.org/series/52699/
> State : failure
> 
> == Summary ==
> 
> IGT patchset build failed on latest successful build
> 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code
> 
> 
> 
> The output from the failed tests:
> 
> 138/266 testcase check: gem_ctx_isolation       FAIL     0.36 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
> --- stderr ---
> Received signal SIGSEGV.
> Stack trace: 
>  #0 [fatal_sig_handler+0xd5]
>  #1 [killpg+0x40]
>  #2 [__real_main687+0x810]
>  #3 [main+0x44]
>  #4 [__libc_start_main+0xe7]
>  #5 [_start+0x2a]
> -------
> 
> 248/266 testcase check: perf_pmu                FAIL     0.21 s
> 
> --- command ---
> /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu
> --- stdout ---
> tests/perf_pmu:
>   Checking invalid option handling...
>   Checking valid option handling...
>   Checking subtest enumeration...
> FAIL: tests/perf_pmu
> --- stderr ---
> Received signal SIGSEGV.
> Stack trace: 
>  #0 [fatal_sig_handler+0xd5]
>  #1 [killpg+0x40]
>  #2 [__real_main1671+0xf1f]
>  #3 [main+0x44]
>  #4 [__libc_start_main+0xe7]
>  #5 [_start+0x2a]
> -------
>


These tests are constructing subtests for each known engine. Your
engine loop changes make it not possible to enumerate any names
statically, without accessing the driver.


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

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

[Andi Shyti]

Hi Petri,

> > 138/266 testcase check: gem_ctx_isolation       FAIL     0.36 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
> > --- stderr ---
> > Received signal SIGSEGV.
> > Stack trace: 
> >  #0 [fatal_sig_handler+0xd5]
> >  #1 [killpg+0x40]
> >  #2 [__real_main687+0x810]
> >  #3 [main+0x44]
> >  #4 [__libc_start_main+0xe7]
> >  #5 [_start+0x2a]
> > -------
> > 
> > 248/266 testcase check: perf_pmu                FAIL     0.21 s
> > 
> > --- command ---
> > /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu
> > --- stdout ---
> > tests/perf_pmu:
> >   Checking invalid option handling...
> >   Checking valid option handling...
> >   Checking subtest enumeration...
> > FAIL: tests/perf_pmu
> > --- stderr ---
> > Received signal SIGSEGV.
> > Stack trace: 
> >  #0 [fatal_sig_handler+0xd5]
> >  #1 [killpg+0x40]
> >  #2 [__real_main1671+0xf1f]
> >  #3 [main+0x44]
> >  #4 [__libc_start_main+0xe7]
> >  #5 [_start+0x2a]
> > -------
> >
> 
> 
> These tests are constructing subtests for each known engine. Your
> engine loop changes make it not possible to enumerate any names
> statically, without accessing the driver.

dammit! I didn't see it. This means that I have to leave the
intel_execution_engines2[] as it is. But this has to be a
temporary solution.

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

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

[Andi Shyti]

Hi Antonio,

> > A new function gem_init_engine_list() is addedthat is called
> 
> nit: missing white space ^^^^^^^^^^^^^^^^^^^^^

Thanks :)

> > +	igt_assert(!igt_ioctl(fd,
> > +			      DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> 
> It would be good to have a wrapper for assigning the context to an engine
> instance and then another wrapper (or maybe a macro) for assigning the
> context to all engines, so not to hide the intent of the IOCLT underneath. I
> assume tests will want to have contexts assigned only to some engines and
> not others.

yes, that's the same thing Chris pointed out, I'll put everything
into a macro.

> > +	igt_assert((intel_execution_engines2 =
> > +		    calloc(64, sizeof(*intel_execution_engines2))));
> 
> If you require !ret, I think you should require ^. Also, don't you want to
> request 'query_engine->num_engines' elements?

Yes, the design of this changed form the previous iterations and
this is a leftover. It can, indeed, be safely num_engines. Thanks.

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

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

[Chris Wilson]

Quoting Andi Shyti (2019-02-08 11:03:21)
> Hi Antonio,
> 
> > > A new function gem_init_engine_list() is addedthat is called
> > 
> > nit: missing white space ^^^^^^^^^^^^^^^^^^^^^
> 
> Thanks :)
> 
> > > +   igt_assert(!igt_ioctl(fd,
> > > +                         DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
> > 
> > It would be good to have a wrapper for assigning the context to an engine
> > instance and then another wrapper (or maybe a macro) for assigning the
> > context to all engines, so not to hide the intent of the IOCLT underneath. I
> > assume tests will want to have contexts assigned only to some engines and
> > not others.
> 
> yes, that's the same thing Chris pointed out, I'll put everything
> into a macro.

Not macros! Functions. The macros get stringified in all their
glory making the failure message hideous and unreadable.

The functions should already exist, for the most part at least.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Andi Shyti]

Hi Chris,

Thanks for the review!

I think fixed everything, just a few questions on the following

> > @@ -1459,6 +1460,8 @@ void igt_require_gem(int fd)
> >         err = 0;
> >         if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE))
> >                 err = -errno;
> > +       else
> > +               gem_init_engine_list(fd);
> 
> igt_require_gem() maybe called multiple times per subtest.

I'm actually checking that with:

        if (intel_execution_engines2)
                return;

inside the gem_init_engine_list(). Shouldn't that be enough to
prevent multiple calls?

> Just gem_require_engine_list() ? Gives a good indicator which tests are
> converted and which not, and a reminder that we still need to test
> legacy ABIs in conjunction with the new era.

Can do that, but I thought that in the new era, when your and
Tvrtko's patch will get in, we will convert everything to the new
ABI's and this would remain an extra step (which BTW, wouldn't
prevent multiple calls either, as above).

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

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

[Chris Wilson]

Quoting Andi Shyti (2019-02-08 12:55:42)
> Hi Chris,
> 
> Thanks for the review!
> 
> I think fixed everything, just a few questions on the following
> 
> > > @@ -1459,6 +1460,8 @@ void igt_require_gem(int fd)
> > >         err = 0;
> > >         if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE))
> > >                 err = -errno;
> > > +       else
> > > +               gem_init_engine_list(fd);
> > 
> > igt_require_gem() maybe called multiple times per subtest.
> 
> I'm actually checking that with:
> 
>         if (intel_execution_engines2)
>                 return;
> 
> inside the gem_init_engine_list(). Shouldn't that be enough to
> prevent multiple calls?

Still not keen on having it here. It doesn't smell right; the purpose of
igt_require_gem() is to check the gpu is functional. Having a
side-effect of initialising an obscure struct used by a few tests?

> > Just gem_require_engine_list() ? Gives a good indicator which tests are
> > converted and which not, and a reminder that we still need to test
> > legacy ABIs in conjunction with the new era.
> 
> Can do that, but I thought that in the new era, when your and
> Tvrtko's patch will get in, we will convert everything to the new
> ABI's and this would remain an extra step (which BTW, wouldn't
> prevent multiple calls either, as above).

NO! The ABI doesn't simply no longer exist because we wish it so. The
tests for the old ABI will have to remain for as long as it is
accessible; which means that we need both sets of ABI for anything that
looks suspiciously like an ABI test.

It only bits and pieces that merely want to run over a set of HW that we
can simplify.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Tvrtko Ursulin via igt-dev]

On 07/02/2019 08:58, Petri Latvala wrote:
> On Wed, Feb 06, 2019 at 09:25:38AM +0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: new engine discovery interface (rev7)
>> URL   : https://patchwork.freedesktop.org/series/52699/
>> State : failure
>>
>> == Summary ==
>>
>> IGT patchset build failed on latest successful build
>> 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code
>>
>>
>>
>> The output from the failed tests:
>>
>> 138/266 testcase check: gem_ctx_isolation       FAIL     0.36 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
>> --- stderr ---
>> Received signal SIGSEGV.
>> Stack trace:
>>   #0 [fatal_sig_handler+0xd5]
>>   #1 [killpg+0x40]
>>   #2 [__real_main687+0x810]
>>   #3 [main+0x44]
>>   #4 [__libc_start_main+0xe7]
>>   #5 [_start+0x2a]
>> -------
>>
>> 248/266 testcase check: perf_pmu                FAIL     0.21 s
>>
>> --- command ---
>> /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu
>> --- stdout ---
>> tests/perf_pmu:
>>    Checking invalid option handling...
>>    Checking valid option handling...
>>    Checking subtest enumeration...
>> FAIL: tests/perf_pmu
>> --- stderr ---
>> Received signal SIGSEGV.
>> Stack trace:
>>   #0 [fatal_sig_handler+0xd5]
>>   #1 [killpg+0x40]
>>   #2 [__real_main1671+0xf1f]
>>   #3 [main+0x44]
>>   #4 [__libc_start_main+0xe7]
>>   #5 [_start+0x2a]
>> -------
>>
> 
> 
> These tests are constructing subtests for each known engine. Your
> engine loop changes make it not possible to enumerate any names
> statically, without accessing the driver.

Is generating subtest names without accessing the driver an absolute 
requirement?

This particular failure would be fixable if the engine list was 
initialized on the first call to __for_each_engine_class_instance, as 
called by the perf_pmu above. And the fixture block is already ran in 
test enumeration, where the driver is opened. I think a lot of tests are 
like this. So in this context a simple query ioctl on top doesn't sound bad.

But I also remember, and I think it was due bug tracking limitations, 
that in the past the desire was test enumeration is constant regardless 
of the execution platform.

I think it would be simpler if we didn't have to maintain a separate 
static list of all possible engines. To make that work we would need to 
detect the mode of execution (list vs execute) in the engine iterator as 
well. So gain, to me it sounds preferable that tests would be allowed to 
enumerate dynamically.

So question is can CI/bugtracking cope with that, and are we okay with 
doing a trivial ioctl from the for_each_engine_<something>.

Regards,

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

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

[Arkadiusz Hiler via igt-dev]

On Tue, Feb 12, 2019 at 08:43:24AM +0000, Tvrtko Ursulin via igt-dev wrote:
> 
> On 07/02/2019 08:58, Petri Latvala wrote:
> > On Wed, Feb 06, 2019 at 09:25:38AM +0000, Patchwork wrote:
> > > == Series Details ==
> > > 
> > > Series: new engine discovery interface (rev7)
> > > URL   : https://patchwork.freedesktop.org/series/52699/
> > > State : failure
> > > 
> > > == Summary ==
> > > 
> > > IGT patchset build failed on latest successful build
> > > 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code
> > > 
> > > 
> > > 
> > > The output from the failed tests:
> > > 
> > > 138/266 testcase check: gem_ctx_isolation       FAIL     0.36 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
> > > --- stderr ---
> > > Received signal SIGSEGV.
> > > Stack trace:
> > >   #0 [fatal_sig_handler+0xd5]
> > >   #1 [killpg+0x40]
> > >   #2 [__real_main687+0x810]
> > >   #3 [main+0x44]
> > >   #4 [__libc_start_main+0xe7]
> > >   #5 [_start+0x2a]
> > > -------
> > > 
> > > 248/266 testcase check: perf_pmu                FAIL     0.21 s
> > > 
> > > --- command ---
> > > /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu
> > > --- stdout ---
> > > tests/perf_pmu:
> > >    Checking invalid option handling...
> > >    Checking valid option handling...
> > >    Checking subtest enumeration...
> > > FAIL: tests/perf_pmu
> > > --- stderr ---
> > > Received signal SIGSEGV.
> > > Stack trace:
> > >   #0 [fatal_sig_handler+0xd5]
> > >   #1 [killpg+0x40]
> > >   #2 [__real_main1671+0xf1f]
> > >   #3 [main+0x44]
> > >   #4 [__libc_start_main+0xe7]
> > >   #5 [_start+0x2a]
> > > -------
> > > 
> > 
> > 
> > These tests are constructing subtests for each known engine. Your
> > engine loop changes make it not possible to enumerate any names
> > statically, without accessing the driver.
> 
> Is generating subtest names without accessing the driver an absolute
> requirement?
> 
> This particular failure would be fixable if the engine list was initialized
> on the first call to __for_each_engine_class_instance, as called by the
> perf_pmu above. And the fixture block is already ran in test enumeration,
> where the driver is opened. I think a lot of tests are like this. So in this
> context a simple query ioctl on top doesn't sound bad.
> 
> But I also remember, and I think it was due bug tracking limitations, that
> in the past the desire was test enumeration is constant regardless of the
> execution platform.
> 
> I think it would be simpler if we didn't have to maintain a separate static
> list of all possible engines. To make that work we would need to detect the
> mode of execution (list vs execute) in the engine iterator as well. So gain,
> to me it sounds preferable that tests would be allowed to enumerate
> dynamically.
> 
> So question is can CI/bugtracking cope with that, and are we okay with doing
> a trivial ioctl from the for_each_engine_<something>.

For quite some time it is enforced that, in IGT, listing tests is
exhaustive and independent of the machine. CI heavily relies on this
assumption. We generate and manage the testlists on macines that are
GT-less.

Especially for 'shards':

1. list all test,
   split it in 35 'randomized' chunks that have roughly the same execution time

2. currnet_chunk=35;
   while (current_chunk)
     if(any(machines, is_free))
       schedule(current_chunk--, first_free(machines))

I don't really see how we can improve upon that.

Overall keeping static list of all the possible engines for listing and
then "skipping" the actual exectuion if it's not there seems like a
simpler issue to tackle.

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

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

[Tvrtko Ursulin via igt-dev]

On 12/02/2019 11:39, Arkadiusz Hiler wrote:
> On Tue, Feb 12, 2019 at 08:43:24AM +0000, Tvrtko Ursulin via igt-dev wrote:
>>
>> On 07/02/2019 08:58, Petri Latvala wrote:
>>> On Wed, Feb 06, 2019 at 09:25:38AM +0000, Patchwork wrote:
>>>> == Series Details ==
>>>>
>>>> Series: new engine discovery interface (rev7)
>>>> URL   : https://patchwork.freedesktop.org/series/52699/
>>>> State : failure
>>>>
>>>> == Summary ==
>>>>
>>>> IGT patchset build failed on latest successful build
>>>> 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code
>>>>
>>>>
>>>>
>>>> The output from the failed tests:
>>>>
>>>> 138/266 testcase check: gem_ctx_isolation       FAIL     0.36 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
>>>> --- stderr ---
>>>> Received signal SIGSEGV.
>>>> Stack trace:
>>>>    #0 [fatal_sig_handler+0xd5]
>>>>    #1 [killpg+0x40]
>>>>    #2 [__real_main687+0x810]
>>>>    #3 [main+0x44]
>>>>    #4 [__libc_start_main+0xe7]
>>>>    #5 [_start+0x2a]
>>>> -------
>>>>
>>>> 248/266 testcase check: perf_pmu                FAIL     0.21 s
>>>>
>>>> --- command ---
>>>> /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu
>>>> --- stdout ---
>>>> tests/perf_pmu:
>>>>     Checking invalid option handling...
>>>>     Checking valid option handling...
>>>>     Checking subtest enumeration...
>>>> FAIL: tests/perf_pmu
>>>> --- stderr ---
>>>> Received signal SIGSEGV.
>>>> Stack trace:
>>>>    #0 [fatal_sig_handler+0xd5]
>>>>    #1 [killpg+0x40]
>>>>    #2 [__real_main1671+0xf1f]
>>>>    #3 [main+0x44]
>>>>    #4 [__libc_start_main+0xe7]
>>>>    #5 [_start+0x2a]
>>>> -------
>>>>
>>>
>>>
>>> These tests are constructing subtests for each known engine. Your
>>> engine loop changes make it not possible to enumerate any names
>>> statically, without accessing the driver.
>>
>> Is generating subtest names without accessing the driver an absolute
>> requirement?
>>
>> This particular failure would be fixable if the engine list was initialized
>> on the first call to __for_each_engine_class_instance, as called by the
>> perf_pmu above. And the fixture block is already ran in test enumeration,
>> where the driver is opened. I think a lot of tests are like this. So in this
>> context a simple query ioctl on top doesn't sound bad.
>>
>> But I also remember, and I think it was due bug tracking limitations, that
>> in the past the desire was test enumeration is constant regardless of the
>> execution platform.
>>
>> I think it would be simpler if we didn't have to maintain a separate static
>> list of all possible engines. To make that work we would need to detect the
>> mode of execution (list vs execute) in the engine iterator as well. So gain,
>> to me it sounds preferable that tests would be allowed to enumerate
>> dynamically.
>>
>> So question is can CI/bugtracking cope with that, and are we okay with doing
>> a trivial ioctl from the for_each_engine_<something>.
> 
> For quite some time it is enforced that, in IGT, listing tests is
> exhaustive and independent of the machine. CI heavily relies on this
> assumption. We generate and manage the testlists on macines that are
> GT-less.
> 
> Especially for 'shards':
> 
> 1. list all test,
>     split it in 35 'randomized' chunks that have roughly the same execution time
> 
> 2. currnet_chunk=35;
>     while (current_chunk)
>       if(any(machines, is_free))
>         schedule(current_chunk--, first_free(machines))
> 
> I don't really see how we can improve upon that.
> 
> Overall keeping static list of all the possible engines for listing and
> then "skipping" the actual exectuion if it's not there seems like a
> simpler issue to tackle.

Downside there is that it creates a coupling between test code and CI 
implementation.

Would it be feasible to generate the test list on sharded platforms?

It could be a "list tests" job which you could send to each of the shard 
platforms (one machine of each platforms) before randomizing and chunking.

Advantage there would be that you don't send chunks containing tests 
which a particular platform cannot run.

And of course another advantage is that we remove unnecessary coupling 
between two components so IGT maintenance becomes easier. (No two sets 
of engines, no two ways to iterate those sets, and no need to remember 
when to use each.)

Regards,

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

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

[Petri Latvala via igt-dev]

On Tue, Feb 12, 2019 at 08:43:24AM +0000, Tvrtko Ursulin wrote:
> Is generating subtest names without accessing the driver an absolute
> requirement?

Yes.


> This particular failure would be fixable if the engine list was initialized
> on the first call to __for_each_engine_class_instance, as called by the
> perf_pmu above. And the fixture block is already ran in test enumeration,
> where the driver is opened. I think a lot of tests are like this. So in this
> context a simple query ioctl on top doesn't sound bad.

Fixtures are not executed when enumerating tests.


> I think it would be simpler if we didn't have to maintain a separate static
> list of all possible engines. To make that work we would need to detect the
> mode of execution (list vs execute) in the engine iterator as well. So gain,
> to me it sounds preferable that tests would be allowed to enumerate
> dynamically.

Can the tests be refactored to have a subtest per engine _class_?
That's a static list, isn't it? Within the subtest they would loop
over instances of the class. Kind of like tests that loop over
connectors of $type.


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

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

[Tvrtko Ursulin]

On 15/02/2019 09:50, Petri Latvala wrote:
> On Tue, Feb 12, 2019 at 08:43:24AM +0000, Tvrtko Ursulin wrote:
>> Is generating subtest names without accessing the driver an absolute
>> requirement?
> 
> Yes.

I don't like how this creates a coupling between CI _implementation_ and 
IGT codebase and think can be improved.

> 
>> This particular failure would be fixable if the engine list was initialized
>> on the first call to __for_each_engine_class_instance, as called by the
>> perf_pmu above. And the fixture block is already ran in test enumeration,
>> where the driver is opened. I think a lot of tests are like this. So in this
>> context a simple query ioctl on top doesn't sound bad.
> 
> Fixtures are not executed when enumerating tests.

Okay.

> 
>> I think it would be simpler if we didn't have to maintain a separate static
>> list of all possible engines. To make that work we would need to detect the
>> mode of execution (list vs execute) in the engine iterator as well. So gain,
>> to me it sounds preferable that tests would be allowed to enumerate
>> dynamically.
> 
> Can the tests be refactored to have a subtest per engine _class_?
> That's a static list, isn't it? Within the subtest they would loop
> over instances of the class. Kind of like tests that loop over
> connectors of $type.

Sounds quite invasive, fundamental and limiting (how to run a test 
against a single engine in this world?) change. How about instead, which 
is more or less copy & paste from my reply to Arek:

Would it be feasible to generate the test list on sharded platforms?

It could be a "list tests" job which you could send to each of the shard 
platforms (one machine of each platforms), executed before randomizing 
and chunking.

Advantage there would be that you don't send chunks containing tests 
which a particular platform cannot run.

And of course another advantage is that we remove unnecessary coupling 
between two components so IGT maintenance becomes easier. (No two sets 
of engines, no two ways to iterate those sets, and no need to remember 
when to use each.)

Regards,

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

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

[Petri Latvala]

On Mon, Feb 25, 2019 at 01:22:02PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/02/2019 09:50, Petri Latvala wrote:
> > On Tue, Feb 12, 2019 at 08:43:24AM +0000, Tvrtko Ursulin wrote:
> > > Is generating subtest names without accessing the driver an absolute
> > > requirement?
> > 
> > Yes.
> 
> I don't like how this creates a coupling between CI _implementation_ and IGT
> codebase and think can be improved.


I see the underlying difference in our thinking now. To me, having
subtest lists statically available _enables_ decoupling IGT codebase
from systems that run the tests.



> 
> > 
> > > This particular failure would be fixable if the engine list was initialized
> > > on the first call to __for_each_engine_class_instance, as called by the
> > > perf_pmu above. And the fixture block is already ran in test enumeration,
> > > where the driver is opened. I think a lot of tests are like this. So in this
> > > context a simple query ioctl on top doesn't sound bad.
> > 
> > Fixtures are not executed when enumerating tests.
> 
> Okay.
> 
> > 
> > > I think it would be simpler if we didn't have to maintain a separate static
> > > list of all possible engines. To make that work we would need to detect the
> > > mode of execution (list vs execute) in the engine iterator as well. So gain,
> > > to me it sounds preferable that tests would be allowed to enumerate
> > > dynamically.
> > 
> > Can the tests be refactored to have a subtest per engine _class_?
> > That's a static list, isn't it? Within the subtest they would loop
> > over instances of the class. Kind of like tests that loop over
> > connectors of $type.
> 
> Sounds quite invasive, fundamental and limiting (how to run a test against a
> single engine in this world?) change.


How often does this requirement come up?

CI, by definition, wants to run all tests there are. FSVO all.

Developers and validation people might want to execute tests for a
single engine?

Two ways of executing, one machinery doesn't fit all, one needs to do
something special. Why not an environment variable that says
IGT_ONLY_ENGINES_IN_I915_ARE=rcs2,bsd1?


>How about instead, which is more or
> less copy & paste from my reply to Arek:
> 
> Would it be feasible to generate the test list on sharded platforms?
> 
> It could be a "list tests" job which you could send to each of the shard
> platforms (one machine of each platforms), executed before randomizing and
> chunking.

Feasible sure, but I can't say anything how hard that is to make
happen. There are advantages to this, but I don't see how engine
enumeration is one, when CI wants to anyway execute tests on all of
them.


> 
> Advantage there would be that you don't send chunks containing tests which a
> particular platform cannot run.


This would indeed be nice.

I have been fermenting some thoughts about refactoring the tests to
express their requirements more declaratively instead of with
imperative code. Having the possibility of skipping a whole bunch of
tests as soon as you know a declared requirement is not available
could tie well with this kind of a feature.


> And of course another advantage is that we remove unnecessary coupling
> between two components so IGT maintenance becomes easier. (No two sets of
> engines, no two ways to iterate those sets, and no need to remember when to
> use each.)


The thing is, this kind of a decoupling IGT's test enumeration from
CI's usage of the enumeration keeps current CI working but hinders
other usages. I regularly run IGT with ezbench, which does bisecting
automatically for me, and having tests just come and go depending on
the running kernel on occasion breaks things in an irritating way. We
already have those, in the form of kernel selftests.

I'm not completely opposed of changing the philosophy of having
statically enumerated subtests, but changing that will require the
rest of the world to be ready for the change. Currently, I don't even
have an exhaustive list of the world changes needed.


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