drm/i915: Engine discovery query

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  |  9 ++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 54 +++++++++++++++++++++
  4 files changed, 129 insertions(+)

drivers/gpu/drm/i915/i915_query.c +63 -0

index 5821002cad42..294f8195efa7 100644
@@ -84,9 +84,72 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
  	return total_length;
  }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+		  struct drm_i915_query_item *query_item)
+{
+	struct drm_i915_query_engine_info __user *query_ptr =
+				u64_to_user_ptr(query_item->data_ptr);
+	struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+	struct drm_i915_query_engine_info query;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int len;
+
+	if (query_item->flags)
+		return -EINVAL;
+
+	len = sizeof(struct drm_i915_query_engine_info) +
+	      I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);
+
+	if (!query_item->length)
+		return len;
+	else if (query_item->length < len)
+		return -EINVAL;
+
+	if (copy_from_user(&query, query_ptr, sizeof(query)))
+		return -EFAULT;
+
+	if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+	    query.rsvd[2])
+		return -EINVAL;
+
+	if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+		return -EFAULT;
+
+	for_each_engine(engine, i915, id) {
+		struct drm_i915_engine_info info;
+
+		if (__copy_from_user(&info, info_ptr, sizeof(info)))
+			return -EFAULT;
+
+		if (info.flags || info.class || info.instance ||
+		    info.capabilities || info.rsvd0 || info.rsvd1[0] ||
+		    info.rsvd1[1])
+			return -EINVAL;
+
+		info.class = engine->uabi_class;
+		info.instance = engine->instance;
+		info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI;
+		info.capabilities = engine->capabilities;
+
+		if (__copy_to_user(info_ptr, &info, sizeof(info)))
+			return -EFAULT;
+
+		query.num_engines++;
+		info_ptr++;
+	}
+
+	if (__copy_to_user(query_ptr, &query, sizeof(query)))
+		return -EFAULT;
+
+	return len;
+}
+
  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
  					struct drm_i915_query_item *query_item) = {
  	query_topology_info,
+	query_engine_info,
  };
 
  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +0 -0

index 1c6143bdf5a4..97b4acf8af5c 100644
@@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
  	engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases);
  	engine->class = info->class;

drivers/gpu/drm/i915/intel_engine_cs.c +0 -0

  	engine->uabi_id = info->uabi_id;
  	engine->uabi_class = intel_engine_classes[info->class].uabi_class;

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index 1534de5bb852..90cf4eea0de2 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
 
  	u8 class;
  	u8 instance;
+
+	u32 capabilities;
+
  	u32 context_size;
  	u32 mmio_base;
 

include/uapi/drm/i915_drm.h +47 -0

index 298b2e197744..c4292e5fed52 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
  struct drm_i915_query_item {
  	__u64 query_id;
  #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
  	/*
  	 * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,59 @@ struct drm_i915_query_topology_info {
  	__u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+	/**
+	 * Engine flags.
+	 *
+	 * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware
+	 * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf
+	 */
+	__u64 flags;
+#define I915_ENGINE_FLAG_PHYSICAL	(1 << 0)
+#define I915_ENGINE_FLAG_ABI		(1 << 1)
+
+	/** Engine class as in enum drm_i915_gem_engine_class. */
+	__u16 class;
+
+	/** Engine instance number. */
+	__u16 instance;
+
+	/** Reserved field must be cleared to zero. */
+	__u32 rsvd0;
+
+	/** Capabilities of this engine. */
+	__u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	(1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC	(1 << 1)
+
+	/** Reserved fields must be cleared to zero. */
+	__u64 rsvd1[2];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+	/** Number of struct drm_i915_engine_info structs following. */
+	__u32 num_engines;
+

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
 drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c  |  9 ++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
 include/uapi/drm/i915_drm.h             | 54 +++++++++++++++++++++
 4 files changed, 129 insertions(+)

drivers/gpu/drm/i915/i915_query.c +7 -0

index 5821002cad42..294f8195efa7 100644
@@ -84,9 +84,72 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
        return total_length;
 }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       struct drm_i915_query_engine_info __user *query_ptr =
+                               u64_to_user_ptr(query_item->data_ptr);
+       struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];

drivers/gpu/drm/i915/i915_query.c +46 -0

+
+       if (!query_item->length)
+               return len;
+       else if (query_item->length < len)
+               return -EINVAL;
+
+       if (copy_from_user(&query, query_ptr, sizeof(query)))
+               return -EFAULT;
+
+       if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+           query.rsvd[2])
+               return -EINVAL;
+
+       if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+               return -EFAULT;
+
+       for_each_engine(engine, i915, id) {
+               struct drm_i915_engine_info info;
+
+               if (__copy_from_user(&info, info_ptr, sizeof(info)))
+                       return -EFAULT;
+
+               if (info.flags || info.class || info.instance ||
+                   info.capabilities || info.rsvd0 || info.rsvd1[0] ||
+                   info.rsvd1[1])
+                       return -EINVAL;
+
+               info.class = engine->uabi_class;
+               info.instance = engine->instance;
+               info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI;
+               info.capabilities = engine->capabilities;
+
+               if (__copy_to_user(info_ptr, &info, sizeof(info)))
+                       return -EFAULT;
+
+               query.num_engines++;
+               info_ptr++;
+       }
+
+       if (__copy_to_user(query_ptr, &query, sizeof(query)))
+               return -EFAULT;
+
+       return len;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
                                        struct drm_i915_query_item *query_item) = {
        query_topology_info,
+       query_engine_info,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +9 -0

index 1c6143bdf5a4..97b4acf8af5c 100644
@@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
        engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases);
        engine->class = info->class;
        engine->instance = info->instance;
+       if (engine->class == VIDEO_DECODE_CLASS) {
+               /* HEVC support is present only on vcs0. */
+               if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+                       engine->capabilities = I915_VCS_CLASS_CAPABILITY_HEVC;
+
+               /* SFC support is wired only to even VCS instances. */
+               if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+                       engine->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC;
+       }
 
        engine->uabi_id = info->uabi_id;
        engine->uabi_class = intel_engine_classes[info->class].uabi_class;

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -0

index 1534de5bb852..90cf4eea0de2 100644

drivers/gpu/drm/i915/intel_ringbuffer.h +1 -0

+
        u32 context_size;
        u32 mmio_base;
 

include/uapi/drm/i915_drm.h +54 -0

index 298b2e197744..c4292e5fed52 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
 struct drm_i915_query_item {
        __u64 query_id;
 #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO     2
 
        /*
         * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,59 @@ struct drm_i915_query_topology_info {
        __u8 data[];
 };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+       /**
+        * Engine flags.
+        *
+        * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware
+        * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf
+        */
+       __u64 flags;
+#define I915_ENGINE_FLAG_PHYSICAL      (1 << 0)
+#define I915_ENGINE_FLAG_ABI           (1 << 1)
+
+       /** Engine class as in enum drm_i915_gem_engine_class. */
+       __u16 class;
+
+       /** Engine instance number. */
+       __u16 instance;
+
+       /** Reserved field must be cleared to zero. */
+       __u32 rsvd0;
+
+       /** Capabilities of this engine. */
+       __u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC  (1 << 1)
+
+       /** Reserved fields must be cleared to zero. */
+       __u64 rsvd1[2];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+       /** Number of struct drm_i915_engine_info structs following. */
+       __u32 num_engines;
+
+       /** MBZ */
+       __u32 rsvd[3];
+
+       /** Marker for drm_i915_engine_info structures. */
+       struct drm_i915_engine_info engines[];
+};
+
 #if defined(__cplusplus)
 }
 #endif

Signature · 1 line

2.17.1

Quoting Tvrtko Ursulin (2018-10-01 17:15:24)

Quoted 72 lines

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  |  9 ++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 54 +++++++++++++++++++++
  4 files changed, 129 insertions(+)

drivers/gpu/drm/i915/i915_query.c +13 -0

index 5821002cad42..294f8195efa7 100644
@@ -84,9 +84,72 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
         return total_length;
  }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       struct drm_i915_query_engine_info __user *query_ptr =
+                               u64_to_user_ptr(query_item->data_ptr);
+       struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+       struct drm_i915_query_engine_info query;
+       struct intel_engine_cs *engine;
+       enum intel_engine_id id;
+       int len;
+
+       if (query_item->flags)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 103

Quoted 4 lines

drivers/gpu/drm/i915/i915_query.c +4 -0

+               return -EINVAL;
+
+       len = sizeof(struct drm_i915_query_engine_info) +
+             I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);

Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step query (1st to find length, 2nd to grab details), it should work with info->num_rings. If not, we have problems later ;)

Quoted 78 lines

drivers/gpu/drm/i915/i915_query.c +46 -0

+
+       if (!query_item->length)
+               return len;
+       else if (query_item->length < len)
+               return -EINVAL;
+
+       if (copy_from_user(&query, query_ptr, sizeof(query)))
+               return -EFAULT;
+
+       if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+           query.rsvd[2])
+               return -EINVAL;
+
+       if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+               return -EFAULT;
+
+       for_each_engine(engine, i915, id) {
+               struct drm_i915_engine_info info;
+
+               if (__copy_from_user(&info, info_ptr, sizeof(info)))
+                       return -EFAULT;
+
+               if (info.flags || info.class || info.instance ||
+                   info.capabilities || info.rsvd0 || info.rsvd1[0] ||
+                   info.rsvd1[1])
+                       return -EINVAL;
+
+               info.class = engine->uabi_class;
+               info.instance = engine->instance;
+               info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI;
+               info.capabilities = engine->capabilities;
+
+               if (__copy_to_user(info_ptr, &info, sizeof(info)))
+                       return -EFAULT;
+
+               query.num_engines++;
+               info_ptr++;
+       }
+
+       if (__copy_to_user(query_ptr, &query, sizeof(query)))
+               return -EFAULT;
+
+       return len;
+}
+
  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
                                         struct drm_i915_query_item *query_item) = {
         query_topology_info,
+       query_engine_info,
  };
 
  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +9 -0

index 1c6143bdf5a4..97b4acf8af5c 100644
@@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
         engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases);
         engine->class = info->class;
         engine->instance = info->instance;
+       if (engine->class == VIDEO_DECODE_CLASS) {
+               /* HEVC support is present only on vcs0. */
+               if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+                       engine->capabilities = I915_VCS_CLASS_CAPABILITY_HEVC;
+
+               /* SFC support is wired only to even VCS instances. */
+               if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+                       engine->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC;
+       }
 
         engine->uabi_id = info->uabi_id;
         engine->uabi_class = intel_engine_classes[info->class].uabi_class;

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -0

index 1534de5bb852..90cf4eea0de2 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
 

drivers/gpu/drm/i915/intel_ringbuffer.h struct intel_engine_cs { · line 374

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.h +2 -0

         u8 class;
         u8 instance;
+
+       u32 capabilities;

I wonder if uabi_capabilities would be a helpful reminder that we can't change this field without wider repercussions.

Quoted 76 lines

drivers/gpu/drm/i915/intel_ringbuffer.h +1 -0

+
         u32 context_size;
         u32 mmio_base;
 

include/uapi/drm/i915_drm.h +54 -0

index 298b2e197744..c4292e5fed52 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
  struct drm_i915_query_item {
         __u64 query_id;
  #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO     2
 
         /*
          * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,59 @@ struct drm_i915_query_topology_info {
         __u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+       /**
+        * Engine flags.
+        *
+        * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware
+        * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf
+        */
+       __u64 flags;
+#define I915_ENGINE_FLAG_PHYSICAL      (1 << 0)
+#define I915_ENGINE_FLAG_ABI           (1 << 1)
+
+       /** Engine class as in enum drm_i915_gem_engine_class. */
+       __u16 class;
+
+       /** Engine instance number. */
+       __u16 instance;
+
+       /** Reserved field must be cleared to zero. */
+       __u32 rsvd0;
+
+       /** Capabilities of this engine. */
+       __u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC  (1 << 1)
+
+       /** Reserved fields must be cleared to zero. */
+       __u64 rsvd1[2];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+       /** Number of struct drm_i915_engine_info structs following. */
+       __u32 num_engines;
+
+       /** MBZ */
+       __u32 rsvd[3];
+
+       /** Marker for drm_i915_engine_info structures. */
+       struct drm_i915_engine_info engines[];
+};
+
  #if defined(__cplusplus)
  }

Quoted 4 lines

include/uapi/drm/i915_drm.h +0 -0

  #endif

Signature · 1 line

2.17.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 7 lines

Quoting Tvrtko Ursulin (2018-10-01 17:15:24)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 103

Quoted 2 lines

drivers/gpu/drm/i915/i915_query.c +2 -0

+       len = sizeof(struct drm_i915_query_engine_info) +
+             I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);

Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step query (1st to find length, 2nd to grab details), it should work with info->num_rings. If not, we have problems later ;)

I don't follow, what do you plan to enumerate that would cause a problem?

Quoting Tvrtko Ursulin (2018-10-01 17:41:00)

Quoted 6 lines · Chris Wilson

Quoted 4 lines

Quoting Tvrtko Ursulin (2018-10-01 17:15:24)

Quoted 2 lines

drivers/gpu/drm/i915/i915_query.c +2 -0

+       len = sizeof(struct drm_i915_query_engine_info) +
+             I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);

Quoted 5 lines

Quoted 3 lines

Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step query (1st to find length, 2nd to grab details), it should work with info->num_rings. If not, we have problems later ;)

I don't follow, what do you plan to enumerate that would cause a problem?

All I'm suggesting is to avoid any notion that this length is fixed indefinitely :)

Quoted 13 lines

Quoting Tvrtko Ursulin (2018-10-01 17:41:00)

Quoted 11 lines · Chris Wilson

Quoted 7 lines

Quoting Tvrtko Ursulin (2018-10-01 17:15:24)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 103

Quoted 2 lines

drivers/gpu/drm/i915/i915_query.c +2 -0

+       len = sizeof(struct drm_i915_query_engine_info) +
+             I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);

Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step query (1st to find length, 2nd to grab details), it should work with info->num_rings. If not, we have problems later ;)

I don't follow, what do you plan to enumerate that would cause a problem?

Quoted 2 lines

All I'm suggesting is to avoid any notion that this length is fixed indefinitely :)

You mean extra paranoid step of making sure reported len exactly matches the number of reported engines * sizeof(engine_info)? uAPI is defined to query required memory block size and then read the reported num_engines so I think that's fine, but sure, I can make it match exactly.

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  |  9 ++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 54 +++++++++++++++++++++
  4 files changed, 129 insertions(+)

drivers/gpu/drm/i915/i915_query.c +63 -0

index 5821002cad42..294f8195efa7 100644
@@ -84,9 +84,72 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
  	return total_length;
  }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+		  struct drm_i915_query_item *query_item)
+{
+	struct drm_i915_query_engine_info __user *query_ptr =
+				u64_to_user_ptr(query_item->data_ptr);
+	struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+	struct drm_i915_query_engine_info query;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int len;
+
+	if (query_item->flags)
+		return -EINVAL;
+
+	len = sizeof(struct drm_i915_query_engine_info) +
+	      I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);
+
+	if (!query_item->length)
+		return len;
+	else if (query_item->length < len)
+		return -EINVAL;
+
+	if (copy_from_user(&query, query_ptr, sizeof(query)))
+		return -EFAULT;
+
+	if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+	    query.rsvd[2])
+		return -EINVAL;
+
+	if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+		return -EFAULT;
+
+	for_each_engine(engine, i915, id) {
+		struct drm_i915_engine_info info;
+
+		if (__copy_from_user(&info, info_ptr, sizeof(info)))
+			return -EFAULT;
+
+		if (info.flags || info.class || info.instance ||
+		    info.capabilities || info.rsvd0 || info.rsvd1[0] ||
+		    info.rsvd1[1])
+			return -EINVAL;
+
+		info.class = engine->uabi_class;
+		info.instance = engine->instance;
+		info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI;
+		info.capabilities = engine->capabilities;
+
+		if (__copy_to_user(info_ptr, &info, sizeof(info)))
+			return -EFAULT;
+
+		query.num_engines++;
+		info_ptr++;
+	}
+
+	if (__copy_to_user(query_ptr, &query, sizeof(query)))
+		return -EFAULT;
+
+	return len;
+}
+
  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
  					struct drm_i915_query_item *query_item) = {
  	query_topology_info,
+	query_engine_info,
  };
 
  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +0 -0

index 1c6143bdf5a4..97b4acf8af5c 100644
@@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
  	engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases);
  	engine->class = info->class;

drivers/gpu/drm/i915/intel_engine_cs.c +0 -0

  	engine->uabi_id = info->uabi_id;
  	engine->uabi_class = intel_engine_classes[info->class].uabi_class;

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index 1534de5bb852..90cf4eea0de2 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
 
  	u8 class;
  	u8 instance;
+
+	u32 capabilities;
+
  	u32 context_size;
  	u32 mmio_base;
 

include/uapi/drm/i915_drm.h +47 -0

index 298b2e197744..c4292e5fed52 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
  struct drm_i915_query_item {
  	__u64 query_id;
  #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
  	/*
  	 * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,59 @@ struct drm_i915_query_topology_info {
  	__u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+	/**
+	 * Engine flags.
+	 *
+	 * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware
+	 * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf
+	 */
+	__u64 flags;
+#define I915_ENGINE_FLAG_PHYSICAL	(1 << 0)
+#define I915_ENGINE_FLAG_ABI		(1 << 1)
+
+	/** Engine class as in enum drm_i915_gem_engine_class. */
+	__u16 class;
+
+	/** Engine instance number. */
+	__u16 instance;
+
+	/** Reserved field must be cleared to zero. */
+	__u32 rsvd0;
+
+	/** Capabilities of this engine. */
+	__u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	(1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC	(1 << 1)
+
+	/** Reserved fields must be cleared to zero. */
+	__u64 rsvd1[2];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+	/** Number of struct drm_i915_engine_info structs following. */
+	__u32 num_engines;
+

Quoting Tvrtko Ursulin (2018-10-03 10:58:55)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 131

Quoted 12 lines

drivers/gpu/drm/i915/i915_query.c +12 -0

+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       for_each_engine(engine, i915, id) {
+               struct drm_i915_engine_info info;
+
+               if (__copy_from_user(&info, info_ptr, sizeof(info)))
+                       return -EFAULT;
+
+               if (memcmp(&info, &zero_info, sizeof(info)))
+                       return -EINVAL;

You want to have this be an in/out?

Quoting Chris Wilson (2018-10-03 13:28:09)

Quoted 15 lines

Quoting Tvrtko Ursulin (2018-10-03 10:58:55)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 131

Quoted 12 lines

drivers/gpu/drm/i915/i915_query.c +12 -0

+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       for_each_engine(engine, i915, id) {
+               struct drm_i915_engine_info info;
+
+               if (__copy_from_user(&info, info_ptr, sizeof(info)))
+                       return -EFAULT;
+
+               if (memcmp(&info, &zero_info, sizeof(info)))
+                       return -EINVAL;

You want to have this be an in/out?

Fwiw, you can use memchr_inv() instead of having a zeroed struct.

Quoted 16 lines

Quoting Chris Wilson (2018-10-03 13:28:09)

Quoted 15 lines

Quoting Tvrtko Ursulin (2018-10-03 10:58:55)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 131

Quoted 12 lines

drivers/gpu/drm/i915/i915_query.c +12 -0

+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       for_each_engine(engine, i915, id) {
+               struct drm_i915_engine_info info;
+
+               if (__copy_from_user(&info, info_ptr, sizeof(info)))
+                       return -EFAULT;
+
+               if (memcmp(&info, &zero_info, sizeof(info)))
+                       return -EINVAL;

You want to have this be an in/out?

I am not sure, perhaps there isn't any benefit in checking that userspace provided a zeroed block at this level since it is an out only block. Instead I could just zero the unused fields for them?

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

v5:
* Add uabi_ prefix to engine capabilities. (Chris Wilson)
* Report exact size of engine info array. (Chris Wilson)
* Drop the engine flags. (Joonas Lahtinen)
* Added some more reserved fields.
* Move flags after class/instance.

v6:
* Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++
  4 files changed, 118 insertions(+)

drivers/gpu/drm/i915/i915_query.c +0 -0

index 5821002cad42..5ac8ef9f5de4 100644
@@ -84,9 +84,65 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
  	return total_length;
  }

drivers/gpu/drm/i915/i915_query.c +4 -0

+
+	if (!query_item->length)
+		return len;
+	else if (query_item->length < len)

drivers/gpu/drm/i915/i915_query.c +21 -0

+		return -EFAULT;
+
+	for_each_engine(engine, i915, id) {
+		info.class = engine->uabi_class;
+		info.instance = engine->instance;
+		info.capabilities = engine->uabi_capabilities;
+
+		if (__copy_to_user(info_ptr, &info, sizeof(info)))
+			return -EFAULT;
+
+		query.num_engines++;
+		info_ptr++;
+	}
+
+	if (__copy_to_user(query_ptr, &query, sizeof(query)))
+		return -EFAULT;
+
+	return len;
+}
+
  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
  					struct drm_i915_query_item *query_item) = {
  	query_topology_info,
+	query_engine_info,
  };
 
  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +12 -0

index 1c6143bdf5a4..134f0cec724c 100644
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
  	engine->uabi_id = info->uabi_id;
  	engine->uabi_class = intel_engine_classes[info->class].uabi_class;
 
+	if (engine->class == VIDEO_DECODE_CLASS) {
+		/* HEVC support is present only on vcs0. */
+		if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+			engine->uabi_capabilities =
+				I915_VCS_CLASS_CAPABILITY_HEVC;
+
+		/* SFC support is wired only to even VCS instances. */
+		if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+			engine->uabi_capabilities |=
+				I915_VCS_CLASS_CAPABILITY_SFC;
+	}
+
  	engine->context_size = __intel_engine_context_size(dev_priv,
  							   engine->class);
  	if (WARN_ON(engine->context_size > BIT(20)))

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index f6ec48a75a69..9dc738f1b175 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
 
  	u8 class;
  	u8 instance;
+
+	u32 uabi_capabilities;
+
  	u32 context_size;
  	u32 mmio_base;
 

include/uapi/drm/i915_drm.h +40 -0

index 298b2e197744..3b0373fb0a93 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
  struct drm_i915_query_item {
  	__u64 query_id;
  #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
  	/*
  	 * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
  	__u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+	/** Engine class as in enum drm_i915_gem_engine_class. */
+	__u16 class;
+
+	/** Engine instance number. */
+	__u16 instance;
+
+	/** Reserved field must be cleared to zero. */
+	__u32 rsvd0;
+
+	/** Engine flags. */
+	__u64 flags;
+
+	/** Capabilities of this engine. */
+	__u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	(1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC	(1 << 1)
+
+	/** Reserved fields must be cleared to zero. */
+	__u64 rsvd1[4];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+	/** Number of struct drm_i915_engine_info structs following. */
+	__u32 num_engines;
+

Quoted 77 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
  * Fixed HEVC assignment.
  * Reorder some fields, rename type to flags, increase width. (Lionel)
  * No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
  * Describe engine flags and mark mbz fields. (Lionel)
  * HEVC only applies to VCS.

v4:
  * Squash SFC flag into main patch.
  * Tidy some comments.

v5:
  * Add uabi_ prefix to engine capabilities. (Chris Wilson)
  * Report exact size of engine info array. (Chris Wilson)
  * Drop the engine flags. (Joonas Lahtinen)
  * Added some more reserved fields.
  * Move flags after class/instance.

v6:
  * Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++ drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++ include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++

  4 files changed, 118 insertions(+)

drivers/gpu/drm/i915/i915_query.c +0 -0

index 5821002cad42..5ac8ef9f5de4 100644
@@ -84,9 +84,65 @@ static int query_topology_info(struct 
 drm_i915_private *dev_priv,
       return total_length;
   }

  +static int

drivers/gpu/drm/i915/i915_query.c +6 -0

+query_engine_info(struct drm_i915_private *i915,
+          struct drm_i915_query_item *query_item)
+{
+    struct drm_i915_query_engine_info __user *query_ptr =
+                u64_to_user_ptr(query_item->data_ptr);
+    struct drm_i915_engine_info __user *info_ptr = 

Quoted 3 lines

&query_ptr->engines[0];
+    struct drm_i915_query_engine_info query;
+    struct drm_i915_engine_info info = { };

I would move the info variable down into the second for_each_engine() loop.

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 107

Quoted 12 lines

+    struct intel_engine_cs *engine;
+    enum intel_engine_id id;
+    int len;

drivers/gpu/drm/i915/i915_query.c +9 -0

+
+    if (query_item->flags)
+        return -EINVAL;
+
+    len = 0;
+    for_each_engine(engine, i915, id)
+        len++;
+    len *= sizeof(struct drm_i915_engine_info);
+    len += sizeof(struct drm_i915_query_engine_info);

Nitpicky, but what about :

len = sizeof(struct drm_i915_query_engine_info);

for_each_engine(engine, i915, id)

       len += sizeof(struct drm_i915_engine_info);

Quoted 4 lines

drivers/gpu/drm/i915/i915_query.c +4 -0

+
+    if (!query_item->length)
+        return len;
+    else if (query_item->length < len)

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 121

Quoted 10 lines

drivers/gpu/drm/i915/i915_query.c +10 -0

+        return -EINVAL;
+
+    if (copy_from_user(&query, query_ptr, sizeof(query)))
+        return -EFAULT;
+
+    if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+        query.rsvd[2])
+        return -EINVAL;
+
+    if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))

Do we want to only verify only the length we're going to write (len)?

Quoted 125 lines

+        return -EFAULT;

drivers/gpu/drm/i915/i915_query.c +19 -0

+
+    for_each_engine(engine, i915, id) {
+        info.class = engine->uabi_class;
+        info.instance = engine->instance;
+        info.capabilities = engine->uabi_capabilities;
+
+        if (__copy_to_user(info_ptr, &info, sizeof(info)))
+            return -EFAULT;
+
+        query.num_engines++;
+        info_ptr++;
+    }
+
+    if (__copy_to_user(query_ptr, &query, sizeof(query)))
+        return -EFAULT;
+
+    return len;
+}
+

  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, struct drm_i915_query_item *query_item) = { query_topology_info,
+    query_engine_info,
  };
    int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +11 -0

index 1c6143bdf5a4..134f0cec724c 100644
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private 
 *dev_priv,
       engine->uabi_id = info->uabi_id;
       engine->uabi_class = intel_engine_classes[info->class].uabi_class;
   +    if (engine->class == VIDEO_DECODE_CLASS) {
+        /* HEVC support is present only on vcs0. */
+        if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+            engine->uabi_capabilities =
+                I915_VCS_CLASS_CAPABILITY_HEVC;
+
+        /* SFC support is wired only to even VCS instances. */
+        if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+            engine->uabi_capabilities |=
+                I915_VCS_CLASS_CAPABILITY_SFC;
+    }
+
       engine->context_size = __intel_engine_context_size(dev_priv,
                                  engine->class);

      if (WARN_ON(engine->context_size > BIT(20)))

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index f6ec48a75a69..9dc738f1b175 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
         u8 class;
       u8 instance;
+
+    u32 uabi_capabilities;
+
       u32 context_size;
       u32 mmio_base;

  diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 298b2e197744..3b0373fb0a93 100644

include/uapi/drm/i915_drm.h +1 -0

@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
   struct drm_i915_query_item {
       __u64 query_id;
   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO    2
         /*
        * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
       __u8 data[];
   };

  +/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */

include/uapi/drm/i915_drm.h +26 -0

+struct drm_i915_engine_info {
+    /** Engine class as in enum drm_i915_gem_engine_class. */
+    __u16 class;
+
+    /** Engine instance number. */
+    __u16 instance;
+
+    /** Reserved field must be cleared to zero. */
+    __u32 rsvd0;
+
+    /** Engine flags. */
+    __u64 flags;
+
+    /** Capabilities of this engine. */
+    __u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC    (1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC    (1 << 1)
+
+    /** Reserved fields must be cleared to zero. */
+    __u64 rsvd1[4];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by 

filling in
+ * an array of struct drm_i915_engine_info structures.
+ */

include/uapi/drm/i915_drm.h +11 -0

+struct drm_i915_query_engine_info {
+    /** Number of struct drm_i915_engine_info structs following. */
+    __u32 num_engines;
+
+    /** MBZ */
+    __u32 rsvd[3];
+
+    /** Marker for drm_i915_engine_info structures. */
+    struct drm_i915_engine_info engines[];
+};
+

  #if defined(__cplusplus)

Quoted 2 lines

  }
  #endif

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 75 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
  * Fixed HEVC assignment.
  * Reorder some fields, rename type to flags, increase width. (Lionel)
  * No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
  * Describe engine flags and mark mbz fields. (Lionel)
  * HEVC only applies to VCS.

v4:
  * Squash SFC flag into main patch.
  * Tidy some comments.

v5:
  * Add uabi_ prefix to engine capabilities. (Chris Wilson)
  * Report exact size of engine info array. (Chris Wilson)
  * Drop the engine flags. (Joonas Lahtinen)
  * Added some more reserved fields.
  * Move flags after class/instance.

v6:
  * Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++ drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++ include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++

  4 files changed, 118 insertions(+)

drivers/gpu/drm/i915/i915_query.c +5 -0

index 5821002cad42..5ac8ef9f5de4 100644
@@ -84,9 +84,65 @@ static int query_topology_info(struct 
 drm_i915_private *dev_priv,
       return total_length;
   }
+static int
+query_engine_info(struct drm_i915_private *i915,
+          struct drm_i915_query_item *query_item)
+{
+    struct drm_i915_query_engine_info __user *query_ptr =

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 95

Quoted 5 lines

drivers/gpu/drm/i915/i915_query.c +4 -0

+                u64_to_user_ptr(query_item->data_ptr);
+    struct drm_i915_engine_info __user *info_ptr = 
 &query_ptr->engines[0];
+    struct drm_i915_query_engine_info query;
+    struct drm_i915_engine_info info = { };

I would move the info variable down into the second for_each_engine() loop.

Quoted 4 lines

drivers/gpu/drm/i915/i915_query.c +2 -0

+    struct intel_engine_cs *engine;
+    enum intel_engine_id id;

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 107

Quoted 10 lines

drivers/gpu/drm/i915/i915_query.c +10 -0

+    int len;
+
+    if (query_item->flags)
+        return -EINVAL;
+
+    len = 0;
+    for_each_engine(engine, i915, id)
+        len++;
+    len *= sizeof(struct drm_i915_engine_info);
+    len += sizeof(struct drm_i915_query_engine_info);

Nitpicky, but what about :

len = sizeof(struct drm_i915_query_engine_info);

for_each_engine(engine, i915, id)

       len += sizeof(struct drm_i915_engine_info);

Quoted 10 lines

drivers/gpu/drm/i915/i915_query.c +9 -0

+
+    if (!query_item->length)
+        return len;
+    else if (query_item->length < len)
+        return -EINVAL;
+
+    if (copy_from_user(&query, query_ptr, sizeof(query)))
+        return -EFAULT;
+

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 121

Quoted 5 lines

drivers/gpu/drm/i915/i915_query.c +5 -0

+    if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+        query.rsvd[2])
+        return -EINVAL;
+
+    if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))

Do we want to only verify only the length we're going to write (len)?

Quoted 127 lines

drivers/gpu/drm/i915/i915_query.c +21 -0

+        return -EFAULT;
+
+    for_each_engine(engine, i915, id) {
+        info.class = engine->uabi_class;
+        info.instance = engine->instance;
+        info.capabilities = engine->uabi_capabilities;
+
+        if (__copy_to_user(info_ptr, &info, sizeof(info)))
+            return -EFAULT;
+
+        query.num_engines++;
+        info_ptr++;
+    }
+
+    if (__copy_to_user(query_ptr, &query, sizeof(query)))
+        return -EFAULT;
+
+    return len;
+}
+
   static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
                       struct drm_i915_query_item *query_item) = {
       query_topology_info,
+    query_engine_info,
   };
   int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +12 -0

index 1c6143bdf5a4..134f0cec724c 100644
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private 
 *dev_priv,
       engine->uabi_id = info->uabi_id;
       engine->uabi_class = intel_engine_classes[info->class].uabi_class;
+    if (engine->class == VIDEO_DECODE_CLASS) {
+        /* HEVC support is present only on vcs0. */
+        if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+            engine->uabi_capabilities =
+                I915_VCS_CLASS_CAPABILITY_HEVC;
+
+        /* SFC support is wired only to even VCS instances. */
+        if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+            engine->uabi_capabilities |=
+                I915_VCS_CLASS_CAPABILITY_SFC;
+    }
+
       engine->context_size = __intel_engine_context_size(dev_priv,
                                  engine->class);
       if (WARN_ON(engine->context_size > BIT(20)))

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index f6ec48a75a69..9dc738f1b175 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
       u8 class;
       u8 instance;
+
+    u32 uabi_capabilities;
+
       u32 context_size;
       u32 mmio_base;

include/uapi/drm/i915_drm.h +5 -0

index 298b2e197744..3b0373fb0a93 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
   struct drm_i915_query_item {
       __u64 query_id;
   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO    2
       /*
        * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
       __u8 data[];
   };
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an 

user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */

include/uapi/drm/i915_drm.h +26 -0

+struct drm_i915_engine_info {
+    /** Engine class as in enum drm_i915_gem_engine_class. */
+    __u16 class;
+
+    /** Engine instance number. */
+    __u16 instance;
+
+    /** Reserved field must be cleared to zero. */
+    __u32 rsvd0;
+
+    /** Engine flags. */
+    __u64 flags;
+
+    /** Capabilities of this engine. */
+    __u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC    (1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC    (1 << 1)
+
+    /** Reserved fields must be cleared to zero. */
+    __u64 rsvd1[4];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by 

filling in
+ * an array of struct drm_i915_engine_info structures.
+ */

include/uapi/drm/i915_drm.h +11 -0

+struct drm_i915_query_engine_info {
+    /** Number of struct drm_i915_engine_info structs following. */
+    __u32 num_engines;
+
+    /** MBZ */
+    __u32 rsvd[3];
+
+    /** Marker for drm_i915_engine_info structures. */
+    struct drm_i915_engine_info engines[];
+};
+

  #if defined(__cplusplus)

Quoted 2 lines

  }
  #endif

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

v5:
* Add uabi_ prefix to engine capabilities. (Chris Wilson)
* Report exact size of engine info array. (Chris Wilson)
* Drop the engine flags. (Joonas Lahtinen)
* Added some more reserved fields.
* Move flags after class/instance.

v6:
* Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

v7:
* Simplify length calculation loop. (Lionel Landwerlin)

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 54 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++
  4 files changed, 116 insertions(+)

drivers/gpu/drm/i915/i915_query.c +54 -0

index 5821002cad42..c374075850a6 100644
@@ -84,9 +84,63 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
  	return total_length;
  }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+		  struct drm_i915_query_item *query_item)
+{
+	struct drm_i915_query_engine_info __user *query_ptr =
+				u64_to_user_ptr(query_item->data_ptr);
+	struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+	struct drm_i915_query_engine_info query;
+	struct drm_i915_engine_info info = { };
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int len;
+
+	if (query_item->flags)
+		return -EINVAL;
+
+	len = sizeof(struct drm_i915_query_engine_info);
+	for_each_engine(engine, i915, id)
+		len += sizeof(struct drm_i915_engine_info);
+
+	if (!query_item->length)
+		return len;
+	else if (query_item->length < len)
+		return -EINVAL;
+
+	if (copy_from_user(&query, query_ptr, sizeof(query)))
+		return -EFAULT;
+
+	if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+	    query.rsvd[2])
+		return -EINVAL;
+
+	if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+		return -EFAULT;
+
+	for_each_engine(engine, i915, id) {
+		info.class = engine->uabi_class;
+		info.instance = engine->instance;
+		info.capabilities = engine->uabi_capabilities;
+
+		if (__copy_to_user(info_ptr, &info, sizeof(info)))
+			return -EFAULT;
+
+		query.num_engines++;
+		info_ptr++;
+	}
+
+	if (__copy_to_user(query_ptr, &query, sizeof(query)))
+		return -EFAULT;
+
+	return len;
+}
+
  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
  					struct drm_i915_query_item *query_item) = {
  	query_topology_info,
+	query_engine_info,
  };
 
  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +12 -0

index 1c6143bdf5a4..134f0cec724c 100644
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
  	engine->uabi_id = info->uabi_id;
  	engine->uabi_class = intel_engine_classes[info->class].uabi_class;
 
+	if (engine->class == VIDEO_DECODE_CLASS) {
+		/* HEVC support is present only on vcs0. */
+		if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+			engine->uabi_capabilities =
+				I915_VCS_CLASS_CAPABILITY_HEVC;
+
+		/* SFC support is wired only to even VCS instances. */
+		if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+			engine->uabi_capabilities |=
+				I915_VCS_CLASS_CAPABILITY_SFC;
+	}
+
  	engine->context_size = __intel_engine_context_size(dev_priv,
  							   engine->class);
  	if (WARN_ON(engine->context_size > BIT(20)))

drivers/gpu/drm/i915/intel_ringbuffer.h +3 -0

index f6ec48a75a69..9dc738f1b175 100644
@@ -370,6 +370,9 @@ struct intel_engine_cs {
 
  	u8 class;
  	u8 instance;
+
+	u32 uabi_capabilities;
+
  	u32 context_size;
  	u32 mmio_base;
 

include/uapi/drm/i915_drm.h +40 -0

index 298b2e197744..3b0373fb0a93 100644
@@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config {
  struct drm_i915_query_item {
  	__u64 query_id;
  #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
  	/*
  	 * When set to zero by userspace, this is filled with the size of the

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
  	__u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+	/** Engine class as in enum drm_i915_gem_engine_class. */
+	__u16 class;
+
+	/** Engine instance number. */
+	__u16 instance;
+
+	/** Reserved field must be cleared to zero. */
+	__u32 rsvd0;
+
+	/** Engine flags. */
+	__u64 flags;
+
+	/** Capabilities of this engine. */
+	__u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	(1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC	(1 << 1)
+
+	/** Reserved fields must be cleared to zero. */
+	__u64 rsvd1[4];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+	/** Number of struct drm_i915_engine_info structs following. */
+	__u32 num_engines;
+

include/uapi/drm/i915_drm.h +6 -0

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
        __u8 data[];
 };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.

include/uapi/drm/i915_drm.h +4 -0

+       /** Marker for drm_i915_engine_info structures. */
+       struct drm_i915_engine_info engines[];
+};
+
 #if defined(__cplusplus)
 }
 #endif

Signature · 1 line

2.17.1

Some comments below, mostly related to trying to keep the uapi header nice and tidy.

Quoting Tvrtko Ursulin (2018-10-04 14:32:48)

Quoted 14 lines

include/uapi/drm/i915_drm.h +10 -0

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
         __u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+       /** Engine class as in enum drm_i915_gem_engine_class. */
+       __u16 class;

include/uapi/drm/i915_drm.h struct drm_i915_query_topology_info { · line 1766

Quoted 6 lines

include/uapi/drm/i915_drm.h +6 -0

+
+       /** Engine instance number. */
+       __u16 instance;
+
+       /** Reserved field must be cleared to zero. */
+       __u32 rsvd0;

u32 class, u32 instance just to put the padding to good use?

include/uapi/drm/i915_drm.h struct drm_i915_query_topology_info { · line 1777

Quoted 12 lines

include/uapi/drm/i915_drm.h +11 -0

+
+       /** Engine flags. */
+       __u64 flags;
+
+       /** Capabilities of this engine. */
+       __u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0)
+#define I915_VCS_CLASS_CAPABILITY_SFC  (1 << 1)
+
+       /** Reserved fields must be cleared to zero. */
+       __u64 rsvd1[4];

Why this at the end of the struct? We have flags which we should be able to use for new versions.

Quoted 4 lines

include/uapi/drm/i915_drm.h +3 -0

+};
+
+/**

include/uapi/drm/i915_drm.h struct drm_i915_query_topology_info { · line 1790

Quoted 10 lines

include/uapi/drm/i915_drm.h +10 -0

+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+       /** Number of struct drm_i915_engine_info structs following. */
+       __u32 num_engines;
+
+       /** MBZ */

Just copy the non-abbreviated comment from other reserved fields.

Review struct drm_i915_query_topology_info { · line 1792

Quoted 3 lines

(quoted diff) +2 -0

+       __u32 rsvd[3];
+

Might as well do just __u32 flags, which must be zero?

I don't think we need to get too excited about adding the reserved fields in every spot :)

Regards, Joonas

Quoted 10 lines

include/uapi/drm/i915_drm.h +4 -0

+       /** Marker for drm_i915_engine_info structures. */
+       struct drm_i915_engine_info engines[];
+};
+
  #if defined(__cplusplus)
  }
  #endif

Signature · 1 line

2.17.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 18 lines

Some comments below, mostly related to trying to keep the uapi header nice and tidy.

Quoting Tvrtko Ursulin (2018-10-04 14:32:48)

Quoted 14 lines

include/uapi/drm/i915_drm.h +10 -0

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
         __u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+       /** Engine class as in enum drm_i915_gem_engine_class. */
+       __u16 class;

Quoted 8 lines

include/uapi/drm/i915_drm.h struct drm_i915_query_topology_info { · line 1766

Quoted 6 lines

include/uapi/drm/i915_drm.h +6 -0

+
+       /** Engine instance number. */
+       __u16 instance;
+
+       /** Reserved field must be cleared to zero. */
+       __u32 rsvd0;

u32 class, u32 instance just to put the padding to good use?

There is some attractiveness to lose the padding, but I think in general we trashed it out to be u16:u16. So it is a question of consistency vs elegance and I give preference to consistency.

Chris, is your recollection also that we said u16:u16 for class:instance in all uAPI?

Quoting Tvrtko Ursulin (2018-10-05 09:34:35)

Quoted 33 lines · Joonas Lahtinen

Quoted 18 lines

Some comments below, mostly related to trying to keep the uapi header nice and tidy.

Quoting Tvrtko Ursulin (2018-10-04 14:32:48)

Quoted 14 lines

include/uapi/drm/i915_drm.h +10 -0

@@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
         __u8 data[];
  };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not it is an user-
+ * accessible or hardware only engine, and what are it's capabilities where
+ * applicable.
+ */
+struct drm_i915_engine_info {
+       /** Engine class as in enum drm_i915_gem_engine_class. */
+       __u16 class;

Quoted 8 lines

include/uapi/drm/i915_drm.h struct drm_i915_query_topology_info { · line 1766

Quoted 6 lines

include/uapi/drm/i915_drm.h +6 -0

+
+       /** Engine instance number. */
+       __u16 instance;
+
+       /** Reserved field must be cleared to zero. */
+       __u32 rsvd0;

u32 class, u32 instance just to put the padding to good use?

There is some attractiveness to lose the padding, but I think in general we trashed it out to be u16:u16. So it is a question of consistency vs elegance and I give preference to consistency.

Quoted 2 lines

Chris, is your recollection also that we said u16:u16 for class:instance in all uAPI?

Yes, that is the conclusion we came to. I've changed my uABI to u16:u16 as well.

u8:u8 too tight, u32:u32 very unlikely. u16 is goldilocks.

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

v5:
* Add uabi_ prefix to engine capabilities. (Chris Wilson)
* Report exact size of engine info array. (Chris Wilson)
* Drop the engine flags. (Joonas Lahtinen)
* Added some more reserved fields.
* Move flags after class/instance.

v6:
* Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
 drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
 include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++
 4 files changed, 118 insertions(+)

drivers/gpu/drm/i915/i915_query.c +9 -0

index 5821002cad42..5ac8ef9f5de4 100644
@@ -84,9 +84,65 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
        return total_length;
 }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       struct drm_i915_query_engine_info __user *query_ptr =
+                               u64_to_user_ptr(query_item->data_ptr);
+       struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+       struct drm_i915_query_engine_info query;
+       struct drm_i915_engine_info info = { };

drivers/gpu/drm/i915/i915_query.c +7 -0

+       len *= sizeof(struct drm_i915_engine_info);
+       len += sizeof(struct drm_i915_query_engine_info);
+
+       if (!query_item->length)
+               return len;
+       else if (query_item->length < len)
+               return -EINVAL;

drivers/gpu/drm/i915/i915_query.c +14 -0

+               if (__copy_to_user(info_ptr, &info, sizeof(info)))
+                       return -EFAULT;
+
+               query.num_engines++;
+               info_ptr++;
+       }
+
+       if (__copy_to_user(query_ptr, &query, sizeof(query)))
+               return -EFAULT;
+
+       return len;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
                                        struct drm_i915_query_item *query_item) = {
        query_topology_info,
+       query_engine_info,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +0 -0

index 1c6143bdf5a4..134f0cec724c 100644
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
        engine->uabi_id = info->uabi_id;
        engine->uabi_class = intel_engine_classes[info->class].uabi_class;
 

drivers/gpu/drm/i915/intel_engine_cs.c +5 -0

+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.

Quoting Tvrtko Ursulin (2018-10-04 11:51:18)

Quoted 83 lines

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

v5:
* Add uabi_ prefix to engine capabilities. (Chris Wilson)
* Report exact size of engine info array. (Chris Wilson)
* Drop the engine flags. (Joonas Lahtinen)
* Added some more reserved fields.
* Move flags after class/instance.

v6:
* Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++
  4 files changed, 118 insertions(+)

drivers/gpu/drm/i915/i915_query.c +13 -0

index 5821002cad42..5ac8ef9f5de4 100644
@@ -84,9 +84,65 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
         return total_length;
  }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       struct drm_i915_query_engine_info __user *query_ptr =
+                               u64_to_user_ptr(query_item->data_ptr);
+       struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+       struct drm_i915_query_engine_info query;
+       struct drm_i915_engine_info info = { };
+       struct intel_engine_cs *engine;
+       enum intel_engine_id id;
+       int len;
+

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 105

Quoted 6 lines

drivers/gpu/drm/i915/i915_query.c +6 -0

+       if (query_item->flags)
+               return -EINVAL;
+
+       len = 0;
+       for_each_engine(engine, i915, id)
+               len++;

(Isn't this INTEL_INFO()->num_rings?)

Quoted 7 lines

drivers/gpu/drm/i915/i915_query.c +7 -0

+       len *= sizeof(struct drm_i915_engine_info);
+       len += sizeof(struct drm_i915_query_engine_info);
+
+       if (!query_item->length)
+               return len;
+       else if (query_item->length < len)
+               return -EINVAL;

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 122

Quoted 10 lines

drivers/gpu/drm/i915/i915_query.c +10 -0

+
+       if (copy_from_user(&query, query_ptr, sizeof(query)))
+               return -EFAULT;
+
+       if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+           query.rsvd[2])
+               return -EINVAL;
+
+       if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+               return -EFAULT;

Hmm, so length is the sizeof of the whole struct and not the space in the pointed at block? Ok. I was just a little confused by the lack of checking for info_ptr, as by this point the connection with query_ptr is off the page.

May I beg

	info_ptr = &query_ptr->engines[0];

here. That should make it more obvious for feeble readers like myself to see that info_ptr is contained within the access_ok check.

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 128

Quoted 6 lines

drivers/gpu/drm/i915/i915_query.c +5 -0

+       for_each_engine(engine, i915, id) {
+               info.class = engine->uabi_class;
+               info.instance = engine->instance;
+               info.capabilities = engine->uabi_capabilities;
+

GEM_BUG_ON((void *)info_ptr > (void *)query_ptr + query_item->length - sizeof(info));

Quoted 29 lines

drivers/gpu/drm/i915/i915_query.c +14 -0

+               if (__copy_to_user(info_ptr, &info, sizeof(info)))
+                       return -EFAULT;
+
+               query.num_engines++;
+               info_ptr++;
+       }
+
+       if (__copy_to_user(query_ptr, &query, sizeof(query)))
+               return -EFAULT;
+
+       return len;
+}
+
  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
                                         struct drm_i915_query_item *query_item) = {
         query_topology_info,
+       query_engine_info,
  };
 
  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)

drivers/gpu/drm/i915/intel_engine_cs.c +0 -0

index 1c6143bdf5a4..134f0cec724c 100644
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
         engine->uabi_id = info->uabi_id;
         engine->uabi_class = intel_engine_classes[info->class].uabi_class;
 

drivers/gpu/drm/i915/intel_engine_cs.c intel_engine_setup(struct drm_i915_private *dev_priv, · line 310

Quoted 10 lines

drivers/gpu/drm/i915/intel_engine_cs.c +10 -0

+       if (engine->class == VIDEO_DECODE_CLASS) {
+               /* HEVC support is present only on vcs0. */
+               if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+                       engine->uabi_capabilities =
+                               I915_VCS_CLASS_CAPABILITY_HEVC;
+
+               /* SFC support is wired only to even VCS instances. */
+               if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+                       engine->uabi_capabilities |=
+                               I915_VCS_CLASS_CAPABILITY_SFC;

I trust your judgement here.

Quoted 10 lines

drivers/gpu/drm/i915/intel_engine_cs.c +9 -0

+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+       /** Number of struct drm_i915_engine_info structs following. */
+       __u32 num_engines;

drivers/gpu/drm/i915/intel_engine_cs.c

Quoted 6 lines

drivers/gpu/drm/i915/intel_engine_cs.c +6 -0

+
+       /** MBZ */
+       __u32 rsvd[3];
+
+       /** Marker for drm_i915_engine_info structures. */
+       struct drm_i915_engine_info engines[];

Do we need [0] for old-c/c++ compatibility?

Reviewed-by Chris Wilson <[email protected]>

I'd value an ack from Lionel in case I've forgotten a quirk about the query api, and an ack from a user that this meets their needs. It will certainly simplify some of our igt code!

Question for overall design, do we want a conjoint capability flag space, or one per class? One per class gives us more room, so I think should be preferred, but I wonder if a set of common flags would make it easier for userspace to filter. (Though not hard to match on both class and caps!)

Quoted 84 lines

Quoting Tvrtko Ursulin (2018-10-04 11:51:18)

Quoted 83 lines

From: Tvrtko Ursulin <[email protected]>

Engine discovery query allows userspace to enumerate engines, probe their configuration features, all without needing to maintain the internal PCI ID based database.

A new query for the generic i915 query ioctl is added named DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure drm_i915_query_engine_info. The address of latter should be passed to the kernel in the query.data_ptr field, and should be large enough for the kernel to fill out all known engines as struct drm_i915_engine_info elements trailing the query.

As with other queries, setting the item query length to zero allows userspace to query minimum required buffer size.

Enumerated engines have common type mask which can be used to query all hardware engines, versus engines userspace can submit to using the execbuf uAPI.

Engines also have capabilities which are per engine class namespace of bits describing features not present on all engine instances.

v2:
* Fixed HEVC assignment.
* Reorder some fields, rename type to flags, increase width. (Lionel)
* No need to allocate temporary storage if we do it engine by engine. (Lionel)

v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.

v4:
* Squash SFC flag into main patch.
* Tidy some comments.

v5:
* Add uabi_ prefix to engine capabilities. (Chris Wilson)
* Report exact size of engine info array. (Chris Wilson)
* Drop the engine flags. (Joonas Lahtinen)
* Added some more reserved fields.
* Move flags after class/instance.

v6:
* Do not check engine info array was zeroed by userspace but zero the unused fields for them instead.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Jon Bloomfield <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Lionel Landwerlin <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Tony Ye <[email protected]>

---
  drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
  include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++
  4 files changed, 118 insertions(+)

drivers/gpu/drm/i915/i915_query.c +13 -0

index 5821002cad42..5ac8ef9f5de4 100644
@@ -84,9 +84,65 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
         return total_length;
  }
 
+static int
+query_engine_info(struct drm_i915_private *i915,
+                 struct drm_i915_query_item *query_item)
+{
+       struct drm_i915_query_engine_info __user *query_ptr =
+                               u64_to_user_ptr(query_item->data_ptr);
+       struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
+       struct drm_i915_query_engine_info query;
+       struct drm_i915_engine_info info = { };
+       struct intel_engine_cs *engine;
+       enum intel_engine_id id;
+       int len;
+

Quoted 8 lines

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 105

Quoted 6 lines

drivers/gpu/drm/i915/i915_query.c +6 -0

+       if (query_item->flags)
+               return -EINVAL;
+
+       len = 0;
+       for_each_engine(engine, i915, id)
+               len++;

(Isn't this INTEL_INFO()->num_rings?)

/o\

Quoted 23 lines

Quoted 7 lines

drivers/gpu/drm/i915/i915_query.c +7 -0

+       len *= sizeof(struct drm_i915_engine_info);
+       len += sizeof(struct drm_i915_query_engine_info);
+
+       if (!query_item->length)
+               return len;
+       else if (query_item->length < len)
+               return -EINVAL;

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 122

Quoted 10 lines

drivers/gpu/drm/i915/i915_query.c +10 -0

+
+       if (copy_from_user(&query, query_ptr, sizeof(query)))
+               return -EFAULT;
+
+       if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
+           query.rsvd[2])
+               return -EINVAL;
+
+       if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
+               return -EFAULT;

Hmm, so length is the sizeof of the whole struct and not the space in the pointed at block? Ok. I was just a little confused by the lack of checking for info_ptr, as by this point the connection with query_ptr is off the page.

Quoted 4 lines

May I beg

      info_ptr = &query_ptr->engines[0];

here. That should make it more obvious for feeble readers like myself to see that info_ptr is contained within the access_ok check.

Ok.

Quoted 7 lines

Quoted 6 lines

drivers/gpu/drm/i915/i915_query.c +5 -0

+       for_each_engine(engine, i915, id) {
+               info.class = engine->uabi_class;
+               info.instance = engine->instance;
+               info.capabilities = engine->uabi_capabilities;
+

Quoted 1 line

GEM_BUG_ON((void *)info_ptr > (void *)query_ptr + query_item->length - sizeof(info));

There is a check above that len fits in query_item->length. So unless the code distrusts itself in a space of a few lines, or we distrust INTEL_INFO->num_engines vs for_each_engine I don't see it is needed?

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/i915/i915_query.c +9 -0

+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+       /** Number of struct drm_i915_engine_info structs following. */
+       __u32 num_engines;

Quoted 8 lines

drivers/gpu/drm/i915/i915_query.c static int query_topology_info(struct drm_i915_private *dev_priv, · line 142

Quoted 6 lines

drivers/gpu/drm/i915/i915_query.c +6 -0

+
+       /** MBZ */
+       __u32 rsvd[3];
+
+       /** Marker for drm_i915_engine_info structures. */
+       struct drm_i915_engine_info engines[];

Do we need [0] for old-c/c++ compatibility?

Hm.. trying to remember. It is GNU C vs C99 for zero sized array vs flexible array member. We went for the latter in the topology query after some discussion but I don't remember the details now. Grepping the uapi headers there are both so don't know.. GCC documentation says flexible members are preferred.

Quoted 6 lines

Reviewed-by Chris Wilson <[email protected]>

I'd value an ack from Lionel in case I've forgotten a quirk about the query api, and an ack from a user that this meets their needs. It will certainly simplify some of our igt code!

Have r-b from Lionel already.

Quoted 5 lines

Question for overall design, do we want a conjoint capability flag space, or one per class? One per class gives us more room, so I think should be preferred, but I wonder if a set of common flags would make it easier for userspace to filter. (Though not hard to match on both class and caps!)

Could split common and per class, 32-bits each with separate fields?

        __u32 capabilities;
        __u32 class_capabilities;

Or keep __u64 capabilities and say common start from bit0 and per class start at bit32? this option could be added/defined post factum as well, with no required changes now - once there is a first common cap. As long as there is a nice block of free bits at that point.