diff mbox

[RFC] drm/i915: Engine discovery query

Message ID 20180314140539.9287-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin March 14, 2018, 2:05 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@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(+)

Comments

Lionel Landwerlin March 14, 2018, 4:40 p.m. UTC | #1
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@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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@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
Tvrtko Ursulin March 14, 2018, 5:53 p.m. UTC | #2
On 14/03/2018 16:40, Lionel Landwerlin wrote:
> 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@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@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@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?

Yep, that will make it simpler.

>> +
>> +    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?

I had the same dilemma. In essence I wanted to have a split between flag 
namespaces for things common to all engines, and things specific to 
engine classes. I thought it would be cleaner like that.

So flags and capabilities?

> Maybe just flags or property?

For pure renaming of type to flags would make sense from the point of 
view of being more generic. So I agree with that.

> Maybe a 64bits is better suited?

Yes, I was thinking about it. u16 had appeal of plugging the hole after 
class and instance. And I couldn't make myself use u32 for those.

Maybe I reorder it like:

	u32 flags;
	u16 class;
	u16 instance;
	u32/u64 capabilities;
	u32 rdsvd[some];

> 
>> +
>> +    /** Capabilities of this engine. */
>> +    __u32 capabilities;
>> +#define I915_VCS_CLASS_CAPABILITY_HEVC    BIT(0)
> 
> Consider a u64 here too?

Could do. But I'm guesstimating 32 flags should be enough, especially 
since their namespace is per class. But can do u64 if that will be the 
consensus.

Regards,

Tvrtko

> 
>> +
>> +    __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
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lionel Landwerlin March 14, 2018, 5:58 p.m. UTC | #3
On 14/03/18 17:53, Tvrtko Ursulin wrote:
>
> Maybe I reorder it like:
>
>     u32 flags;
>     u16 class;
>     u16 instance;
>     u32/u64 capabilities;
>     u32 rdsvd[some]; 

Looks good :)
Bloomfield, Jon March 14, 2018, 6:18 p.m. UTC | #4
> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tursulin@ursulin.net]
> Sent: Wednesday, March 14, 2018 7:06 AM
> To: Intel-gfx@lists.freedesktop.org
> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris
> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon
> <jon.bloomfield@intel.com>; Rogozhkin, Dmitry V
> <dmitry.v.rogozhkin@intel.com>; Landwerlin, Lionel G
> <lionel.g.landwerlin@intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>
> Subject: [RFC] drm/i915: Engine discovery query
> 
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@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.

The patch looks good, but...

I can't see any use for exposing the unreachable engines. Can you elaborate on why a umd running
in a VM would need to know about the engines assigned to other VM's ? Is it even desirable to
leak the physical capabilities to VM's ?

In general a VM would have a very limited view of the underlying hardware. It's unlikely to even
be capable of discovering true h/w engine counts.
diff mbox

Patch

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;
+
+	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)
+
+	/** Capabilities of this engine. */
+	__u32 capabilities;
+#define I915_VCS_CLASS_CAPABILITY_HEVC	BIT(0)
+
+	__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