[Intel-gfx] [RFC] drm/i915: Engine discovery query
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Wed Mar 14 16:40:29 UTC 2018
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 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.
>
> 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>
> ---
> 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
More information about the Intel-gfx
mailing list