[Intel-gfx] [RFC v2] drm/i915: Engine discovery query
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Apr 16 10:11:32 UTC 2018
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 at intel.com>
Thanks!
> On 11/04/18 02:46, 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)
>>
>> 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 | 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list