[PATCH] drm/i915: Engine discovery query

[PATCH] drm/i915: Engine discovery query

[Tvrtko Ursulin]

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

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 <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tony Ye <tony.ye@intel.com>
---
 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(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5821002cad42..294f8195efa7 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -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)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 1c6143bdf5a4..97b4acf8af5c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1534de5bb852..90cf4eea0de2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -370,6 +370,9 @@ struct intel_engine_cs {
 
 	u8 class;
 	u8 instance;
+
+	u32 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..c4292e5fed52 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -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
-- 
2.17.1

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

Re: [PATCH] drm/i915: Engine discovery query

[Tvrtko Ursulin]

Hi,

Media experts please scroll down...

On 01/10/2018 17:15, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>   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(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..294f8195efa7 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -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)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..97b4acf8af5c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -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;
> +	}

... to here! :)

I need help with the above two capabilities assignments. Primarily are 
they correct? HEVC situation might be different on Gen11 for instance?

Otherwise it is basically the same patch I sent months ago so 
essentially sending it again just for another review pass, if someone 
will have some new comments.

Regards,

Tvrtko

>   
>   	engine->uabi_id = info->uabi_id;
>   	engine->uabi_class = intel_engine_classes[info->class].uabi_class;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 1534de5bb852..90cf4eea0de2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -370,6 +370,9 @@ struct intel_engine_cs {
>   
>   	u8 class;
>   	u8 instance;
> +
> +	u32 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..c4292e5fed52 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Engine discovery query

[Tvrtko Ursulin]

Hi,

Media experts please scroll down...

On 01/10/2018 17:15, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>   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(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..294f8195efa7 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -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)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..97b4acf8af5c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -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;
> +	}

... to here! :)

I need help with the above two capabilities assignments. Primarily are 
they correct? HEVC situation might be different on Gen11 for instance?

Otherwise it is basically the same patch I sent months ago so 
essentially sending it again just for another review pass, if someone 
will have some new comments.

Regards,

Tvrtko

>   
>   	engine->uabi_id = info->uabi_id;
>   	engine->uabi_class = intel_engine_classes[info->class].uabi_class;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 1534de5bb852..90cf4eea0de2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -370,6 +370,9 @@ struct intel_engine_cs {
>   
>   	u8 class;
>   	u8 instance;
> +
> +	u32 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..c4292e5fed52 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-01 17:15:24)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>  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(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..294f8195efa7 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -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);

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 ;)

> +
> +       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)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..97b4acf8af5c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 1534de5bb852..90cf4eea0de2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -370,6 +370,9 @@ struct intel_engine_cs {
>  
>         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.

> +
>         u32 context_size;
>         u32 mmio_base;
>  
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 298b2e197744..c4292e5fed52 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -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
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev3)
URL   : https://patchwork.freedesktop.org/series/39958/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9c2c6952bb56 drm/i915: Engine discovery query
-:179: WARNING:TYPO_SPELLING: 'an user' may be misspelled - perhaps 'a user'?
#179: FILE: include/uapi/drm/i915_drm.h:1754:
+ * Describes one engine known to the driver, whether or not it is an user-

total: 0 errors, 1 warnings, 0 checks, 162 lines checked

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

Re: [PATCH] drm/i915: Engine discovery query

[Tvrtko Ursulin]

On 01/10/2018 17:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-01 17:15:24)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Tony Ye <tony.ye@intel.com>
>> ---
>>   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(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
>> index 5821002cad42..294f8195efa7 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -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);
> 
> 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?

>> +
>> +       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)
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 1c6143bdf5a4..97b4acf8af5c 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -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;
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 1534de5bb852..90cf4eea0de2 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -370,6 +370,9 @@ struct intel_engine_cs {
>>   
>>          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.

Spot on!

Regards,

Tvrtko

> 
>> +
>>          u32 context_size;
>>          u32 mmio_base;
>>   
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 298b2e197744..c4292e5fed52 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -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
>> -- 
>> 2.17.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Engine discovery query (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev3)
URL   : https://patchwork.freedesktop.org/series/39958/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4908 -> Patchwork_10310 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/39958/revisions/3/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#106685)

    igt@gem_exec_suspend@basic-s3:
      fi-bdw-samus:       PASS -> INCOMPLETE (fdo#107773)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       NOTRUN -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-cfl-8109u:       DMESG-WARN (fdo#107345) -> PASS +3

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    
  fdo#106685 https://bugs.freedesktop.org/show_bug.cgi?id=106685
  fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773


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

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


== Build changes ==

    * Linux: CI_DRM_4908 -> Patchwork_10310

  CI_DRM_4908: d287e1ba5382518eb1ce7cac144f428c1522b8a1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4658: cab89ce2c5da684d01deff402d4e8e11441beadb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10310: 9c2c6952bb563c07750379f55c1ea11646506674 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

9c2c6952bb56 drm/i915: Engine discovery query

== Logs ==

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

✓ Fi.CI.IGT: success for drm/i915: Engine discovery query (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev3)
URL   : https://patchwork.freedesktop.org/series/39958/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4908_full -> Patchwork_10310_full =

== Summary - WARNING ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@drm_read@empty-block:
      shard-snb:          SKIP -> PASS +1

    igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
      shard-snb:          PASS -> SKIP +2

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133, fdo#106886)

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

    igt@kms_flip@flip-vs-modeset-vs-hang:
      shard-apl:          PASS -> DMESG-WARN (fdo#105602, fdo#103558) +35

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

    
    ==== Possible fixes ====

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

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  Missing    (1): shard-skl 


== Build changes ==

    * Linux: CI_DRM_4908 -> Patchwork_10310

  CI_DRM_4908: d287e1ba5382518eb1ce7cac144f428c1522b8a1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4658: cab89ce2c5da684d01deff402d4e8e11441beadb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10310: 9c2c6952bb563c07750379f55c1ea11646506674 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-01 17:41:00)
> 
> On 01/10/2018 17:24, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-10-01 17:15:24)
> >> +       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?

All I'm suggesting is to avoid any notion that this length is fixed
indefinitely :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Engine discovery query

[Tvrtko Ursulin]

On 01/10/2018 20:39, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-01 17:41:00)
>>
>> On 01/10/2018 17:24, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-10-01 17:15:24)
>>>> +       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?
> 
> 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.

Regards,

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

Re: [PATCH] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-02 10:05:11)
> 
> On 01/10/2018 20:39, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-10-01 17:41:00)
> >>
> >> On 01/10/2018 17:24, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-10-01 17:15:24)
> >>>> +       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?
> > 
> > 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.

No, my paranoia is towards people seeing I915_NUM_ENGINES and so
assuming this is constant.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5] drm/i915: Engine discovery query

[Tvrtko Ursulin]

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

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tony Ye <tony.ye@intel.com>
---
 drivers/gpu/drm/i915/i915_query.c       | 64 +++++++++++++++++++++++++
 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, 126 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5821002cad42..beaab2b73ad6 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -84,9 +84,73 @@ 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];
+	const struct drm_i915_engine_info zero_info = { };
+	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 = 0;
+	for_each_engine(engine, i915, id)
+		len++;
+	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;
+
+	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 (memcmp(&info, &zero_info, sizeof(info)))
+			return -EINVAL;
+
+		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)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 1c6143bdf5a4..134f0cec724c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -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)))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f6ec48a75a69..9dc738f1b175 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -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
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -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;
+
+	/** MBZ */
+	__u32 rsvd[3];
+
+	/** Marker for drm_i915_engine_info structures. */
+	struct drm_i915_engine_info engines[];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.17.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev4)
URL   : https://patchwork.freedesktop.org/series/39958/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c387de8457ce drm/i915: Engine discovery query
-:190: WARNING:TYPO_SPELLING: 'an user' may be misspelled - perhaps 'a user'?
#190: FILE: include/uapi/drm/i915_drm.h:1754:
+ * Describes one engine known to the driver, whether or not it is an user-

total: 0 errors, 1 warnings, 0 checks, 159 lines checked

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

✗ Fi.CI.BAT: failure for drm/i915: Engine discovery query (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev4)
URL   : https://patchwork.freedesktop.org/series/39958/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4918 -> Patchwork_10337 =

