[igt-dev] [RFC 0/2] new engine discovery interface

[igt-dev] [RFC 0/2] new engine discovery interface

[Andi Shyti]

Hi,

this is a request for comments for the new engine discovery
interface developed by Tvrtko and Chris[*].

I would like to receive some feedback about its implementation
and whether the libraries are implemented following the igt
style.

Perhaps Tvrtko and Chris could check if I'm making a proper use
of their new ioctls.

Thanks,
Andi

Andi Shyti (2):
  include/drm-uapi: import i915_drm.h header file
  lib: implement new engine discovery interface

 include/drm-uapi/i915_drm.h | 239 +++++++++++++++++++++++++++++++++++-
 lib/igt_gt.c                |  89 ++++++++++++++
 lib/igt_gt.h                |   6 +
 lib/ioctl_wrappers.c        |  25 ++++
 lib/ioctl_wrappers.h        |   1 +
 5 files changed, 358 insertions(+), 2 deletions(-)

-- 
2.19.1

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

[igt-dev] [RFC 1/2] 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 | 239 +++++++++++++++++++++++++++++++++++-
 1 file changed, 237 insertions(+), 2 deletions(-)

diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
index 16e452aa..b14ca969 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)
@@ -412,6 +433,14 @@ typedef struct drm_i915_irq_wait {
 	int irq_seq;
 } drm_i915_irq_wait_t;
 
+/*
+ * Different modes of per-process Graphics Translation Table,
+ * see I915_PARAM_HAS_ALIASING_PPGTT
+ */
+#define I915_GEM_PPGTT_NONE	0
+#define I915_GEM_PPGTT_ALIASING	1
+#define I915_GEM_PPGTT_FULL	2
+
 /* Ioctl to query kernel params:
  */
 #define I915_PARAM_IRQ_ACTIVE            1
@@ -529,6 +558,35 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_CS_TIMESTAMP_FREQUENCY 51
 
+/*
+ * Once upon a time we supposed that writes through the GGTT would be
+ * immediately in physical memory (once flushed out of the CPU path). However,
+ * on a few different processors and chipsets, this is not necessarily the case
+ * as the writes appear to be buffered internally. Thus a read of the backing
+ * storage (physical memory) via a different path (with different physical tags
+ * to the indirect write via the GGTT) will see stale values from before
+ * the GGTT write. Inside the kernel, we can for the most part keep track of
+ * the different read/write domains in use (e.g. set-domain), but the assumption
+ * of coherency is baked into the ABI, hence reporting its true state in this
+ * parameter.
+ *
+ * Reports true when writes via mmap_gtt are immediately visible following an
+ * lfence to flush the WCB.
+ *
+ * Reports false when writes via mmap_gtt are indeterminately delayed in an in
+ * internal buffer and are _not_ immediately visible to third parties accessing
+ * directly via mmap_cpu/mmap_wc. Use of mmap_gtt as part of an IPC
+ * communications channel when reporting false is strongly disadvised.
+ */
+#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;
 	/*
@@ -942,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)
@@ -1048,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) \
@@ -1387,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;
@@ -1456,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 */
@@ -1620,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
@@ -1717,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.19.1

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

[igt-dev] [RFC 2/2] 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 way uapi for engine
discovery that consist in first coupling context with engine [2]
and then query the engines through a specific structure [1].

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

The new engine discovery macro requires, though, to first bind a
context to the engines and the unbind it:

Here's an example of usage:

  int main(void)
  {
     int fd;
     uint32_t n, ctx_id, e;

     fd = drm_open_driver(DRIVER_INTEL);

     if(bind_ctx_to_engine(fd, &n, &ctx_id))
        goto out;

     for_each_engine_ctx(fd, n, ctx_id, e)
        printf("%d OK:\n", e+1);

     unbind_ctx_from_engine(fd, ctx_id);

  out:
     close(fd);

     return 0;
  }

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

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
 lib/igt_gt.c         | 89 ++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_gt.h         |  6 +++
 lib/ioctl_wrappers.c | 25 +++++++++++++
 lib/ioctl_wrappers.h |  1 +
 4 files changed, 121 insertions(+)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a2061924..bafe1f86 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -650,3 +650,92 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
 
 	return gem_has_ring(fd, ring);
 }
