Re: [PATCH] drm/i915: Specify bsd rings through exec flag

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 17, 2014 at 03:06:01AM -0800, Rodrigo Vivi wrote:
> From: Zhipeng Gong <zhipeng.gong@intel.com>
> 
> On Broadwell GT3 we have 2 Video Command Streamers (VCS), but userspace
> has no control when using VCS1 or VCS2. This patch introduces a mechanism
> to avoid the default ping-pong mode and use one specific ring through
> execution flag.
> 
> v2: fix whitespace (Rodrigo)
> v3: remove incorrect chunk that came on -collector rebase. (Rodrigo)
> 
> Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>

Somehow Zhipeng isn't on the CC list - check your git send-email settings?
Iirc the author should get added by default ...

> Reviewed-by-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Already explained internally: If we don't have public&reviewed (by the
libva maintainer) code using this then Dave Airlie will rip my head off if
I merge this ;-)

Also this needs adjustments to gem_exec_params (prts should catch the
regression, if not gem_exec_params/invalid-flags is broken). And some tiny
nits below.

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 19 +++++++++++++++++--
>  include/uapi/drm/i915_drm.h                |  8 +++++++-
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index e1ed85a..d9081ec 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1273,8 +1273,23 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
>  		if (HAS_BSD2(dev)) {
>  			int ring_id;
> -			ring_id = gen8_dispatch_bsd_ring(dev, file);
> -			ring = &dev_priv->ring[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];

Do we have any use-case for selecting ring1 specifically? I've thought
it's only ring2 that is special?

> +				break;
> +			case I915_EXEC_BSD_RING2:

This needs a if (!IS_SKL(dev) return -EINVAL; check since the flag isnt
valid anywhere else. Also you must add code to reject these flags if
userspace selects a ring other than bsd.

And all these new -EINVAL cases need new subtests to validate them in
gem_exec_params.c.

And I might have missed some case, ioctl validation is hard ;-) So please
double-check that really no insane combination that userspace might try to
abuse is caught and has a testcase in gem_exec_params.

> +				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
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2502622..fcb16bf 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -737,7 +737,13 @@ struct drm_i915_gem_execbuffer2 {
>   */
>  #define I915_EXEC_HANDLE_LUT		(1<<12)
>  
> -#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_HANDLE_LUT<<1)
> +/** Used for switching BSD rings on the platforms with two BSD rings */
> +#define I915_EXEC_BSD_MASK		(3<<13)
> +#define I915_EXEC_BSD_DEFAULT		(0<<13) /* default ping-pong mode */
> +#define I915_EXEC_BSD_RING1		(1<<13)
> +#define I915_EXEC_BSD_RING2		(2<<13)
> +
> +#define __I915_EXEC_UNKNOWN_FLAGS -(1<<15)
>  
>  #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
>  #define i915_execbuffer2_set_context_id(eb2, context) \
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx