On Fri, Jan 15, 2016 at 03:12:50PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxx>
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
