[v2] drm/i915: Decouple execbuf uAPI from internal implementation
diff mbox

Message ID 1452870770-13981-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Jan. 15, 2016, 3:12 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

At the moment execbuf ring selection is fully coupled to
internal ring ids which is not a good thing on its own.

This dependency is also spread between two source files and
not spelled out at either side which makes it hidden and
fragile.

This patch decouples this dependency by introducing an explicit
translation table of execbuf uAPI to ring id close to the only
call site (i915_gem_do_execbuffer).

This way we are free to change driver internal implementation
details without breaking userspace. All state relating to the
uAPI is now contained in, or next to, i915_gem_do_execbuffer.

As a side benefit, this patch decreases the compiled size
of i915_gem_do_execbuffer.

v2: Extract ring selection into eb_select_ring. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h            |   4 +-
 drivers/gpu/drm/i915/i915_gem.c            |   2 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 139 +++++++++++++++--------------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  10 +--
 4 files changed, 81 insertions(+), 74 deletions(-)

Comments

Chris Wilson Jan. 15, 2016, 3:53 p.m. UTC | #1
On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote:
> +static int
> +eb_select_ring(struct drm_i915_private *dev_priv,
> +	       struct drm_file *file,
> +	       struct drm_i915_gem_execbuffer2 *args,
> +	       struct intel_engine_cs **ring)
> +{
> +	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
> +
> +	if (user_ring_id > I915_USER_RINGS) {
> +		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
> +		return -EINVAL;
> +	}
> +
> +	if ((user_ring_id != I915_EXEC_BSD) &&
> +	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
> +		DRM_DEBUG("execbuf with non bsd ring but with invalid "
> +			  "bsd dispatch flags: %d\n", (int)(args->flags));

Not your bug, but we should limit the flags reported here to
(int)(args->flags & I915_EXEC_BSD_MASK).

Though actually just nuke the test. At the moment, we complain for !BSD,
then allow them even if we don't have BSD2 (and ignore the setting).
A little inconsistent.

If we just document that these flags only provide extra selection
criteria for the I915_EXEC_BSD ring, we would be done. I'll pretend that
it is adequately documented...

Looks good and we completed our review of ABI impact for reordering
the ring ids, so

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Daniel Vetter Jan. 21, 2016, 10:22 a.m. UTC | #2
On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> At the moment execbuf ring selection is fully coupled to
> internal ring ids which is not a good thing on its own.
> 
> This dependency is also spread between two source files and
> not spelled out at either side which makes it hidden and
> fragile.
> 
> This patch decouples this dependency by introducing an explicit
> translation table of execbuf uAPI to ring id close to the only
> call site (i915_gem_do_execbuffer).
> 
> This way we are free to change driver internal implementation
> details without breaking userspace. All state relating to the
> uAPI is now contained in, or next to, i915_gem_do_execbuffer.
> 
> As a side benefit, this patch decreases the compiled size
> of i915_gem_do_execbuffer.
> 
> v2: Extract ring selection into eb_select_ring. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Yeah, this decoupling is nice, after all the point of include/uapi was to
make the uapi vs. internal headers clear.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |   4 +-
>  drivers/gpu/drm/i915/i915_gem.c            |   2 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 139 +++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  10 +--
>  4 files changed, 81 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index eb7bb97f7316..35d5d6099a44 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -334,7 +334,7 @@ struct drm_i915_file_private {
>  		unsigned boosts;
>  	} rps;
>  
> -	struct intel_engine_cs *bsd_ring;
> +	unsigned int bsd_ring;
>  };
>  
>  enum intel_dpll_id {
> @@ -1300,7 +1300,7 @@ struct i915_gem_mm {
>  	bool busy;
>  
>  	/* the indicator for dispatch video commands on two BSD rings */
> -	int bsd_ring_dispatch_index;
> +	unsigned int bsd_ring_dispatch_index;
>  
>  	/** Bit 6 swizzling required for X tiling */
>  	uint32_t bit_6_swizzle_x;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ddc21d4b388d..26e6842f2df3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5112,6 +5112,8 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
>  	spin_lock_init(&file_priv->mm.lock);
>  	INIT_LIST_HEAD(&file_priv->mm.request_list);
>  
> +	file_priv->bsd_ring = -1;
> +
>  	ret = i915_gem_context_open(dev, file);
>  	if (ret)
>  		kfree(file_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d469c4779ff5..497a2f87836c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1328,33 +1328,23 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  
>  /**
>   * Find one BSD ring to dispatch the corresponding BSD command.
> - * The Ring ID is returned.
> + * The ring index is returned.
>   */
> -static int gen8_dispatch_bsd_ring(struct drm_device *dev,
> -				  struct drm_file *file)
> +static unsigned int
> +gen8_dispatch_bsd_ring(struct drm_i915_private *dev_priv, struct drm_file *file)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  
> -	/* Check whether the file_priv is using one ring */
> -	if (file_priv->bsd_ring)
> -		return file_priv->bsd_ring->id;
> -	else {
> -		/* If no, use the ping-pong mechanism to select one ring */
> -		int ring_id;
> -
> -		mutex_lock(&dev->struct_mutex);
> -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> -			ring_id = VCS;
> -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> -		} else {
> -			ring_id = VCS2;
> -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> -		}
> -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> -		mutex_unlock(&dev->struct_mutex);
> -		return ring_id;
> +	/* Check whether the file_priv has already selected one ring. */
> +	if ((int)file_priv->bsd_ring < 0) {
> +		/* If not, use the ping-pong mechanism to select one. */
> +		mutex_lock(&dev_priv->dev->struct_mutex);
> +		file_priv->bsd_ring = dev_priv->mm.bsd_ring_dispatch_index;
> +		dev_priv->mm.bsd_ring_dispatch_index ^= 1;
> +		mutex_unlock(&dev_priv->dev->struct_mutex);
>  	}
> +
> +	return file_priv->bsd_ring;
>  }
>  
>  static struct drm_i915_gem_object *
> @@ -1377,6 +1367,63 @@ eb_get_batch(struct eb_vmas *eb)
>  	return vma->obj;
>  }
>  
> +#define I915_USER_RINGS (4)
> +
> +static const enum intel_ring_id user_ring_map[I915_USER_RINGS + 1] = {
> +	[I915_EXEC_DEFAULT]	= RCS,
> +	[I915_EXEC_RENDER]	= RCS,
> +	[I915_EXEC_BLT]		= BCS,
> +	[I915_EXEC_BSD]		= VCS,
> +	[I915_EXEC_VEBOX]	= VECS
> +};
> +
> +static int
> +eb_select_ring(struct drm_i915_private *dev_priv,
> +	       struct drm_file *file,
> +	       struct drm_i915_gem_execbuffer2 *args,
> +	       struct intel_engine_cs **ring)
> +{
> +	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
> +
> +	if (user_ring_id > I915_USER_RINGS) {
> +		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
> +		return -EINVAL;
> +	}
> +
> +	if ((user_ring_id != I915_EXEC_BSD) &&
> +	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
> +		DRM_DEBUG("execbuf with non bsd ring but with invalid "
> +			  "bsd dispatch flags: %d\n", (int)(args->flags));
> +		return -EINVAL;
> +	}
> +
> +	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
> +		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
> +
> +		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
> +			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
> +		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> +			   bsd_idx <= I915_EXEC_BSD_RING2) {
> +			bsd_idx--;
> +		} else {
> +			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
> +				  bsd_idx);
> +			return -EINVAL;
> +		}
> +
> +		*ring = &dev_priv->ring[_VCS(bsd_idx)];
> +	} else {
> +		*ring = &dev_priv->ring[user_ring_map[user_ring_id]];
> +	}
> +
> +	if (!intel_ring_initialized(*ring)) {
> +		DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		       struct drm_file *file,
> @@ -1414,51 +1461,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	if (args->flags & I915_EXEC_IS_PINNED)
>  		dispatch_flags |= I915_DISPATCH_PINNED;
>  
> -	if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
> -		DRM_DEBUG("execbuf with unknown ring: %d\n",
> -			  (int)(args->flags & I915_EXEC_RING_MASK));
> -		return -EINVAL;
> -	}
> -
> -	if (((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_BSD) &&
> -	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
> -		DRM_DEBUG("execbuf with non bsd ring but with invalid "
> -			"bsd dispatch flags: %d\n", (int)(args->flags));
> -		return -EINVAL;
> -	} 
> -
> -	if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT)
> -		ring = &dev_priv->ring[RCS];
> -	else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
> -		if (HAS_BSD2(dev)) {
> -			int ring_id;
> -
> -			switch (args->flags & I915_EXEC_BSD_MASK) {
> -			case I915_EXEC_BSD_DEFAULT:
> -				ring_id = gen8_dispatch_bsd_ring(dev, file);
> -				ring = &dev_priv->ring[ring_id];
> -				break;
> -			case I915_EXEC_BSD_RING1:
> -				ring = &dev_priv->ring[VCS];
> -				break;
> -			case I915_EXEC_BSD_RING2:
> -				ring = &dev_priv->ring[VCS2];
> -				break;
> -			default:
> -				DRM_DEBUG("execbuf with unknown bsd ring: %d\n",
> -					  (int)(args->flags & I915_EXEC_BSD_MASK));
> -				return -EINVAL;
> -			}
> -		} else
> -			ring = &dev_priv->ring[VCS];
> -	} else
> -		ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1];
> -
> -	if (!intel_ring_initialized(ring)) {
> -		DRM_DEBUG("execbuf with invalid ring: %d\n",
> -			  (int)(args->flags & I915_EXEC_RING_MASK));
> -		return -EINVAL;
> -	}
> +	ret = eb_select_ring(dev_priv, file, args, &ring);
> +	if (ret)
> +		return ret;
>  
>  	if (args->buffer_count < 1) {
>  		DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 7349d9258191..fdc220fc2b18 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -148,14 +148,14 @@ struct  i915_ctx_workarounds {
>  struct  intel_engine_cs {
>  	const char	*name;
>  	enum intel_ring_id {
> -		RCS = 0x0,
> -		VCS,
> +		RCS = 0,
>  		BCS,
> -		VECS,
> -		VCS2
> +		VCS,
> +		VCS2,	/* Keep instances of the same type engine together. */
> +		VECS
>  	} id;
>  #define I915_NUM_RINGS 5
> -#define LAST_USER_RING (VECS + 1)
> +#define _VCS(n) (VCS + (n))
>  	u32		mmio_base;
>  	struct		drm_device *dev;
>  	struct intel_ringbuffer *buffer;
> -- 
> 1.9.1
>
Zhipeng Gong Jan. 27, 2016, 6:15 a.m. UTC | #3
> On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote:

> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

> > +	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {

> > +		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;

> > +

> > +		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {

> > +			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);

> > +		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&

> > +			   bsd_idx <= I915_EXEC_BSD_RING2) {

> > +			bsd_idx--;


There is one regression here, I915_EXEC_BSD_RING1/2 is left-shifted 13 bits, 
so we should add something like
	bsd_idx >>= 13;
Otherwise the kernel panics.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eb7bb97f7316..35d5d6099a44 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -334,7 +334,7 @@  struct drm_i915_file_private {
 		unsigned boosts;
 	} rps;
 
-	struct intel_engine_cs *bsd_ring;
+	unsigned int bsd_ring;
 };
 
 enum intel_dpll_id {
@@ -1300,7 +1300,7 @@  struct i915_gem_mm {
 	bool busy;
 
 	/* the indicator for dispatch video commands on two BSD rings */
-	int bsd_ring_dispatch_index;
+	unsigned int bsd_ring_dispatch_index;
 
 	/** Bit 6 swizzling required for X tiling */
 	uint32_t bit_6_swizzle_x;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ddc21d4b388d..26e6842f2df3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5112,6 +5112,8 @@  int i915_gem_open(struct drm_device *dev, struct drm_file *file)
 	spin_lock_init(&file_priv->mm.lock);
 	INIT_LIST_HEAD(&file_priv->mm.request_list);
 
+	file_priv->bsd_ring = -1;
+
 	ret = i915_gem_context_open(dev, file);
 	if (ret)
 		kfree(file_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d469c4779ff5..497a2f87836c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1328,33 +1328,23 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 
 /**
  * Find one BSD ring to dispatch the corresponding BSD command.
- * The Ring ID is returned.
+ * The ring index is returned.
  */
-static int gen8_dispatch_bsd_ring(struct drm_device *dev,
-				  struct drm_file *file)
+static unsigned int
+gen8_dispatch_bsd_ring(struct drm_i915_private *dev_priv, struct drm_file *file)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 
-	/* Check whether the file_priv is using one ring */
-	if (file_priv->bsd_ring)
-		return file_priv->bsd_ring->id;
-	else {
-		/* If no, use the ping-pong mechanism to select one ring */
-		int ring_id;
-
-		mutex_lock(&dev->struct_mutex);
-		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
-			ring_id = VCS;
-			dev_priv->mm.bsd_ring_dispatch_index = 1;
-		} else {
-			ring_id = VCS2;
-			dev_priv->mm.bsd_ring_dispatch_index = 0;
-		}
-		file_priv->bsd_ring = &dev_priv->ring[ring_id];
-		mutex_unlock(&dev->struct_mutex);
-		return ring_id;
+	/* Check whether the file_priv has already selected one ring. */
+	if ((int)file_priv->bsd_ring < 0) {
+		/* If not, use the ping-pong mechanism to select one. */
+		mutex_lock(&dev_priv->dev->struct_mutex);
+		file_priv->bsd_ring = dev_priv->mm.bsd_ring_dispatch_index;
+		dev_priv->mm.bsd_ring_dispatch_index ^= 1;
+		mutex_unlock(&dev_priv->dev->struct_mutex);
 	}
+
+	return file_priv->bsd_ring;
 }
 
 static struct drm_i915_gem_object *
@@ -1377,6 +1367,63 @@  eb_get_batch(struct eb_vmas *eb)
 	return vma->obj;
 }
 
+#define I915_USER_RINGS (4)
+
+static const enum intel_ring_id user_ring_map[I915_USER_RINGS + 1] = {
+	[I915_EXEC_DEFAULT]	= RCS,
+	[I915_EXEC_RENDER]	= RCS,
+	[I915_EXEC_BLT]		= BCS,
+	[I915_EXEC_BSD]		= VCS,
+	[I915_EXEC_VEBOX]	= VECS
+};
+
+static int
+eb_select_ring(struct drm_i915_private *dev_priv,
+	       struct drm_file *file,
+	       struct drm_i915_gem_execbuffer2 *args,
+	       struct intel_engine_cs **ring)
+{
+	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
+
+	if (user_ring_id > I915_USER_RINGS) {
+		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
+		return -EINVAL;
+	}
+
+	if ((user_ring_id != I915_EXEC_BSD) &&
+	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
+		DRM_DEBUG("execbuf with non bsd ring but with invalid "
+			  "bsd dispatch flags: %d\n", (int)(args->flags));
+		return -EINVAL;
+	}
+
+	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
+		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
+
+		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
+			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
+		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
+			   bsd_idx <= I915_EXEC_BSD_RING2) {
+			bsd_idx--;
+		} else {
+			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
+				  bsd_idx);
+			return -EINVAL;
+		}
+
+		*ring = &dev_priv->ring[_VCS(bsd_idx)];
+	} else {
+		*ring = &dev_priv->ring[user_ring_map[user_ring_id]];
+	}
+
+	if (!intel_ring_initialized(*ring)) {
+		DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int
 i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_file *file,
@@ -1414,51 +1461,9 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (args->flags & I915_EXEC_IS_PINNED)
 		dispatch_flags |= I915_DISPATCH_PINNED;
 
-	if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
-		DRM_DEBUG("execbuf with unknown ring: %d\n",
-			  (int)(args->flags & I915_EXEC_RING_MASK));
-		return -EINVAL;
-	}
-
-	if (((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_BSD) &&
-	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
-		DRM_DEBUG("execbuf with non bsd ring but with invalid "
-			"bsd dispatch flags: %d\n", (int)(args->flags));
-		return -EINVAL;
-	} 
-
-	if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT)
-		ring = &dev_priv->ring[RCS];
-	else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
-		if (HAS_BSD2(dev)) {
-			int ring_id;
-
-			switch (args->flags & I915_EXEC_BSD_MASK) {
-			case I915_EXEC_BSD_DEFAULT:
-				ring_id = gen8_dispatch_bsd_ring(dev, file);
-				ring = &dev_priv->ring[ring_id];
-				break;
-			case I915_EXEC_BSD_RING1:
-				ring = &dev_priv->ring[VCS];
-				break;
-			case I915_EXEC_BSD_RING2:
-				ring = &dev_priv->ring[VCS2];
-				break;
-			default:
-				DRM_DEBUG("execbuf with unknown bsd ring: %d\n",
-					  (int)(args->flags & I915_EXEC_BSD_MASK));
-				return -EINVAL;
-			}
-		} else
-			ring = &dev_priv->ring[VCS];
-	} else
-		ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1];
-
-	if (!intel_ring_initialized(ring)) {
-		DRM_DEBUG("execbuf with invalid ring: %d\n",
-			  (int)(args->flags & I915_EXEC_RING_MASK));
-		return -EINVAL;
-	}
+	ret = eb_select_ring(dev_priv, file, args, &ring);
+	if (ret)
+		return ret;
 
 	if (args->buffer_count < 1) {
 		DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 7349d9258191..fdc220fc2b18 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -148,14 +148,14 @@  struct  i915_ctx_workarounds {
 struct  intel_engine_cs {
 	const char	*name;
 	enum intel_ring_id {
-		RCS = 0x0,
-		VCS,
+		RCS = 0,
 		BCS,
-		VECS,
-		VCS2
+		VCS,
+		VCS2,	/* Keep instances of the same type engine together. */
+		VECS
 	} id;
 #define I915_NUM_RINGS 5
-#define LAST_USER_RING (VECS + 1)
+#define _VCS(n) (VCS + (n))
 	u32		mmio_base;
 	struct		drm_device *dev;
 	struct intel_ringbuffer *buffer;