== Summary - FAILURE ==

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/39958/revisions/4/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       PASS -> WARN

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@amdgpu/amd_cs_nop@fork-gfx0:
      fi-kbl-8809g:       PASS -> DMESG-WARN (fdo#107762)

    igt@gem_exec_suspend@basic-s3:
      fi-bdw-samus:       PASS -> INCOMPLETE (fdo#107773)

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


== Participating hosts (48 -> 45) ==

  Additional (1): fi-cfl-guc 
  Missing    (4): fi-bsw-cyan fi-byt-squawks fi-icl-u2 fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4918 -> Patchwork_10337

  CI_DRM_4918: f595aba3a6e2f6972bb158eb8434b58d22d0e5f0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4662: ebf6a1dd1795e2f014ff3c47fe2eb4d5255845bd @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10337: c387de8457ce542f3c47d636ff2b8931c0bb24dc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c387de8457ce drm/i915: Engine discovery query

== Logs ==

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

Re: [PATCH v5] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-03 10:58:55)
> +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?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Chris Wilson (2018-10-03 13:28:09)
> Quoting Tvrtko Ursulin (2018-10-03 10:58:55)
> > +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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Engine discovery query

[Tvrtko Ursulin]

On 03/10/2018 13:42, Chris Wilson wrote:
> Quoting Chris Wilson (2018-10-03 13:28:09)
>> Quoting Tvrtko Ursulin (2018-10-03 10:58:55)
>>> +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?

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

Ah cool, thanks, saw that one before but forgot about it.

Regards,

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

Re: [PATCH v5] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-03 13:51:58)
> 
> On 03/10/2018 13:42, Chris Wilson wrote:
> > Quoting Chris Wilson (2018-10-03 13:28:09)
> >> Quoting Tvrtko Ursulin (2018-10-03 10:58:55)
> >>> +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?

Just struck me as being a bit overzealous (as I was thinking of a
write-esque syscall onto stack). (The first use that springs to mind for
info in here would be for filtering, and I'm not sure if we want to
start down that road ;) Yes, if we didn't do a copy_from_user
here, we would need a memset or info = {};
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6] drm/i915: Engine discovery query

[Tvrtko Ursulin]

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

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 <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tony Ye <tony.ye@intel.com>
---
 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(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5821002cad42..5ac8ef9f5de4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -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;
+
+	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);
+
+	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)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 1c6143bdf5a4..134f0cec724c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -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)))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f6ec48a75a69..9dc738f1b175 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -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
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -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;
+
+	/** MBZ */
+	__u32 rsvd[3];
+
+	/** Marker for drm_i915_engine_info structures. */
+	struct drm_i915_engine_info engines[];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.17.1

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

Re: [PATCH v6] drm/i915: Engine discovery query

[Lionel Landwerlin]

On 04/10/2018 12:51, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>   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(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..5ac8ef9f5de4 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -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 = { };


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


> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	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);


> +
> +	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))


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


> +		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)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..134f0cec724c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -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)))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index f6ec48a75a69..9dc738f1b175 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -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
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -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;
> +
> +	/** MBZ */
> +	__u32 rsvd[3];
> +
> +	/** Marker for drm_i915_engine_info structures. */
> +	struct drm_i915_engine_info engines[];
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif


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

Re: [PATCH v6] drm/i915: Engine discovery query

[Lionel Landwerlin]

Other than the nitpicking, I can't see anything wrong with this patch.

-
Lionel

On 04/10/2018 13:03, Lionel Landwerlin wrote:
> On 04/10/2018 12:51, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Tony Ye <tony.ye@intel.com>
>> ---
>>   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(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 5821002cad42..5ac8ef9f5de4 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -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 = { };
>
>
> I would move the info variable down into the second for_each_engine() 
> loop.
>
>
>> +    struct intel_engine_cs *engine;
>> +    enum intel_engine_id id;
>> +    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);
>
>
>> +
>> +    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))
>
>
> Do we want to only verify only the length we're going to write (len)?
>
>
>> +        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)
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 1c6143bdf5a4..134f0cec724c 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -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)))
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index f6ec48a75a69..9dc738f1b175 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -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
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -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;
>> +
>> +    /** MBZ */
>> +    __u32 rsvd[3];
>> +
>> +    /** Marker for drm_i915_engine_info structures. */
>> +    struct drm_i915_engine_info engines[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

Re: [PATCH v6] drm/i915: Engine discovery query

[Tvrtko Ursulin]

On 04/10/2018 12:03, Lionel Landwerlin wrote:
> On 04/10/2018 12:51, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Tony Ye <tony.ye@intel.com>
>> ---
>>   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(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 5821002cad42..5ac8ef9f5de4 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -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 = { };
> 
> 
> I would move the info variable down into the second for_each_engine() loop.

Hey this saves some CPU cycles! :))

> 
> 
>> +    struct intel_engine_cs *engine;
>> +    enum intel_engine_id id;
>> +    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);

Saves a multiply, makes sense, thanks!

> 
>> +
>> +    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))
> 
> 
> Do we want to only verify only the length we're going to write (len)?

I thought to check all userspace claims it provided, even though extra 
robustness feels like an only hand-wavy benefit.

Regards,

Tvrtko

> 
> 
>> +        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)
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 1c6143bdf5a4..134f0cec724c 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -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)))
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index f6ec48a75a69..9dc738f1b175 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -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
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -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;
>> +
>> +    /** MBZ */
>> +    __u32 rsvd[3];
>> +
>> +    /** Marker for drm_i915_engine_info structures. */
>> +    struct drm_i915_engine_info engines[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v7] drm/i915: Engine discovery query

[Tvrtko Ursulin]

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

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 <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tony Ye <tony.ye@intel.com>
---
 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(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5821002cad42..c374075850a6 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -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)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 1c6143bdf5a4..134f0cec724c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -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)))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f6ec48a75a69..9dc738f1b175 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -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
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -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;
+
+	/** MBZ */
+	__u32 rsvd[3];
+
+	/** Marker for drm_i915_engine_info structures. */
+	struct drm_i915_engine_info engines[];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.17.1

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

Re: [PATCH v7] drm/i915: Engine discovery query

[Lionel Landwerlin]

On 04/10/2018 13:32, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>


Looks good to me (might want someone else to rubberstamp the uapi structs) :


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


> ---
>   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(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..c374075850a6 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -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)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..134f0cec724c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -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)))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index f6ec48a75a69..9dc738f1b175 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -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
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -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;
> +
> +	/** MBZ */
> +	__u32 rsvd[3];
> +
> +	/** Marker for drm_i915_engine_info structures. */
> +	struct drm_i915_engine_info engines[];
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif


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

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev6)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev6)
URL   : https://patchwork.freedesktop.org/series/39958/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
918342893373 drm/i915: Engine discovery query
-:188: WARNING:TYPO_SPELLING: 'an user' may be misspelled - perhaps 'a user'?
#188: FILE: include/uapi/drm/i915_drm.h:1754:
+ * Describes one engine known to the driver, whether or not it is an user-

total: 0 errors, 1 warnings, 0 checks, 149 lines checked

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

✓ Fi.CI.BAT: success for drm/i915: Engine discovery query (rev6)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev6)
URL   : https://patchwork.freedesktop.org/series/39958/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4931 -> Patchwork_10358 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/39958/revisions/6/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#107139, fdo#105128)
      fi-bdw-samus:       NOTRUN -> INCOMPLETE (fdo#107773)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@gem_ctx_param@basic-default:
      fi-pnv-d510:        INCOMPLETE -> SKIP

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

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-cfl-8109u:       INCOMPLETE (fdo#108126, fdo#106070) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126


== Participating hosts (48 -> 44) ==

  Additional (2): fi-icl-u fi-bdw-samus 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-u2 fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4931 -> Patchwork_10358

  CI_DRM_4931: 826702bf60ae2b37841c051ed769b44af194fbb1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4667: 596f48dcd59fd2f8c16671514f3e69d4a2891374 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10358: 91834289337303eddffa1646cd56961fca8030c7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

918342893373 drm/i915: Engine discovery query

== Logs ==

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

Re: [PATCH v7] drm/i915: Engine discovery query

[Joonas Lahtinen]

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

Quoting Tvrtko Ursulin (2018-10-04 14:32:48)
> @@ -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;

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

> +
> +       /** 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.

> +};
> +
> +/**
> + * 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.

> +       __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

> +       /** Marker for drm_i915_engine_info structures. */
> +       struct drm_i915_engine_info engines[];
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-04 11:51:18)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>  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(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..5ac8ef9f5de4 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -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;
> +
> +       if (query_item->flags)
> +               return -EINVAL;
> +
> +       len = 0;
> +       for_each_engine(engine, i915, id)
> +               len++;

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

> +       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;
> +
> +       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.

> +       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));