+
+static struct drm_i915_query_engine_info *__query_engines(int fd)
+{
+	struct drm_i915_query query;
+	struct drm_i915_query_item item;
+	struct drm_i915_query_engine_info *query_engines;
+	ssize_t size = sizeof(*query_engines) + 10 * sizeof(*query_engines->engines);
+	int err;
+
+	query_engines = malloc(size);
+	if (!query_engines)
+		return NULL;
+
+	memset(&query, 0, sizeof(query));
+	memset(&item, 0, sizeof(item));
+	memset(query_engines, 0, sizeof(*query_engines));
+
+	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
+	item.length = size;
+	item.flags = 0;
+	item.data_ptr = to_user_pointer(query_engines);
+
+	query.items_ptr = to_user_pointer(&item);
+	query.num_items = 1;
+
+	err = igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query);
+	if (err) {
+		free(query_engines);
+		return NULL;
+	}
+
+	return query_engines;
+}
+
+static int __bind_ctx_to_engine(int fd, unsigned ctx_id,
+		struct drm_i915_query_engine_info *query_engine)
+{
+	int i, ret;
+	struct drm_i915_gem_context_param ctx_param;
+	struct i915_context_param_engines *ctx_engine;
+	size_t size = sizeof(struct i915_context_param_engines) +
+		      query_engine->num_engines *
+		      sizeof(*ctx_engine->class_instance);
+
+	ctx_engine = malloc(size);
+	if (!ctx_engine)
+		return errno;
+
+	ctx_engine->extensions = 0;
+	for (i = 0; i < query_engine->num_engines; i++) {
+		ctx_engine->class_instance[i].class = query_engine->engines[i].class;
+		ctx_engine->class_instance[i].instance = query_engine->engines[i].instance;
+	}
+
+	ctx_param.ctx_id = ctx_id;
+	ctx_param.size = size;
+	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
+	ctx_param.value = to_user_pointer(ctx_engine);
+
+	ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param);
+	free(ctx_engine);
+
+	return ret;
+}
+
+int bind_ctx_to_engine(int fd, uint32_t *n, uint32_t *ctx_id)
+{
+	struct drm_i915_query_engine_info *query_engine;
+	int ret;
+
+	query_engine = __query_engines(fd);
+	if (!query_engine)
+		return -1;
+
+	*ctx_id = gem_context_create(fd);
+	ret = __bind_ctx_to_engine(fd, *ctx_id, query_engine);
+	if (ret)
+		return ret;
+
+	*n = query_engine->num_engines;
+	free(query_engine);
+
+	return 0;
+}
+
+void unbind_ctx_from_engine(int fd, uint32_t ctx_id)
+{
+	gem_context_destroy(fd, ctx_id);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 54e95da9..d8937318 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -86,8 +86,14 @@ 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, n, id, e) \
+	for (e = 0; e < n; e++) \
+		for_if(gem_has_ring_ctx(fd, e + 1, id))
+
 bool gem_ring_is_physical_engine(int fd, unsigned int ring);
 bool gem_ring_has_physical_engine(int fd, unsigned int ring);
+int bind_ctx_to_engine(int fd, uint32_t *n, uint32_t *ctx_id);
+void unbind_ctx_from_engine(int fd, uint32_t ctx_id);
 
 bool gem_can_store_dword(int fd, unsigned int engine);
 
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 9f255508..2115bad1 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1472,6 +1472,31 @@ bool gem_has_ring(int fd, unsigned ring)
 	return __gem_execbuf(fd, &execbuf) == -ENOENT;
 }
 
