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

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

[Zhipeng Gong]

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.

Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
---
 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 60998fc..f9ed8e0 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1279,8 +1279,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];
+				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
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ff57f07..421420a 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -736,7 +736,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) \
-- 
2.0.3

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

[Chris Wilson]

On Tue, Aug 05, 2014 at 03:54:04PM +0800, Zhipeng Gong wrote:
> 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.
> 
> Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> ---
>  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 60998fc..f9ed8e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1279,8 +1279,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];
> +				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
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index ff57f07..421420a 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -736,7 +736,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)

There is room in the ring selection flags for expansion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Tue, Aug 05, 2014 at 03:54:04PM +0800, Zhipeng Gong wrote:
> 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.
> 
> Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>

This needs an open-source user and proper justification why we need this.
On bdw. Iirc the only users is content protection which isn't open-source
due to the usual concerns, so if that hasn't changed this patch is
rejected.

Also you'd need igt tests and all that too.
-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 60998fc..f9ed8e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1279,8 +1279,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];
> +				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
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index ff57f07..421420a 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -736,7 +736,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) \
> -- 
> 2.0.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

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

[Zhao, Yakui]

On Tue, 2014-08-05 at 02:44 -0600, Daniel Vetter wrote:
> On Tue, Aug 05, 2014 at 03:54:04PM +0800, Zhipeng Gong wrote:
> > 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.
> > 
> > Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> 
> This needs an open-source user and proper justification why we need this.
> On bdw. Iirc the only users is content protection which isn't open-source
> due to the usual concerns, so if that hasn't changed this patch is
> rejected.

Hi, Daniel

    The open-source media driver also needs this feature for BDW. Now we
are planning to add the following function that depends on this flag for
BDW with two BSD rings.
    >After the GPU hang occurs on BSD ring during decoding, it needs to
specify the corresponding BSD ring to read the decoding status registers
related with the BSD ring. 
     
      Can this be regarded as one open-source usage scenario?

> 
> Also you'd need igt tests and all that too.

I agree. The igt test is needed.

Maybe one patch is missing that exposes the flag of dual BSD rings.
Only when the flag exists, we can specify which bsd ring to dispatch the
BSD video command.

> -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 60998fc..f9ed8e0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1279,8 +1279,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];
> > +				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
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index ff57f07..421420a 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -736,7 +736,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) \
> > -- 
> > 2.0.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

[Gong, Zhipeng]

> 
> On Tue, Aug 05, 2014 at 03:54:04PM +0800, Zhipeng Gong wrote:
> > 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.
> > 
> > Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
> > ---
> >  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 60998fc..f9ed8e0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1279,8 +1279,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];
> > +				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
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h 
> > index ff57f07..421420a 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -736,7 +736,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)
> 
> There is room in the ring selection flags for expansion.
> -Chris

Hi Chris

There is only three rooms in the I915_EXEC_RING_MASK, this feature will
occupy two rooms. If use these rooms, it will left few rooms for more
possible rings in the future and it also introduces a little bit
complexity to get the ring in the i915_do_gem_execbuffer.

-Zhipeng

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

[Daniel Vetter]

On Wed, Aug 6, 2014 at 3:32 AM, Zhao, Yakui <yakui.zhao@intel.com> wrote:
>     >After the GPU hang occurs on BSD ring during decoding, it needs to
> specify the corresponding BSD ring to read the decoding status registers
> related with the BSD ring.
>
>       Can this be regarded as one open-source usage scenario?

Sure, but I'm not sure whether this is the correct way to do this - if
some other libva client does an execbuf in-between couldn't these
register get overwritten? It sounds like the kernel should capture
these registers for userspace in a race-free way and then provide them
with maybe an extension to the reset-stats ioctl?

I definitely want to see how this is used in libva before signing up
for a new kernel interface.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Zhipeng Gong]

On Skylake GT3 we have 2 Video Command Streamers (VCS), which is asymmetrical.
For example, HEVC GPU commands can be only dispatched to VCS1 ring.
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. This mechanism is usable for all the platforms
with 2 VCS rings.

v2: fix whitespace (Rodrigo)
v3: remove incorrect chunk that came on -collector rebase. (Rodrigo)
v4: change the comment (Zhipeng)

Signed-off-by: Zhipeng Gong <zhipeng.gong@intel.com>
Reviewed-by-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 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 0c25f62..de652a5 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1317,8 +1317,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];
+				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
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) \
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=290/291->290/291
PNV: pass/total=356/356->354/356
ILK: pass/total=371/372->371/372
IVB: pass/total=544/546->545/546
SNB: pass/total=424/425->424/425
HSW: pass/total=579/579->578/579
BDW: pass/total=432/435->432/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M36)PASS(1, M36) -> FAIL(1, M36)PASS(3, M36)
BYT: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M36)PASS(9, M31M36) -> TIMEOUT(3, M36)PASS(1, M36)
PNV: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M25)PASS(1, M25) -> FAIL(1, M25)PASS(3, M25)
PNV: Intel_gpu_tools, igt_kms_sysfs_edid_timing, PASS(4, M25) -> FAIL(1, M25)PASS(3, M25)
ILK: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M37)PASS(1, M37) -> FAIL(1, M37)PASS(3, M37)
ILK: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M37)PASS(9, M26M37) -> TIMEOUT(3, M37)PASS(1, M37)
IVB: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M34)PASS(1, M21) -> FAIL(1, M34)PASS(3, M34)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-random, DMESG_WARN(1, M21)PASS(6, M21M34) -> TIMEOUT(1, M34)PASS(3, M34)
IVB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M21)PASS(6, M21M34) -> TIMEOUT(3, M34)PASS(1, M34)
SNB: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M35)PASS(1, M35) -> FAIL(1, M35)PASS(3, M35)
SNB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M35)PASS(9, M35M22) -> TIMEOUT(3, M35)PASS(1, M35)
HSW: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M20)PASS(1, M19) -> FAIL(1, M20)PASS(3, M20)
BDW: Intel_gpu_tools, igt_gem_exec_params_invalid-flag, FAIL(3, M28)PASS(1, M28) -> FAIL(1, M28)PASS(3, M28)
BDW: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M28)PASS(6, M30M28) -> TIMEOUT(3, M28)PASS(1, M28)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Rodrigo Vivi]

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>
Reviewed-by-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 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];
+				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
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