drm/i915: Specify bsd rings through exec flag
diff mbox

Message ID 1407225244-6457-1-git-send-email-zhipeng.gong@intel.com
State New, archived
Headers show

Commit Message

Zhipeng Gong Aug. 5, 2014, 7:54 a.m. UTC
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(-)

Comments

Chris Wilson Aug. 5, 2014, 8:28 a.m. UTC | #1
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
Daniel Vetter Aug. 5, 2014, 8:44 a.m. UTC | #2
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
Zhao, Yakui Aug. 6, 2014, 1:32 a.m. UTC | #3
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
>
Zhipeng Gong Aug. 6, 2014, 3:19 a.m. UTC | #4
> 
> 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
Daniel Vetter Aug. 6, 2014, 7:45 a.m. UTC | #5
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

Patch
diff mbox

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) \