[Mesa-dev] [RFC v3] drm/i915: Select engines via class and instance in execbuffer2

Chris Wilson chris at chris-wilson.co.uk
Wed May 17 16:44:08 UTC 2017


On Wed, May 17, 2017 at 04:40:57PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
> 
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
> 
> Proposed idea here is to reserve 16-bits of flags, to pass in
> the engine class and instance (8 bits each), and a new flag
> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> selection API is in use.
> 
> The new uAPI also removes access to the weak VCS engine
> balancing as currently existing in the driver.
> 
> Example usage to send a command to VCS0:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> 
> Or to send a command to VCS1:
> 
>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
> 
> v2:
>  * Fix unknown flags mask.
>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
> 
> v3:
>  * Add a map for fast class-instance engine lookup. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
> Cc: Daniel Charles <daniel.charles at intel.com>
> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong at intel.com>
> Cc: intel-vaapi-media at lists.01.org
> Cc: mesa-dev at lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  3 +++
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  7 +++++++
>  include/uapi/drm/i915_drm.h                | 11 ++++++++++-
>  5 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5dfa4a12e647..7bf4fd42480c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2066,6 +2066,7 @@ struct drm_i915_private {
>  	struct pci_dev *bridge_dev;
>  	struct i915_gem_context *kernel_context;
>  	struct intel_engine_cs *engine[I915_NUM_ENGINES];
> +	struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1][MAX_ENGINE_INSTANCE + 1];
>  	struct i915_vma *semaphore;
>  
>  	struct drm_dma_handle *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index af1965774e7b..c1ad49ab64cd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1492,6 +1492,33 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>  	return file_priv->bsd_engine;
>  }
>  
> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
> +
> +static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
> +						u8 class, u8 instance)
> +{
> +	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
> +		return NULL;
> +
> +	return i915->engine_class[class][instance];
> +}

Be bold make this this an intel_engine_lookup(), I forsee some other
users appearing very shortly.

> +static struct intel_engine_cs *
> +eb_select_engine_class_instance(struct drm_i915_private *i915,
> +				struct drm_i915_gem_execbuffer2 *args)
> +{
> +	u8 class, instance;
> +
> +	class = args->flags & I915_EXEC_RING_MASK;
> +	if (class >= DRM_I915_ENGINE_CLASS_MAX)
> +		return NULL;
> +
> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> +		   I915_EXEC_INSTANCE_MASK;
> +
> +	return get_engine_class(i915, user_class_map[class], instance);
> +}
> +
>  #define I915_USER_RINGS (4)
>  
>  static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> @@ -1510,6 +1537,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
>  	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
>  	struct intel_engine_cs *engine;
>  
> +	if (args->flags & I915_EXEC_CLASS_INSTANCE)
> +		return eb_select_engine_class_instance(dev_priv, args);
> +
>  	if (user_ring_id > I915_USER_RINGS) {
>  		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
>  		return NULL;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ee144ec57935..a3b59043b991 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define VIDEO_ENHANCEMENT_CLASS	2
>  #define COPY_ENGINE_CLASS	3
>  #define OTHER_CLASS		4
> +#define MAX_ENGINE_CLASS	4
> +
> +#define MAX_ENGINE_INSTANCE	1

We also need the names in the uapi.h as well. Do we want to mention
OTHER_CLASS before it is defined? I trust your crystal ball.

Amusingly I notice that you do claim that you have these names in the
changelog. :) We don't need DRM_I915 all the time. I915_ENGINE_CLASS is
going to be unique, at least for those users of i915_drm.h

>  /* PCI config space */
>  
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 7566cf48012f..c5ad51c43d23 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>  
>  	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
>  
> +	if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
> +		    info->instance > MAX_ENGINE_INSTANCE ||
> +		    dev_priv->engine_class[info->class][info->instance]))

Spread these out or add a message telling what was wrong.

> +		return -EINVAL;
> +
> +	dev_priv->engine_class[info->class][info->instance] = engine;
>  	dev_priv->engine[id] = engine;
> +
>  	return 0;
>  }
>  
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2ac6667e57ea..6a26bdf5e684 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
>   */
>  #define I915_EXEC_FENCE_OUT		(1<<17)
>  
> -#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
> +#define I915_EXEC_CLASS_INSTANCE	(1<<18)
> +
> +#define I915_EXEC_INSTANCE_SHIFT	(19)
> +#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
> +
> +#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
>  
>  #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
>  #define i915_execbuffer2_set_context_id(eb2, context) \
> @@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
>  #define i915_execbuffer2_get_context_id(eb2) \
>  	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
>  
> +#define i915_execbuffer2_engine(class, instance) \
> +	(I915_EXEC_CLASS_INSTANCE | (class) | \
> +	((instance) << I915_EXEC_INSTANCE_SHIFT))

(class |
 (instance) << I915_EXEC_INSTANCE_SHIFT |
 I915_EXEC_CLASS_INSTANCE)

Just suggesting to spread it out over another line for better
readability.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre


More information about the mesa-dev mailing list