[RFC,v2] drm/i915: Engine discovery query

Commit Message

Tvrtko Ursulin April 11, 2018, 9:46 a.m. UTC

From: Tvrtko Ursulin <[e-mail]>

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 <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Jon Bloomfield <[e-mail]>
Cc: Dmitry Rogozhkin <[e-mail]>
Cc: Lionel Landwerlin <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
---
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(+)

Comments

Chris Wilson April 11, 2018, 10:27 a.m. UTC | #1

Quoting Tvrtko Ursulin (2018-04-11 10:46:31)
> +/**
> + * 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];

Ok, I can understand having some reserved fields before the [], but
within struct drm_i915_engine_info isn't the size and layout determined
by flags? (Thinking about the tight protocol packing of the 80s and
slightly less tight packing for the 90s ;) Trade-off between ease of
decoder and running out of space?
-Chris

Tvrtko Ursulin April 11, 2018, 11:10 a.m. UTC | #2

On 11/04/2018 11:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-04-11 10:46:31)
>> +/**
>> + * 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];
> 
> Ok, I can understand having some reserved fields before the [], but
> within struct drm_i915_engine_info isn't the size and layout determined
> by flags? (Thinking about the tight protocol packing of the 80s and
> slightly less tight packing for the 90s ;) Trade-off between ease of
> decoder and running out of space?

How did that old saying go, was it "When in doubt, pad it out."? :)

But I think it is good to have some reserved fields rather than rely on 
flags to extend the size of the structure. In any case I don't see a 
significant downside to reserved fields.

Regards,

Tvrtko

Chris Wilson April 11, 2018, 11:15 a.m. UTC | #3

Quoting Tvrtko Ursulin (2018-04-11 12:10:05)
> 
> On 11/04/2018 11:27, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-04-11 10:46:31)
> >> +/**
> >> + * 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];
> > 
> > Ok, I can understand having some reserved fields before the [], but
> > within struct drm_i915_engine_info isn't the size and layout determined
> > by flags? (Thinking about the tight protocol packing of the 80s and
> > slightly less tight packing for the 90s ;) Trade-off between ease of
> > decoder and running out of space?
> 
> How did that old saying go, was it "When in doubt, pad it out."? :)
> 
> But I think it is good to have some reserved fields rather than rely on 
> flags to extend the size of the structure. In any case I don't see a 
> significant downside to reserved fields.

The only downside is running and starting with an new query-id. We've
only 64b of them!
-Chris

Lionel Landwerlin April 11, 2018, 4:32 p.m. UTC | #4

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 <[e-mail]>

On 11/04/18 02:46, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Jon Bloomfield <[e-mail]>
> Cc: Dmitry Rogozhkin <[e-mail]>
> Cc: Lionel Landwerlin <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>
> ---
> 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

Tvrtko Ursulin April 16, 2018, 10:11 a.m. UTC | #5

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 <[e-mail]>

Thanks!

> On 11/04/18 02:46, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Chris Wilson <[e-mail]>
>> Cc: Jon Bloomfield <[e-mail]>
>> Cc: Dmitry Rogozhkin <[e-mail]>
>> Cc: Lionel Landwerlin <[e-mail]>
>> Cc: Joonas Lahtinen <[e-mail]>
>> ---
>> 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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lionel Landwerlin April 16, 2018, 2:50 p.m. UTC | #6

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 <[e-mail]>
>
> Thanks!
>
>> On 11/04/18 02:46, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Cc: Chris Wilson <[e-mail]>
>>> Cc: Jon Bloomfield <[e-mail]>
>>> Cc: Dmitry Rogozhkin <[e-mail]>
>>> Cc: Lionel Landwerlin <[e-mail]>
>>> Cc: Joonas Lahtinen <[e-mail]>
>>> ---
>>> 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 :)

Patch

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