[Intel-gfx] [PATCH v6] drm/i915: Engine discovery query
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Thu Oct 4 11:03:20 UTC 2018
On 04/10/2018 12:51, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Tony Ye <tony.ye at 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
More information about the Intel-gfx
mailing list