> +               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)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..134f0cec724c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -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;

I trust your judgement here.

> +/**
> + * 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[];

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

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

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!)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915: Engine discovery query (rev6)

[Patchwork]

== Series Details ==

Series: drm/i915: Engine discovery query (rev6)
URL   : https://patchwork.freedesktop.org/series/39958/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4931_full -> Patchwork_10358_full =

== Summary - FAILURE ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_flip_tiling@flip-to-x-tiled:
      shard-skl:          PASS -> FAIL

    
    ==== Warnings ====

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_flush@basic-batch-kernel-default-wb:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133)

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-skl:          NOTRUN -> TIMEOUT (fdo#108039)

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

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
      shard-snb:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +2

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-glk:          PASS -> FAIL (fdo#103232)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363, fdo#102887)

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

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-1p-shrfb-fliptrack:
      shard-glk:          PASS -> DMESG-WARN (fdo#105763, fdo#106538) +2

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-blt:
      shard-glk:          PASS -> DMESG-FAIL (fdo#106538)

    igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-wc:
      shard-skl:          PASS -> FAIL (fdo#105682)

    igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-blt:
      shard-skl:          PASS -> FAIL (fdo#103167, fdo#105682)

    igt@kms_frontbuffer_tracking@fbcpsr-suspend:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108)

    igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-pgflip-blt:
      shard-skl:          PASS -> FAIL (fdo#103167) +2

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#106885)

    {igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb}:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +1

    
    ==== Possible fixes ====

    igt@drv_suspend@fence-restore-untiled:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

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

    igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-xtiled:
      shard-skl:          FAIL (fdo#107791) -> PASS

    igt@kms_draw_crc@fill-fb:
      shard-skl:          FAIL -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-skl:          FAIL (fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-cpu:
      shard-skl:          FAIL (fdo#103167) -> PASS +2

    {igt@kms_plane_alpha_blend@pipe-a-coverage-7efc}:
      shard-skl:          FAIL (fdo#108145) -> PASS

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

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

    igt@pm_rpm@legacy-planes:
      shard-skl:          INCOMPLETE (fdo#105959, fdo#107807) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#105959 https://bugs.freedesktop.org/show_bug.cgi?id=105959
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
  fdo#107791 https://bugs.freedesktop.org/show_bug.cgi?id=107791
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4931 -> Patchwork_10358

  CI_DRM_4931: 826702bf60ae2b37841c051ed769b44af194fbb1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4667: 596f48dcd59fd2f8c16671514f3e69d4a2891374 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10358: 91834289337303eddffa1646cd56961fca8030c7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v6] drm/i915: Engine discovery query

[Tvrtko Ursulin]

On 04/10/2018 17:33, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-04 11:51:18)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Tony Ye <tony.ye@intel.com>
>> ---
>>   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(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
>> index 5821002cad42..5ac8ef9f5de4 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -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;
>> +
>> +       if (query_item->flags)
>> +               return -EINVAL;
>> +
>> +       len = 0;
>> +       for_each_engine(engine, i915, id)
>> +               len++;
> 
> (Isn't this INTEL_INFO()->num_rings?)

/o\

>> +       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;
>> +
>> +       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.

Ok.

> 
>> +       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));

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?

> 
>> +               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)
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 1c6143bdf5a4..134f0cec724c 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -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;
> 
> I trust your judgement here.

And I trust pending feedback from media team. :)

> 
>> +/**
>> + * 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[];
> 
> 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.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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.

> 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.

Regards,

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

Re: [PATCH v7] drm/i915: Engine discovery query

[Tvrtko Ursulin]

On 04/10/2018 15:32, Joonas Lahtinen wrote:
> Some comments below, mostly related to trying to keep the uapi header
> nice and tidy.
> 
> Quoting Tvrtko Ursulin (2018-10-04 14:32:48)
>> @@ -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;
> 
> 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?

> 
>> +
>> +       /** 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.

This is to allow some growing space without complicating the userspace 
array member start calculation.

If we have to grow struct engine_info itself when adding a new member, 
then we have to define the array (via documentation) as potentially not 
aligned to sizeof(struct engine_info) as known by userspace.

I don't have such a big aversion to rsvd fields so for me it seems 
easier to have some, with the benefit of simpler userspace code.

But if the consensus will be to change it, no problem.

> 
>> +};
>> +
>> +/**
>> + * 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.

I need to nuke this comment since from v6 code is not mandating MBZ for 
the array.

> 
>> +       __u32 rsvd[3];
>> +
> 
> Might as well do just __u32 flags, which must be zero?

Could do flags..

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

... but I do like my rsvd! :))

Regards,

Tvrtko

> Regards, Joonas
> 
>> +       /** Marker for drm_i915_engine_info structures. */
>> +       struct drm_i915_engine_info engines[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> -- 
>> 2.17.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-05 09:34:35)
> 
> On 04/10/2018 15:32, Joonas Lahtinen wrote:
> > Some comments below, mostly related to trying to keep the uapi header
> > nice and tidy.
> > 
> > Quoting Tvrtko Ursulin (2018-10-04 14:32:48)
> >> @@ -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;
> > 
> > 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?

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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6] drm/i915: Engine discovery query

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-10-05 09:26:37)
> 
> On 04/10/2018 17:33, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-10-04 11:51:18)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> 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 <tvrtko.ursulin@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> >> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Tony Ye <tony.ye@intel.com>
> >> ---
> >>   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(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> >> index 5821002cad42..5ac8ef9f5de4 100644
> >> --- a/drivers/gpu/drm/i915/i915_query.c
> >> +++ b/drivers/gpu/drm/i915/i915_query.c
> >> @@ -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;
> >> +
> >> +       if (query_item->flags)
> >> +               return -EINVAL;
> >> +
> >> +       len = 0;
> >> +       for_each_engine(engine, i915, id)
> >> +               len++;
> > 
> > (Isn't this INTEL_INFO()->num_rings?)
> 
> /o\
> 
> >> +       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;
> >> +
> >> +       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.
> 
> Ok.
> 
> > 
> >> +       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));
> 
> 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?

My only point was to keep tying info_ptr back to the early checks on
query_ptr. From the standpoint of looking at this loop copying into
userspace, I wanted to reassure myself that everything was checked and
cross-checked.

> >> +/**
> >> + * 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[];
> > 
> > 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.
> 
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > 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.
> 
> > 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.

I like the suggestion to carve out some bits ahead of time for common
caps. Then I just worry if 32 is enough :)

Hmm, how about a challenge of describing which engines are compatible
for v.eng? Trial-and-error strikes me as being the simplest discovery
method still.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7] drm/i915: Engine discovery query

[Joonas Lahtinen]

Quoting Chris Wilson (2018-10-05 12:21:12)
> Quoting Tvrtko Ursulin (2018-10-05 09:34:35)
> > 
> > On 04/10/2018 15:32, Joonas Lahtinen wrote:
> > > Some comments below, mostly related to trying to keep the uapi header
> > > nice and tidy.
> > > 
> > > Quoting Tvrtko Ursulin (2018-10-04 14:32:48)
> > >> @@ -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;
> > > 
> > > 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?
> 
> 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.

u32:u32 nicely aligns with u64 which is required in all structs ;)

As mentioned in IRC, I'd try to keep the uAPI structs lean and simple as
possible, but it's not a blocker to sprinkle some rsvd if others think
they are beneficial/pessimism is required.

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