+bool gem_has_ring_ctx(int fd, unsigned ring, unsigned ctx_id)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec;
+	uint32_t bbend = MI_BATCH_BUFFER_END;
+	int ret;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	memset(&exec, 0, sizeof(exec));
+
+	exec.handle = gem_create(fd, 4096);
+	gem_write(fd, exec.handle, 0, &bbend, sizeof(bbend));
+
+	execbuf.buffers_ptr = to_user_pointer(&exec);
+	execbuf.buffer_count = 1;
+	execbuf.flags = ring;
+	i915_execbuffer2_set_context_id(execbuf, ctx_id);
+
+	ret = __gem_execbuf(fd, &execbuf);
+
+	gem_close(fd, exec.handle);
+
+	return !ret;
+}
+
 /**
  * gem_require_ring:
  * @fd: open i915 drm file descriptor
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index b22b36b0..d4adf912 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -165,6 +165,7 @@ 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_ctx(int fd, unsigned ring, unsigned ctx_id);
 void gem_require_ring(int fd, unsigned ring);
 bool gem_has_mocs_registers(int fd);
 void gem_require_mocs_registers(int fd);
-- 
2.19.1

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

[igt-dev] ✓ Fi.CI.BAT: success for new engine discovery interface

[Patchwork]

== Series Details ==

Series: new engine discovery interface
URL   : https://patchwork.freedesktop.org/series/52699/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4720 -> IGTPW_2078 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52699/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_2078 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191) +1

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-icl-u:           PASS -> INCOMPLETE (fdo#107713)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS

    igt@i915_selftest@live_sanitycheck:
      fi-gdg-551:         INCOMPLETE (fdo#108789) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#108789 https://bugs.freedesktop.org/show_bug.cgi?id=108789


== Participating hosts (53 -> 47) ==

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * IGT: IGT_4720 -> IGTPW_2078

  CI_DRM_5159: af98442486c4eeed23ed036dfa2b556def4203bd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2078: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2078/
  IGT_4720: c27aaca295d3ca2a38521e571c012449371e4bb5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2078/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Tvrtko Ursulin]

On 19/11/2018 15:55, Andi Shyti wrote:
> Kernel commits:
> 
> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> 
> from [*] repository, implement a new way uapi for engine
> discovery that consist in first coupling context with engine [2]
> and then query the engines through a specific structure [1].
> 
> In igt the classic way for discovering engines is done trhough
> the for_each_physical_engine() macro, that is replaced by the
> new for_each_engine_ctx().

My idea was that we migrate for_each_physical_engine to this new scheme.

As an easy starting point I was thinking to keep the current 
for_each_physical_engine as is, and just add new 
for_each_physical_engine_new and migrate one test to it as a demo.

Then when this part is reviewed, as a second step we convert the rest 
and rename the macro stripping the _new suffix and nuking the old one.

With regards to implementation details I was thinking along these lines:

On first invocation for_each_physical_engine_new initializes some hidden 
data stored in one or more globals (which will live in igt_gt.c).

This would query the engines and create a context with all engines mapped.

We also add a helper to get this internal ctx id to use within 
for_each_physical_engine_new loops.

That should make it easy to convert simple tests over like:

   igt_subtest {
   	for_each_physical_engine_new(fd, engine) {
   		...
   		execbuf.rsvd1 = gem_physical_engine_ctx();
   		execbuf.flags = gem_physical_engine_idx(engine);
   		gem_execbuf(fd, execbuf);
   	}
   }

We also need to think about what kind of helpers would be needed for 
tests which use for_each_physical_engine and their own contexts. If such 
exist, and I haven't checked, a helper like 
gem_setup_context_for_each_physical_engine or something?

eg:

	igt_subtest {
		unit32_t test_ctx;

		...
		gem_init_context_for_each_physical_engine(fd, test_ctx);

		for_each_physical_engine_new(fd, engine) {
	  		...
	  		execbuf.rsvd1 = test_ctx;
	  		execbuf.flags = gem_physical_engine_idx(engine);
	  		gem_execbuf(fd, execbuf);
		}
	}			

Regards,

Tvrtko

> The new engine discovery macro requires, though, to first bind a
> context to the engines and the unbind it:
> 
> Here's an example of usage:
> 
>    int main(void)
>    {
>       int fd;
>       uint32_t n, ctx_id, e;
> 
>       fd = drm_open_driver(DRIVER_INTEL);
> 
>       if(bind_ctx_to_engine(fd, &n, &ctx_id))
>          goto out;
> 
>       for_each_engine_ctx(fd, n, ctx_id, e)
>          printf("%d OK:\n", e+1);
> 
>       unbind_ctx_from_engine(fd, ctx_id);
> 
>    out:
>       close(fd);
> 
>       return 0;
>    }
> 
> [*] git://people.freedesktop.org/~tursulin/drm-intel
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>   lib/igt_gt.c         | 89 ++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_gt.h         |  6 +++
>   lib/ioctl_wrappers.c | 25 +++++++++++++
>   lib/ioctl_wrappers.h |  1 +
>   4 files changed, 121 insertions(+)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a2061924..bafe1f86 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -650,3 +650,92 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>   
>   	return gem_has_ring(fd, ring);
>   }
> +
> +static struct drm_i915_query_engine_info *__query_engines(int fd)
> +{
> +	struct drm_i915_query query;
> +	struct drm_i915_query_item item;
> +	struct drm_i915_query_engine_info *query_engines;
> +	ssize_t size = sizeof(*query_engines) + 10 * sizeof(*query_engines->engines);
> +	int err;
> +
> +	query_engines = malloc(size);
> +	if (!query_engines)
> +		return NULL;
> +
> +	memset(&query, 0, sizeof(query));
> +	memset(&item, 0, sizeof(item));
> +	memset(query_engines, 0, sizeof(*query_engines));
> +
> +	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> +	item.length = size;
> +	item.flags = 0;
> +	item.data_ptr = to_user_pointer(query_engines);
> +
> +	query.items_ptr = to_user_pointer(&item);
> +	query.num_items = 1;
> +
> +	err = igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query);
> +	if (err) {
> +		free(query_engines);
> +		return NULL;
> +	}
> +
> +	return query_engines;
> +}
> +
> +static int __bind_ctx_to_engine(int fd, unsigned ctx_id,
> +		struct drm_i915_query_engine_info *query_engine)
> +{
> +	int i, ret;
> +	struct drm_i915_gem_context_param ctx_param;
> +	struct i915_context_param_engines *ctx_engine;
> +	size_t size = sizeof(struct i915_context_param_engines) +
> +		      query_engine->num_engines *
> +		      sizeof(*ctx_engine->class_instance);
> +
> +	ctx_engine = malloc(size);
> +	if (!ctx_engine)
> +		return errno;
> +
> +	ctx_engine->extensions = 0;
> +	for (i = 0; i < query_engine->num_engines; i++) {
> +		ctx_engine->class_instance[i].class = query_engine->engines[i].class;
> +		ctx_engine->class_instance[i].instance = query_engine->engines[i].instance;
> +	}
> +
> +	ctx_param.ctx_id = ctx_id;
> +	ctx_param.size = size;
> +	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> +	ctx_param.value = to_user_pointer(ctx_engine);
> +
> +	ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param);
> +	free(ctx_engine);
> +
> +	return ret;
> +}
> +
> +int bind_ctx_to_engine(int fd, uint32_t *n, uint32_t *ctx_id)
> +{
> +	struct drm_i915_query_engine_info *query_engine;
> +	int ret;
> +
> +	query_engine = __query_engines(fd);
> +	if (!query_engine)
> +		return -1;
> +
> +	*ctx_id = gem_context_create(fd);
> +	ret = __bind_ctx_to_engine(fd, *ctx_id, query_engine);
> +	if (ret)
> +		return ret;
> +
> +	*n = query_engine->num_engines;
> +	free(query_engine);
> +
> +	return 0;
> +}
> +
> +void unbind_ctx_from_engine(int fd, uint32_t ctx_id)
> +{
> +	gem_context_destroy(fd, ctx_id);
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da9..d8937318 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,8 +86,14 @@ 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, n, id, e) \
> +	for (e = 0; e < n; e++) \
> +		for_if(gem_has_ring_ctx(fd, e + 1, id))
> +
>   bool gem_ring_is_physical_engine(int fd, unsigned int ring);
>   bool gem_ring_has_physical_engine(int fd, unsigned int ring);
> +int bind_ctx_to_engine(int fd, uint32_t *n, uint32_t *ctx_id);
> +void unbind_ctx_from_engine(int fd, uint32_t ctx_id);
>   
>   bool gem_can_store_dword(int fd, unsigned int engine);
>   
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..2115bad1 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1472,6 +1472,31 @@ bool gem_has_ring(int fd, unsigned ring)
>   	return __gem_execbuf(fd, &execbuf) == -ENOENT;
>   }
>   
> +bool gem_has_ring_ctx(int fd, unsigned ring, unsigned ctx_id)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec;
> +	uint32_t bbend = MI_BATCH_BUFFER_END;
> +	int ret;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	memset(&exec, 0, sizeof(exec));
> +
> +	exec.handle = gem_create(fd, 4096);
> +	gem_write(fd, exec.handle, 0, &bbend, sizeof(bbend));
> +
> +	execbuf.buffers_ptr = to_user_pointer(&exec);
> +	execbuf.buffer_count = 1;
> +	execbuf.flags = ring;
> +	i915_execbuffer2_set_context_id(execbuf, ctx_id);
> +
> +	ret = __gem_execbuf(fd, &execbuf);
> +
> +	gem_close(fd, exec.handle);
> +
> +	return !ret;
> +}
> +
>   /**
>    * gem_require_ring:
>    * @fd: open i915 drm file descriptor
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b22b36b0..d4adf912 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -165,6 +165,7 @@ 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_ctx(int fd, unsigned ring, unsigned ctx_id);
>   void gem_require_ring(int fd, unsigned ring);
>   bool gem_has_mocs_registers(int fd);
>   void gem_require_mocs_registers(int fd);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-11-19 19:59:19)
> 
> On 19/11/2018 15:55, Andi Shyti wrote:
> > Kernel commits:
> > 
> > [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> > [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> > 
> > from [*] repository, implement a new way uapi for engine
> > discovery that consist in first coupling context with engine [2]
> > and then query the engines through a specific structure [1].
> > 
> > In igt the classic way for discovering engines is done trhough
> > the for_each_physical_engine() macro, that is replaced by the
> > new for_each_engine_ctx().
> 
> My idea was that we migrate for_each_physical_engine to this new scheme.
> 
> As an easy starting point I was thinking to keep the current 
> for_each_physical_engine as is, and just add new 
> for_each_physical_engine_new and migrate one test to it as a demo.
> 
> Then when this part is reviewed, as a second step we convert the rest 
> and rename the macro stripping the _new suffix and nuking the old one.
> 
> With regards to implementation details I was thinking along these lines:
> 
> On first invocation for_each_physical_engine_new initializes some hidden 
> data stored in one or more globals (which will live in igt_gt.c).
> 
> This would query the engines and create a context with all engines mapped.
> 
> We also add a helper to get this internal ctx id to use within 
> for_each_physical_engine_new loops.
> 
> That should make it easy to convert simple tests over like:
> 
>    igt_subtest {
>         for_each_physical_engine_new(fd, engine) {
>                 ...
>                 execbuf.rsvd1 = gem_physical_engine_ctx();
>                 execbuf.flags = gem_physical_engine_idx(engine);
>                 gem_execbuf(fd, execbuf);
>         }
>    }

Granted that we have a lot of tests that just use the default ctx, I
don't think the iterator interface we create should enforce that.

for_each_physical_engine_new(fd, ctx, engine) {
	execbuf.rsvd1 = ctx;
	execbuf.flags = engine;
	gem_execbuf(fd, execbuf);
}

where ctx is provided, and engine the iterator. Off the top of my head,
I have a silly idea like

for (int __max_engine__; (__max_engine__ = igt_physical_engine_iter_init(fd, ctx)); )
	for (engine = 1; engine <= __max_engine__; engine++)

where igt_physical_engine_iter_init(int fd, uint32_t ctx) {
	if (ctx_getparam(fd, ctx, ENGINES).count > 0) {
		ctx_setparam(fd, ctx, ENGINES, NULL);
		return 0;
	}

	// query and set engine array
	return count;
}
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for new engine discovery interface

[Patchwork]

== Series Details ==

Series: new engine discovery interface
URL   : https://patchwork.freedesktop.org/series/52699/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4720_full -> IGTPW_2078_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_2078_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2078_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52699/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_2078_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in IGTPW_2078_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          PASS -> FAIL (fdo#103232) +9

    igt@kms_cursor_crc@cursor-256x256-dpms:
      shard-kbl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-256x85-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +6

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-apl:          PASS -> FAIL (fdo#103232, fdo#103191) +1

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          PASS -> DMESG-WARN (fdo#105763, fdo#106538)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-wc:
      shard-apl:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
      shard-glk:          PASS -> FAIL (fdo#103167) +6

    igt@kms_plane_alpha_blend@pipe-b-alpha-7efc:
      shard-kbl:          NOTRUN -> FAIL (fdo#108145, fdo#108590)

    igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
      shard-kbl:          NOTRUN -> FAIL (fdo#108145)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166) +6

    igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
      shard-kbl:          PASS -> FAIL (fdo#103166)

    igt@kms_sysfs_edid_timing:
      shard-kbl:          NOTRUN -> FAIL (fdo#100047)

    igt@kms_universal_plane@universal-plane-pipe-b-functional:
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    
    ==== Possible fixes ====

    igt@gem_eio@in-flight-1us:
      shard-glk:          FAIL (fdo#107799) -> PASS +1

    igt@gem_eio@unwedge-stress:
      shard-glk:          FAIL -> PASS

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665, fdo#106887) -> PASS

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-apl:          FAIL (fdo#106641) -> PASS

    igt@kms_chv_cursor_fail@pipe-c-128x128-bottom-edge:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS +1

    igt@kms_cursor_crc@cursor-64x21-onscreen:
      shard-glk:          FAIL (fdo#103232) -> PASS +2

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
      shard-apl:          FAIL (fdo#103167) -> PASS +1
      shard-kbl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-glk:          FAIL (fdo#103167) -> PASS +3

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-glk:          FAIL (fdo#103166) -> PASS +1

    igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@perf@blocking:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106887 https://bugs.freedesktop.org/show_bug.cgi?id=106887
  fdo#107799 https://bugs.freedesktop.org/show_bug.cgi?id=107799
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108590 https://bugs.freedesktop.org/show_bug.cgi?id=108590
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (7 -> 5) ==

  Missing    (2): shard-skl shard-iclb 


== Build changes ==

    * IGT: IGT_4720 -> IGTPW_2078

  CI_DRM_5159: af98442486c4eeed23ed036dfa2b556def4203bd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2078: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2078/
  IGT_4720: c27aaca295d3ca2a38521e571c012449371e4bb5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2078/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Andi Shyti]

Hi Tvrtko,

thanks for your inputs!

> > Kernel commits:
> > 
> > [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> > [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> > 
> > from [*] repository, implement a new way uapi for engine
> > discovery that consist in first coupling context with engine [2]
> > and then query the engines through a specific structure [1].
> > 
> > In igt the classic way for discovering engines is done trhough
> > the for_each_physical_engine() macro, that is replaced by the
> > new for_each_engine_ctx().
> 
> My idea was that we migrate for_each_physical_engine to this new scheme.
> 
> As an easy starting point I was thinking to keep the current
> for_each_physical_engine as is, and just add new
> for_each_physical_engine_new and migrate one test to it as a demo.
> 
> Then when this part is reviewed, as a second step we convert the rest and
> rename the macro stripping the _new suffix and nuking the old one.

The for_each_physical_engine is left as it is.

I have indeed posted only the changes in the libraries, I can
send a demo test as well.

> With regards to implementation details I was thinking along these lines:
> 
> On first invocation for_each_physical_engine_new initializes some hidden
> data stored in one or more globals (which will live in igt_gt.c).
> 
> This would query the engines and create a context with all engines mapped.
> 
> We also add a helper to get this internal ctx id to use within
> for_each_physical_engine_new loops.
> 
> That should make it easy to convert simple tests over like:
> 
>   igt_subtest {
>   	for_each_physical_engine_new(fd, engine) {
>   		...
>   		execbuf.rsvd1 = gem_physical_engine_ctx();
>   		execbuf.flags = gem_physical_engine_idx(engine);
>   		gem_execbuf(fd, execbuf);
>   	}
>   }
> 
> We also need to think about what kind of helpers would be needed for tests
> which use for_each_physical_engine and their own contexts. If such exist,
> and I haven't checked, a helper like
> gem_setup_context_for_each_physical_engine or something?
> 
> eg:
> 
> 	igt_subtest {
> 		unit32_t test_ctx;
> 
> 		...
> 		gem_init_context_for_each_physical_engine(fd, test_ctx);
> 
> 		for_each_physical_engine_new(fd, engine) {
> 	  		...
> 	  		execbuf.rsvd1 = test_ctx;
> 	  		execbuf.flags = gem_physical_engine_idx(engine);
> 	  		gem_execbuf(fd, execbuf);
> 		}
> 	}			

I tried to keep the "for_each_physical_engine_new" (which I
called "for_each_engine_ctx" because it looks more meaningful)
similar to what it was by adding a few arguments more and a
"bind" and an "unbind" functions before and after in order to
create and remove the structures and contexts I am creating.

I think it's not that different from what you are suggesting and
what we discussed offline. But perhaps a demo test would make
it more clear.

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

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

[Tvrtko Ursulin]

On 19/11/2018 20:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-11-19 19:59:19)
>>
>> On 19/11/2018 15:55, Andi Shyti wrote:
>>> Kernel commits:
>>>
>>> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
>>> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
>>>
>>> from [*] repository, implement a new way uapi for engine
>>> discovery that consist in first coupling context with engine [2]
>>> and then query the engines through a specific structure [1].
>>>
>>> In igt the classic way for discovering engines is done trhough
>>> the for_each_physical_engine() macro, that is replaced by the
>>> new for_each_engine_ctx().
>>
>> My idea was that we migrate for_each_physical_engine to this new scheme.
>>
>> As an easy starting point I was thinking to keep the current
>> for_each_physical_engine as is, and just add new
>> for_each_physical_engine_new and migrate one test to it as a demo.
>>
>> Then when this part is reviewed, as a second step we convert the rest
>> and rename the macro stripping the _new suffix and nuking the old one.
>>
>> With regards to implementation details I was thinking along these lines:
>>
>> On first invocation for_each_physical_engine_new initializes some hidden
>> data stored in one or more globals (which will live in igt_gt.c).
>>
>> This would query the engines and create a context with all engines mapped.
>>
>> We also add a helper to get this internal ctx id to use within
>> for_each_physical_engine_new loops.
>>
>> That should make it easy to convert simple tests over like:
>>
>>     igt_subtest {
>>          for_each_physical_engine_new(fd, engine) {
>>                  ...
>>                  execbuf.rsvd1 = gem_physical_engine_ctx();
>>                  execbuf.flags = gem_physical_engine_idx(engine);
>>                  gem_execbuf(fd, execbuf);
>>          }
>>     }
> 
> Granted that we have a lot of tests that just use the default ctx, I
> don't think the iterator interface we create should enforce that.
> 
> for_each_physical_engine_new(fd, ctx, engine) {
> 	execbuf.rsvd1 = ctx;
> 	execbuf.flags = engine;
> 	gem_execbuf(fd, execbuf);
> }

To be clear, you think we should convert all such tests to use a 
non-default context? My idea was to minimize the churn, but I am also 
okay with this plan.

> where ctx is provided, and engine the iterator. Off the top of my head,
> I have a silly idea like
> 
> for (int __max_engine__; (__max_engine__ = igt_physical_engine_iter_init(fd, ctx)); )
> 	for (engine = 1; engine <= __max_engine__; engine++)
> 
> where igt_physical_engine_iter_init(int fd, uint32_t ctx) {
> 	if (ctx_getparam(fd, ctx, ENGINES).count > 0) {
> 		ctx_setparam(fd, ctx, ENGINES, NULL);
> 		return 0;
> 	}
> 
> 	// query and set engine array
> 	return count;
> }

That works and avoids the global on the face of it. However with engine 
iterator a simple int, we will probably still need some to enable 
querying iterated engine properties like class, instance and name for 
subtest enumeration and similar.

Regards,

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

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-11-20 10:00:25)
> 
> On 19/11/2018 20:38, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-11-19 19:59:19)
> >>
> >> On 19/11/2018 15:55, Andi Shyti wrote:
> >>> Kernel commits:
> >>>
> >>> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> >>> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> >>>
> >>> from [*] repository, implement a new way uapi for engine
> >>> discovery that consist in first coupling context with engine [2]
> >>> and then query the engines through a specific structure [1].
> >>>
> >>> In igt the classic way for discovering engines is done trhough
> >>> the for_each_physical_engine() macro, that is replaced by the
> >>> new for_each_engine_ctx().
> >>
> >> My idea was that we migrate for_each_physical_engine to this new scheme.
> >>
> >> As an easy starting point I was thinking to keep the current
> >> for_each_physical_engine as is, and just add new
> >> for_each_physical_engine_new and migrate one test to it as a demo.
> >>
> >> Then when this part is reviewed, as a second step we convert the rest
> >> and rename the macro stripping the _new suffix and nuking the old one.
> >>
> >> With regards to implementation details I was thinking along these lines:
> >>
> >> On first invocation for_each_physical_engine_new initializes some hidden
> >> data stored in one or more globals (which will live in igt_gt.c).
> >>
> >> This would query the engines and create a context with all engines mapped.
> >>
> >> We also add a helper to get this internal ctx id to use within
> >> for_each_physical_engine_new loops.
> >>
> >> That should make it easy to convert simple tests over like:
> >>
> >>     igt_subtest {
> >>          for_each_physical_engine_new(fd, engine) {
> >>                  ...
> >>                  execbuf.rsvd1 = gem_physical_engine_ctx();
> >>                  execbuf.flags = gem_physical_engine_idx(engine);
> >>                  gem_execbuf(fd, execbuf);
> >>          }
> >>     }
> > 
> > Granted that we have a lot of tests that just use the default ctx, I
> > don't think the iterator interface we create should enforce that.
> > 
> > for_each_physical_engine_new(fd, ctx, engine) {
> >       execbuf.rsvd1 = ctx;
> >       execbuf.flags = engine;
> >       gem_execbuf(fd, execbuf);
> > }
> 
> To be clear, you think we should convert all such tests to use a 
> non-default context? My idea was to minimize the churn, but I am also 
> okay with this plan.

No, perfectly fine to use ctx=0 and even to have the common iterator
default to ctx=0. My opinion is that we don't restrict the iterator to
only work on a pre-defined context, but accept that using the iterator
will adjust the ctx->engines[] to suit (and so allow the iterator to
work on any context).

> > where ctx is provided, and engine the iterator. Off the top of my head,
> > I have a silly idea like
> > 
> > for (int __max_engine__; (__max_engine__ = igt_physical_engine_iter_init(fd, ctx)); )
> >       for (engine = 1; engine <= __max_engine__; engine++)
> > 
> > where igt_physical_engine_iter_init(int fd, uint32_t ctx) {
> >       if (ctx_getparam(fd, ctx, ENGINES).count > 0) {
> >               ctx_setparam(fd, ctx, ENGINES, NULL);
> >               return 0;
> >       }
> > 
> >       // query and set engine array
> >       return count;
> > }
> 
> That works and avoids the global on the face of it. However with engine 
> iterator a simple int, we will probably still need some to enable 
> querying iterated engine properties like class, instance and name for 
> subtest enumeration and similar.

True worst case would be

class_instance_get_name(magic_ctx_getparam(fd, ctx, ENGINES)[idx]);

Passable for the one-off uses, but I guess we want

	ctx_get_engine_names(fd, ctx, name_array, name_count);

Or a
	struct engine_attribute {
		u16 class, instance;
		const char *name;
		... other common details ...
	};
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev