All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] drm/i915: Engine discovery query
@ 2018-03-14 14:05 Tvrtko Ursulin
  2018-03-14 14:44 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2018-03-14 14:05 UTC (permalink / raw)
  To: Intel-gfx

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.

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>
---
This is RFC for now since it is not very usable before the new execbuf API
or virtual engine queue submission gets closer.

In this version I have added capability of distinguishing between hardware
engines and ABI engines. This is to account for probable future use cases
like virtualization, where guest might only see one engine instance, but
will want to know overall hardware capabilities in order to tune its
behaviour.

In general I think we will have to wait a bit before defining the final
look of the uAPI, but in the meantime I thought it useful to share as RFC.
---
 drivers/gpu/drm/i915/i915_query.c       | 61 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
 include/uapi/drm/i915_drm.h             | 44 ++++++++++++++++++++++++
 4 files changed, 111 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 3ace929dd90f..e00d02796d42 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -82,9 +82,70 @@ 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 query, *q;
+	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) +
+	      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, u64_to_user_ptr(query_item->data_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, u64_to_user_ptr(query_item->data_ptr),
+		       len))
+		return -EFAULT;
+
+	q = kzalloc(len, GFP_KERNEL);
+	if (!q)
+		return -ENOMEM;
+
+	info = &q->engines[0];
+
+	for_each_engine(engine, i915, id) {
+		info->class = engine->uabi_class;
+		info->instance = engine->instance;
+		info->type = I915_ENGINE_TYPE_PHYSICAL |
+			     I915_ENGINE_TYPE_UAPI;
+		info->capabilities = engine->capabilities;
+		info++;
+		q->num_engines++;
+	}
+
+	if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr), q, len)) {
+		len = -EFAULT;
+		goto out;
+	}
+
+out:
+	kfree(q);
+
+	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 a2b1e9e2c008..81be4acd8358 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -86,6 +86,7 @@ struct engine_info {
 	unsigned int uabi_id;
 	u8 class;
 	u8 instance;
+	u32 capabilities;
 	u32 mmio_base;
 	unsigned irq_shift;
 };
@@ -114,6 +115,7 @@ static const struct engine_info intel_engines[] = {
 		.instance = 0,
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
+		.capabilities = I915_VCS_CLASS_CAPABILITY_HEVC,
 	},
 	[VCS2] = {
 		.hw_id = VCS2_HW,
@@ -279,6 +281,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	engine->irq_shift = info->irq_shift;
 	engine->class = info->class;
 	engine->instance = info->instance;
+	engine->capabilities = info->capabilities;
 
 	engine->uabi_id = info->uabi_id;
 	engine->uabi_class = class_info->uabi_class;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 81cdbbf257ec..c73e1345f376 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -329,6 +329,9 @@ struct intel_engine_cs {
 
 	u8 class;
 	u8 instance;
+
+	u32 capabilities;
+
 	u32 context_size;
 	u32 mmio_base;
 	unsigned int irq_shift;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7f5634ce8e88..57b61dbda723 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1620,6 +1620,7 @@ struct drm_i915_perf_oa_config {
 struct drm_i915_query_item {
 	__u64 query_id;
 #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
 	/*
 	 * When set to zero by userspace, this is filled with the size of the
@@ -1717,6 +1718,49 @@ struct drm_i915_query_topology_info {
 	__u8 data[];
 };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not is physical engine
+ * only, or it can also be targetted from userspace, and what are its
+ * capabilities where applicable.
+ */
+struct drm_i915_engine_info {
+	/** Engine class as in enum drm_i915_gem_engine_class. */
+	__u8 class;
+
+	/** Engine instance number. */
+	__u8 instance;
+
+	/** Engine type flags. */
+	__u16 type;
+#define I915_ENGINE_TYPE_PHYSICAL	BIT(0)
+#define I915_ENGINE_TYPE_UAPI		BIT(1)
+
+	/** Capabilities of this engine. */
+	__u32 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	BIT(0)
+
+	__u32 rsvd[4];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all the engines known to the driver returning
+ * the 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.14.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Engine discovery query
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
@ 2018-03-14 14:44 ` Patchwork
  2018-03-14 16:40 ` [RFC] " Lionel Landwerlin
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-03-14 14:44 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

Series 39958v1 drm/i915: Engine discovery query
https://patchwork.freedesktop.org/api/1.0/series/39958/revisions/1/mbox/

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> FAIL       (fi-ivb-3520m) k.org#198519 +2
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

k.org#198519 https://bugzilla.kernel.org/show_bug.cgi?id=198519
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:435s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:300s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:513s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:517s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:507s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:413s
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:512s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:577s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:421s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:321s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:412s
fi-ivb-3520m     total:288  pass:256  dwarn:0   dfail:0   fail:3   skip:29  time:429s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:427s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:475s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:466s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:515s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:657s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:525s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:539s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:512s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:442s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:398s
Blacklisted hosts:
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:28  time:519s

62a7da6be4acf84decbacbdb8493d3357190cef3 drm-tip: 2018y-03m-14d-13h-52m-46s UTC integration manifest
106853548fb9 drm/i915: Engine discovery query

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] drm/i915: Engine discovery query
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
  2018-03-14 14:44 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-03-14 16:40 ` Lionel Landwerlin
  2018-03-14 17:53   ` Tvrtko Ursulin
  2018-03-14 18:18 ` Bloomfield, Jon
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Lionel Landwerlin @ 2018-03-14 16:40 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

Looks mostly good to me. I have a few comments below.

On 14/03/18 14:05, 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.
>
> 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>
> ---
> This is RFC for now since it is not very usable before the new execbuf API
> or virtual engine queue submission gets closer.
>
> In this version I have added capability of distinguishing between hardware
> engines and ABI engines. This is to account for probable future use cases
> like virtualization, where guest might only see one engine instance, but
> will want to know overall hardware capabilities in order to tune its
> behaviour.
>
> In general I think we will have to wait a bit before defining the final
> look of the uAPI, but in the meantime I thought it useful to share as RFC.
> ---
>   drivers/gpu/drm/i915/i915_query.c       | 61 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>   include/uapi/drm/i915_drm.h             | 44 ++++++++++++++++++++++++
>   4 files changed, 111 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 3ace929dd90f..e00d02796d42 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -82,9 +82,70 @@ 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 query, *q;
> +	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) +
> +	      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, u64_to_user_ptr(query_item->data_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, u64_to_user_ptr(query_item->data_ptr),
> +		       len))
> +		return -EFAULT;
> +
> +	q = kzalloc(len, GFP_KERNEL);
> +	if (!q)
> +		return -ENOMEM;

Do you actually need to kalloc?
Why not a drm_i915_engine_info on the stack that you copy to the right 
user pointer directly?

> +
> +	info = &q->engines[0];
> +
> +	for_each_engine(engine, i915, id) {
> +		info->class = engine->uabi_class;
> +		info->instance = engine->instance;
> +		info->type = I915_ENGINE_TYPE_PHYSICAL |
> +			     I915_ENGINE_TYPE_UAPI;
> +		info->capabilities = engine->capabilities;
> +		info++;
> +		q->num_engines++;
> +	}
> +
> +	if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr), q, len)) {
> +		len = -EFAULT;
> +		goto out;
> +	}
> +
> +out:
> +	kfree(q);
> +
> +	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 a2b1e9e2c008..81be4acd8358 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -86,6 +86,7 @@ struct engine_info {
>   	unsigned int uabi_id;
>   	u8 class;
>   	u8 instance;
> +	u32 capabilities;
>   	u32 mmio_base;
>   	unsigned irq_shift;
>   };
> @@ -114,6 +115,7 @@ static const struct engine_info intel_engines[] = {
>   		.instance = 0,
>   		.mmio_base = GEN6_BSD_RING_BASE,
>   		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
> +		.capabilities = I915_VCS_CLASS_CAPABILITY_HEVC,
>   	},
>   	[VCS2] = {
>   		.hw_id = VCS2_HW,
> @@ -279,6 +281,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>   	engine->irq_shift = info->irq_shift;
>   	engine->class = info->class;
>   	engine->instance = info->instance;
> +	engine->capabilities = info->capabilities;
>   
>   	engine->uabi_id = info->uabi_id;
>   	engine->uabi_class = class_info->uabi_class;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 81cdbbf257ec..c73e1345f376 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -329,6 +329,9 @@ struct intel_engine_cs {
>   
>   	u8 class;
>   	u8 instance;
> +
> +	u32 capabilities;
> +
>   	u32 context_size;
>   	u32 mmio_base;
>   	unsigned int irq_shift;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 7f5634ce8e88..57b61dbda723 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1620,6 +1620,7 @@ struct drm_i915_perf_oa_config {
>   struct drm_i915_query_item {
>   	__u64 query_id;
>   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
> +#define DRM_I915_QUERY_ENGINE_INFO	2
>   
>   	/*
>   	 * When set to zero by userspace, this is filled with the size of the
> @@ -1717,6 +1718,49 @@ struct drm_i915_query_topology_info {
>   	__u8 data[];
>   };
>   
> +/**
> + * struct drm_i915_engine_info
> + *
> + * Describes one engine known to the driver, whether or not is physical engine
> + * only, or it can also be targetted from userspace, and what are its
> + * capabilities where applicable.
> + */
> +struct drm_i915_engine_info {
> +	/** Engine class as in enum drm_i915_gem_engine_class. */
> +	__u8 class;
> +
> +	/** Engine instance number. */
> +	__u8 instance;
> +
> +	/** Engine type flags. */
> +	__u16 type;
> +#define I915_ENGINE_TYPE_PHYSICAL	BIT(0)
> +#define I915_ENGINE_TYPE_UAPI		BIT(1)

Type almost sounds equivalent to class, no?
Maybe just flags or property?
Maybe a 64bits is better suited?

> +
> +	/** Capabilities of this engine. */
> +	__u32 capabilities;
> +#define I915_VCS_CLASS_CAPABILITY_HEVC	BIT(0)

Consider a u64 here too?


> +
> +	__u32 rsvd[4];
> +};
> +
> +/**
> + * struct drm_i915_query_engine_info
> + *
> + * Engine info query enumerates all the engines known to the driver returning
> + * the 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] drm/i915: Engine discovery query
  2018-03-14 16:40 ` [RFC] " Lionel Landwerlin
@ 2018-03-14 17:53   ` Tvrtko Ursulin
  2018-03-14 17:58     ` Lionel Landwerlin
  0 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2018-03-14 17:53 UTC (permalink / raw)
  To: Lionel Landwerlin, Tvrtko Ursulin, Intel-gfx


On 14/03/2018 16:40, Lionel Landwerlin wrote:
> Looks mostly good to me. I have a few comments below.
> 
> On 14/03/18 14:05, 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.
>>
>> 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>
>> ---
>> This is RFC for now since it is not very usable before the new execbuf 
>> API
>> or virtual engine queue submission gets closer.
>>
>> In this version I have added capability of distinguishing between 
>> hardware
>> engines and ABI engines. This is to account for probable future use cases
>> like virtualization, where guest might only see one engine instance, but
>> will want to know overall hardware capabilities in order to tune its
>> behaviour.
>>
>> In general I think we will have to wait a bit before defining the final
>> look of the uAPI, but in the meantime I thought it useful to share as 
>> RFC.
>> ---
>>   drivers/gpu/drm/i915/i915_query.c       | 61 
>> +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_engine_cs.c  |  3 ++
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>>   include/uapi/drm/i915_drm.h             | 44 ++++++++++++++++++++++++
>>   4 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 3ace929dd90f..e00d02796d42 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -82,9 +82,70 @@ 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 query, *q;
>> +    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) +
>> +          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, u64_to_user_ptr(query_item->data_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, u64_to_user_ptr(query_item->data_ptr),
>> +               len))
>> +        return -EFAULT;
>> +
>> +    q = kzalloc(len, GFP_KERNEL);
>> +    if (!q)
>> +        return -ENOMEM;
> 
> Do you actually need to kalloc?
> Why not a drm_i915_engine_info on the stack that you copy to the right 
> user pointer directly?

Yep, that will make it simpler.

>> +
>> +    info = &q->engines[0];
>> +
>> +    for_each_engine(engine, i915, id) {
>> +        info->class = engine->uabi_class;
>> +        info->instance = engine->instance;
>> +        info->type = I915_ENGINE_TYPE_PHYSICAL |
>> +                 I915_ENGINE_TYPE_UAPI;
>> +        info->capabilities = engine->capabilities;
>> +        info++;
>> +        q->num_engines++;
>> +    }
>> +
>> +    if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr), q, len)) {
>> +        len = -EFAULT;
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    kfree(q);
>> +
>> +    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 a2b1e9e2c008..81be4acd8358 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -86,6 +86,7 @@ struct engine_info {
>>       unsigned int uabi_id;
>>       u8 class;
>>       u8 instance;
>> +    u32 capabilities;
>>       u32 mmio_base;
>>       unsigned irq_shift;
>>   };
>> @@ -114,6 +115,7 @@ static const struct engine_info intel_engines[] = {
>>           .instance = 0,
>>           .mmio_base = GEN6_BSD_RING_BASE,
>>           .irq_shift = GEN8_VCS1_IRQ_SHIFT,
>> +        .capabilities = I915_VCS_CLASS_CAPABILITY_HEVC,
>>       },
>>       [VCS2] = {
>>           .hw_id = VCS2_HW,
>> @@ -279,6 +281,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>>       engine->irq_shift = info->irq_shift;
>>       engine->class = info->class;
>>       engine->instance = info->instance;
>> +    engine->capabilities = info->capabilities;
>>       engine->uabi_id = info->uabi_id;
>>       engine->uabi_class = class_info->uabi_class;
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 81cdbbf257ec..c73e1345f376 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -329,6 +329,9 @@ struct intel_engine_cs {
>>       u8 class;
>>       u8 instance;
>> +
>> +    u32 capabilities;
>> +
>>       u32 context_size;
>>       u32 mmio_base;
>>       unsigned int irq_shift;
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 7f5634ce8e88..57b61dbda723 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -1620,6 +1620,7 @@ struct drm_i915_perf_oa_config {
>>   struct drm_i915_query_item {
>>       __u64 query_id;
>>   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
>> +#define DRM_I915_QUERY_ENGINE_INFO    2
>>       /*
>>        * When set to zero by userspace, this is filled with the size 
>> of the
>> @@ -1717,6 +1718,49 @@ struct drm_i915_query_topology_info {
>>       __u8 data[];
>>   };
>> +/**
>> + * struct drm_i915_engine_info
>> + *
>> + * Describes one engine known to the driver, whether or not is 
>> physical engine
>> + * only, or it can also be targetted from userspace, and what are its
>> + * capabilities where applicable.
>> + */
>> +struct drm_i915_engine_info {
>> +    /** Engine class as in enum drm_i915_gem_engine_class. */
>> +    __u8 class;
>> +
>> +    /** Engine instance number. */
>> +    __u8 instance;
>> +
>> +    /** Engine type flags. */
>> +    __u16 type;
>> +#define I915_ENGINE_TYPE_PHYSICAL    BIT(0)
>> +#define I915_ENGINE_TYPE_UAPI        BIT(1)
> 
> Type almost sounds equivalent to class, no?

I had the same dilemma. In essence I wanted to have a split between flag 
namespaces for things common to all engines, and things specific to 
engine classes. I thought it would be cleaner like that.

So flags and capabilities?

> Maybe just flags or property?

For pure renaming of type to flags would make sense from the point of 
view of being more generic. So I agree with that.

> Maybe a 64bits is better suited?

Yes, I was thinking about it. u16 had appeal of plugging the hole after 
class and instance. And I couldn't make myself use u32 for those.

Maybe I reorder it like:

	u32 flags;
	u16 class;
	u16 instance;
	u32/u64 capabilities;
	u32 rdsvd[some];

> 
>> +
>> +    /** Capabilities of this engine. */
>> +    __u32 capabilities;
>> +#define I915_VCS_CLASS_CAPABILITY_HEVC    BIT(0)
> 
> Consider a u64 here too?

Could do. But I'm guesstimating 32 flags should be enough, especially 
since their namespace is per class. But can do u64 if that will be the 
consensus.

Regards,

Tvrtko

> 
>> +
>> +    __u32 rsvd[4];
>> +};
>> +
>> +/**
>> + * struct drm_i915_query_engine_info
>> + *
>> + * Engine info query enumerates all the engines known to the driver 
>> returning
>> + * the 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] drm/i915: Engine discovery query
  2018-03-14 17:53   ` Tvrtko Ursulin
@ 2018-03-14 17:58     ` Lionel Landwerlin
  0 siblings, 0 replies; 14+ messages in thread
From: Lionel Landwerlin @ 2018-03-14 17:58 UTC (permalink / raw)
  To: Tvrtko Ursulin, Tvrtko Ursulin, Intel-gfx

On 14/03/18 17:53, Tvrtko Ursulin wrote:
>
> Maybe I reorder it like:
>
>     u32 flags;
>     u16 class;
>     u16 instance;
>     u32/u64 capabilities;
>     u32 rdsvd[some]; 

Looks good :)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] drm/i915: Engine discovery query
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
  2018-03-14 14:44 ` ✓ Fi.CI.BAT: success for " Patchwork
  2018-03-14 16:40 ` [RFC] " Lionel Landwerlin
@ 2018-03-14 18:18 ` Bloomfield, Jon
  2018-03-14 19:34 ` ✓ Fi.CI.IGT: success for " Patchwork
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Bloomfield, Jon @ 2018-03-14 18:18 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tursulin@ursulin.net]
> Sent: Wednesday, March 14, 2018 7:06 AM
> To: Intel-gfx@lists.freedesktop.org
> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris
> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> <jon.bloomfield@intel.com>; Rogozhkin, Dmitry V
> <dmitry.v.rogozhkin@intel.com>; Landwerlin, Lionel G
> <lionel.g.landwerlin@intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>
> Subject: [RFC] drm/i915: Engine discovery query
> 
> 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.
> 
> 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>
> ---
> This is RFC for now since it is not very usable before the new execbuf API
> or virtual engine queue submission gets closer.
> 
> In this version I have added capability of distinguishing between hardware
> engines and ABI engines. This is to account for probable future use cases
> like virtualization, where guest might only see one engine instance, but
> will want to know overall hardware capabilities in order to tune its
> behaviour.

The patch looks good, but...

I can't see any use for exposing the unreachable engines. Can you elaborate on why a umd running
in a VM would need to know about the engines assigned to other VM's ? Is it even desirable to
leak the physical capabilities to VM's ?

In general a VM would have a very limited view of the underlying hardware. It's unlikely to even
be capable of discovering true h/w engine counts.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Engine discovery query
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
                   ` (2 preceding siblings ...)
  2018-03-14 18:18 ` Bloomfield, Jon
@ 2018-03-14 19:34 ` Patchwork
  2018-04-11  9:46 ` [RFC v2] " Tvrtko Ursulin
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-03-14 19:34 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

---- Known issues:

Test kms_flip:
        Subgroup 2x-plain-flip-fb-recreate-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368

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

shard-apl        total:3401 pass:1795 dwarn:1   dfail:0   fail:7   skip:1596 time:12800s
shard-hsw        total:3444 pass:1769 dwarn:1   dfail:0   fail:2   skip:1671 time:11958s
shard-snb        total:3444 pass:1360 dwarn:1   dfail:0   fail:2   skip:2081 time:7290s

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC v2] drm/i915: Engine discovery query
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
                   ` (3 preceding siblings ...)
  2018-03-14 19:34 ` ✓ Fi.CI.IGT: success for " Patchwork
@ 2018-04-11  9:46 ` Tvrtko Ursulin
  2018-04-11 16:32   ` Lionel Landwerlin
  2018-04-11 11:33 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev2) Patchwork
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2018-04-11  9:46 UTC (permalink / raw)
  To: Intel-gfx

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)

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>
---
This is RFC for now since it is not very usable before the new execbuf API
or virtual engine queue submission gets closer.

In this version I have added capability of distinguishing between hardware
engines and ABI engines. This is to account for probable future use cases
like virtualization, where guest might only see one engine instance, but
will want to know overall hardware capabilities in order to tune its
behaviour.

In general I think we will have to wait a bit before defining the final
look of the uAPI, but in the meantime I thought it useful to share as RFC.
---
 drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c  |  2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
 include/uapi/drm/i915_drm.h             | 46 ++++++++++++++++++++++++
 4 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 3ace929dd90f..a1c0e3acc882 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -82,9 +82,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 12486d8f534b..9d26c34dfec4 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -294,6 +294,8 @@ 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 (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+		engine->capabilities = I915_VCS_CLASS_CAPABILITY_HEVC;
 
 	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 256d58487559..b23be52fc882 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -330,6 +330,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 7f5634ce8e88..2eafb677cd40 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1620,6 +1620,7 @@ struct drm_i915_perf_oa_config {
 struct drm_i915_query_item {
 	__u64 query_id;
 #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_ENGINE_INFO	2
 
 	/*
 	 * When set to zero by userspace, this is filled with the size of the
@@ -1717,6 +1718,51 @@ struct drm_i915_query_topology_info {
 	__u8 data[];
 };
 
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine known to the driver, whether or not is physical engine
+ * only, or it can also be targetted from userspace, and what are its
+ * capabilities where applicable.
+ */
+struct drm_i915_engine_info {
+	/** Engine flags. */
+	__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;
+
+	__u32 rsvd0;
+
+	/** Capabilities of this engine. */
+	__u64 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	(1 << 0)
+
+	__u64 rsvd1[2];
+};
+
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all the engines known to the driver returning
+ * the 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.14.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev2)
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
                   ` (4 preceding siblings ...)
  2018-04-11  9:46 ` [RFC v2] " Tvrtko Ursulin
@ 2018-04-11 11:33 ` Patchwork
  2018-04-11 11:49 ` ✓ Fi.CI.BAT: success " Patchwork
  2018-04-11 13:53 ` ✓ Fi.CI.IGT: " Patchwork
  7 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-04-11 11:33 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
487fa7f5e720 drm/i915: Engine discovery query
-:164: WARNING:TYPO_SPELLING: 'targetted' may be misspelled - perhaps 'targeted'?
#164: FILE: include/uapi/drm/i915_drm.h:1725:
+ * only, or it can also be targetted from userspace, and what are its

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Engine discovery query (rev2)
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
                   ` (5 preceding siblings ...)
  2018-04-11 11:33 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev2) Patchwork
@ 2018-04-11 11:49 ` Patchwork
  2018-04-11 13:53 ` ✓ Fi.CI.IGT: " Patchwork
  7 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-04-11 11:49 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4044 -> Patchwork_8665 =

== Summary - WARNING ==

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8665/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         PASS -> FAIL (fdo#102575)

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


== Participating hosts (34 -> 31) ==

  Missing    (3): fi-ilk-m540 fi-glk-j4005 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4044 -> Patchwork_8665

  CI_DRM_4044: bb98639a2babe9d26edca8ea44c3e9f968c6c30f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4421: 27cd3793a3a183e4ec70c392f293e840642f2799 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8665: 487fa7f5e720e1e791ab688204c0285a9270774d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4421: 45e115f293fd6acc0c9647cf2d3b76be78819ba5 @ git://anongit.freedesktop.org/piglit


== Patchwork commits ==

487fa7f5e720 drm/i915: Engine discovery query

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Engine discovery query (rev2)
  2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
                   ` (6 preceding siblings ...)
  2018-04-11 11:49 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-04-11 13:53 ` Patchwork
  7 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-04-11 13:53 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4044_full -> Patchwork_8665_full =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8665/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@flip-vs-expired-vblank:
      shard-hsw:          PASS -> FAIL (fdo#105189)

    igt@kms_flip@wf_vblank-ts-check:
      shard-hsw:          PASS -> FAIL (fdo#103928)

    
    ==== Possible fixes ====

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-hsw:          FAIL (fdo#103928) -> PASS +1

    igt@kms_rotation_crc@primary-rotation-180:
      shard-hsw:          FAIL (fdo#103925) -> PASS

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

    igt@perf_pmu@busy-accuracy-50-bcs0:
      shard-kbl:          FAIL (fdo#105157) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105157 https://bugs.freedesktop.org/show_bug.cgi?id=105157
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  Missing    (2): shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4044 -> Patchwork_8665

  CI_DRM_4044: bb98639a2babe9d26edca8ea44c3e9f968c6c30f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4421: 27cd3793a3a183e4ec70c392f293e840642f2799 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8665: 487fa7f5e720e1e791ab688204c0285a9270774d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4421: 45e115f293fd6acc0c9647cf2d3b76be78819ba5 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC v2] drm/i915: Engine discovery query
  2018-04-11  9:46 ` [RFC v2] " Tvrtko Ursulin
@ 2018-04-11 16:32   ` Lionel Landwerlin
  2018-04-16 10:11     ` Tvrtko Ursulin
  0 siblings, 1 reply; 14+ messages in thread
From: Lionel Landwerlin @ 2018-04-11 16:32 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

Looks good to me, a few nits below.
I'll try to find some time to display this information in gputop.

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

On 11/04/18 02:46, 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)
>
> 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>
> ---
> This is RFC for now since it is not very usable before the new execbuf API
> or virtual engine queue submission gets closer.
>
> In this version I have added capability of distinguishing between hardware
> engines and ABI engines. This is to account for probable future use cases
> like virtualization, where guest might only see one engine instance, but
> will want to know overall hardware capabilities in order to tune its
> behaviour.
>
> In general I think we will have to wait a bit before defining the final
> look of the uAPI, but in the meantime I thought it useful to share as RFC.
> ---
>   drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_engine_cs.c  |  2 ++
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>   include/uapi/drm/i915_drm.h             | 46 ++++++++++++++++++++++++
>   4 files changed, 114 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 3ace929dd90f..a1c0e3acc882 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -82,9 +82,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))

You could limit this to len ?

> +		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 12486d8f534b..9d26c34dfec4 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -294,6 +294,8 @@ 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 (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
> +		engine->capabilities = I915_VCS_CLASS_CAPABILITY_HEVC;
>   
>   	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 256d58487559..b23be52fc882 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -330,6 +330,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 7f5634ce8e88..2eafb677cd40 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1620,6 +1620,7 @@ struct drm_i915_perf_oa_config {
>   struct drm_i915_query_item {
>   	__u64 query_id;
>   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
> +#define DRM_I915_QUERY_ENGINE_INFO	2
>   
>   	/*
>   	 * When set to zero by userspace, this is filled with the size of the
> @@ -1717,6 +1718,51 @@ struct drm_i915_query_topology_info {
>   	__u8 data[];
>   };
>   
> +/**
> + * struct drm_i915_engine_info
> + *
> + * Describes one engine known to the driver, whether or not is physical engine
> + * only, or it can also be targetted from userspace, and what are its
> + * capabilities where applicable.
> + */
> +struct drm_i915_engine_info {
> +	/** Engine flags. */
> +	__u64 flags;
> +#define I915_ENGINE_FLAG_PHYSICAL	(1 << 0)
> +#define I915_ENGINE_FLAG_ABI		(1 << 1)

Could you describe the meaning for ABI?

> +
> +	/** Engine class as in enum drm_i915_gem_engine_class. */
> +	__u16 class;
> +
> +	/** Engine instance number. */
> +	__u16 instance;
> +
MBZ
> +	__u32 rsvd0;
> +
> +	/** Capabilities of this engine. */
> +	__u64 capabilities;
> +#define I915_VCS_CLASS_CAPABILITY_HEVC	(1 << 0)
> +
MBZ
> +	__u64 rsvd1[2];
> +};
> +
> +/**
> + * struct drm_i915_query_engine_info
> + *
> + * Engine info query enumerates all the engines known to the driver returning
> + * the 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC v2] drm/i915: Engine discovery query
  2018-04-11 16:32   ` Lionel Landwerlin
@ 2018-04-16 10:11     ` Tvrtko Ursulin
  2018-04-16 14:50       ` Lionel Landwerlin
  0 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2018-04-16 10:11 UTC (permalink / raw)
  To: Lionel Landwerlin, Tvrtko Ursulin, Intel-gfx


On 11/04/2018 17:32, Lionel Landwerlin wrote:
> Looks good to me, a few nits below.
> I'll try to find some time to display this information in gputop.
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Thanks!

> On 11/04/18 02:46, 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)
>>
>> 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>
>> ---
>> This is RFC for now since it is not very usable before the new execbuf 
>> API
>> or virtual engine queue submission gets closer.
>>
>> In this version I have added capability of distinguishing between 
>> hardware
>> engines and ABI engines. This is to account for probable future use cases
>> like virtualization, where guest might only see one engine instance, but
>> will want to know overall hardware capabilities in order to tune its
>> behaviour.
>>
>> In general I think we will have to wait a bit before defining the final
>> look of the uAPI, but in the meantime I thought it useful to share as 
>> RFC.
>> ---
>>   drivers/gpu/drm/i915/i915_query.c       | 63 
>> +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_engine_cs.c  |  2 ++
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>>   include/uapi/drm/i915_drm.h             | 46 ++++++++++++++++++++++++
>>   4 files changed, 114 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 3ace929dd90f..a1c0e3acc882 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -82,9 +82,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))
> 
> You could limit this to len ?

I could, I am just wondering if you are leading somewhere with this 
suggestion like something I missed?

Check against query_item->length has the semantics of "you told me you 
are giving me this big of a buffer, and I'll check if you were lying 
even if I won't use it all", versus check against len would be "I'll 
check only the part I'll write into, even if you told me buffer is bigger".

The current check is a protecting userspace against more mistakes, but 
perhaps you are thinking about some tricks which would be possible by 
just checking len?

> 
>> +        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 12486d8f534b..9d26c34dfec4 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -294,6 +294,8 @@ 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 (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
>> +        engine->capabilities = I915_VCS_CLASS_CAPABILITY_HEVC;
>>       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 256d58487559..b23be52fc882 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -330,6 +330,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 7f5634ce8e88..2eafb677cd40 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -1620,6 +1620,7 @@ struct drm_i915_perf_oa_config {
>>   struct drm_i915_query_item {
>>       __u64 query_id;
>>   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
>> +#define DRM_I915_QUERY_ENGINE_INFO    2
>>       /*
>>        * When set to zero by userspace, this is filled with the size 
>> of the
>> @@ -1717,6 +1718,51 @@ struct drm_i915_query_topology_info {
>>       __u8 data[];
>>   };
>> +/**
>> + * struct drm_i915_engine_info
>> + *
>> + * Describes one engine known to the driver, whether or not is 
>> physical engine
>> + * only, or it can also be targetted from userspace, and what are its
>> + * capabilities where applicable.
>> + */
>> +struct drm_i915_engine_info {
>> +    /** Engine flags. */
>> +    __u64 flags;
>> +#define I915_ENGINE_FLAG_PHYSICAL    (1 << 0)
>> +#define I915_ENGINE_FLAG_ABI        (1 << 1)
> 
> Could you describe the meaning for ABI?

Must do that explicitly for each, you are right.

> 
>> +
>> +    /** Engine class as in enum drm_i915_gem_engine_class. */
>> +    __u16 class;
>> +
>> +    /** Engine instance number. */
>> +    __u16 instance;
>> +
> MBZ
>> +    __u32 rsvd0;
>> +
>> +    /** Capabilities of this engine. */
>> +    __u64 capabilities;
>> +#define I915_VCS_CLASS_CAPABILITY_HEVC    (1 << 0)
>> +
> MBZ

Deja vu.. :) marking as TODO for both.

Thanks for reading it through.

Regards,

Tvrtko

>> +    __u64 rsvd1[2];
>> +};
>> +
>> +/**
>> + * struct drm_i915_query_engine_info
>> + *
>> + * Engine info query enumerates all the engines known to the driver 
>> returning
>> + * the 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC v2] drm/i915: Engine discovery query
  2018-04-16 10:11     ` Tvrtko Ursulin
@ 2018-04-16 14:50       ` Lionel Landwerlin
  0 siblings, 0 replies; 14+ messages in thread
From: Lionel Landwerlin @ 2018-04-16 14:50 UTC (permalink / raw)
  To: Tvrtko Ursulin, Tvrtko Ursulin, Intel-gfx

On 16/04/18 03:11, Tvrtko Ursulin wrote:
>
> On 11/04/2018 17:32, Lionel Landwerlin wrote:
>> Looks good to me, a few nits below.
>> I'll try to find some time to display this information in gputop.
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> Thanks!
>
>> On 11/04/18 02:46, 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)
>>>
>>> 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>
>>> ---
>>> This is RFC for now since it is not very usable before the new 
>>> execbuf API
>>> or virtual engine queue submission gets closer.
>>>
>>> In this version I have added capability of distinguishing between 
>>> hardware
>>> engines and ABI engines. This is to account for probable future use 
>>> cases
>>> like virtualization, where guest might only see one engine instance, 
>>> but
>>> will want to know overall hardware capabilities in order to tune its
>>> behaviour.
>>>
>>> In general I think we will have to wait a bit before defining the final
>>> look of the uAPI, but in the meantime I thought it useful to share 
>>> as RFC.
>>> ---
>>>   drivers/gpu/drm/i915/i915_query.c       | 63 
>>> +++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_engine_cs.c  |  2 ++
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>>>   include/uapi/drm/i915_drm.h             | 46 ++++++++++++++++++++++++
>>>   4 files changed, 114 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>>> b/drivers/gpu/drm/i915/i915_query.c
>>> index 3ace929dd90f..a1c0e3acc882 100644
>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>> @@ -82,9 +82,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))
>>
>> You could limit this to len ?
>
> I could, I am just wondering if you are leading somewhere with this 
> suggestion like something I missed?
>
> Check against query_item->length has the semantics of "you told me you 
> are giving me this big of a buffer, and I'll check if you were lying 
> even if I won't use it all", versus check against len would be "I'll 
> check only the part I'll write into, even if you told me buffer is 
> bigger".
>
> The current check is a protecting userspace against more mistakes, but 
> perhaps you are thinking about some tricks which would be possible by 
> just checking len?

Cool, looks like you thought about everything :)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-04-16 14:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-14 14:05 [RFC] drm/i915: Engine discovery query Tvrtko Ursulin
2018-03-14 14:44 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-03-14 16:40 ` [RFC] " Lionel Landwerlin
2018-03-14 17:53   ` Tvrtko Ursulin
2018-03-14 17:58     ` Lionel Landwerlin
2018-03-14 18:18 ` Bloomfield, Jon
2018-03-14 19:34 ` ✓ Fi.CI.IGT: success for " Patchwork
2018-04-11  9:46 ` [RFC v2] " Tvrtko Ursulin
2018-04-11 16:32   ` Lionel Landwerlin
2018-04-16 10:11     ` Tvrtko Ursulin
2018-04-16 14:50       ` Lionel Landwerlin
2018-04-11 11:33 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine discovery query (rev2) Patchwork
2018-04-11 11:49 ` ✓ Fi.CI.BAT: success " Patchwork
2018-04-11 13:53 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.