[Intel-gfx] [PATCH v6] drm/i915: Engine discovery query

Lionel Landwerlin lionel.g.landwerlin at intel.com
Thu Oct 4 11:10:50 UTC 2018


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

-
Lionel

On 04/10/2018 13:03, Lionel Landwerlin wrote:
> On 04/10/2018 12:51, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin 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
>
>
> _______________________________________________
> 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