drm/i915/hsw: Enable resource streamer (v2)

drm/i915/hsw: Enable resource streamer (v2)

[Abdiel Janulgue]

v2 of drm-i915 part of resource streamer enabling. Re-submitted
finally now that the Mesa portions are starting to take shape.
I also addressed some of the comments from Daniel and Chris from
the previous implementation.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

Abdiel Janulgue (2):
      drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag
      drm/i915/hsw: Enable resource streamer bit on MI_BATCH_BUFFER_START
--

 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 ++
 drivers/gpu/drm/i915/i915_reg.h            | 1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 7 ++++---
 include/uapi/drm/i915_drm.h                | 5 +++++
 4 files changed, 12 insertions(+), 3 deletions(-)

[PATCH 1/2] drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag

[Abdiel Janulgue]

Ensures that the batch buffer is executed by the resource streamer.

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 ++
 include/uapi/drm/i915_drm.h                |    5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ce0d47..4a56c58 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -962,6 +962,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	case I915_EXEC_DEFAULT:
 	case I915_EXEC_RENDER:
 		ring = &dev_priv->ring[RCS];
+		flags |= (args->flags & I915_EXEC_RESOURCE_STREAMER) ?
+		  I915_EXEC_RESOURCE_STREAMER : 0;
 		break;
 	case I915_EXEC_BSD:
 		ring = &dev_priv->ring[VCS];
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 3a4e97b..5a4bd16 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -731,6 +731,11 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_HANDLE_LUT		(1<<12)
 
+/** Tell the kernel that the batchbuffer is processed by
+ *  the resource streamer.
+ */
+#define I915_EXEC_RESOURCE_STREAMER     (1<<13)
+
 #define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_HANDLE_LUT<<1)
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
-- 
1.7.9.5

[PATCH 2/2] drm/i915/hsw: Enable resource streamer bit on MI_BATCH_BUFFER_START

[Abdiel Janulgue]

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |    7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c246727..f3c9103 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -246,6 +246,7 @@
 #define   MI_BATCH_NON_SECURE_HSW 	(1<<13)
 #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
 #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
+#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
 #define MI_SEMAPHORE_MBOX	MI_INSTR(0x16, 1) /* gen6+ */
 #define  MI_SEMAPHORE_GLOBAL_GTT    (1<<22)
 #define  MI_SEMAPHORE_UPDATE	    (1<<21)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b67104a..c5dd71b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1647,14 +1647,15 @@ hsw_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 			      unsigned flags)
 {
 	int ret;
+	int ring_emit_flags = MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
+		(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW);
 
 	ret = intel_ring_begin(ring, 2);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(ring,
-			MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
-			(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW));
+	intel_ring_emit(ring, ring_emit_flags | (flags & I915_EXEC_RESOURCE_STREAMER ?
+			 MI_BATCH_RESOURCE_STREAMER : 0));
 	/* bit0-7 is the length on GEN6+ */
 	intel_ring_emit(ring, offset);
 	intel_ring_advance(ring);
-- 
1.7.9.5

Re: [PATCH 1/2] drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag

[Chris Wilson]

On Wed, Oct 09, 2013 at 12:09:51AM +0300, Abdiel Janulgue wrote:
> Ensures that the batch buffer is executed by the resource streamer.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 ++
>  include/uapi/drm/i915_drm.h                |    5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0ce0d47..4a56c58 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -962,6 +962,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	case I915_EXEC_DEFAULT:
>  	case I915_EXEC_RENDER:
>  		ring = &dev_priv->ring[RCS];
> +		flags |= (args->flags & I915_EXEC_RESOURCE_STREAMER) ?
> +		  I915_EXEC_RESOURCE_STREAMER : 0;

Besides this being flags |= args->flags & I915_EXEC_RESOURCE_STREAMER;
flags is a completely different bitfield and you should be translating
into a dispatch value rather than the execbuffer value.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag

[Ben Widawsky]

On Tue, Oct 08, 2013 at 10:50:50PM +0100, Chris Wilson wrote:
> On Wed, Oct 09, 2013 at 12:09:51AM +0300, Abdiel Janulgue wrote:
> > Ensures that the batch buffer is executed by the resource streamer.
> > 
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 ++
> >  include/uapi/drm/i915_drm.h                |    5 +++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 0ce0d47..4a56c58 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -962,6 +962,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  	case I915_EXEC_DEFAULT:
> >  	case I915_EXEC_RENDER:
> >  		ring = &dev_priv->ring[RCS];
> > +		flags |= (args->flags & I915_EXEC_RESOURCE_STREAMER) ?
> > +		  I915_EXEC_RESOURCE_STREAMER : 0;
> 
> Besides this being flags |= args->flags & I915_EXEC_RESOURCE_STREAMER;
> flags is a completely different bitfield and you should be translating
> into a dispatch value rather than the execbuffer value.
> -Chris
> 

To decrypt Chris just a bit, though he was rather more verbose than
usual ;-), see the translation of I915_EXEC_SECURE to
I915_DISPATCH_SECURE.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag

[Daniel Vetter]

On Tue, Oct 08, 2013 at 04:38:31PM -0700, Ben Widawsky wrote:
> On Tue, Oct 08, 2013 at 10:50:50PM +0100, Chris Wilson wrote:
> > On Wed, Oct 09, 2013 at 12:09:51AM +0300, Abdiel Janulgue wrote:
> > > Ensures that the batch buffer is executed by the resource streamer.
> > > 
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 ++
> > >  include/uapi/drm/i915_drm.h                |    5 +++++
> > >  2 files changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 0ce0d47..4a56c58 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -962,6 +962,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  	case I915_EXEC_DEFAULT:
> > >  	case I915_EXEC_RENDER:
> > >  		ring = &dev_priv->ring[RCS];
> > > +		flags |= (args->flags & I915_EXEC_RESOURCE_STREAMER) ?
> > > +		  I915_EXEC_RESOURCE_STREAMER : 0;
> > 
> > Besides this being flags |= args->flags & I915_EXEC_RESOURCE_STREAMER;
> > flags is a completely different bitfield and you should be translating
> > into a dispatch value rather than the execbuffer value.
> > -Chris
> > 
> 
> To decrypt Chris just a bit, though he was rather more verbose than
> usual ;-), see the translation of I915_EXEC_SECURE to
> I915_DISPATCH_SECURE.

Also an i-g-t testcase which checks that we correctly reject this flag
would be good (i.e. reject it on non-render rings and on platforms that
don't support the resource streamer). We don't yet have any execbuf flags
tests at all, so this is a good opportunity to fix this.

I've tried to sign up Abdiel for this on Jira, but he's not there ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag

[Kenneth Graunke]

On 10/08/2013 02:09 PM, Abdiel Janulgue wrote:
> Ensures that the batch buffer is executed by the resource streamer.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 ++
>  include/uapi/drm/i915_drm.h                |    5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0ce0d47..4a56c58 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -962,6 +962,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	case I915_EXEC_DEFAULT:
>  	case I915_EXEC_RENDER:
>  		ring = &dev_priv->ring[RCS];
> +		flags |= (args->flags & I915_EXEC_RESOURCE_STREAMER) ?
> +		  I915_EXEC_RESOURCE_STREAMER : 0;
>  		break;
>  	case I915_EXEC_BSD:
>  		ring = &dev_priv->ring[VCS];
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 3a4e97b..5a4bd16 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -731,6 +731,11 @@ struct drm_i915_gem_execbuffer2 {
>   */
>  #define I915_EXEC_HANDLE_LUT		(1<<12)
>  
> +/** Tell the kernel that the batchbuffer is processed by
> + *  the resource streamer.
> + */
> +#define I915_EXEC_RESOURCE_STREAMER     (1<<13)
> +
>  #define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_HANDLE_LUT<<1)
>  
>  #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
> 

I think you need to change __I915_EXEC_UNKNOWN_FLAGS to:

#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_RESOURCE_STREAMER<<1)

since you've added a new bit.

[PATCH 1/2] drm/i915/hsw: Add I915_EXEC_RESOURCE_STREAMER flag (v3)

[Abdiel Janulgue]

Ensures that the batch buffer is executed by the resource streamer.

v3: - Make sure batch is only submitted on render ring and Haswell (Daniel)
    - Separate EXEC and DISPATCH flags (Chris)
    - Update __I915_EXEC_UNKNOWN_FLAGS (Kenneth)

Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h    | 1 +
 include/uapi/drm/i915_drm.h                | 7 ++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ce0d47..833677d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -957,6 +957,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 	if (args->flags & I915_EXEC_IS_PINNED)
 		flags |= I915_DISPATCH_PINNED;
+	if (args->flags & I915_EXEC_RESOURCE_STREAMER) {
+		if ((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_RENDER ||
+		    !IS_HASWELL(dev))
+			return -EINVAL;
+
+		flags |= I915_DISPATCH_RS;
+	}
 
 	switch (args->flags & I915_EXEC_RING_MASK) {
 	case I915_EXEC_DEFAULT:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 71a73f4..bd74427 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -110,6 +110,7 @@ struct  intel_ring_buffer {
 					       unsigned flags);
 #define I915_DISPATCH_SECURE 0x1
 #define I915_DISPATCH_PINNED 0x2
+#define I915_DISPATCH_RS     0x4
 	void		(*cleanup)(struct intel_ring_buffer *ring);
 	int		(*sync_to)(struct intel_ring_buffer *ring,
 				   struct intel_ring_buffer *to,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 3a4e97b..edd6e31 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -731,7 +731,12 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_HANDLE_LUT		(1<<12)
 
-#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_HANDLE_LUT<<1)
+/** Tell the kernel that the batchbuffer is processed by
+ *  the resource streamer.
+ */
+#define I915_EXEC_RESOURCE_STREAMER     (1<<13)
+
+#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_RESOURCE_STREAMER <<1)
 
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
-- 
1.8.2.1

[PATCH 2/2] drm/i915/hsw: Enable resource streamer bit on MI_BATCH_BUFFER_START (v3)

[Abdiel Janulgue]

v3: Use the I915_DISPATCH_RS flag to determine if batchbuffer needs
    resource streamer bit.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c246727..f3c9103 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -246,6 +246,7 @@
 #define   MI_BATCH_NON_SECURE_HSW 	(1<<13)
 #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
 #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
+#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
 #define MI_SEMAPHORE_MBOX	MI_INSTR(0x16, 1) /* gen6+ */
 #define  MI_SEMAPHORE_GLOBAL_GTT    (1<<22)
 #define  MI_SEMAPHORE_UPDATE	    (1<<21)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b67104a..f9564b1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1654,7 +1654,8 @@ hsw_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 
 	intel_ring_emit(ring,
 			MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
-			(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW));
+			(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW) |
+			(flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER : 0));
 	/* bit0-7 is the length on GEN6+ */
 	intel_ring_emit(ring, offset);
 	intel_ring_advance(ring);
-- 
1.8.2.1

Re: [PATCH 2/2] drm/i915/hsw: Enable resource streamer bit on MI_BATCH_BUFFER_START (v3)

[Chris Wilson]

On Thu, Oct 10, 2013 at 02:03:00PM +0300, Abdiel Janulgue wrote:
> v3: Use the I915_DISPATCH_RS flag to determine if batchbuffer needs
>     resource streamer bit.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>

Note this should be the first patch so that we enable the feature before
we tell userspace it is supported.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre