[RFC 0/2] New engine discovery and execbuffer2 engine selection uAPI

[RFC 0/2] New engine discovery and execbuffer2 engine selection uAPI

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Inspired by the recent introduction of the engine class and instance concept,
and a chat with Chris Wilson about a potential unification of PCI id based
device discovery across multiple userspace components, I have cooked up two
patches to gather some opinions on whether this sort of uAPI would be
interesting for a wider audience.

First patch is basically and old idea by Jon Bloomfield on how to do engine
discovery, while the second one is just a quick hack to show how a similar
approach could work for execbuffer2.

Both patches are compile tested only and intended only to start a discussion.

Tvrtko Ursulin (2):
  drm/i915: Engine discovery uAPI
  drm/i915: Select engines via class and instance in execbuffer2

 drivers/gpu/drm/i915/i915_drv.c            |  1 +
 drivers/gpu/drm/i915/i915_drv.h            |  3 ++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 36 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c     | 59 ++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h                | 47 +++++++++++++++++++++++-
 5 files changed, 145 insertions(+), 1 deletion(-)

-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine discovery uAPI allows userspace to probe for engine
configuration and features without needing to maintain the
internal PCI id based database.

This enables removal of code duplications across userspace
components.

Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
which returns the number and information on the specified engine
class.

Currently only general engine configuration and HEVC feature of
the VCS engine can be probed but the uAPI is designed to be
generic and extensible.

Code is based almost exactly on the earlier proposal on the
topic by Jon Bloomfield. Engine class and instance refactoring
made recently by Daniele Ceraolo Spurio enabled this to be
implemented in an elegant fashion.

To probe configuration userspace sets the engine class it wants
to query (struct drm_i915_gem_engine_info) and provides an array
of drm_i915_engine_info structs which will be filled in by the
driver. Userspace also has to tell i915 how many elements are in
the array, and the driver will report back the total number of
engine instances in any case.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_drv.c        |  1 +
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 59 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h            | 40 +++++++++++++++++++++++
 4 files changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index cc7393e65e99..b720c9468adf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2607,6 +2607,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6c2f04..6eed0e854561 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3547,6 +3547,9 @@ i915_gem_context_lookup_timeline(struct i915_gem_context *ctx,
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
 
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  u64 min_size, u64 alignment,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 402769d9d840..6deaffd34ae0 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,6 +25,7 @@
 #include "i915_drv.h"
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
+#include <uapi/drm/i915_drm.h>
 
 struct engine_class_info {
 	const char *name;
@@ -1189,6 +1190,64 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
 		engine->set_default_submission(engine);
 }
 
+u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX] = {
+	[DRM_I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
+	[DRM_I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
+	[DRM_I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
+	[DRM_I915_ENGINE_CLASS_VIDEO_DECODE] = VIDEO_DECODE_CLASS,
+	[DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
+};
+
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_engine_info *args = data;
+	enum drm_i915_gem_engine_class engine_class = args->engine_class;
+	struct drm_i915_engine_info __user *user_info =
+					u64_to_user_ptr(args->info_ptr);
+	struct drm_i915_engine_info info;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u8 class;
+
+	if (args->rsvd)
+		return -EINVAL;
+
+	switch (engine_class) {
+	case DRM_I915_ENGINE_CLASS_OTHER:
+	case DRM_I915_ENGINE_CLASS_RENDER:
+	case DRM_I915_ENGINE_CLASS_COPY:
+	case DRM_I915_ENGINE_CLASS_VIDEO_DECODE:
+	case DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE:
+		if (engine_class >= DRM_I915_ENGINE_CLASS_MAX)
+			return -EINVAL;
+		class = user_class_map[engine_class];
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	args->num_engines = 0;
+	for_each_engine(engine, i915, id) {
+		if (class != engine->class)
+			continue;
+
+		if (++args->num_engines > args->info_size)
+			continue;
+
+		memset(&info, 0, sizeof(info));
+		info.instance = engine->instance;
+		if (INTEL_GEN(i915) > 8 && id == VCS)
+			info.info = DRM_I915_ENGINE_HAS_HEVC;
+
+		if (copy_to_user(user_info++, &info, sizeof(info)))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f24a80d2d42e..6058596a9f33 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -260,6 +260,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
 #define DRM_I915_PERF_OPEN		0x36
+#define DRM_I915_GEM_ENGINE_INFO	0x37
 
 #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)
@@ -315,6 +316,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
+#define DRM_IOCTL_I915_GEM_ENGINE_INFO	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_ENGINE_INFO, struct drm_i915_gem_engine_info)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1439,6 +1441,44 @@ enum drm_i915_perf_record_type {
 	DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };
 
+enum drm_i915_gem_engine_class {
+	DRM_I915_ENGINE_CLASS_OTHER = 0,
+	DRM_I915_ENGINE_CLASS_RENDER = 1,
+	DRM_I915_ENGINE_CLASS_COPY = 2,
+	DRM_I915_ENGINE_CLASS_VIDEO_DECODE = 3,
+	DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
+	DRM_I915_ENGINE_CLASS_MAX /* non-ABI */
+};
+
+struct drm_i915_engine_info {
+	/** Engine instance number. */
+	__u32	instance;
+	__u32	rsvd;
+
+	/** Engine specific info. */
+#define DRM_I915_ENGINE_HAS_HEVC	BIT(0)
+	__u64 info;
+};
+
+struct drm_i915_gem_engine_info {
+	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
+	__u32 engine_class;
+
+	/** out: Actual number of hardware engines. */
+	__u32 num_engines;
+
+	/**
+	 * in: Number of struct drm_i915_engine_ifo entries in the provided
+	 * info array.
+	 */
+	__u32 info_size;
+	__u32 rsvd;
+
+	/** in/out: Pointer to array of struct i915_engine_info elements. */
+	__u64 info_ptr;
+
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Building on top of the previous patch which exported the concept
of engine classes and instances, we can also use this instead of
the current awkward engine selection uAPI.

This is primarily interesting for the VCS engine selection which
is a) currently done via disjoint set of flags, and b) the
current I915_EXEC_BSD flags has different semantics depending on
the underlying hardware which is bad.

Proposed idea here is to reserve 16-bits of flags, to pass in
the engine class and instance (8 bits each), and a new flag
named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
selection API is in use.

The new uAPI also removes access to the weak VCS engine
balancing as currently existing in the driver.

Example usage to send a command to VCS0:

  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);

Or to send a command to VCS1:

  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 36 ++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h                |  7 +++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index af1965774e7b..7fc92ec839a1 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1153,6 +1153,10 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
 	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
 		return false;
 
+	if ((exec->flags & I915_EXEC_CLASS_INSTANCE) &&
+	    (exec->flags & I915_EXEC_RING_MASK))
+		return false;
+
 	/* Kernel clipping was a DRI1 misfeature */
 	if (exec->num_cliprects || exec->cliprects_ptr)
 		return false;
@@ -1492,6 +1496,35 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
 	return file_priv->bsd_engine;
 }
 
+extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
+
+static struct intel_engine_cs *
+eb_select_engine_class_instance(struct drm_i915_private *i915,
+				struct drm_i915_gem_execbuffer2 *args)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u16 class_instance;
+	u8 user_class, class, instance;
+
+	class_instance = (args->flags & I915_EXEC_CLASS_INSTANCE_MASK) >>
+			 I915_EXEC_CLASS_INSTANCE_SHIFT;
+
+	user_class = class_instance >> 8;
+	instance = class_instance & 0xff;
+
+	if (user_class >= DRM_I915_ENGINE_CLASS_MAX)
+		return NULL;
+	class = user_class_map[user_class];
+
+	for_each_engine(engine, i915, id) {
+		if (engine->class == class && engine->instance == instance)
+			return engine;
+	}
+
+	return NULL;
+}
+
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
@@ -1510,6 +1543,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
 	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
 
+	if (args->flags & I915_EXEC_CLASS_INSTANCE)
+		return eb_select_engine_class_instance(dev_priv, args);
+
 	if (user_ring_id > I915_USER_RINGS) {
 		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
 		return NULL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 6058596a9f33..727a6dc4b029 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_FENCE_OUT		(1<<17)
 
-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
+#define I915_EXEC_CLASS_INSTANCE	(1<<18)
+
+#define I915_EXEC_CLASS_INSTANCE_SHIFT	(19)
+#define I915_EXEC_CLASS_INSTANCE_MASK	(0xffff << I915_EXEC_CLASS_INSTANCE_SHIFT)
+
+#define __I915_EXEC_UNKNOWN_FLAGS (-(35 << 1))
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for New engine discovery and execbuffer2 engine selection uAPI

[Patchwork]

== Series Details ==

Series: New engine discovery and execbuffer2 engine selection uAPI
URL   : https://patchwork.freedesktop.org/series/23189/
State : failure

== Summary ==

Series 23189v1 New engine discovery and execbuffer2 engine selection uAPI
https://patchwork.freedesktop.org/api/1.0/series/23189/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> FAIL       (fi-ilk-650)
        Subgroup basic-reload-final:
                pass       -> FAIL       (fi-ilk-650)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-bxt-t5700)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-skl-gvtdvm)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-bxt-j4205)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-bdw-gvtdvm)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-kbl-7560u)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-snb-2520m)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> SKIP       (fi-hsw-4770)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-byt-n2820)
                pass       -> SKIP       (fi-bsw-n3050)
                pass       -> SKIP       (fi-hsw-4770r)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-ivb-3520m)
                pass       -> SKIP       (fi-bdw-gvtdvm)
                pass       -> SKIP       (fi-snb-2600)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-ivb-3770)
                pass       -> SKIP       (fi-kbl-7560u)
                pass       -> SKIP       (fi-bdw-5557u)
                pass       -> SKIP       (fi-snb-2520m)
        Subgroup basic-default-heavy:
                pass       -> SKIP       (fi-hsw-4770)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-byt-n2820)
                pass       -> SKIP       (fi-bsw-n3050)
                pass       -> SKIP       (fi-hsw-4770r)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-ivb-3520m)
                pass       -> SKIP       (fi-bdw-gvtdvm)
                pass       -> SKIP       (fi-snb-2600)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-ivb-3770)
                pass       -> SKIP       (fi-kbl-7560u)
                pass       -> SKIP       (fi-bdw-5557u)
                pass       -> SKIP       (fi-snb-2520m)
Test gem_exec_basic:
        Subgroup basic-blt:
                pass       -> SKIP       (fi-hsw-4770)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-skl-6700hq)
                pass       -> SKIP       (fi-skl-gvtdvm)
                pass       -> SKIP       (fi-byt-n2820)
                pass       -> SKIP       (fi-bsw-n3050)
                pass       -> SKIP       (fi-hsw-4770r)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-ivb-3520m)
                pass       -> SKIP       (fi-bdw-gvtdvm)
                pass       -> SKIP       (fi-snb-2600)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6700k)
                pass       -> SKIP       (fi-ivb-3770)
                pass       -> SKIP       (fi-kbl-7560u)
                pass       -> SKIP       (fi-bdw-5557u)
                pass       -> SKIP       (fi-snb-2520m)
        Subgroup basic-bsd:
                pass       -> SKIP       (fi-hsw-4770)
                pass       -> SKIP       (fi-bxt-t5700)
                pass       -> SKIP       (fi-ilk-650)
                pass       -> SKIP       (fi-kbl-7500u)
WARNING: Long output truncated

7204beb80dcdfd1f8b1cff6a448301407f91a99c drm-tip: 2017y-04m-18d-16h-09m-11s UTC integration manifest
b8a7a65 drm/i915: Select engines via class and instance in execbuffer2
f68c903 drm/i915: Engine discovery uAPI

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4513/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/2] drm/i915: Engine discovery uAPI

[Chris Wilson]

On Tue, Apr 18, 2017 at 05:56:14PM +0100, Tvrtko Ursulin wrote:
> +enum drm_i915_gem_engine_class {
> +	DRM_I915_ENGINE_CLASS_OTHER = 0,
> +	DRM_I915_ENGINE_CLASS_RENDER = 1,
> +	DRM_I915_ENGINE_CLASS_COPY = 2,
> +	DRM_I915_ENGINE_CLASS_VIDEO_DECODE = 3,
> +	DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> +	DRM_I915_ENGINE_CLASS_MAX /* non-ABI */
> +};
> +
> +struct drm_i915_engine_info {
> +	/** Engine instance number. */
> +	__u32	instance;
> +	__u32	rsvd;
> +
> +	/** Engine specific info. */
> +#define DRM_I915_ENGINE_HAS_HEVC	BIT(0)
> +	__u64 info;
> +};

So the main question is how can we extend this in future, keeping
forwards/backwards compat?

I think if we put a query version into info and then the kernel supplies
an array matching that version (or reports the most recent version
supported if the request is too modern.)

The kernel has to keep all the old struct variants and exporters
indefinitely.

Another alternative would get an ENGINE_GETPARAM where we just have a
switch of all possibily questions. Maybe better as a CONTEXT_GETPARAM if
we start thinking about allowing CONTEXT_SETPARAM to fine tune
individual clients.

> +struct drm_i915_gem_engine_info {
> +	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
> +	__u32 engine_class;

__u32 [in/out] version ? (see above)

> +
> +	/** out: Actual number of hardware engines. */
> +	__u32 num_engines;
> +
> +	/**
> +	 * in: Number of struct drm_i915_engine_ifo entries in the provided
> +	 * info array.
> +	 */
> +	__u32 info_size;

This is superfluous with num_engines. The standard 2 pass, discovery of
size, followed by allocation and final query.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Tue, Apr 18, 2017 at 05:56:15PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
> 
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
> 
> Proposed idea here is to reserve 16-bits of flags, to pass in
> the engine class and instance (8 bits each), and a new flag
> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> selection API is in use.
> 
> The new uAPI also removes access to the weak VCS engine
> balancing as currently existing in the driver.
> 
> Example usage to send a command to VCS0:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> 
> Or to send a command to VCS1:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);

To save a bit of space, we can use the ring selector as a class selector
if bit18 is set, with 19-27 as instance. That limits us to 64 classes -
hopefully not a problem for near future. At least I might have you sold
you on a flexible execbuf3 by then.

(As a digression, some cryptic notes for an implementation I did over Easter:
/*
 * Execbuf3!
 *
 * ringbuffer 
 *  - per context
 *  - per engine
 *  - PAGE_SIZE ctl [ro head, rw tai] + user pot
 *  - kthread [i915/$ctx-$engine] (optional?)
 *  - assumes NO_RELOC-esque awareness
 *
 * SYNC flags [wait/signal], handle [semaphore/fence]
 *
 * BIND handle, offset [user provided]
 * ALLOC[32,64] handle, flags, *offset [kernel provided, need RELOC]
 * RELOC[32,64] handle, target_handle, offset, delta
 * CLEAR flags, handle
 * UNBIND handle
 *
 * BATCH flags, handle, offset
 * [or SVM flags, address]
 *   PIN flags (MAY_RELOC), count, handle[count]
 *   FENCE flags, count, handle[count]
 * SUBMIT handle [fence/NULL with error]
 */

At the moment it is just trying to do execbuf2, but more compactly and
with fewer ioctls. But one of the main selling points is that we can
extend the information passed around more freely than execbuf2.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/2] drm/i915: Engine discovery uAPI

[Kenneth Graunke]

[-- Attachment #1.1: Type: text/plain, Size: 1345 bytes --]

On Tuesday, April 18, 2017 9:56:14 AM PDT Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine discovery uAPI allows userspace to probe for engine
> configuration and features without needing to maintain the
> internal PCI id based database.

I don't understand why I would want to query the existence of engines
from the kernel.  As a userspace driver developer, I have to know how to
program the specific generation of hardware I'm on.  I better know what
engines that GPU has, or else there's no way I can competently program it.

In Mesa, we recently imported libdrm and deleted all the engine checks
(a460e1eb51406e5ca54abda42112bfb8523ff046).  All generations have an
RCS, Gen6+ has a separate BLT, and we don't need to use the others.
It's completely statically determinable with a simple check.  Runtime
checks make sense for optional things...but not "is there a 3D engine?".

Plus, even if you add this to the kernel, we still support back to 3.6
(and ChromeOS needs us to continue supporting 3.8), so we won't be able
to magically use the new uABI - we'd need to support both.  Which, if
the point is to delete code...we'd actually have /more/ code for a few
years.  Or, we could not use it...but then nobody would be testing it,
and if a bug creeps in...that pushes it back more years still.

Sorry :(

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [RFC 1/2] drm/i915: Engine discovery uAPI

[Gong, Zhipeng]

>-----Original Message-----
>From: Tvrtko Ursulin [mailto:tursulin@ursulin.net] 
>Sent: Wednesday, April 19, 2017 12:56 AM
>To: Intel-gfx@lists.freedesktop.org
>Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Ben Widawsky <ben@bwidawsk.net>; Chris Wilson <chris@chris-wilson.co.uk>; Vetter, Daniel <daniel.vetter@intel.com>; Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>; Charles, Daniel <daniel.charles@intel.com>; Rogozhkin, Dmitry V <dmitry.v.rogozhkin@intel.com>; Mateo Lozano, Oscar <oscar.mateo@intel.com>; Gong, Zhipeng <zhipeng.gong@intel.com>; intel-vaapi-media@lists.01.org; mesa-dev@lists.freedesktop.org
Subject: [RFC 1/2] drm/i915: Engine discovery uAPI

>+u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX] = {
>+	[DRM_I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
>+	[DRM_I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
>+	[DRM_I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
>+	[DRM_I915_ENGINE_CLASS_VIDEO_DECODE] = VIDEO_DECODE_CLASS,
>+	[DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS, };
>+

Not sure whether VIDEO_DECODE is a suitable name, since the same engine is also used in Media Encode. 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

On 18/04/2017 21:13, Chris Wilson wrote:
> On Tue, Apr 18, 2017 at 05:56:14PM +0100, Tvrtko Ursulin wrote:
>> +enum drm_i915_gem_engine_class {
>> +	DRM_I915_ENGINE_CLASS_OTHER = 0,
>> +	DRM_I915_ENGINE_CLASS_RENDER = 1,
>> +	DRM_I915_ENGINE_CLASS_COPY = 2,
>> +	DRM_I915_ENGINE_CLASS_VIDEO_DECODE = 3,
>> +	DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
>> +	DRM_I915_ENGINE_CLASS_MAX /* non-ABI */
>> +};
>> +
>> +struct drm_i915_engine_info {
>> +	/** Engine instance number. */
>> +	__u32	instance;
>> +	__u32	rsvd;
>> +
>> +	/** Engine specific info. */
>> +#define DRM_I915_ENGINE_HAS_HEVC	BIT(0)
>> +	__u64 info;
>> +};
>
> So the main question is how can we extend this in future, keeping
> forwards/backwards compat?
>
> I think if we put a query version into info and then the kernel supplies
> an array matching that version (or reports the most recent version
> supported if the request is too modern.)
>
> The kernel has to keep all the old struct variants and exporters
> indefinitely.

Versioning sounds good to me.

> Another alternative would get an ENGINE_GETPARAM where we just have a
> switch of all possibily questions. Maybe better as a CONTEXT_GETPARAM if
> we start thinking about allowing CONTEXT_SETPARAM to fine tune
> individual clients.

This idea I did not get - what is the switch of all possible questions? 
You mean new ioctl like ENGINE_GETPARAM which would return a list of 
queries supported by CONTEXT_GETPARAM? Which would effectively be a 
dispatcher-in-dispatcher kind of thing?

>> +struct drm_i915_gem_engine_info {
>> +	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
>> +	__u32 engine_class;
>
> __u32 [in/out] version ? (see above)
>
>> +
>> +	/** out: Actual number of hardware engines. */
>> +	__u32 num_engines;
>> +
>> +	/**
>> +	 * in: Number of struct drm_i915_engine_ifo entries in the provided
>> +	 * info array.
>> +	 */
>> +	__u32 info_size;
>
> This is superfluous with num_engines. The standard 2 pass, discovery of
> size, followed by allocation and final query.

This is also fine. I was one the fence whether to actually condense it 
to one field in the first posting or not myself.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Mesa-dev] [RFC 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

On 19/04/2017 06:22, Kenneth Graunke wrote:
> On Tuesday, April 18, 2017 9:56:14 AM PDT Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Engine discovery uAPI allows userspace to probe for engine
>> configuration and features without needing to maintain the
>> internal PCI id based database.
>
> I don't understand why I would want to query the existence of engines
> from the kernel.  As a userspace driver developer, I have to know how to
> program the specific generation of hardware I'm on.  I better know what
> engines that GPU has, or else there's no way I can competently program it.
>
> In Mesa, we recently imported libdrm and deleted all the engine checks
> (a460e1eb51406e5ca54abda42112bfb8523ff046).  All generations have an
> RCS, Gen6+ has a separate BLT, and we don't need to use the others.
> It's completely statically determinable with a simple check.  Runtime
> checks make sense for optional things...but not "is there a 3D engine?".
>
> Plus, even if you add this to the kernel, we still support back to 3.6
> (and ChromeOS needs us to continue supporting 3.8), so we won't be able
> to magically use the new uABI - we'd need to support both.  Which, if
> the point is to delete code...we'd actually have /more/ code for a few
> years.  Or, we could not use it...but then nobody would be testing it,
> and if a bug creeps in...that pushes it back more years still.

Okay the argument of more code for a while is I suppose always true with 
these types of work. But in general, the idea is to consolidate the 
queries and avoid (may be only partially) duplicate PCI databases across 
components.

Because I suspect today you do some device discovery via libpciaccess 
(or something) and some via i915 GET_PARAMs and so. So the idea is to 
consolidate all that and do it via i915. Since another argument you 
raise, is that you have to know how does the GPU looks like to be able 
to competently program it, in which case who knows better than the 
kernel driver?

But I think the main part of the argument is that why collect and derive 
this information from various sources when perhaps it could only be one.

Maybe the exact idea is not so interesting for Mesa, which I wouldn't be 
surprised at all since the idea was born from libva and the BSD engine 
usage there. In which case perhaps Mesa could become more interested if 
the proposal was exporting some other data to userspace?

I don't think it is critical to find something like that in Mesa, but it 
may be interesting. I think Ben mentioned at one point he had some ideas 
in this area, or something was discussed in the past which may be 
similar. I forgot the exact details now.

So I think for now, if there is nothing which would be interesting in 
Mesa along the lines described so far, please just keep an eye on this. 
Just to make sure if some other component will be interested, and we end 
up starting to implement something, it is at least not hindering you, if 
we cannot find anything useful for you in here.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 18/04/2017 22:10, Chris Wilson wrote:
> On Tue, Apr 18, 2017 at 05:56:15PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Building on top of the previous patch which exported the concept
>> of engine classes and instances, we can also use this instead of
>> the current awkward engine selection uAPI.
>>
>> This is primarily interesting for the VCS engine selection which
>> is a) currently done via disjoint set of flags, and b) the
>> current I915_EXEC_BSD flags has different semantics depending on
>> the underlying hardware which is bad.
>>
>> Proposed idea here is to reserve 16-bits of flags, to pass in
>> the engine class and instance (8 bits each), and a new flag
>> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
>> selection API is in use.
>>
>> The new uAPI also removes access to the weak VCS engine
>> balancing as currently existing in the driver.
>>
>> Example usage to send a command to VCS0:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
>>
>> Or to send a command to VCS1:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
>
> To save a bit of space, we can use the ring selector as a class selector
> if bit18 is set, with 19-27 as instance. That limits us to 64 classes -
> hopefully not a problem for near future. At least I might have you sold
> you on a flexible execbuf3 by then.

I was considering re-using those bits yes. I was thinking about the pro 
of keeping it completely separate but I suppose there is not much value 
in that. So I can re-use the ring selector just as well and have a 
smaller impact on number of bits left over.

> (As a digression, some cryptic notes for an implementation I did over Easter:
> /*
>  * Execbuf3!
>  *
>  * ringbuffer
>  *  - per context
>  *  - per engine

We have this already so I am missing something I guess.

>  *  - PAGE_SIZE ctl [ro head, rw tai] + user pot
>  *  - kthread [i915/$ctx-$engine] (optional?)

No idea what these two are. :)

>  *  - assumes NO_RELOC-esque awareness

Ok ok NO_RELOC. :)

>  *
>  * SYNC flags [wait/signal], handle [semaphore/fence]

Sync fence in out just as today, but probably more?

>  *
>  * BIND handle, offset [user provided]
>  * ALLOC[32,64] handle, flags, *offset [kernel provided, need RELOC]
>  * RELOC[32,64] handle, target_handle, offset, delta
>  * CLEAR flags, handle
>  * UNBIND handle

Explicit VMA management? Separate ioctl maybe would be better?

>  *
>  * BATCH flags, handle, offset
>  * [or SVM flags, address]
>  *   PIN flags (MAY_RELOC), count, handle[count]
>  *   FENCE flags, count, handle[count]
>  * SUBMIT handle [fence/NULL with error]
>  */

No idea again. :)

> At the moment it is just trying to do execbuf2, but more compactly and
> with fewer ioctls. But one of the main selling points is that we can
> extend the information passed around more freely than execbuf2.)

I have nothing against a better eb since I trust you know much better it 
is needed and when. But I don't know how long it will take to get there. 
This class/instance idea could be implemented quickly to solve the sore 
point of VCS/VCS2 engine selection. But yeah, it is another uABI to keep 
in that case.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v2 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine discovery uAPI allows userspace to probe for engine
configuration and features without needing to maintain the
internal PCI id based database.

This enables removal of code duplications across userspace
components.

Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
which returns the number and information on the specified engine
class.

Currently only general engine configuration and HEVC feature of
the VCS engine can be probed but the uAPI is designed to be
generic and extensible.

Code is based almost exactly on the earlier proposal on the
topic by Jon Bloomfield. Engine class and instance refactoring
made recently by Daniele Ceraolo Spurio enabled this to be
implemented in an elegant fashion.

To probe configuration userspace sets the engine class it wants
to query (struct drm_i915_gem_engine_info) and provides an array
of drm_i915_engine_info structs which will be filled in by the
driver. Userspace also has to tell i915 how many elements are in
the array, and the driver will report back the total number of
engine instances in any case.

v2:
 * Add a version field and consolidate to one engine count.
   (Chris Wilson)
 * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
   (Gong Zhipeng)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_drv.c        |  1 +
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h            | 40 +++++++++++++++++++++
 4 files changed, 108 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c7d68e789642..1a3f0859227b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2609,6 +2609,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6c2f04..6eed0e854561 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3547,6 +3547,9 @@ i915_gem_context_lookup_timeline(struct i915_gem_context *ctx,
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
 
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  u64 min_size, u64 alignment,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 82a274b336c5..caed32dbd912 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,6 +25,7 @@
 #include "i915_drv.h"
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
+#include <uapi/drm/i915_drm.h>
 
 struct engine_class_info {
 	const char *name;
@@ -1187,6 +1188,69 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
 		engine->set_default_submission(engine);
 }
 
+u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX] = {
+	[DRM_I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
+	[DRM_I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
+	[DRM_I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
+	[DRM_I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
+	[DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
+};
+
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_engine_info *args = data;
+	struct drm_i915_engine_info __user *user_info =
+					u64_to_user_ptr(args->info_ptr);
+	unsigned int info_size = args->num_engines;
+	struct drm_i915_engine_info info;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u8 class;
+
+	if (args->rsvd)
+		return -EINVAL;
+
+	switch (args->engine_class) {
+	case DRM_I915_ENGINE_CLASS_OTHER:
+	case DRM_I915_ENGINE_CLASS_RENDER:
+	case DRM_I915_ENGINE_CLASS_COPY:
+	case DRM_I915_ENGINE_CLASS_VIDEO:
+	case DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE:
+		class = user_class_map[args->engine_class];
+		break;
+	case DRM_I915_ENGINE_CLASS_MAX:
+	default:
+		return -EINVAL;
+	};
+
+	args->num_engines = 0;
+
+	if (args->version != 1) {
+		args->version = 1;
+		return 0;
+	}
+
+	for_each_engine(engine, i915, id) {
+		if (class != engine->class)
+			continue;
+
+		if (++args->num_engines > info_size)
+			continue;
+
+		memset(&info, 0, sizeof(info));
+		info.instance = engine->instance;
+		if (INTEL_GEN(i915) >= 8 && id == VCS)
+			info.info = DRM_I915_ENGINE_HAS_HEVC;
+
+		if (copy_to_user(user_info++, &info, sizeof(info)))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f24a80d2d42e..2ac6667e57ea 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -260,6 +260,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
 #define DRM_I915_PERF_OPEN		0x36
+#define DRM_I915_GEM_ENGINE_INFO	0x37
 
 #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)
@@ -315,6 +316,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
+#define DRM_IOCTL_I915_GEM_ENGINE_INFO	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_ENGINE_INFO, struct drm_i915_gem_engine_info)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1439,6 +1441,44 @@ enum drm_i915_perf_record_type {
 	DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };
 
+enum drm_i915_gem_engine_class {
+	DRM_I915_ENGINE_CLASS_OTHER = 0,
+	DRM_I915_ENGINE_CLASS_RENDER = 1,
+	DRM_I915_ENGINE_CLASS_COPY = 2,
+	DRM_I915_ENGINE_CLASS_VIDEO = 3,
+	DRM_I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
+	DRM_I915_ENGINE_CLASS_MAX /* non-ABI */
+};
+
+struct drm_i915_engine_info {
+	/** Engine instance number. */
+	__u32	instance;
+	__u32	rsvd;
+
+	/** Engine specific info. */
+#define DRM_I915_ENGINE_HAS_HEVC	BIT(0)
+	__u64 info;
+};
+
+struct drm_i915_gem_engine_info {
+	/** in/out: Protocol version requested/supported. */
+	__u32 version;
+
+	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
+	__u32 engine_class;
+
+	/**
+	 * in/out: Number of struct drm_i915_engine_info entries in the provided
+	 * @info_ptr array and actual number of supported hardware engines.
+	 */
+	__u32 num_engines;
+	__u32 rsvd;
+
+	/** in/out: Pointer to array of struct i915_engine_info elements. */
+	__u64 info_ptr;
+
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v2 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Building on top of the previous patch which exported the concept
of engine classes and instances, we can also use this instead of
the current awkward engine selection uAPI.

This is primarily interesting for the VCS engine selection which
is a) currently done via disjoint set of flags, and b) the
current I915_EXEC_BSD flags has different semantics depending on
the underlying hardware which is bad.

Proposed idea here is to reserve 16-bits of flags, to pass in
the engine class and instance (8 bits each), and a new flag
named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
selection API is in use.

The new uAPI also removes access to the weak VCS engine
balancing as currently existing in the driver.

Example usage to send a command to VCS0:

  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);

Or to send a command to VCS1:

  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);

v2:
 * Fix unknown flags mask.
 * Use I915_EXEC_RING_MASK for class. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 +++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h                | 11 ++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index af1965774e7b..ecd1486642a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1492,6 +1492,32 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
 	return file_priv->bsd_engine;
 }
 
+extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
+
+static struct intel_engine_cs *
+eb_select_engine_class_instance(struct drm_i915_private *i915,
+				struct drm_i915_gem_execbuffer2 *args)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u8 class, instance;
+
+	class = args->flags & I915_EXEC_RING_MASK;
+	if (class >= DRM_I915_ENGINE_CLASS_MAX)
+		return NULL;
+	class = user_class_map[class];
+
+	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
+		   I915_EXEC_INSTANCE_MASK;
+
+	for_each_engine(engine, i915, id) {
+		if (engine->class == class && engine->instance == instance)
+			return engine;
+	}
+
+	return NULL;
+}
+
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
@@ -1510,6 +1536,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
 	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
 
+	if (args->flags & I915_EXEC_CLASS_INSTANCE)
+		return eb_select_engine_class_instance(dev_priv, args);
+
 	if (user_ring_id > I915_USER_RINGS) {
 		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
 		return NULL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 2ac6667e57ea..6a26bdf5e684 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_FENCE_OUT		(1<<17)
 
-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
+#define I915_EXEC_CLASS_INSTANCE	(1<<18)
+
+#define I915_EXEC_INSTANCE_SHIFT	(19)
+#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
+
+#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
@@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
 #define i915_execbuffer2_get_context_id(eb2) \
 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
 
+#define i915_execbuffer2_engine(class, instance) \
+	(I915_EXEC_CLASS_INSTANCE | (class) | \
+	((instance) << I915_EXEC_INSTANCE_SHIFT))
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v2 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, Apr 27, 2017 at 10:10:34AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
> 
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
> 
> Proposed idea here is to reserve 16-bits of flags, to pass in
> the engine class and instance (8 bits each), and a new flag
> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> selection API is in use.
> 
> The new uAPI also removes access to the weak VCS engine
> balancing as currently existing in the driver.
> 
> Example usage to send a command to VCS0:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> 
> Or to send a command to VCS1:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
> 
> v2:
>  * Fix unknown flags mask.
>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Daniel Charles <daniel.charles@intel.com>
> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> Cc: intel-vaapi-media@lists.01.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 +++++++++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h                | 11 ++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index af1965774e7b..ecd1486642a7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1492,6 +1492,32 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>  	return file_priv->bsd_engine;
>  }
>  
> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
> +
> +static struct intel_engine_cs *
> +eb_select_engine_class_instance(struct drm_i915_private *i915,
> +				struct drm_i915_gem_execbuffer2 *args)
> +{
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	u8 class, instance;
> +
> +	class = args->flags & I915_EXEC_RING_MASK;
> +	if (class >= DRM_I915_ENGINE_CLASS_MAX)
> +		return NULL;
> +	class = user_class_map[class];
> +
> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> +		   I915_EXEC_INSTANCE_MASK;
> +
> +	for_each_engine(engine, i915, id) {
> +		if (engine->class == class && engine->instance == instance)
> +			return engine;
> +	}

I am underwhelmed. No, i915->class_engine[class][instance] ?

Still, at what point do we kill busy-ioctl per-engine reporting? Should
we update all tracepoints to use class:instance (I think that's a better
abi going forward).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v2 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 27/04/2017 10:25, Chris Wilson wrote:
> On Thu, Apr 27, 2017 at 10:10:34AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Building on top of the previous patch which exported the concept
>> of engine classes and instances, we can also use this instead of
>> the current awkward engine selection uAPI.
>>
>> This is primarily interesting for the VCS engine selection which
>> is a) currently done via disjoint set of flags, and b) the
>> current I915_EXEC_BSD flags has different semantics depending on
>> the underlying hardware which is bad.
>>
>> Proposed idea here is to reserve 16-bits of flags, to pass in
>> the engine class and instance (8 bits each), and a new flag
>> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
>> selection API is in use.
>>
>> The new uAPI also removes access to the weak VCS engine
>> balancing as currently existing in the driver.
>>
>> Example usage to send a command to VCS0:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
>>
>> Or to send a command to VCS1:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
>>
>> v2:
>>  * Fix unknown flags mask.
>>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ben Widawsky <ben@bwidawsk.net>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Daniel Charles <daniel.charles@intel.com>
>> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
>> Cc: intel-vaapi-media@lists.01.org
>> Cc: mesa-dev@lists.freedesktop.org
>> ---
>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 +++++++++++++++++++++++++++++
>>  include/uapi/drm/i915_drm.h                | 11 ++++++++++-
>>  2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index af1965774e7b..ecd1486642a7 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1492,6 +1492,32 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>>  	return file_priv->bsd_engine;
>>  }
>>
>> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
>> +
>> +static struct intel_engine_cs *
>> +eb_select_engine_class_instance(struct drm_i915_private *i915,
>> +				struct drm_i915_gem_execbuffer2 *args)
>> +{
>> +	struct intel_engine_cs *engine;
>> +	enum intel_engine_id id;
>> +	u8 class, instance;
>> +
>> +	class = args->flags & I915_EXEC_RING_MASK;
>> +	if (class >= DRM_I915_ENGINE_CLASS_MAX)
>> +		return NULL;
>> +	class = user_class_map[class];
>> +
>> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
>> +		   I915_EXEC_INSTANCE_MASK;
>> +
>> +	for_each_engine(engine, i915, id) {
>> +		if (engine->class == class && engine->instance == instance)
>> +			return engine;
>> +	}
>
> I am underwhelmed. No, i915->class_engine[class][instance] ?

Hey it's just an RFC for the uAPI proposal! Implementation efficiency 
only comes later! :)

> Still, at what point do we kill busy-ioctl per-engine reporting? Should

It's the one we already broke before without no one noticing, where it 
userspace only effectively cares about a boolean value?

If so you recommend we make it a real boolean?

> we update all tracepoints to use class:instance (I think that's a better
> abi going forward).

I can't think of any big problems doing so. Could rename ring= to 
engine= there as well. engine=<class>.<instance> for example?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for New engine discovery and execbuffer2 engine selection uAPI (rev3)

[Patchwork]

== Series Details ==

Series: New engine discovery and execbuffer2 engine selection uAPI (rev3)
URL   : https://patchwork.freedesktop.org/series/23189/
State : success

== Summary ==

Series 23189v3 New engine discovery and execbuffer2 engine selection uAPI
https://patchwork.freedesktop.org/api/1.0/series/23189/revisions/3/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-skl-6770hq) fdo#99739

fdo#99739 https://bugs.freedesktop.org/show_bug.cgi?id=99739

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:415s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:423s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:522s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:475s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:563s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:481s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:488s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:406s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:487s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:440s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:556s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:438s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:552s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:448s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:473s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:432s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:407s

a0363fea4a56eef81e8cda759a24d8951dd7ac73 drm-tip: 2017y-04m-27d-08h-13m-25s UTC integration manifest
02b92bc drm/i915: Select engines via class and instance in execbuffer2
90aa3b5 drm/i915: Engine discovery uAPI

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4564/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v2 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, Apr 27, 2017 at 11:09:35AM +0100, Tvrtko Ursulin wrote:
> 
> On 27/04/2017 10:25, Chris Wilson wrote:
> >On Thu, Apr 27, 2017 at 10:10:34AM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Building on top of the previous patch which exported the concept
> >>of engine classes and instances, we can also use this instead of
> >>the current awkward engine selection uAPI.
> >>
> >>This is primarily interesting for the VCS engine selection which
> >>is a) currently done via disjoint set of flags, and b) the
> >>current I915_EXEC_BSD flags has different semantics depending on
> >>the underlying hardware which is bad.
> >>
> >>Proposed idea here is to reserve 16-bits of flags, to pass in
> >>the engine class and instance (8 bits each), and a new flag
> >>named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> >>selection API is in use.
> >>
> >>The new uAPI also removes access to the weak VCS engine
> >>balancing as currently existing in the driver.
> >>
> >>Example usage to send a command to VCS0:
> >>
> >>  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> >>
> >>Or to send a command to VCS1:
> >>
> >>  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
> >>
> >>v2:
> >> * Fix unknown flags mask.
> >> * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Cc: Ben Widawsky <ben@bwidawsk.net>
> >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: Daniel Vetter <daniel.vetter@intel.com>
> >>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> >>Cc: Daniel Charles <daniel.charles@intel.com>
> >>Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> >>Cc: Oscar Mateo <oscar.mateo@intel.com>
> >>Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> >>Cc: intel-vaapi-media@lists.01.org
> >>Cc: mesa-dev@lists.freedesktop.org
> >>---
> >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 +++++++++++++++++++++++++++++
> >> include/uapi/drm/i915_drm.h                | 11 ++++++++++-
> >> 2 files changed, 39 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>index af1965774e7b..ecd1486642a7 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>@@ -1492,6 +1492,32 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
> >> 	return file_priv->bsd_engine;
> >> }
> >>
> >>+extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
> >>+
> >>+static struct intel_engine_cs *
> >>+eb_select_engine_class_instance(struct drm_i915_private *i915,
> >>+				struct drm_i915_gem_execbuffer2 *args)
> >>+{
> >>+	struct intel_engine_cs *engine;
> >>+	enum intel_engine_id id;
> >>+	u8 class, instance;
> >>+
> >>+	class = args->flags & I915_EXEC_RING_MASK;
> >>+	if (class >= DRM_I915_ENGINE_CLASS_MAX)
> >>+		return NULL;
> >>+	class = user_class_map[class];
> >>+
> >>+	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> >>+		   I915_EXEC_INSTANCE_MASK;
> >>+
> >>+	for_each_engine(engine, i915, id) {
> >>+		if (engine->class == class && engine->instance == instance)
> >>+			return engine;
> >>+	}
> >
> >I am underwhelmed. No, i915->class_engine[class][instance] ?
> 
> Hey it's just an RFC for the uAPI proposal! Implementation
> efficiency only comes later! :)
> 
> >Still, at what point do we kill busy-ioctl per-engine reporting? Should
> 
> It's the one we already broke before without no one noticing, where
> it userspace only effectively cares about a boolean value?

Userspace does try to distinguish between RCS and !RCS. But it's a rough
heuristic that I'm not going to cry much over since it depends upon on
so many other factors outside of its control as to which placement is
better.
 
> If so you recommend we make it a real boolean?

Once we cross the u16 threshold, yup. Just then a busy read/write pair.
 
> >we update all tracepoints to use class:instance (I think that's a better
> >abi going forward).
> 
> I can't think of any big problems doing so. Could rename ring= to
> engine= there as well. engine=<class>.<instance> for example?

Works for me.

There are still a few other places where we want an index into an array,
so keeping a map to engine->uabi_id seems sensible. Or we always include
engine.instance as part of our uABI for extensible structs. E.g.

struct context_watchdog_param {
	u32 engine;
	u32 instance;
	u64 watchdog_ns;
};

Then set/get_context_param return an array of those rather than an array
of watchdog_ns.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Building on top of the previous patch which exported the concept
of engine classes and instances, we can also use this instead of
the current awkward engine selection uAPI.

This is primarily interesting for the VCS engine selection which
is a) currently done via disjoint set of flags, and b) the
current I915_EXEC_BSD flags has different semantics depending on
the underlying hardware which is bad.

Proposed idea here is to reserve 16-bits of flags, to pass in
the engine class and instance (8 bits each), and a new flag
named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
selection API is in use.

The new uAPI also removes access to the weak VCS engine
balancing as currently existing in the driver.

Example usage to send a command to VCS0:

  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);

Or to send a command to VCS1:

  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);

v2:
 * Fix unknown flags mask.
 * Use I915_EXEC_RING_MASK for class. (Chris Wilson)

v3:
 * Add a map for fast class-instance engine lookup. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  3 +++
 drivers/gpu/drm/i915/intel_engine_cs.c     |  7 +++++++
 include/uapi/drm/i915_drm.h                | 11 ++++++++++-
 5 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5dfa4a12e647..7bf4fd42480c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2066,6 +2066,7 @@ struct drm_i915_private {
 	struct pci_dev *bridge_dev;
 	struct i915_gem_context *kernel_context;
 	struct intel_engine_cs *engine[I915_NUM_ENGINES];
+	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
 	struct i915_vma *semaphore;
 
 	struct drm_dma_handle *status_page_dmah;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index af1965774e7b..c1ad49ab64cd 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1492,6 +1492,33 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
 	return file_priv->bsd_engine;
 }
 
+extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
+
+static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
+						u8 class, u8 instance)
+{
+	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
+		return NULL;
+
+	return i915->engine_class[class][instance];
+}
+
+static struct intel_engine_cs *
+eb_select_engine_class_instance(struct drm_i915_private *i915,
+				struct drm_i915_gem_execbuffer2 *args)
+{
+	u8 class, instance;
+
+	class = args->flags & I915_EXEC_RING_MASK;
+	if (class >= DRM_I915_ENGINE_CLASS_MAX)
+		return NULL;
+
+	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
+		   I915_EXEC_INSTANCE_MASK;
+
+	return get_engine_class(i915, user_class_map[class], instance);
+}
+
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
@@ -1510,6 +1537,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
 	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
 
+	if (args->flags & I915_EXEC_CLASS_INSTANCE)
+		return eb_select_engine_class_instance(dev_priv, args);
+
 	if (user_ring_id > I915_USER_RINGS) {
 		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
 		return NULL;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ee144ec57935..a3b59043b991 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define VIDEO_ENHANCEMENT_CLASS	2
 #define COPY_ENGINE_CLASS	3
 #define OTHER_CLASS		4
+#define MAX_ENGINE_CLASS	4
+
+#define MAX_ENGINE_INSTANCE	1
 
 /* PCI config space */
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 7566cf48012f..c5ad51c43d23 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
 
+	if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
+		    info->instance > MAX_ENGINE_INSTANCE ||
+		    dev_priv->engine_class[info->class][info->instance]))
+		return -EINVAL;
+
+	dev_priv->engine_class[info->class][info->instance] = engine;
 	dev_priv->engine[id] = engine;
+
 	return 0;
 }
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 2ac6667e57ea..6a26bdf5e684 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_FENCE_OUT		(1<<17)
 
-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
+#define I915_EXEC_CLASS_INSTANCE	(1<<18)
+
+#define I915_EXEC_INSTANCE_SHIFT	(19)
+#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
+
+#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
@@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
 #define i915_execbuffer2_get_context_id(eb2) \
 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
 
+#define i915_execbuffer2_engine(class, instance) \
+	(I915_EXEC_CLASS_INSTANCE | (class) | \
+	((instance) << I915_EXEC_INSTANCE_SHIFT))
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Wed, May 17, 2017 at 04:40:57PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
> 
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
> 
> Proposed idea here is to reserve 16-bits of flags, to pass in
> the engine class and instance (8 bits each), and a new flag
> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> selection API is in use.
> 
> The new uAPI also removes access to the weak VCS engine
> balancing as currently existing in the driver.
> 
> Example usage to send a command to VCS0:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> 
> Or to send a command to VCS1:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
> 
> v2:
>  * Fix unknown flags mask.
>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
> 
> v3:
>  * Add a map for fast class-instance engine lookup. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Daniel Charles <daniel.charles@intel.com>
> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> Cc: intel-vaapi-media@lists.01.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  3 +++
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  7 +++++++
>  include/uapi/drm/i915_drm.h                | 11 ++++++++++-
>  5 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5dfa4a12e647..7bf4fd42480c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2066,6 +2066,7 @@ struct drm_i915_private {
>  	struct pci_dev *bridge_dev;
>  	struct i915_gem_context *kernel_context;
>  	struct intel_engine_cs *engine[I915_NUM_ENGINES];
> +	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
>  	struct i915_vma *semaphore;
>  
>  	struct drm_dma_handle *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index af1965774e7b..c1ad49ab64cd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1492,6 +1492,33 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>  	return file_priv->bsd_engine;
>  }
>  
> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
> +
> +static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
> +						u8 class, u8 instance)
> +{
> +	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
> +		return NULL;
> +
> +	return i915->engine_class[class][instance];
> +}

Be bold make this this an intel_engine_lookup(), I forsee some other
users appearing very shortly.

> +static struct intel_engine_cs *
> +eb_select_engine_class_instance(struct drm_i915_private *i915,
> +				struct drm_i915_gem_execbuffer2 *args)
> +{
> +	u8 class, instance;
> +
> +	class = args->flags & I915_EXEC_RING_MASK;
> +	if (class >= DRM_I915_ENGINE_CLASS_MAX)
> +		return NULL;
> +
> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> +		   I915_EXEC_INSTANCE_MASK;
> +
> +	return get_engine_class(i915, user_class_map[class], instance);
> +}
> +
>  #define I915_USER_RINGS (4)
>  
>  static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> @@ -1510,6 +1537,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
>  	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
>  	struct intel_engine_cs *engine;
>  
> +	if (args->flags & I915_EXEC_CLASS_INSTANCE)
> +		return eb_select_engine_class_instance(dev_priv, args);
> +
>  	if (user_ring_id > I915_USER_RINGS) {
>  		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
>  		return NULL;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ee144ec57935..a3b59043b991 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define VIDEO_ENHANCEMENT_CLASS	2
>  #define COPY_ENGINE_CLASS	3
>  #define OTHER_CLASS		4
> +#define MAX_ENGINE_CLASS	4
> +
> +#define MAX_ENGINE_INSTANCE	1

We also need the names in the uapi.h as well. Do we want to mention
OTHER_CLASS before it is defined? I trust your crystal ball.

Amusingly I notice that you do claim that you have these names in the
changelog. :) We don't need DRM_I915 all the time. I915_ENGINE_CLASS is
going to be unique, at least for those users of i915_drm.h

>  /* PCI config space */
>  
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 7566cf48012f..c5ad51c43d23 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>  
>  	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
>  
> +	if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
> +		    info->instance > MAX_ENGINE_INSTANCE ||
> +		    dev_priv->engine_class[info->class][info->instance]))

Spread these out or add a message telling what was wrong.

> +		return -EINVAL;
> +
> +	dev_priv->engine_class[info->class][info->instance] = engine;
>  	dev_priv->engine[id] = engine;
> +
>  	return 0;
>  }
>  
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2ac6667e57ea..6a26bdf5e684 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
>   */
>  #define I915_EXEC_FENCE_OUT		(1<<17)
>  
> -#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
> +#define I915_EXEC_CLASS_INSTANCE	(1<<18)
> +
> +#define I915_EXEC_INSTANCE_SHIFT	(19)
> +#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
> +
> +#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
>  
>  #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
>  #define i915_execbuffer2_set_context_id(eb2, context) \
> @@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
>  #define i915_execbuffer2_get_context_id(eb2) \
>  	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
>  
> +#define i915_execbuffer2_engine(class, instance) \
> +	(I915_EXEC_CLASS_INSTANCE | (class) | \
> +	((instance) << I915_EXEC_INSTANCE_SHIFT))

(class |
 (instance) << I915_EXEC_INSTANCE_SHIFT |
 I915_EXEC_CLASS_INSTANCE)

Just suggesting to spread it out over another line for better
readability.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Joonas Lahtinen]

On ke, 2017-05-17 at 16:40 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
> 
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
> 
> Proposed idea here is to reserve 16-bits of flags, to pass in
> the engine class and instance (8 bits each), and a new flag
> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> selection API is in use.

Would it make sense to use bitmask for future proofing? Then we could
allow passing multiple options in the future.

Other than that, looks good. Chris already commented some on the code
itself.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, May 18, 2017 at 01:55:59PM +0300, Joonas Lahtinen wrote:
> On ke, 2017-05-17 at 16:40 +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Building on top of the previous patch which exported the concept
> > of engine classes and instances, we can also use this instead of
> > the current awkward engine selection uAPI.
> > 
> > This is primarily interesting for the VCS engine selection which
> > is a) currently done via disjoint set of flags, and b) the
> > current I915_EXEC_BSD flags has different semantics depending on
> > the underlying hardware which is bad.
> > 
> > Proposed idea here is to reserve 16-bits of flags, to pass in
> > the engine class and instance (8 bits each), and a new flag
> > named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> > selection API is in use.

Note this text doesn't describe the interface in v3.

> Would it make sense to use bitmask for future proofing? Then we could
> allow passing multiple options in the future.

No. The first use case has to be explicit control of engine. That's
orthogonal to asking to select any of a particular class.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 18/05/2017 12:10, Chris Wilson wrote:
> On Thu, May 18, 2017 at 01:55:59PM +0300, Joonas Lahtinen wrote:
>> On ke, 2017-05-17 at 16:40 +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Building on top of the previous patch which exported the concept
>>> of engine classes and instances, we can also use this instead of
>>> the current awkward engine selection uAPI.
>>>
>>> This is primarily interesting for the VCS engine selection which
>>> is a) currently done via disjoint set of flags, and b) the
>>> current I915_EXEC_BSD flags has different semantics depending on
>>> the underlying hardware which is bad.
>>>
>>> Proposed idea here is to reserve 16-bits of flags, to pass in
>>> the engine class and instance (8 bits each), and a new flag
>>> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
>>> selection API is in use.
>
> Note this text doesn't describe the interface in v3.
>
>> Would it make sense to use bitmask for future proofing? Then we could
>> allow passing multiple options in the future.
>
> No. The first use case has to be explicit control of engine. That's
> orthogonal to asking to select any of a particular class.

Was the suggestion to allow instance=any and instance=N? Or even all the 
way to instance=N-or-M?

If not the latter, then I can think of two other possible approaches:

1. Reserve 0xff as instance=any, aka 128 instances should be enough for 
everbody. :) Could simply enforce in the uAPI that instance == 
I915_EXEC_INSTANCE_MASK = -EINVAL for now or something.

2. Do nothing now and add say I915_EXEC_CLASS at a later point. This 
patch adds I915_EXEC_CLASS_INSTANCE so I915_EXEC_CLASS would not sound 
out of place.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, May 18, 2017 at 01:13:20PM +0100, Tvrtko Ursulin wrote:
> 
> On 18/05/2017 12:10, Chris Wilson wrote:
> >On Thu, May 18, 2017 at 01:55:59PM +0300, Joonas Lahtinen wrote:
> >>On ke, 2017-05-17 at 16:40 +0100, Tvrtko Ursulin wrote:
> >>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>>Building on top of the previous patch which exported the concept
> >>>of engine classes and instances, we can also use this instead of
> >>>the current awkward engine selection uAPI.
> >>>
> >>>This is primarily interesting for the VCS engine selection which
> >>>is a) currently done via disjoint set of flags, and b) the
> >>>current I915_EXEC_BSD flags has different semantics depending on
> >>>the underlying hardware which is bad.
> >>>
> >>>Proposed idea here is to reserve 16-bits of flags, to pass in
> >>>the engine class and instance (8 bits each), and a new flag
> >>>named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> >>>selection API is in use.
> >
> >Note this text doesn't describe the interface in v3.
> >
> >>Would it make sense to use bitmask for future proofing? Then we could
> >>allow passing multiple options in the future.
> >
> >No. The first use case has to be explicit control of engine. That's
> >orthogonal to asking to select any of a particular class.
> 
> Was the suggestion to allow instance=any and instance=N? Or even all
> the way to instance=N-or-M?
> 
> If not the latter, then I can think of two other possible approaches:
> 
> 1. Reserve 0xff as instance=any, aka 128 instances should be enough
> for everbody. :) Could simply enforce in the uAPI that instance ==
> I915_EXEC_INSTANCE_MASK = -EINVAL for now or something.
> 
> 2. Do nothing now and add say I915_EXEC_CLASS at a later point. This
> patch adds I915_EXEC_CLASS_INSTANCE so I915_EXEC_CLASS would not
> sound out of place.

Yes, I would argue to defer it until later. One problem I have at the
moment is that not all members of a class are equal, HEVC-capable
engines and the reset do not belong to the same class (i.e. my hope is
that we could just say <class> | [ <mask> | INSTANCE_MASK ] | LOAD_BALANCE)
So I can see the sense in having instance as a mask, or at least making
the current instance field large enough to store a mask in future. I
just feel uneasy as that field could grow quite large, and maybe it will
be better to set the constraint via a context param (all dependency on
frequency and tuning of the LOAD_BALANCE). Hmm, liking having the
instance-mask on the context atm.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 18/05/2017 13:24, Chris Wilson wrote:
> On Thu, May 18, 2017 at 01:13:20PM +0100, Tvrtko Ursulin wrote:
>>
>> On 18/05/2017 12:10, Chris Wilson wrote:
>>> On Thu, May 18, 2017 at 01:55:59PM +0300, Joonas Lahtinen wrote:
>>>> On ke, 2017-05-17 at 16:40 +0100, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Building on top of the previous patch which exported the concept
>>>>> of engine classes and instances, we can also use this instead of
>>>>> the current awkward engine selection uAPI.
>>>>>
>>>>> This is primarily interesting for the VCS engine selection which
>>>>> is a) currently done via disjoint set of flags, and b) the
>>>>> current I915_EXEC_BSD flags has different semantics depending on
>>>>> the underlying hardware which is bad.
>>>>>
>>>>> Proposed idea here is to reserve 16-bits of flags, to pass in
>>>>> the engine class and instance (8 bits each), and a new flag
>>>>> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
>>>>> selection API is in use.
>>>
>>> Note this text doesn't describe the interface in v3.
>>>
>>>> Would it make sense to use bitmask for future proofing? Then we could
>>>> allow passing multiple options in the future.
>>>
>>> No. The first use case has to be explicit control of engine. That's
>>> orthogonal to asking to select any of a particular class.
>>
>> Was the suggestion to allow instance=any and instance=N? Or even all
>> the way to instance=N-or-M?
>>
>> If not the latter, then I can think of two other possible approaches:
>>
>> 1. Reserve 0xff as instance=any, aka 128 instances should be enough
>> for everbody. :) Could simply enforce in the uAPI that instance ==
>> I915_EXEC_INSTANCE_MASK = -EINVAL for now or something.
>>
>> 2. Do nothing now and add say I915_EXEC_CLASS at a later point. This
>> patch adds I915_EXEC_CLASS_INSTANCE so I915_EXEC_CLASS would not
>> sound out of place.
>
> Yes, I would argue to defer it until later. One problem I have at the
> moment is that not all members of a class are equal, HEVC-capable
> engines and the reset do not belong to the same class (i.e. my hope is
> that we could just say <class> | [ <mask> | INSTANCE_MASK ] | LOAD_BALANCE)
> So I can see the sense in having instance as a mask, or at least making
> the current instance field large enough to store a mask in future. I
> just feel uneasy as that field could grow quite large, and maybe it will
> be better to set the constraint via a context param (all dependency on
> frequency and tuning of the LOAD_BALANCE). Hmm, liking having the
> instance-mask on the context atm.

I don't think per context mask would work unless you won't to mandate 
multi-contexts where they wouldn't otherwise be needed.

But this problem in general can also be solved separately from 
class-instance addressing via engine feature masking.

As the GEM_ENGINE_INFO ioctl proposal defines a set of engine flags, at 
a later point execbuf could be extended with a new flag. For example:

eb.flags = I915_EXEC_CLASS | I915_ENGINE_CLASS_VIDEO | 
I915_EXEC_ENGINE_FEATURES | I915_ENGINE_HAS_HEVC;

Only problem I see that engine flags in the current proposal are u64 so 
it would be a bit challenging to fit that in eb.flags.

Not sure if it would make sense to split the engine info flags into a 
smaller and larger set. u8 which would be flags "compatible" with 
I915_EXEC_ENGINE_FEATURES, and the remainder which would be something 
else, or unused/reserved? Like:

struct drm_i915_engine_info {
	/** Engine instance number. */
	__u32	instance;
	__u32	rsvd;

	/** Engine specific features and info. */
#define DRM_I915_ENGINE_HAS_HEVC	BIT(0)
	__u8 features;
	__u8 rsvd;

	__32 info;
};

Or at that point you bring on eb3.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 17/05/2017 17:44, Chris Wilson wrote:
> On Wed, May 17, 2017 at 04:40:57PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Building on top of the previous patch which exported the concept
>> of engine classes and instances, we can also use this instead of
>> the current awkward engine selection uAPI.
>>
>> This is primarily interesting for the VCS engine selection which
>> is a) currently done via disjoint set of flags, and b) the
>> current I915_EXEC_BSD flags has different semantics depending on
>> the underlying hardware which is bad.
>>
>> Proposed idea here is to reserve 16-bits of flags, to pass in
>> the engine class and instance (8 bits each), and a new flag
>> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
>> selection API is in use.
>>
>> The new uAPI also removes access to the weak VCS engine
>> balancing as currently existing in the driver.
>>
>> Example usage to send a command to VCS0:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
>>
>> Or to send a command to VCS1:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
>>
>> v2:
>>  * Fix unknown flags mask.
>>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
>>
>> v3:
>>  * Add a map for fast class-instance engine lookup. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ben Widawsky <ben@bwidawsk.net>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Daniel Charles <daniel.charles@intel.com>
>> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
>> Cc: intel-vaapi-media@lists.01.org
>> Cc: mesa-dev@lists.freedesktop.org
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 30 ++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_reg.h            |  3 +++
>>  drivers/gpu/drm/i915/intel_engine_cs.c     |  7 +++++++
>>  include/uapi/drm/i915_drm.h                | 11 ++++++++++-
>>  5 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 5dfa4a12e647..7bf4fd42480c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2066,6 +2066,7 @@ struct drm_i915_private {
>>  	struct pci_dev *bridge_dev;
>>  	struct i915_gem_context *kernel_context;
>>  	struct intel_engine_cs *engine[I915_NUM_ENGINES];
>> +	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
>>  	struct i915_vma *semaphore;
>>
>>  	struct drm_dma_handle *status_page_dmah;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index af1965774e7b..c1ad49ab64cd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1492,6 +1492,33 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>>  	return file_priv->bsd_engine;
>>  }
>>
>> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
>> +
>> +static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
>> +						u8 class, u8 instance)
>> +{
>> +	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
>> +		return NULL;
>> +
>> +	return i915->engine_class[class][instance];
>> +}
>
> Be bold make this this an intel_engine_lookup(), I forsee some other
> users appearing very shortly.

Still static or you want to export it straight away? Preference for 
where to put it? Here or intel_engine_cs.c?

>> +static struct intel_engine_cs *
>> +eb_select_engine_class_instance(struct drm_i915_private *i915,
>> +				struct drm_i915_gem_execbuffer2 *args)
>> +{
>> +	u8 class, instance;
>> +
>> +	class = args->flags & I915_EXEC_RING_MASK;
>> +	if (class >= DRM_I915_ENGINE_CLASS_MAX)
>> +		return NULL;
>> +
>> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
>> +		   I915_EXEC_INSTANCE_MASK;
>> +
>> +	return get_engine_class(i915, user_class_map[class], instance);
>> +}
>> +
>>  #define I915_USER_RINGS (4)
>>
>>  static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
>> @@ -1510,6 +1537,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
>>  	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
>>  	struct intel_engine_cs *engine;
>>
>> +	if (args->flags & I915_EXEC_CLASS_INSTANCE)
>> +		return eb_select_engine_class_instance(dev_priv, args);
>> +
>>  	if (user_ring_id > I915_USER_RINGS) {
>>  		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
>>  		return NULL;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index ee144ec57935..a3b59043b991 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>>  #define VIDEO_ENHANCEMENT_CLASS	2
>>  #define COPY_ENGINE_CLASS	3
>>  #define OTHER_CLASS		4
>> +#define MAX_ENGINE_CLASS	4
>> +
>> +#define MAX_ENGINE_INSTANCE	1
>
> We also need the names in the uapi.h as well. Do we want to mention
> OTHER_CLASS before it is defined? I trust your crystal ball.

I have mentioned it just by the virtue of it existing in i915_reg.h 
Crystal ball is otherwise quiet.

> Amusingly I notice that you do claim that you have these names in the
> changelog. :) We don't need DRM_I915 all the time. I915_ENGINE_CLASS is
> going to be unique, at least for those users of i915_drm.h

They are all there courtesy of the previous patch.

I will drop the DRM_ prefix throughout.

>
>>  /* PCI config space */
>>
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 7566cf48012f..c5ad51c43d23 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>>
>>  	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
>>
>> +	if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
>> +		    info->instance > MAX_ENGINE_INSTANCE ||
>> +		    dev_priv->engine_class[info->class][info->instance]))
>
> Spread these out or add a message telling what was wrong.

Can do, but I considered these to be such a basic programming errors 
which should be caught during development. Only thing which prevented me 
from putting in under the GEM_BUG_ON is the fact someone could not have 
it enabled and that this function can already handle failures.

>> +		return -EINVAL;
>> +
>> +	dev_priv->engine_class[info->class][info->instance] = engine;
>>  	dev_priv->engine[id] = engine;
>> +
>>  	return 0;
>>  }
>>
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 2ac6667e57ea..6a26bdf5e684 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
>>   */
>>  #define I915_EXEC_FENCE_OUT		(1<<17)
>>
>> -#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
>> +#define I915_EXEC_CLASS_INSTANCE	(1<<18)
>> +
>> +#define I915_EXEC_INSTANCE_SHIFT	(19)
>> +#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
>> +
>> +#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
>>
>>  #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
>>  #define i915_execbuffer2_set_context_id(eb2, context) \
>> @@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
>>  #define i915_execbuffer2_get_context_id(eb2) \
>>  	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
>>
>> +#define i915_execbuffer2_engine(class, instance) \
>> +	(I915_EXEC_CLASS_INSTANCE | (class) | \
>> +	((instance) << I915_EXEC_INSTANCE_SHIFT))
>
> (class |
>  (instance) << I915_EXEC_INSTANCE_SHIFT |
>  I915_EXEC_CLASS_INSTANCE)
>
> Just suggesting to spread it out over another line for better
> readability.

Okay but I'd like for the flag to come first, followed by class and then 
instance. Okay with that?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, May 18, 2017 at 02:06:35PM +0100, Tvrtko Ursulin wrote:
> 
> On 18/05/2017 13:24, Chris Wilson wrote:
> >Yes, I would argue to defer it until later. One problem I have at the
> >moment is that not all members of a class are equal, HEVC-capable
> >engines and the reset do not belong to the same class (i.e. my hope is
> >that we could just say <class> | [ <mask> | INSTANCE_MASK ] | LOAD_BALANCE)
> >So I can see the sense in having instance as a mask, or at least making
> >the current instance field large enough to store a mask in future. I
> >just feel uneasy as that field could grow quite large, and maybe it will
> >be better to set the constraint via a context param (all dependency on
> >frequency and tuning of the LOAD_BALANCE). Hmm, liking having the
> >instance-mask on the context atm.
> 
> I don't think per context mask would work unless you won't to
> mandate multi-contexts where they wouldn't otherwise be needed.

Contexts are not thread-friendly. About the only way you can make them
safe (wrt execbuf) is through the use of userspace GTT allocation (i.e.
assigning an address on creation and making it permanent with softpin).

So in general you end up serialising around execbuf and copying the
state in/out of the ioctl. That gives a window of opportunity to use
context_setparam as an extension for irregular parameter updates.

It is not as nice as providing space in the execbuf ioctl, because of
the extra state being carried around in the context.
 
> But this problem in general can also be solved separately from
> class-instance addressing via engine feature masking.

But imo all members of a class should have the same features. That would
be my definition of a class!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, May 18, 2017 at 02:30:56PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/05/2017 17:44, Chris Wilson wrote:
> >On Wed, May 17, 2017 at 04:40:57PM +0100, Tvrtko Ursulin wrote:
> >>+static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
> >>+						u8 class, u8 instance)
> >>+{
> >>+	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
> >>+		return NULL;
> >>+
> >>+	return i915->engine_class[class][instance];
> >>+}
> >
> >Be bold make this this an intel_engine_lookup(), I forsee some other
> >users appearing very shortly.
> 
> Still static or you want to export it straight away? Preference for
> where to put it? Here or intel_engine_cs.c?

Let's go with intel_engine_cs.c. We know we're going to move it
otherwise.

> >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>index ee144ec57935..a3b59043b991 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >> #define VIDEO_ENHANCEMENT_CLASS	2
> >> #define COPY_ENGINE_CLASS	3
> >> #define OTHER_CLASS		4
> >>+#define MAX_ENGINE_CLASS	4
> >>+
> >>+#define MAX_ENGINE_INSTANCE	1
> >
> >We also need the names in the uapi.h as well. Do we want to mention
> >OTHER_CLASS before it is defined? I trust your crystal ball.
> 
> I have mentioned it just by the virtue of it existing in i915_reg.h
> Crystal ball is otherwise quiet.
> 
> >Amusingly I notice that you do claim that you have these names in the
> >changelog. :) We don't need DRM_I915 all the time. I915_ENGINE_CLASS is
> >going to be unique, at least for those users of i915_drm.h
> 
> They are all there courtesy of the previous patch.

Hey, definitely only one v3 patch in my inbox and no 2/2 to clue me in
:)

> I will drop the DRM_ prefix throughout.
> 
> >
> >> /* PCI config space */
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>index 7566cf48012f..c5ad51c43d23 100644
> >>--- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>@@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> >>
> >> 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
> >>
> >>+	if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
> >>+		    info->instance > MAX_ENGINE_INSTANCE ||
> >>+		    dev_priv->engine_class[info->class][info->instance]))
> >
> >Spread these out or add a message telling what was wrong.
> 
> Can do, but I considered these to be such a basic programming errors
> which should be caught during development. Only thing which
> prevented me from putting in under the GEM_BUG_ON is the fact
> someone could not have it enabled and that this function can already
> handle failures.

Spread them out and use GEM_WARN_ON?

> 
> >>+		return -EINVAL;
> >>+
> >>+	dev_priv->engine_class[info->class][info->instance] = engine;
> >> 	dev_priv->engine[id] = engine;
> >>+
> >> 	return 0;
> >> }
> >>
> >>diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >>index 2ac6667e57ea..6a26bdf5e684 100644
> >>--- a/include/uapi/drm/i915_drm.h
> >>+++ b/include/uapi/drm/i915_drm.h
> >>@@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
> >>  */
> >> #define I915_EXEC_FENCE_OUT		(1<<17)
> >>
> >>-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
> >>+#define I915_EXEC_CLASS_INSTANCE	(1<<18)
> >>+
> >>+#define I915_EXEC_INSTANCE_SHIFT	(19)
> >>+#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
> >>+
> >>+#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
> >>
> >> #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
> >> #define i915_execbuffer2_set_context_id(eb2, context) \
> >>@@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
> >> #define i915_execbuffer2_get_context_id(eb2) \
> >> 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
> >>
> >>+#define i915_execbuffer2_engine(class, instance) \
> >>+	(I915_EXEC_CLASS_INSTANCE | (class) | \
> >>+	((instance) << I915_EXEC_INSTANCE_SHIFT))
> >
> >(class |
> > (instance) << I915_EXEC_INSTANCE_SHIFT |
> > I915_EXEC_CLASS_INSTANCE)
> >
> >Just suggesting to spread it out over another line for better
> >readability.
> 
> Okay but I'd like for the flag to come first, followed by class and
> then instance. Okay with that?

Ok. I don't think we can stop it forming a triangle however we reorder
them.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Intel-gfx] [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Emil Velikov]

Hi Tvrtko,

On 18 May 2017 at 14:06, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

> struct drm_i915_engine_info {
>         /** Engine instance number. */
>         __u32   instance;
>         __u32   rsvd;
>
>         /** Engine specific features and info. */
> #define DRM_I915_ENGINE_HAS_HEVC        BIT(0)
>         __u8 features;
>         __u8 rsvd;
>
>         __32 info;
> };
>
To spare yourself from writing a compat ioctl, you want to have the
members at the same offset on both 32 and 64bit builds.
At the same times the whole struct size should be multiple of 64bit.

This and a few others are covered in Daniel Vetter's "Botching up ioctls" [1]

-Emil

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 18/05/2017 15:10, Emil Velikov wrote:
> Hi Tvrtko,
>
> On 18 May 2017 at 14:06, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>
>> struct drm_i915_engine_info {
>>         /** Engine instance number. */
>>         __u32   instance;
>>         __u32   rsvd;
>>
>>         /** Engine specific features and info. */
>> #define DRM_I915_ENGINE_HAS_HEVC        BIT(0)
>>         __u8 features;
>>         __u8 rsvd;
>>
>>         __32 info;
>> };
>>
> To spare yourself from writing a compat ioctl, you want to have the
> members at the same offset on both 32 and 64bit builds.
> At the same times the whole struct size should be multiple of 64bit.
>
> This and a few others are covered in Daniel Vetter's "Botching up ioctls" [1]

Yes, thanks, I failed to add up to 64. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v3 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine discovery uAPI allows userspace to probe for engine
configuration and features without needing to maintain the
internal PCI id based database.

This enables removal of code duplications across userspace
components.

Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
which returns the number and information on the specified engine
class.

Currently only general engine configuration and HEVC feature of
the VCS engine can be probed but the uAPI is designed to be
generic and extensible.

Code is based almost exactly on the earlier proposal on the
topic by Jon Bloomfield. Engine class and instance refactoring
made recently by Daniele Ceraolo Spurio enabled this to be
implemented in an elegant fashion.

To probe configuration userspace sets the engine class it wants
to query (struct drm_i915_gem_engine_info) and provides an array
of drm_i915_engine_info structs which will be filled in by the
driver. Userspace also has to tell i915 how many elements are in
the array, and the driver will report back the total number of
engine instances in any case.

v2:
 * Add a version field and consolidate to one engine count.
   (Chris Wilson)
 * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
   (Gong Zhipeng)

v3:
 * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
 * Only reserve 8-bits for the engine info for time being.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_drv.c        |  1 +
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
 4 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 72fb47a439d2..dc268f41a609 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2623,6 +2623,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 17883a84b8c1..1e08b82c4823 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3521,6 +3521,9 @@ i915_gem_context_lookup_timeline(struct i915_gem_context *ctx,
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
 
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  u64 min_size, u64 alignment,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 413bfd8d4bf4..8d370c5d07bc 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,6 +25,7 @@
 #include "i915_drv.h"
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
+#include <uapi/drm/i915_drm.h>
 
 /* Haswell does have the CXT_SIZE register however it does not appear to be
  * valid. Now, docs explain in dwords what is in the context object. The full
@@ -1286,6 +1287,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
 	}
 }
 
+u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
+	[I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
+	[I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
+	[I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
+	[I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
+	[I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
+};
+
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_engine_info *args = data;
+	struct drm_i915_engine_info __user *user_info =
+					u64_to_user_ptr(args->info_ptr);
+	unsigned int info_size = args->num_engines;
+	struct drm_i915_engine_info info;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u8 class;
+
+	if (args->rsvd)
+		return -EINVAL;
+
+	switch (args->engine_class) {
+	case I915_ENGINE_CLASS_OTHER:
+	case I915_ENGINE_CLASS_RENDER:
+	case I915_ENGINE_CLASS_COPY:
+	case I915_ENGINE_CLASS_VIDEO:
+	case I915_ENGINE_CLASS_VIDEO_ENHANCE:
+		class = user_class_map[args->engine_class];
+		break;
+	case I915_ENGINE_CLASS_MAX:
+	default:
+		return -EINVAL;
+	};
+
+	args->num_engines = 0;
+
+	if (args->version != 1) {
+		args->version = 1;
+		return 0;
+	}
+
+	for_each_engine(engine, i915, id) {
+		if (class != engine->class)
+			continue;
+
+		if (++args->num_engines > info_size)
+			continue;
+
+		memset(&info, 0, sizeof(info));
+		info.instance = engine->instance;
+		if (INTEL_GEN(i915) >= 8 && id == VCS)
+			info.info = I915_ENGINE_HAS_HEVC;
+
+		if (copy_to_user(user_info++, &info, sizeof(info)))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f24a80d2d42e..8f0926edc44f 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -260,6 +260,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
 #define DRM_I915_PERF_OPEN		0x36
+#define DRM_I915_GEM_ENGINE_INFO	0x37
 
 #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)
@@ -315,6 +316,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
+#define DRM_IOCTL_I915_GEM_ENGINE_INFO	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_ENGINE_INFO, struct drm_i915_gem_engine_info)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1439,6 +1441,45 @@ enum drm_i915_perf_record_type {
 	DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };
 
+enum drm_i915_gem_engine_class {
+	I915_ENGINE_CLASS_OTHER = 0,
+	I915_ENGINE_CLASS_RENDER = 1,
+	I915_ENGINE_CLASS_COPY = 2,
+	I915_ENGINE_CLASS_VIDEO = 3,
+	I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
+	I915_ENGINE_CLASS_MAX /* non-ABI */
+};
+
+struct drm_i915_engine_info {
+	/** Engine instance number. */
+	__u8	instance;
+
+	/** Engine specific info. */
+#define I915_ENGINE_HAS_HEVC	BIT(0)
+	__u8	info;
+
+	__u8	rsvd2[6];
+};
+
+struct drm_i915_gem_engine_info {
+	/** in/out: Protocol version requested/supported. */
+	__u32 version;
+
+	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
+	__u32 engine_class;
+
+	/**
+	 * in/out: Number of struct drm_i915_engine_info entries in the provided
+	 * @info_ptr array and actual number of supported hardware engines.
+	 */
+	__u32 num_engines;
+	__u32 rsvd;
+
+	/** in/out: Pointer to array of struct i915_engine_info elements. */
+	__u64 info_ptr;
+
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v4 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Building on top of the previous patch which exported the concept
of engine classes and instances, we can also use this instead of
the current awkward engine selection uAPI.

This is primarily interesting for the VCS engine selection which
is a) currently done via disjoint set of flags, and b) the
current I915_EXEC_BSD flags has different semantics depending on
the underlying hardware which is bad.

Proposed idea here is to reserve 8-bits of flags, to pass in the
engine instance, re-use the existing engine selection bits for
the class selection, and a new flag named
I915_EXEC_CLASS_INSTANCE to tell the kernel this new engine
selection API is in use.

The new uAPI also removes access to the weak VCS engine
balancing as currently existing in the driver.

Example usage to send a command to VCS0:

  eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 0);

Or to send a command to VCS1:

  eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 1);

v2:
 * Fix unknown flags mask.
 * Use I915_EXEC_RING_MASK for class. (Chris Wilson)

v3:
 * Add a map for fast class-instance engine lookup. (Chris Wilson)

v4:
 * Update commit to reflect v3.
 * Export intel_engine_lookup for other users. (Chris Wilson)
 * Split out some warns. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Daniel Charles <daniel.charles@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: intel-vaapi-media@lists.01.org
Cc: mesa-dev@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  3 +++
 drivers/gpu/drm/i915/intel_engine_cs.c     | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  3 +++
 include/uapi/drm/i915_drm.h                | 12 +++++++++++-
 6 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e08b82c4823..53b41963f672 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2115,6 +2115,7 @@ struct drm_i915_private {
 	struct pci_dev *bridge_dev;
 	struct i915_gem_context *kernel_context;
 	struct intel_engine_cs *engine[I915_NUM_ENGINES];
+	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
 	struct i915_vma *semaphore;
 
 	struct drm_dma_handle *status_page_dmah;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index af1965774e7b..006c8046af5f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1492,6 +1492,23 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
 	return file_priv->bsd_engine;
 }
 
+static struct intel_engine_cs *
+eb_select_engine_class_instance(struct drm_i915_private *i915,
+				struct drm_i915_gem_execbuffer2 *args)
+{
+	u8 class = args->flags & I915_EXEC_RING_MASK;
+	extern u8 user_class_map[I915_ENGINE_CLASS_MAX];
+	u8 instance;
+
+	if (class >= ARRAY_SIZE(user_class_map))
+		return NULL;
+
+	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
+		   I915_EXEC_INSTANCE_MASK;
+
+	return intel_engine_lookup(i915, user_class_map[class], instance);
+}
+
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
@@ -1510,6 +1527,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
 	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
 
+	if (args->flags & I915_EXEC_CLASS_INSTANCE)
+		return eb_select_engine_class_instance(dev_priv, args);
+
 	if (user_ring_id > I915_USER_RINGS) {
 		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
 		return NULL;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 89888adb9af1..fa4a3841c4af 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define VIDEO_ENHANCEMENT_CLASS	2
 #define COPY_ENGINE_CLASS	3
 #define OTHER_CLASS		4
+#define MAX_ENGINE_CLASS	4
+
+#define MAX_ENGINE_INSTANCE	1
 
 /* PCI config space */
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 8d370c5d07bc..8f31514b01d5 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -198,6 +198,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	GEM_BUG_ON(info->class >= ARRAY_SIZE(intel_engine_classes));
 	class_info = &intel_engine_classes[info->class];
 
+	if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS))
+		return -EINVAL;
+
+	if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE))
+		return -EINVAL;
+
+	if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance]))
+		return -EINVAL;
+
 	GEM_BUG_ON(dev_priv->engine[id]);
 	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
 	if (!engine)
@@ -225,7 +234,9 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
 
+	dev_priv->engine_class[info->class][info->instance] = engine;
 	dev_priv->engine[id] = engine;
+
 	return 0;
 }
 
@@ -1350,6 +1361,15 @@ int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
+struct intel_engine_cs *
+intel_engine_lookup(struct drm_i915_private *i915, u8 class, u8 instance)
+{
+	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
+		return NULL;
+
+	return i915->engine_class[class][instance];
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 6aa20ac8cde3..692823b33d75 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -734,4 +734,7 @@ bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
 void intel_engines_mark_idle(struct drm_i915_private *i915);
 void intel_engines_reset_default_submission(struct drm_i915_private *i915);
 
+struct intel_engine_cs *
+intel_engine_lookup(struct drm_i915_private *i915, u8 class, u8 instance);
+
 #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 8f0926edc44f..a48ef426f60d 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_FENCE_OUT		(1<<17)
 
-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
+#define I915_EXEC_CLASS_INSTANCE	(1<<18)
+
+#define I915_EXEC_INSTANCE_SHIFT	(19)
+#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
+
+#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
@@ -914,6 +919,11 @@ struct drm_i915_gem_execbuffer2 {
 #define i915_execbuffer2_get_context_id(eb2) \
 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
 
+#define i915_execbuffer2_engine(class, instance) \
+	(I915_EXEC_CLASS_INSTANCE | \
+	(class) | \
+	((instance) << I915_EXEC_INSTANCE_SHIFT))
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v4 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, May 18, 2017 at 03:58:26PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
> 
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
> 
> Proposed idea here is to reserve 8-bits of flags, to pass in the
> engine instance, re-use the existing engine selection bits for
> the class selection, and a new flag named
> I915_EXEC_CLASS_INSTANCE to tell the kernel this new engine
> selection API is in use.
> 
> The new uAPI also removes access to the weak VCS engine
> balancing as currently existing in the driver.
> 
> Example usage to send a command to VCS0:
> 
>   eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> 
> Or to send a command to VCS1:
> 
>   eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 1);
> 
> v2:
>  * Fix unknown flags mask.
>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
> 
> v3:
>  * Add a map for fast class-instance engine lookup. (Chris Wilson)
> 
> v4:
>  * Update commit to reflect v3.
>  * Export intel_engine_lookup for other users. (Chris Wilson)
>  * Split out some warns. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Daniel Charles <daniel.charles@intel.com>
> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> Cc: intel-vaapi-media@lists.01.org
> Cc: mesa-dev@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  3 +++
>  drivers/gpu/drm/i915/intel_engine_cs.c     | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  3 +++
>  include/uapi/drm/i915_drm.h                | 12 +++++++++++-
>  6 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e08b82c4823..53b41963f672 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2115,6 +2115,7 @@ struct drm_i915_private {
>  	struct pci_dev *bridge_dev;
>  	struct i915_gem_context *kernel_context;
>  	struct intel_engine_cs *engine[I915_NUM_ENGINES];
> +	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
>  	struct i915_vma *semaphore;
>  
>  	struct drm_dma_handle *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index af1965774e7b..006c8046af5f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1492,6 +1492,23 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>  	return file_priv->bsd_engine;
>  }
>  
> +static struct intel_engine_cs *
> +eb_select_engine_class_instance(struct drm_i915_private *i915,
> +				struct drm_i915_gem_execbuffer2 *args)
> +{
> +	u8 class = args->flags & I915_EXEC_RING_MASK;
> +	extern u8 user_class_map[I915_ENGINE_CLASS_MAX];
> +	u8 instance;
> +
> +	if (class >= ARRAY_SIZE(user_class_map))
> +		return NULL;
> +
> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> +		   I915_EXEC_INSTANCE_MASK;
> +
> +	return intel_engine_lookup(i915, user_class_map[class], instance);

The class mapping is going to be the same for all users of
intel_engine_lookup() presumably? (At least I hope we plan on using the
same id everywhere!)

intel_engine_lookup_from_user(), just to reinforce where those ids are
coming from?

So other than the conversation of what class means, looks good to me.

To give some context for other users, one example is a context param for
setting per-engine settings (e.g. sseu, watchdog thresholds):

struct drm_i915_gem_context_param_sseu {
        __u32 engine;
        __u32 instance;

        __u32 slice_mask;
        __u32 subslice_mask;
        __u32 min_eu_per_subslice;
        __u32 max_eu_per_subslice;
};

static int
i915_gem_context_setparam__sseu(struct i915_gem_context *ctx,
                                const struct drm_i915_gem_context_param *args)
{
        const struct drm_i915_gem_context_param_sseu *user =
                u64_to_user_ptr(args->value);
        const unsigned int size = args->size;
        unsigned int total;
 
        total = 0;
        while (total + sizeof(*user) <= size) {
                struct drm_i915_gem_context_param_sseu sseu;
                int ret;
                
                if (copy_from_user(&sseu, user++, sizeof(sseu)))
                        return -EFAULT;
 
                ret = intel_lr_context_set_sseu(ctx, &sseu);
                if (ret)
                        return ret;
 
                total += sizeof(sseu);
        }
 
        return 0;
}


int intel_lr_context_set_sseu(struct i915_gem_context *ctx,
                              const struct drm_i915_gem_context_param_sseu *user)
{
        struct drm_i915_private *i915 = ctx->i915;
        struct intel_engine_cs *engine;
        struct intel_context *ce;
        struct sseu_dev_info sseu;
        int ret;
 
        lockdep_assert_held(&i915->drm.struct_mutex);
 
        engine = intel_engine_lookup(i915, user->engine, user->instance);
        if (IS_ERR(engine))
                return ERR_PTR(engine);

	...

My goal is that we only have to use one set of class/instance defines!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 18/05/2017 14:37, Chris Wilson wrote:
> On Thu, May 18, 2017 at 02:06:35PM +0100, Tvrtko Ursulin wrote:
>>
>> On 18/05/2017 13:24, Chris Wilson wrote:
>>> Yes, I would argue to defer it until later. One problem I have at the
>>> moment is that not all members of a class are equal, HEVC-capable
>>> engines and the reset do not belong to the same class (i.e. my hope is
>>> that we could just say <class> | [ <mask> | INSTANCE_MASK ] | LOAD_BALANCE)
>>> So I can see the sense in having instance as a mask, or at least making
>>> the current instance field large enough to store a mask in future. I
>>> just feel uneasy as that field could grow quite large, and maybe it will
>>> be better to set the constraint via a context param (all dependency on
>>> frequency and tuning of the LOAD_BALANCE). Hmm, liking having the
>>> instance-mask on the context atm.
>>
>> I don't think per context mask would work unless you won't to
>> mandate multi-contexts where they wouldn't otherwise be needed.
>
> Contexts are not thread-friendly. About the only way you can make them
> safe (wrt execbuf) is through the use of userspace GTT allocation (i.e.
> assigning an address on creation and making it permanent with softpin).
>
> So in general you end up serialising around execbuf and copying the
> state in/out of the ioctl. That gives a window of opportunity to use
> context_setparam as an extension for irregular parameter updates.
>
> It is not as nice as providing space in the execbuf ioctl, because of
> the extra state being carried around in the context.

Yes not nice, I can't say that I like this very much.

>> But this problem in general can also be solved separately from
>> class-instance addressing via engine feature masking.
>
> But imo all members of a class should have the same features. That would
> be my definition of a class!

That sounds very totalitarian! :)) To me a class is a group of some 
entities which share some common characteristics - not necessarily 
completely uniform.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, May 18, 2017 at 05:20:38PM +0100, Tvrtko Ursulin wrote:
> 
> On 18/05/2017 14:37, Chris Wilson wrote:
> >On Thu, May 18, 2017 at 02:06:35PM +0100, Tvrtko Ursulin wrote:
> >>
> >>But this problem in general can also be solved separately from
> >>class-instance addressing via engine feature masking.
> >
> >But imo all members of a class should have the same features. That would
> >be my definition of a class!
> 
> That sounds very totalitarian! :)) To me a class is a group of some
> entities which share some common characteristics - not necessarily
> completely uniform.

The problem otherwise is that we then have to define yet another
interface based on features. To me that sounds like too much
duplication, that we could avoid from the beginning. Curse the hw for
being asymmetical!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

✓ Fi.CI.BAT: success for New engine discovery and execbuffer2 engine selection uAPI (rev6)

[Patchwork]

== Series Details ==

Series: New engine discovery and execbuffer2 engine selection uAPI (rev6)
URL   : https://patchwork.freedesktop.org/series/23189/
State : success

== Summary ==

Series 23189v6 New engine discovery and execbuffer2 engine selection uAPI
https://patchwork.freedesktop.org/api/1.0/series/23189/revisions/6/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:445s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:435s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:596s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:500s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:485s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:416s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:411s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:423s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:496s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:461s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:463s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:466s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:583s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:505s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:440s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:534s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:404s

ab08cb2750e769d074b2f147c8298ccd0cd08340 drm-tip: 2017y-05m-18d-15h-36m-17s UTC integration manifest
d5f300b drm/i915: Select engines via class and instance in execbuffer2
128602d drm/i915: Engine discovery uAPI

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4743/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 18/05/2017 18:00, Chris Wilson wrote:
> On Thu, May 18, 2017 at 05:20:38PM +0100, Tvrtko Ursulin wrote:
>>
>> On 18/05/2017 14:37, Chris Wilson wrote:
>>> On Thu, May 18, 2017 at 02:06:35PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> But this problem in general can also be solved separately from
>>>> class-instance addressing via engine feature masking.
>>>
>>> But imo all members of a class should have the same features. That would
>>> be my definition of a class!
>>
>> That sounds very totalitarian! :)) To me a class is a group of some
>> entities which share some common characteristics - not necessarily
>> completely uniform.
>
> The problem otherwise is that we then have to define yet another
> interface based on features. To me that sounds like too much
> duplication, that we could avoid from the beginning. Curse the hw for
> being asymmetical!

Hm I don't see a problem with the feature base engine selection on top. 
You still do because of the desire classes were equal in features?

To sum up what I (and we) talked about in various parts of the thread(s):

Step 1a: New execbuf engine selection uAPI.

  - execbuf class=VCS instance=1

Step 1b: Engine discovery uAPI.

Same as above but userpace can figure out how many VCS engines there
are without PCI probing.

I didn't get much feedback on this one. :(

Step 2: Feature masks for execbuf.

  - execbuf class=VCS instance=0 features=HEVC = OK
  - execbuf class=VCS instance=1 features=HEVC = FAIL

But userspace can use engine discovery to figure out which are the valid 
combinations.

This could be a simpler, but less featureful and not very elegant 
alternative to step 2.

Otherwise just a prep step for the subsequent steps below.

Step 3a: (One day maybe) userspace selects a class, i915 picks the engine

  - execbuf class=VCS instance=any

Step 3b: userspace selected class and features

  - execbuf class=VCS instance=any features=HEVC

This RFC proposed steps 1a and 1b. The rest we leave for later.

How does that sound? Acceptable?

In case of engine discovery useful enough or what other features could 
we put it in to make it more useful for userspace? Potentially enable 
dropping PCI id probing altogether and enable libva/mesa/??? to probe 
everything using i915 ioctls.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Wed, May 24, 2017 at 12:28:07PM +0100, Tvrtko Ursulin wrote:
> 
> On 18/05/2017 18:00, Chris Wilson wrote:
> >On Thu, May 18, 2017 at 05:20:38PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 18/05/2017 14:37, Chris Wilson wrote:
> >>>On Thu, May 18, 2017 at 02:06:35PM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>But this problem in general can also be solved separately from
> >>>>class-instance addressing via engine feature masking.
> >>>
> >>>But imo all members of a class should have the same features. That would
> >>>be my definition of a class!
> >>
> >>That sounds very totalitarian! :)) To me a class is a group of some
> >>entities which share some common characteristics - not necessarily
> >>completely uniform.
> >
> >The problem otherwise is that we then have to define yet another
> >interface based on features. To me that sounds like too much
> >duplication, that we could avoid from the beginning. Curse the hw for
> >being asymmetical!
> 
> Hm I don't see a problem with the feature base engine selection on
> top. You still do because of the desire classes were equal in
> features?

Ok, having another think about it, I agree that the features define a
subclass. Basically it comes down to VCS | HEVC being a subset of the
VCS group and we need a way to express any of VCS | HEVC and any of VCS
cleanly.

> To sum up what I (and we) talked about in various parts of the thread(s):
> 
> Step 1a: New execbuf engine selection uAPI.
> 
>  - execbuf class=VCS instance=1
> 
> Step 1b: Engine discovery uAPI.
> 
> Same as above but userpace can figure out how many VCS engines there
> are without PCI probing.
> 
> I didn't get much feedback on this one. :(

Mainly because we didn't emphasis that this would be the only way to
discover the parameters for execbuf / svm and it lost in the idea that
this would replace their device_info tables. We are suggesting that we
can phase out those tables (but we should put some effort into making
sure we discard unwanted ones after init) since we will have to provide
the information anyway.
 
> Step 2: Feature masks for execbuf.
> 
>  - execbuf class=VCS instance=0 features=HEVC = OK
>  - execbuf class=VCS instance=1 features=HEVC = FAIL
> 
> But userspace can use engine discovery to figure out which are the
> valid combinations.
> 
> This could be a simpler, but less featureful and not very elegant
> alternative to step 2.
> 
> Otherwise just a prep step for the subsequent steps below.
> 
> Step 3a: (One day maybe) userspace selects a class, i915 picks the engine
> 
>  - execbuf class=VCS instance=any
> 
> Step 3b: userspace selected class and features
> 
>  - execbuf class=VCS instance=any features=HEVC
> 
> This RFC proposed steps 1a and 1b. The rest we leave for later.
> 
> How does that sound? Acceptable?

I want "1c: extensible interface for per-engine settings elsewhere in the
uAPI" as well.

We also need an answer to the i915_gem_busy_ioctl conundrum - if/when we
should deprecated the engine id being exposed. It's the same question
more or less as for how long (or whether we should) continue to support
I915_EXEC_RING. Plus how to expose these via tracepoints.

The quick answer is while we have less than 16 engines, we don't have to
worry about the uABI breakage.
 
> In case of engine discovery useful enough or what other features
> could we put it in to make it more useful for userspace? Potentially
> enable dropping PCI id probing altogether and enable libva/mesa/???
> to probe everything using i915 ioctls.

To be convenient we have to beat the ease of device_info[PCIID], and
being futureproof. The resistance will be because then they are
dependent on the kernel for features that are not dependent on that
kernel for execution, i.e. being able to use new userspace on old
kernels and remain feature complete. To counter that we need to
have a complete description of a new device the moment we enable it.
That makes it a hard sell. The benefit is basically only a reduction in
RO library mmappings, a very small improvement of start up speed at
best.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 25/05/2017 15:14, Chris Wilson wrote:
> On Wed, May 24, 2017 at 12:28:07PM +0100, Tvrtko Ursulin wrote:
>>
>> On 18/05/2017 18:00, Chris Wilson wrote:
>>> On Thu, May 18, 2017 at 05:20:38PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 18/05/2017 14:37, Chris Wilson wrote:
>>>>> On Thu, May 18, 2017 at 02:06:35PM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> But this problem in general can also be solved separately from
>>>>>> class-instance addressing via engine feature masking.
>>>>>
>>>>> But imo all members of a class should have the same features. That would
>>>>> be my definition of a class!
>>>>
>>>> That sounds very totalitarian! :)) To me a class is a group of some
>>>> entities which share some common characteristics - not necessarily
>>>> completely uniform.
>>>
>>> The problem otherwise is that we then have to define yet another
>>> interface based on features. To me that sounds like too much
>>> duplication, that we could avoid from the beginning. Curse the hw for
>>> being asymmetical!
>>
>> Hm I don't see a problem with the feature base engine selection on
>> top. You still do because of the desire classes were equal in
>> features?
> 
> Ok, having another think about it, I agree that the features define a
> subclass. Basically it comes down to VCS | HEVC being a subset of the
> VCS group and we need a way to express any of VCS | HEVC and any of VCS
> cleanly.

Cool, I'll respin with engine features on the top to see how that looks. 
And I am also thinking about dropping the GT discovery bits. More on 
that below.

>> To sum up what I (and we) talked about in various parts of the thread(s):
>>
>> Step 1a: New execbuf engine selection uAPI.
>>
>>   - execbuf class=VCS instance=1
>>
>> Step 1b: Engine discovery uAPI.
>>
>> Same as above but userpace can figure out how many VCS engines there
>> are without PCI probing.
>>
>> I didn't get much feedback on this one. :(
> 
> Mainly because we didn't emphasis that this would be the only way to
> discover the parameters for execbuf / svm and it lost in the idea that
> this would replace their device_info tables. We are suggesting that we
> can phase out those tables (but we should put some effort into making
> sure we discard unwanted ones after init) since we will have to provide
> the information anyway.

It has become more unlikely we would get libva (my hopeful first user) 
on board for this in the short/medium term. So as said above, I am 
thinking to drop this for now.

If at some point we go for "step 2" below that would also enable some 
sort of engine enumeration/probing via execbuf null batches.

>> Step 2: Feature masks for execbuf.
>>
>>   - execbuf class=VCS instance=0 features=HEVC = OK
>>   - execbuf class=VCS instance=1 features=HEVC = FAIL
>>
>> But userspace can use engine discovery to figure out which are the
>> valid combinations.
>>
>> This could be a simpler, but less featureful and not very elegant
>> alternative to step 2.
>>
>> Otherwise just a prep step for the subsequent steps below.
>>
>> Step 3a: (One day maybe) userspace selects a class, i915 picks the engine
>>
>>   - execbuf class=VCS instance=any
>>
>> Step 3b: userspace selected class and features
>>
>>   - execbuf class=VCS instance=any features=HEVC
>>
>> This RFC proposed steps 1a and 1b. The rest we leave for later.
>>
>> How does that sound? Acceptable?
> 
> I want "1c: extensible interface for per-engine settings elsewhere in the
> uAPI" as well.

What do you have in mind here, what other uAPI deals with engines?

Regards,

Tvrtko
> We also need an answer to the i915_gem_busy_ioctl conundrum - if/when we
> should deprecated the engine id being exposed. It's the same question
> more or less as for how long (or whether we should) continue to support
> I915_EXEC_RING. Plus how to expose these via tracepoints.
> 
> The quick answer is while we have less than 16 engines, we don't have to
> worry about the uABI breakage.
>   
>> In case of engine discovery useful enough or what other features
>> could we put it in to make it more useful for userspace? Potentially
>> enable dropping PCI id probing altogether and enable libva/mesa/???
>> to probe everything using i915 ioctls.
> 
> To be convenient we have to beat the ease of device_info[PCIID], and
> being futureproof. The resistance will be because then they are
> dependent on the kernel for features that are not dependent on that
> kernel for execution, i.e. being able to use new userspace on old
> kernels and remain feature complete. To counter that we need to
> have a complete description of a new device the moment we enable it.
> That makes it a hard sell. The benefit is basically only a reduction in
> RO library mmappings, a very small improvement of start up speed at
> best.
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v4 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine discovery uAPI allows userspace to probe for engine
configuration and features without needing to maintain the
internal PCI id based database.

This enables removal of code duplications across userspace
components.

Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
which returns the number and information on the specified engine
class.

Currently only general engine configuration and HEVC feature of
the VCS engine can be probed but the uAPI is designed to be
generic and extensible.

Code is based almost exactly on the earlier proposal on the
topic by Jon Bloomfield. Engine class and instance refactoring
made recently by Daniele Ceraolo Spurio enabled this to be
implemented in an elegant fashion.

To probe configuration userspace sets the engine class it wants
to query (struct drm_i915_gem_engine_info) and provides an array
of drm_i915_engine_info structs which will be filled in by the
driver. Userspace also has to tell i915 how many elements are in
the array, and the driver will report back the total number of
engine instances in any case.

v2:
 * Add a version field and consolidate to one engine count.
   (Chris Wilson)
 * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
   (Gong Zhipeng)

v3:
 * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
 * Only reserve 8-bits for the engine info for time being.

v4:
 * Made user_class_map static.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c        |  1 +
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
 4 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e9d8e9ee51b2..44dd2f37937f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 427d10c7eca5..496565e1753f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
 			    struct i915_gem_context *ctx,
 			    uint32_t *reg_state);
 
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  u64 min_size, u64 alignment,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 3b46c1f7b88b..a98669f6ad85 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,6 +25,7 @@
 #include "i915_drv.h"
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
+#include <uapi/drm/i915_drm.h>
 
 /* Haswell does have the CXT_SIZE register however it does not appear to be
  * valid. Now, docs explain in dwords what is in the context object. The full
@@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
 	}
 }
 
+static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
+	[I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
+	[I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
+	[I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
+	[I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
+	[I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
+};
+
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_engine_info *args = data;
+	struct drm_i915_engine_info __user *user_info =
+					u64_to_user_ptr(args->info_ptr);
+	unsigned int info_size = args->num_engines;
+	struct drm_i915_engine_info info;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u8 class;
+
+	if (args->rsvd)
+		return -EINVAL;
+
+	switch (args->engine_class) {
+	case I915_ENGINE_CLASS_OTHER:
+	case I915_ENGINE_CLASS_RENDER:
+	case I915_ENGINE_CLASS_COPY:
+	case I915_ENGINE_CLASS_VIDEO:
+	case I915_ENGINE_CLASS_VIDEO_ENHANCE:
+		class = user_class_map[args->engine_class];
+		break;
+	case I915_ENGINE_CLASS_MAX:
+	default:
+		return -EINVAL;
+	};
+
+	args->num_engines = 0;
+
+	if (args->version != 1) {
+		args->version = 1;
+		return 0;
+	}
+
+	for_each_engine(engine, i915, id) {
+		if (class != engine->class)
+			continue;
+
+		if (++args->num_engines > info_size)
+			continue;
+
+		memset(&info, 0, sizeof(info));
+		info.instance = engine->instance;
+		if (INTEL_GEN(i915) >= 8 && id == VCS)
+			info.info = I915_VCS_HAS_HEVC;
+
+		if (copy_to_user(user_info++, &info, sizeof(info)))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7ccbd6a2bbe0..fbb1f6b99959 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -260,6 +260,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
 #define DRM_I915_PERF_OPEN		0x36
+#define DRM_I915_GEM_ENGINE_INFO	0x37
 
 #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)
@@ -315,6 +316,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
+#define DRM_IOCTL_I915_GEM_ENGINE_INFO	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_ENGINE_INFO, struct drm_i915_gem_engine_info)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1467,6 +1469,45 @@ enum drm_i915_perf_record_type {
 	DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };
 
+enum drm_i915_gem_engine_class {
+	I915_ENGINE_CLASS_OTHER = 0,
+	I915_ENGINE_CLASS_RENDER = 1,
+	I915_ENGINE_CLASS_COPY = 2,
+	I915_ENGINE_CLASS_VIDEO = 3,
+	I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
+	I915_ENGINE_CLASS_MAX /* non-ABI */
+};
+
+struct drm_i915_engine_info {
+	/** Engine instance number. */
+	__u8	instance;
+
+	/** Engine specific info. */
+#define I915_VCS_HAS_HEVC	BIT(0)
+	__u8	info;
+
+	__u8	rsvd2[6];
+};
+
+struct drm_i915_gem_engine_info {
+	/** in/out: Protocol version requested/supported. */
+	__u32 version;
+
+	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
+	__u32 engine_class;
+
+	/**
+	 * in/out: Number of struct drm_i915_engine_info entries in the provided
+	 * @info_ptr array and actual number of supported hardware engines.
+	 */
+	__u32 num_engines;
+	__u32 rsvd;
+
+	/** in/out: Pointer to array of struct i915_engine_info elements. */
+	__u64 info_ptr;
+
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v6 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Building on top of the previous patch which exported the concept
of engine classes and instances, we can also use this instead of
the current awkward engine selection uAPI.

This is primarily interesting for the VCS engine selection which
is a) currently done via disjoint set of flags, and b) the
current I915_EXEC_BSD flags has different semantics depending on
the underlying hardware which is bad.

Proposed idea here is to reserve 8-bits of flags, to pass in the
engine instance, re-use the existing engine selection bits for
the class selection, and a new flag named
I915_EXEC_CLASS_INSTANCE to tell the kernel this new engine
selection API is in use.

The new uAPI also removes access to the weak VCS engine
balancing as currently existing in the driver.

Example usage to send a command to VCS0:

  eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 0);

Or to send a command to VCS1:

  eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 1);

v2:
 * Fix unknown flags mask.
 * Use I915_EXEC_RING_MASK for class. (Chris Wilson)

v3:
 * Add a map for fast class-instance engine lookup. (Chris Wilson)

v4:
 * Update commit to reflect v3.
 * Export intel_engine_lookup for other users. (Chris Wilson)
 * Split out some warns. (Chris Wilson)

v5:
 * Fixed shift and mask logic.
 * Rebased to be standalone.

v6:
 * Rebased back to follow engine info ioctl.
 * Rename helper to intel_engine_lookup_user. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  3 +++
 drivers/gpu/drm/i915/intel_engine_cs.c     | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  3 +++
 include/uapi/drm/i915_drm.h                | 13 ++++++++++++-
 6 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 496565e1753f..4950de8c3de5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2137,6 +2137,7 @@ struct drm_i915_private {
 	struct pci_dev *bridge_dev;
 	struct i915_gem_context *kernel_context;
 	struct intel_engine_cs *engine[I915_NUM_ENGINES];
+	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
 	struct i915_vma *semaphore;
 
 	struct drm_dma_handle *status_page_dmah;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ec33b358fba9..63241840f00a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2050,6 +2050,17 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
 	return file_priv->bsd_engine;
 }
 
+static struct intel_engine_cs *
+eb_select_engine_class_instance(struct drm_i915_private *i915,
+				struct drm_i915_gem_execbuffer2 *args)
+{
+	u8 class = args->flags & I915_EXEC_RING_MASK;
+	u8 instance = (args->flags & I915_EXEC_INSTANCE_MASK) >>
+		      I915_EXEC_INSTANCE_SHIFT;
+
+	return intel_engine_lookup_user(i915, class, instance);
+}
+
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
@@ -2068,6 +2079,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
 	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
 	struct intel_engine_cs *engine;
 
+	if (args->flags & I915_EXEC_CLASS_INSTANCE)
+		return eb_select_engine_class_instance(dev_priv, args);
+
 	if (user_ring_id > I915_USER_RINGS) {
 		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
 		return NULL;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c8647cfa81ba..834673c9ee6b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -95,6 +95,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define VIDEO_ENHANCEMENT_CLASS	2
 #define COPY_ENGINE_CLASS	3
 #define OTHER_CLASS		4
+#define MAX_ENGINE_CLASS	4
+
+#define MAX_ENGINE_INSTANCE	1
 
 /* PCI config space */
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a98669f6ad85..1089b22d2090 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -198,6 +198,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	GEM_BUG_ON(info->class >= ARRAY_SIZE(intel_engine_classes));
 	class_info = &intel_engine_classes[info->class];
 
+	if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS))
+		return -EINVAL;
+
+	if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE))
+		return -EINVAL;
+
+	if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance]))
+		return -EINVAL;
+
 	GEM_BUG_ON(dev_priv->engine[id]);
 	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
 	if (!engine)
@@ -225,7 +234,9 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
 
+	dev_priv->engine_class[info->class][info->instance] = engine;
 	dev_priv->engine[id] = engine;
+
 	return 0;
 }
 
@@ -1396,6 +1407,23 @@ int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
+struct intel_engine_cs *
+intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
+{
+	if (class >= ARRAY_SIZE(user_class_map))
+		return NULL;
+
+	class = user_class_map[class];
+
+	if (WARN_ON_ONCE(class > MAX_ENGINE_CLASS))
+		return NULL;
+
+	if (instance > MAX_ENGINE_INSTANCE)
+		return NULL;
+
+	return i915->engine_class[class][instance];
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index d33c93444c0d..283444f9be10 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -735,4 +735,7 @@ bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
 void intel_engines_mark_idle(struct drm_i915_private *i915);
 void intel_engines_reset_default_submission(struct drm_i915_private *i915);
 
+struct intel_engine_cs *
+intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance);
+
 #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index fbb1f6b99959..c3fabb89e1c2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -929,7 +929,13 @@ struct drm_i915_gem_execbuffer2 {
  * element).
  */
 #define I915_EXEC_BATCH_FIRST		(1<<18)
-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_BATCH_FIRST<<1))
+
+#define I915_EXEC_CLASS_INSTANCE	(1<<19)
+
+#define I915_EXEC_INSTANCE_SHIFT	(20)
+#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
+
+#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
@@ -937,6 +943,11 @@ struct drm_i915_gem_execbuffer2 {
 #define i915_execbuffer2_get_context_id(eb2) \
 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
 
+#define i915_execbuffer2_engine(class, instance) \
+	(I915_EXEC_CLASS_INSTANCE | \
+	(class) | \
+	((instance) << I915_EXEC_INSTANCE_SHIFT))
+
 struct drm_i915_gem_pin {
 	/** Handle of the buffer to be pinned. */
 	__u32 handle;
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for New engine discovery and execbuffer2 engine selection uAPI (rev8)

[Patchwork]

== Series Details ==

Series: New engine discovery and execbuffer2 engine selection uAPI (rev8)
URL   : https://patchwork.freedesktop.org/series/23189/
State : success

== Summary ==

Series 23189v8 New engine discovery and execbuffer2 engine selection uAPI
https://patchwork.freedesktop.org/api/1.0/series/23189/revisions/8/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-r) fdo#100125
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-blb-e6850) fdo#101599
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900) fdo#101516 +1

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101599 https://bugs.freedesktop.org/show_bug.cgi?id=101599
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#101516 https://bugs.freedesktop.org/show_bug.cgi?id=101516

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:450s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:433s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:355s
fi-bsw-n3050     total:279  pass:242  dwarn:1   dfail:0   fail:0   skip:36  time:523s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:512s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:486s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:587s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:440s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:498s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:468s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:468s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:572s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:586s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:555s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:455s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:465s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:480s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:438s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:406s

21d74e215ad650f0e8e30de609bd65601f0aa11d drm-tip: 2017y-06m-26d-09h-12m-14s UTC integration manifest
f1a5a50 drm/i915: Select engines via class and instance in execbuffer2
14e0807 drm/i915: Engine discovery uAPI

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5043/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v4 1/2] drm/i915: Engine discovery uAPI

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-06-26 16:47:41)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine discovery uAPI allows userspace to probe for engine
> configuration and features without needing to maintain the
> internal PCI id based database.
> 
> This enables removal of code duplications across userspace
> components.
> 
> Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
> which returns the number and information on the specified engine
> class.
> 
> Currently only general engine configuration and HEVC feature of
> the VCS engine can be probed but the uAPI is designed to be
> generic and extensible.
> 
> Code is based almost exactly on the earlier proposal on the
> topic by Jon Bloomfield. Engine class and instance refactoring
> made recently by Daniele Ceraolo Spurio enabled this to be
> implemented in an elegant fashion.
> 
> To probe configuration userspace sets the engine class it wants
> to query (struct drm_i915_gem_engine_info) and provides an array
> of drm_i915_engine_info structs which will be filled in by the
> driver. Userspace also has to tell i915 how many elements are in
> the array, and the driver will report back the total number of
> engine instances in any case.
> 
> v2:
>  * Add a version field and consolidate to one engine count.
>    (Chris Wilson)
>  * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
>    (Gong Zhipeng)
> 
> v3:
>  * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
>  * Only reserve 8-bits for the engine info for time being.
> 
> v4:
>  * Made user_class_map static.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c        |  1 +
>  drivers/gpu/drm/i915/i915_drv.h        |  3 ++
>  drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
>  4 files changed, 109 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e9d8e9ee51b2..44dd2f37937f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
>  };
>  
>  static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 427d10c7eca5..496565e1753f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>                             struct i915_gem_context *ctx,
>                             uint32_t *reg_state);
>  
> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
> +                              struct drm_file *file);
> +
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct i915_address_space *vm,
>                                           u64 min_size, u64 alignment,
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 3b46c1f7b88b..a98669f6ad85 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -25,6 +25,7 @@
>  #include "i915_drv.h"
>  #include "intel_ringbuffer.h"
>  #include "intel_lrc.h"
> +#include <uapi/drm/i915_drm.h>
>  
>  /* Haswell does have the CXT_SIZE register however it does not appear to be
>   * valid. Now, docs explain in dwords what is in the context object. The full
> @@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
>         }
>  }
>  
> +static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
> +       [I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
> +       [I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
> +       [I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
> +       [I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
> +       [I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
> +};
> +
> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
> +                              struct drm_file *file)
> +{
> +       struct drm_i915_private *i915 = to_i915(dev);
> +       struct drm_i915_gem_engine_info *args = data;
> +       struct drm_i915_engine_info __user *user_info =
> +                                       u64_to_user_ptr(args->info_ptr);
> +       unsigned int info_size = args->num_engines;
> +       struct drm_i915_engine_info info;
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
> +       u8 class;
> +
> +       if (args->rsvd)
> +               return -EINVAL;
> +
> +       switch (args->engine_class) {
> +       case I915_ENGINE_CLASS_OTHER:
> +       case I915_ENGINE_CLASS_RENDER:
> +       case I915_ENGINE_CLASS_COPY:
> +       case I915_ENGINE_CLASS_VIDEO:
> +       case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> +               class = user_class_map[args->engine_class];
> +               break;
> +       case I915_ENGINE_CLASS_MAX:
> +       default:
> +               return -EINVAL;
> +       };
> +
> +       args->num_engines = 0;
> +
> +       if (args->version != 1) {
> +               args->version = 1;
> +               return 0;

My gut feeling says we want to report both the version and count in the
first pass. Otherwise we have to do a version check, followed by a count
query, followed by the actual retrieval. If we can combine the version +
count check into one pass, it may be a little less hassle?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v4 1/2] drm/i915: Engine discovery uAPI

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-06-26 16:47:41)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine discovery uAPI allows userspace to probe for engine
> configuration and features without needing to maintain the
> internal PCI id based database.
> 
> This enables removal of code duplications across userspace
> components.
> 
> Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
> which returns the number and information on the specified engine
> class.
> 
> Currently only general engine configuration and HEVC feature of
> the VCS engine can be probed but the uAPI is designed to be
> generic and extensible.
> 
> Code is based almost exactly on the earlier proposal on the
> topic by Jon Bloomfield. Engine class and instance refactoring
> made recently by Daniele Ceraolo Spurio enabled this to be
> implemented in an elegant fashion.
> 
> To probe configuration userspace sets the engine class it wants
> to query (struct drm_i915_gem_engine_info) and provides an array
> of drm_i915_engine_info structs which will be filled in by the
> driver. Userspace also has to tell i915 how many elements are in
> the array, and the driver will report back the total number of
> engine instances in any case.

Include psuedo code to first show how to do the multipass query followed
by an example hooking it to execbuf.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v4 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

On 28/06/2017 12:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-06-26 16:47:41)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Engine discovery uAPI allows userspace to probe for engine
>> configuration and features without needing to maintain the
>> internal PCI id based database.
>>
>> This enables removal of code duplications across userspace
>> components.
>>
>> Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
>> which returns the number and information on the specified engine
>> class.
>>
>> Currently only general engine configuration and HEVC feature of
>> the VCS engine can be probed but the uAPI is designed to be
>> generic and extensible.
>>
>> Code is based almost exactly on the earlier proposal on the
>> topic by Jon Bloomfield. Engine class and instance refactoring
>> made recently by Daniele Ceraolo Spurio enabled this to be
>> implemented in an elegant fashion.
>>
>> To probe configuration userspace sets the engine class it wants
>> to query (struct drm_i915_gem_engine_info) and provides an array
>> of drm_i915_engine_info structs which will be filled in by the
>> driver. Userspace also has to tell i915 how many elements are in
>> the array, and the driver will report back the total number of
>> engine instances in any case.
>>
>> v2:
>>   * Add a version field and consolidate to one engine count.
>>     (Chris Wilson)
>>   * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
>>     (Gong Zhipeng)
>>
>> v3:
>>   * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
>>   * Only reserve 8-bits for the engine info for time being.
>>
>> v4:
>>   * Made user_class_map static.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ben Widawsky <ben@bwidawsk.net>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c        |  1 +
>>   drivers/gpu/drm/i915/i915_drv.h        |  3 ++
>>   drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
>>   include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
>>   4 files changed, 109 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index e9d8e9ee51b2..44dd2f37937f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
>>          DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
>> +       DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
>>   };
>>   
>>   static struct drm_driver driver = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 427d10c7eca5..496565e1753f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>                              struct i915_gem_context *ctx,
>>                              uint32_t *reg_state);
>>   
>> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
>> +                              struct drm_file *file);
>> +
>>   /* i915_gem_evict.c */
>>   int __must_check i915_gem_evict_something(struct i915_address_space *vm,
>>                                            u64 min_size, u64 alignment,
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 3b46c1f7b88b..a98669f6ad85 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -25,6 +25,7 @@
>>   #include "i915_drv.h"
>>   #include "intel_ringbuffer.h"
>>   #include "intel_lrc.h"
>> +#include <uapi/drm/i915_drm.h>
>>   
>>   /* Haswell does have the CXT_SIZE register however it does not appear to be
>>    * valid. Now, docs explain in dwords what is in the context object. The full
>> @@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
>>          }
>>   }
>>   
>> +static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
>> +       [I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
>> +       [I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
>> +       [I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
>> +       [I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
>> +       [I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
>> +};
>> +
>> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
>> +                              struct drm_file *file)
>> +{
>> +       struct drm_i915_private *i915 = to_i915(dev);
>> +       struct drm_i915_gem_engine_info *args = data;
>> +       struct drm_i915_engine_info __user *user_info =
>> +                                       u64_to_user_ptr(args->info_ptr);
>> +       unsigned int info_size = args->num_engines;
>> +       struct drm_i915_engine_info info;
>> +       struct intel_engine_cs *engine;
>> +       enum intel_engine_id id;
>> +       u8 class;
>> +
>> +       if (args->rsvd)
>> +               return -EINVAL;
>> +
>> +       switch (args->engine_class) {
>> +       case I915_ENGINE_CLASS_OTHER:
>> +       case I915_ENGINE_CLASS_RENDER:
>> +       case I915_ENGINE_CLASS_COPY:
>> +       case I915_ENGINE_CLASS_VIDEO:
>> +       case I915_ENGINE_CLASS_VIDEO_ENHANCE:
>> +               class = user_class_map[args->engine_class];
>> +               break;
>> +       case I915_ENGINE_CLASS_MAX:
>> +       default:
>> +               return -EINVAL;
>> +       };
>> +
>> +       args->num_engines = 0;
>> +
>> +       if (args->version != 1) {
>> +               args->version = 1;
>> +               return 0;
> 
> My gut feeling says we want to report both the version and count in the
> first pass. Otherwise we have to do a version check, followed by a count
> query, followed by the actual retrieval. If we can combine the version +
> count check into one pass, it may be a little less hassle?

Could do, I don't see any downsides to it at the moment.

But the only benefit seems it would be in cases where userspace would 
want to allocate the info array to the exact size, rather than just 
using a large enough number.

In both cases userspace has to check both return code from the ioctl and 
that the version number matches the requested one. And in both cases the 
whole operation can be done with just one ioctl. Or if you want to 
allocate the exact size info array in both cases you need two ioctls.

info.version = 1;
info.class = <some class>;
info.num_engines = <a large number should be enough for everyone>;
ret = ioctl(&info);
if (ret || info.version != 1)
	goto try_a_lower_version;	
for_each_engine(info) {
	...
}

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v4 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

On 28/06/2017 14:15, Tvrtko Ursulin wrote:
> On 28/06/2017 12:27, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2017-06-26 16:47:41)
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Engine discovery uAPI allows userspace to probe for engine
>>> configuration and features without needing to maintain the
>>> internal PCI id based database.
>>>
>>> This enables removal of code duplications across userspace
>>> components.
>>>
>>> Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
>>> which returns the number and information on the specified engine
>>> class.
>>>
>>> Currently only general engine configuration and HEVC feature of
>>> the VCS engine can be probed but the uAPI is designed to be
>>> generic and extensible.
>>>
>>> Code is based almost exactly on the earlier proposal on the
>>> topic by Jon Bloomfield. Engine class and instance refactoring
>>> made recently by Daniele Ceraolo Spurio enabled this to be
>>> implemented in an elegant fashion.
>>>
>>> To probe configuration userspace sets the engine class it wants
>>> to query (struct drm_i915_gem_engine_info) and provides an array
>>> of drm_i915_engine_info structs which will be filled in by the
>>> driver. Userspace also has to tell i915 how many elements are in
>>> the array, and the driver will report back the total number of
>>> engine instances in any case.
>>>
>>> v2:
>>>   * Add a version field and consolidate to one engine count.
>>>     (Chris Wilson)
>>>   * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
>>>     (Gong Zhipeng)
>>>
>>> v3:
>>>   * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
>>>   * Only reserve 8-bits for the engine info for time being.
>>>
>>> v4:
>>>   * Made user_class_map static.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Ben Widawsky <ben@bwidawsk.net>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>>> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c        |  1 +
>>>   drivers/gpu/drm/i915/i915_drv.h        |  3 ++
>>>   drivers/gpu/drm/i915/intel_engine_cs.c | 64 
>>> ++++++++++++++++++++++++++++++++++
>>>   include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
>>>   4 files changed, 109 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index e9d8e9ee51b2..44dd2f37937f 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc 
>>> i915_ioctls[] = {
>>>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, 
>>> i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>>>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, 
>>> i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
>>>          DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, 
>>> DRM_RENDER_ALLOW),
>>> +       DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, 
>>> i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
>>>   };
>>>   static struct drm_driver driver = {
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 427d10c7eca5..496565e1753f 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct 
>>> intel_engine_cs *engine,
>>>                              struct i915_gem_context *ctx,
>>>                              uint32_t *reg_state);
>>> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
>>> +                              struct drm_file *file);
>>> +
>>>   /* i915_gem_evict.c */
>>>   int __must_check i915_gem_evict_something(struct i915_address_space 
>>> *vm,
>>>                                            u64 min_size, u64 alignment,
>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> index 3b46c1f7b88b..a98669f6ad85 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -25,6 +25,7 @@
>>>   #include "i915_drv.h"
>>>   #include "intel_ringbuffer.h"
>>>   #include "intel_lrc.h"
>>> +#include <uapi/drm/i915_drm.h>
>>>   /* Haswell does have the CXT_SIZE register however it does not 
>>> appear to be
>>>    * valid. Now, docs explain in dwords what is in the context 
>>> object. The full
>>> @@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct 
>>> drm_i915_private *i915)
>>>          }
>>>   }
>>> +static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
>>> +       [I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
>>> +       [I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
>>> +       [I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
>>> +       [I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
>>> +       [I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
>>> +};
>>> +
>>> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
>>> +                              struct drm_file *file)
>>> +{
>>> +       struct drm_i915_private *i915 = to_i915(dev);
>>> +       struct drm_i915_gem_engine_info *args = data;
>>> +       struct drm_i915_engine_info __user *user_info =
>>> +                                       u64_to_user_ptr(args->info_ptr);
>>> +       unsigned int info_size = args->num_engines;
>>> +       struct drm_i915_engine_info info;
>>> +       struct intel_engine_cs *engine;
>>> +       enum intel_engine_id id;
>>> +       u8 class;
>>> +
>>> +       if (args->rsvd)
>>> +               return -EINVAL;
>>> +
>>> +       switch (args->engine_class) {
>>> +       case I915_ENGINE_CLASS_OTHER:
>>> +       case I915_ENGINE_CLASS_RENDER:
>>> +       case I915_ENGINE_CLASS_COPY:
>>> +       case I915_ENGINE_CLASS_VIDEO:
>>> +       case I915_ENGINE_CLASS_VIDEO_ENHANCE:
>>> +               class = user_class_map[args->engine_class];
>>> +               break;
>>> +       case I915_ENGINE_CLASS_MAX:
>>> +       default:
>>> +               return -EINVAL;
>>> +       };
>>> +
>>> +       args->num_engines = 0;
>>> +
>>> +       if (args->version != 1) {
>>> +               args->version = 1;
>>> +               return 0;
>>
>> My gut feeling says we want to report both the version and count in the
>> first pass. Otherwise we have to do a version check, followed by a count
>> query, followed by the actual retrieval. If we can combine the version +
>> count check into one pass, it may be a little less hassle?
> 
> Could do, I don't see any downsides to it at the moment.
> 
> But the only benefit seems it would be in cases where userspace would 
> want to allocate the info array to the exact size, rather than just 
> using a large enough number.

Sorry, that was one outdated paragraph. From the one below it follows 
that the only benefit is offering a little bit more info on a version 
mismatch. But since we can do the whole query with one ioctl as below, I 
don't see that it would be useful. But could implement it just as well 
since as I said I don't see a downside to it.

Regards,

Tvrtko

> In both cases userspace has to check both return code from the ioctl and 
> that the version number matches the requested one. And in both cases the 
> whole operation can be done with just one ioctl. Or if you want to 
> allocate the exact size info array in both cases you need two ioctls.
> 
> info.version = 1;
> info.class = <some class>;
> info.num_engines = <a large number should be enough for everyone>;
> ret = ioctl(&info);
> if (ret || info.version != 1)
>      goto try_a_lower_version;
> for_each_engine(info) {
>      ...
> }
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v4 1/2] drm/i915: Engine discovery uAPI

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-06-28 14:15:05)
> 
> On 28/06/2017 12:27, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-06-26 16:47:41)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Engine discovery uAPI allows userspace to probe for engine
> >> configuration and features without needing to maintain the
> >> internal PCI id based database.
> >>
> >> This enables removal of code duplications across userspace
> >> components.
> >>
> >> Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
> >> which returns the number and information on the specified engine
> >> class.
> >>
> >> Currently only general engine configuration and HEVC feature of
> >> the VCS engine can be probed but the uAPI is designed to be
> >> generic and extensible.
> >>
> >> Code is based almost exactly on the earlier proposal on the
> >> topic by Jon Bloomfield. Engine class and instance refactoring
> >> made recently by Daniele Ceraolo Spurio enabled this to be
> >> implemented in an elegant fashion.
> >>
> >> To probe configuration userspace sets the engine class it wants
> >> to query (struct drm_i915_gem_engine_info) and provides an array
> >> of drm_i915_engine_info structs which will be filled in by the
> >> driver. Userspace also has to tell i915 how many elements are in
> >> the array, and the driver will report back the total number of
> >> engine instances in any case.
> >>
> >> v2:
> >>   * Add a version field and consolidate to one engine count.
> >>     (Chris Wilson)
> >>   * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
> >>     (Gong Zhipeng)
> >>
> >> v3:
> >>   * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
> >>   * Only reserve 8-bits for the engine info for time being.
> >>
> >> v4:
> >>   * Made user_class_map static.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Cc: Ben Widawsky <ben@bwidawsk.net>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> >> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> >> Cc: Oscar Mateo <oscar.mateo@intel.com>
> >> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.c        |  1 +
> >>   drivers/gpu/drm/i915/i915_drv.h        |  3 ++
> >>   drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
> >>   include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
> >>   4 files changed, 109 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index e9d8e9ee51b2..44dd2f37937f 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
> >>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
> >>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
> >>          DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
> >> +       DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
> >>   };
> >>   
> >>   static struct drm_driver driver = {
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 427d10c7eca5..496565e1753f 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
> >>                              struct i915_gem_context *ctx,
> >>                              uint32_t *reg_state);
> >>   
> >> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
> >> +                              struct drm_file *file);
> >> +
> >>   /* i915_gem_evict.c */
> >>   int __must_check i915_gem_evict_something(struct i915_address_space *vm,
> >>                                            u64 min_size, u64 alignment,
> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> index 3b46c1f7b88b..a98669f6ad85 100644
> >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> @@ -25,6 +25,7 @@
> >>   #include "i915_drv.h"
> >>   #include "intel_ringbuffer.h"
> >>   #include "intel_lrc.h"
> >> +#include <uapi/drm/i915_drm.h>
> >>   
> >>   /* Haswell does have the CXT_SIZE register however it does not appear to be
> >>    * valid. Now, docs explain in dwords what is in the context object. The full
> >> @@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
> >>          }
> >>   }
> >>   
> >> +static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
> >> +       [I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
> >> +       [I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
> >> +       [I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
> >> +       [I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
> >> +       [I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
> >> +};
> >> +
> >> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
> >> +                              struct drm_file *file)
> >> +{
> >> +       struct drm_i915_private *i915 = to_i915(dev);
> >> +       struct drm_i915_gem_engine_info *args = data;
> >> +       struct drm_i915_engine_info __user *user_info =
> >> +                                       u64_to_user_ptr(args->info_ptr);
> >> +       unsigned int info_size = args->num_engines;
> >> +       struct drm_i915_engine_info info;
> >> +       struct intel_engine_cs *engine;
> >> +       enum intel_engine_id id;
> >> +       u8 class;
> >> +
> >> +       if (args->rsvd)
> >> +               return -EINVAL;
> >> +
> >> +       switch (args->engine_class) {
> >> +       case I915_ENGINE_CLASS_OTHER:
> >> +       case I915_ENGINE_CLASS_RENDER:
> >> +       case I915_ENGINE_CLASS_COPY:
> >> +       case I915_ENGINE_CLASS_VIDEO:
> >> +       case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> >> +               class = user_class_map[args->engine_class];
> >> +               break;
> >> +       case I915_ENGINE_CLASS_MAX:
> >> +       default:
> >> +               return -EINVAL;
> >> +       };
> >> +
> >> +       args->num_engines = 0;
> >> +
> >> +       if (args->version != 1) {
> >> +               args->version = 1;
> >> +               return 0;
> > 
> > My gut feeling says we want to report both the version and count in the
> > first pass. Otherwise we have to do a version check, followed by a count
> > query, followed by the actual retrieval. If we can combine the version +
> > count check into one pass, it may be a little less hassle?
> 
> Could do, I don't see any downsides to it at the moment.
> 
> But the only benefit seems it would be in cases where userspace would 
> want to allocate the info array to the exact size, rather than just 
> using a large enough number.
> 
> In both cases userspace has to check both return code from the ioctl and 
> that the version number matches the requested one. And in both cases the 
> whole operation can be done with just one ioctl. Or if you want to 
> allocate the exact size info array in both cases you need two ioctls.
> 
> info.version = 1;
> info.class = <some class>;
> info.num_engines = <a large number should be enough for everyone>;
> ret = ioctl(&info);
> if (ret || info.version != 1)
>         goto try_a_lower_version;       
> for_each_engine(info) {
>         ...
> }

Even if you do preallocate, you need to be prepared for the kernel to
report something is off. I agree that my recommended approach would be
to go with a static allocation that is good enough and fallback to the
multi-pass query with dynamic allocation. If you design with that in
mind, we get the required behaviour correct and then include the
optimistic preallocation as an optimisation. But spell out how userspace
is meant to work, and remember the forward-compatibility problem :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC v5 1/2] drm/i915: Engine discovery uAPI

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine discovery uAPI allows userspace to probe for engine
configuration and features without needing to maintain the
internal PCI id based database.

This enables removal of code duplications across userspace
components.

Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
which returns the number and information on the specified engine
class.

Currently only general engine configuration and HEVC feature of
the VCS engine can be probed but the uAPI is designed to be
generic and extensible.

Code is based almost exactly on the earlier proposal on the
topic by Jon Bloomfield. Engine class and instance refactoring
made recently by Daniele Ceraolo Spurio enabled this to be
implemented in an elegant fashion.

To probe configuration userspace sets the engine class it wants
to query (struct drm_i915_gem_engine_info) and provides an array
of drm_i915_engine_info structs which will be filled in by the
driver. Userspace also has to tell i915 how many elements are in
the array, and the driver will report back the total number of
engine instances in any case.

Pseudo-code example of userspace using the uAPI:

  struct drm_i915_gem_engine_info info = {};
  struct drm_i915_engine_info engines[8];
  int i, ret;

  info.version = 1;
  info.engine_class = I915_ENGINE_CLASS_VIDEO;
  info.num_engines = ARRAY_SIZE(engines);
  info.info_ptr = to_user_pointer(&engines[0]);

  ret = ioctl(..., &info);
  assert(ret == 0);

  if (info.version != 1)
      goto kernel_does_not_support_a_version_this_new;

  for (i = 0; i < info.num_engines; i++)
      printf("VCS%u: flags=%x\n",
             engines[i].instance,
             engines[i].info);

If the userspace wants to allocate the exact number of
drm_i915_engine_info structures instead of pesimistically
over-allocating, it can submit the ioctl two times.

First time with info.num_engines set to zero, so the kernel
reports back the total number of available engines, and the
second time with the field set to the number of allocated
array elements.

v2:
 * Add a version field and consolidate to one engine count.
   (Chris Wilson)
 * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
   (Gong Zhipeng)

v3:
 * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
 * Only reserve 8-bits for the engine info for time being.

v4:
 * Made user_class_map static.

v5:
 * Example usage added to commit msg. (Chris Wilson)
 * Report engine count in case of version mismatch. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c        |  1 +
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
 4 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e9d8e9ee51b2..44dd2f37937f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 427d10c7eca5..496565e1753f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
 			    struct i915_gem_context *ctx,
 			    uint32_t *reg_state);
 
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
 					  u64 min_size, u64 alignment,
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 3b46c1f7b88b..d88198933262 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,6 +25,7 @@
 #include "i915_drv.h"
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
+#include <uapi/drm/i915_drm.h>
 
 /* Haswell does have the CXT_SIZE register however it does not appear to be
  * valid. Now, docs explain in dwords what is in the context object. The full
@@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
 	}
 }
 
+static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
+	[I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
+	[I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
+	[I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
+	[I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
+	[I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
+};
+
+int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_engine_info *args = data;
+	struct drm_i915_engine_info __user *user_info =
+					u64_to_user_ptr(args->info_ptr);
+	unsigned int info_size = args->num_engines;
+	struct drm_i915_engine_info info;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	u8 class;
+
+	if (args->rsvd)
+		return -EINVAL;
+
+	switch (args->engine_class) {
+	case I915_ENGINE_CLASS_OTHER:
+	case I915_ENGINE_CLASS_RENDER:
+	case I915_ENGINE_CLASS_COPY:
+	case I915_ENGINE_CLASS_VIDEO:
+	case I915_ENGINE_CLASS_VIDEO_ENHANCE:
+		class = user_class_map[args->engine_class];
+		break;
+	case I915_ENGINE_CLASS_MAX:
+	default:
+		return -EINVAL;
+	};
+
+	args->num_engines = 0;
+
+	for_each_engine(engine, i915, id) {
+		if (class != engine->class)
+			continue;
+
+		if (++args->num_engines > info_size)
+			continue;
+
+		if (args->version != 1)
+			continue;
+
+		memset(&info, 0, sizeof(info));
+		info.instance = engine->instance;
+		if (INTEL_GEN(i915) >= 8 && id == VCS)
+			info.info = I915_VCS_HAS_HEVC;
+
+		if (copy_to_user(user_info++, &info, sizeof(info)))
+			return -EFAULT;
+	}
+
+	args->version = 1;
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7ccbd6a2bbe0..fbb1f6b99959 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -260,6 +260,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
 #define DRM_I915_PERF_OPEN		0x36
+#define DRM_I915_GEM_ENGINE_INFO	0x37
 
 #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)
@@ -315,6 +316,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_PERF_OPEN	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
+#define DRM_IOCTL_I915_GEM_ENGINE_INFO	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_ENGINE_INFO, struct drm_i915_gem_engine_info)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1467,6 +1469,45 @@ enum drm_i915_perf_record_type {
 	DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };
 
+enum drm_i915_gem_engine_class {
+	I915_ENGINE_CLASS_OTHER = 0,
+	I915_ENGINE_CLASS_RENDER = 1,
+	I915_ENGINE_CLASS_COPY = 2,
+	I915_ENGINE_CLASS_VIDEO = 3,
+	I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
+	I915_ENGINE_CLASS_MAX /* non-ABI */
+};
+
+struct drm_i915_engine_info {
+	/** Engine instance number. */
+	__u8	instance;
+
+	/** Engine specific info. */
+#define I915_VCS_HAS_HEVC	BIT(0)
+	__u8	info;
+
+	__u8	rsvd2[6];
+};
+
+struct drm_i915_gem_engine_info {
+	/** in/out: Protocol version requested/supported. */
+	__u32 version;
+
+	/** in: Engine class to probe (enum drm_i915_gem_engine_class). */
+	__u32 engine_class;
+
+	/**
+	 * in/out: Number of struct drm_i915_engine_info entries in the provided
+	 * @info_ptr array and actual number of supported hardware engines.
+	 */
+	__u32 num_engines;
+	__u32 rsvd;
+
+	/** in/out: Pointer to array of struct i915_engine_info elements. */
+	__u64 info_ptr;
+
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.9.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for New engine discovery and execbuffer2 engine selection uAPI (rev9)

[Patchwork]

== Series Details ==

Series: New engine discovery and execbuffer2 engine selection uAPI (rev9)
URL   : https://patchwork.freedesktop.org/series/23189/
State : failure

== Summary ==

Series 23189v9 New engine discovery and execbuffer2 engine selection uAPI
https://patchwork.freedesktop.org/api/1.0/series/23189/revisions/9/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_parse:
        Subgroup basic-allowed:
                pass       -> FAIL       (fi-byt-j1900)
        Subgroup basic-rejected:
                pass       -> FAIL       (fi-byt-j1900)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:438s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:430s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:357s
fi-bsw-n3050     total:279  pass:242  dwarn:1   dfail:0   fail:0   skip:36  time:529s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:504s
fi-byt-j1900     total:279  pass:251  dwarn:2   dfail:0   fail:2   skip:24  time:439s
fi-byt-n2820     total:279  pass:249  dwarn:2   dfail:0   fail:0   skip:28  time:486s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:602s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:433s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:417s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:501s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:471s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:574s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:587s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:562s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:454s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:343s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:462s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:479s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:548s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:403s

85a692e2c6a7cf93082044d776e838cb9e9b2146 drm-tip: 2017y-06m-28d-14h-24m-59s UTC integration manifest
b3ca660 drm/i915: Select engines via class and instance in execbuffer2
6e37821 drm/i915: Engine discovery uAPI

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5060/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx