Re: [PATCH v2] drm/i915: Decouple execbuf uAPI from internal implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux