Re: [PATCH] drm/i915: Engine discovery query

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

Media experts please scroll down...

On 01/10/2018 17:15, 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.
> 
> 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.
> 
> 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>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_query.c       | 63 +++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_engine_cs.c  |  9 ++++
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>   include/uapi/drm/i915_drm.h             | 54 +++++++++++++++++++++
>   4 files changed, 129 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..294f8195efa7 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -84,9 +84,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 1c6143bdf5a4..97b4acf8af5c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -294,6 +294,15 @@ 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 (engine->class == VIDEO_DECODE_CLASS) {
> +		/* HEVC support is present only on vcs0. */
> +		if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
> +			engine->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->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC;
> +	}

... to here! :)

I need help with the above two capabilities assignments. Primarily are 
they correct? HEVC situation might be different on Gen11 for instance?

Otherwise it is basically the same patch I sent months ago so 
essentially sending it again just for another review pass, if someone 
will have some new comments.

Regards,

Tvrtko

>   
>   	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 1534de5bb852..90cf4eea0de2 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 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..c4292e5fed52 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,59 @@ 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 flags.
> +	 *
> +	 * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware
> +	 * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf
> +	 */
> +	__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;
> +
> +	/** Reserved field must be cleared to zero. */
> +	__u32 rsvd0;
> +
> +	/** 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[2];
> +};
> +
> +/**
> + * 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx