[1/3] drm/i915: Enable resource streamer bits on MI_BATCH_BUFFER_START
diff mbox

Message ID 1433757849-3139-2-git-send-email-abdiel.janulgue@linux.intel.com
State New
Headers show

Commit Message

Abdiel Janulgue June 8, 2015, 10:04 a.m. UTC
Adds support for executing the resource streamer on BDW and HSW

v2: Add support for Execlists (Minu Mathai <minu.mathai@intel.com>)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_lrc.c        | 4 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
 4 files changed, 11 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä June 8, 2015, 4:10 p.m. UTC | #1
On Mon, Jun 08, 2015 at 01:04:07PM +0300, Abdiel Janulgue wrote:
> Adds support for executing the resource streamer on BDW and HSW
> 
> v2: Add support for Execlists (Minu Mathai <minu.mathai@intel.com>)
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_lrc.c        | 4 +++-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
>  4 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b522eb6..238bb25 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -356,6 +356,7 @@
>  #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
>  #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
>  #define MI_BATCH_BUFFER_START_GEN8	MI_INSTR(0x31, 1)
> +#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
>  
>  #define MI_PREDICATE_SRC0	(0x2400)
>  #define MI_PREDICATE_SRC1	(0x2408)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index fcb074b..3b168f6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1172,7 +1172,9 @@ static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
>  		return ret;
>  
>  	/* FIXME(BDW): Address space and security selectors. */
> -	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
> +	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 |
> +				(ppgtt<<8) | (I915_DISPATCH_RS ?

That doesn't look right.

> +					      MI_BATCH_RESOURCE_STREAMER : 0));
>  	intel_logical_ring_emit(ringbuf, lower_32_bits(offset));
>  	intel_logical_ring_emit(ringbuf, upper_32_bits(offset));
>  	intel_logical_ring_emit(ringbuf, MI_NOOP);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 441e250..715cb2a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2385,7 +2385,9 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
>  		return ret;
>  
>  	/* FIXME(BDW): Address space and security selectors. */
> -	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
> +	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8) |
> +			(dispatch_flags & I915_DISPATCH_RS ?
> +			 MI_BATCH_RESOURCE_STREAMER : 0));
>  	intel_ring_emit(ring, lower_32_bits(offset));
>  	intel_ring_emit(ring, upper_32_bits(offset));
>  	intel_ring_emit(ring, MI_NOOP);
> @@ -2408,7 +2410,9 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
>  	intel_ring_emit(ring,
>  			MI_BATCH_BUFFER_START |
>  			(dispatch_flags & I915_DISPATCH_SECURE ?
> -			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW));
> +			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW) |
> +			(dispatch_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);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c761fe0..3521bc0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -167,6 +167,7 @@ struct  intel_engine_cs {
>  					       unsigned dispatch_flags);
>  #define I915_DISPATCH_SECURE 0x1
>  #define I915_DISPATCH_PINNED 0x2
> +#define I915_DISPATCH_RS     0x4
>  	void		(*cleanup)(struct intel_engine_cs *ring);
>  
>  	/* GEN8 signal/wait table - never trust comments!
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Abdiel Janulgue June 8, 2015, 5:42 p.m. UTC | #2
On 06/08/2015 07:10 PM, Ville Syrjälä wrote:
> On Mon, Jun 08, 2015 at 01:04:07PM +0300, Abdiel Janulgue wrote:
>> Adds support for executing the resource streamer on BDW and HSW
>>
>> v2: Add support for Execlists (Minu Mathai <minu.mathai@intel.com>)
>>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>>  drivers/gpu/drm/i915/intel_lrc.c        | 4 +++-
>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
>>  drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
>>  4 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b522eb6..238bb25 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -356,6 +356,7 @@
>>  #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
>>  #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
>>  #define MI_BATCH_BUFFER_START_GEN8	MI_INSTR(0x31, 1)
>> +#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
>>  
>>  #define MI_PREDICATE_SRC0	(0x2400)
>>  #define MI_PREDICATE_SRC1	(0x2408)
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index fcb074b..3b168f6 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1172,7 +1172,9 @@ static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
>>  		return ret;
>>  
>>  	/* FIXME(BDW): Address space and security selectors. */
>> -	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
>> +	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 |
>> +				(ppgtt<<8) | (I915_DISPATCH_RS ?
> 
> That doesn't look right.

Yay.. Didn't catch these since this path never gets executed under GEN8
anyway which uses execlist not legacy batch buffer execution. Better
remove this then.
> 
>> +					      MI_BATCH_RESOURCE_STREAMER : 0));
>>  	intel_logical_ring_emit(ringbuf, lower_32_bits(offset));
>>  	intel_logical_ring_emit(ringbuf, upper_32_bits(offset));
>>  	intel_logical_ring_emit(ringbuf, MI_NOOP);
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 441e250..715cb2a 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -2385,7 +2385,9 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
>>  		return ret;
>>  
>>  	/* FIXME(BDW): Address space and security selectors. */
>> -	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
>> +	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8) |
>> +			(dispatch_flags & I915_DISPATCH_RS ?
>> +			 MI_BATCH_RESOURCE_STREAMER : 0));
>>  	intel_ring_emit(ring, lower_32_bits(offset));
>>  	intel_ring_emit(ring, upper_32_bits(offset));
>>  	intel_ring_emit(ring, MI_NOOP);
>> @@ -2408,7 +2410,9 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
>>  	intel_ring_emit(ring,
>>  			MI_BATCH_BUFFER_START |
>>  			(dispatch_flags & I915_DISPATCH_SECURE ?
>> -			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW));
>> +			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW) |
>> +			(dispatch_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);
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index c761fe0..3521bc0 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -167,6 +167,7 @@ struct  intel_engine_cs {
>>  					       unsigned dispatch_flags);
>>  #define I915_DISPATCH_SECURE 0x1
>>  #define I915_DISPATCH_PINNED 0x2
>> +#define I915_DISPATCH_RS     0x4
>>  	void		(*cleanup)(struct intel_engine_cs *ring);
>>  
>>  	/* GEN8 signal/wait table - never trust comments!
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Chris Wilson June 8, 2015, 5:55 p.m. UTC | #3
On Mon, Jun 08, 2015 at 08:42:20PM +0300, Abdiel Janulgue wrote:
> 
> 
> On 06/08/2015 07:10 PM, Ville Syrjälä wrote:
> > On Mon, Jun 08, 2015 at 01:04:07PM +0300, Abdiel Janulgue wrote:
> >> Adds support for executing the resource streamer on BDW and HSW
> >>
> >> v2: Add support for Execlists (Minu Mathai <minu.mathai@intel.com>)
> >>
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
> >>  drivers/gpu/drm/i915/intel_lrc.c        | 4 +++-
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
> >>  drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
> >>  4 files changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index b522eb6..238bb25 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -356,6 +356,7 @@
> >>  #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
> >>  #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
> >>  #define MI_BATCH_BUFFER_START_GEN8	MI_INSTR(0x31, 1)
> >> +#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
> >>  
> >>  #define MI_PREDICATE_SRC0	(0x2400)
> >>  #define MI_PREDICATE_SRC1	(0x2408)
> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >> index fcb074b..3b168f6 100644
> >> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >> @@ -1172,7 +1172,9 @@ static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
> >>  		return ret;
> >>  
> >>  	/* FIXME(BDW): Address space and security selectors. */
> >> -	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
> >> +	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 |
> >> +				(ppgtt<<8) | (I915_DISPATCH_RS ?
> > 
> > That doesn't look right.
> 
> Yay.. Didn't catch these since this path never gets executed under GEN8
> anyway which uses execlist not legacy batch buffer execution. Better
> remove this then.

Which makes gen8 support conditional and so needs reflecting in the
interface.

Just test the flag correctly.
-Chris
Dave Gordon June 9, 2015, 11:48 a.m. UTC | #4
On 08/06/15 18:42, Abdiel Janulgue wrote:
> 
> On 06/08/2015 07:10 PM, Ville Syrjälä wrote:
>> On Mon, Jun 08, 2015 at 01:04:07PM +0300, Abdiel Janulgue wrote:
>>> Adds support for executing the resource streamer on BDW and HSW
>>>
>>> v2: Add support for Execlists (Minu Mathai <minu.mathai@intel.com>)
>>>
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>>>  drivers/gpu/drm/i915/intel_lrc.c        | 4 +++-
>>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++--
>>>  drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
>>>  4 files changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index b522eb6..238bb25 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -356,6 +356,7 @@
>>>  #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
>>>  #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
>>>  #define MI_BATCH_BUFFER_START_GEN8	MI_INSTR(0x31, 1)
>>> +#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
>>>  
>>>  #define MI_PREDICATE_SRC0	(0x2400)
>>>  #define MI_PREDICATE_SRC1	(0x2408)
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index fcb074b..3b168f6 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1172,7 +1172,9 @@ static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
>>>  		return ret;
>>>  
>>>  	/* FIXME(BDW): Address space and security selectors. */
>>> -	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
>>> +	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 |
>>> +				(ppgtt<<8) | (I915_DISPATCH_RS ?
>>
>> That doesn't look right.
> 
> Yay.. Didn't catch these since this path never gets executed under GEN8
> anyway which uses execlist not legacy batch buffer execution. Better
> remove this then.

But GEN8 HW can run in legacy ringbuffer mode, and the driver continues
to support it, at least for now, so AFAIK there's nothing preventing you
using the Resource Streamer in ringbuffer mode; indeed I note that the
MI_RS_CONTEXT instruction can ONLY be used in ringbuffer mode.

Please don't conflate changes that are (or ought to be) orthogonal, such
as 32- vs 48-bit addressing and ringbuffer vs. execlists, just because
they were introduced in the same h/w generation ...

.Dave.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b522eb6..238bb25 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -356,6 +356,7 @@ 
 #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
 #define   MI_BATCH_GTT		    (2<<6) /* aliased with (1<<7) on gen4 */
 #define MI_BATCH_BUFFER_START_GEN8	MI_INSTR(0x31, 1)
+#define   MI_BATCH_RESOURCE_STREAMER (1<<10)
 
 #define MI_PREDICATE_SRC0	(0x2400)
 #define MI_PREDICATE_SRC1	(0x2408)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fcb074b..3b168f6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1172,7 +1172,9 @@  static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
 		return ret;
 
 	/* FIXME(BDW): Address space and security selectors. */
-	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
+	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_START_GEN8 |
+				(ppgtt<<8) | (I915_DISPATCH_RS ?
+					      MI_BATCH_RESOURCE_STREAMER : 0));
 	intel_logical_ring_emit(ringbuf, lower_32_bits(offset));
 	intel_logical_ring_emit(ringbuf, upper_32_bits(offset));
 	intel_logical_ring_emit(ringbuf, MI_NOOP);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 441e250..715cb2a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2385,7 +2385,9 @@  gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
 		return ret;
 
 	/* FIXME(BDW): Address space and security selectors. */
-	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8));
+	intel_ring_emit(ring, MI_BATCH_BUFFER_START_GEN8 | (ppgtt<<8) |
+			(dispatch_flags & I915_DISPATCH_RS ?
+			 MI_BATCH_RESOURCE_STREAMER : 0));
 	intel_ring_emit(ring, lower_32_bits(offset));
 	intel_ring_emit(ring, upper_32_bits(offset));
 	intel_ring_emit(ring, MI_NOOP);
@@ -2408,7 +2410,9 @@  hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
 	intel_ring_emit(ring,
 			MI_BATCH_BUFFER_START |
 			(dispatch_flags & I915_DISPATCH_SECURE ?
-			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW));
+			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW) |
+			(dispatch_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);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c761fe0..3521bc0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -167,6 +167,7 @@  struct  intel_engine_cs {
 					       unsigned dispatch_flags);
 #define I915_DISPATCH_SECURE 0x1
 #define I915_DISPATCH_PINNED 0x2
+#define I915_DISPATCH_RS     0x4
 	void		(*cleanup)(struct intel_engine_cs *ring);
 
 	/* GEN8 signal/wait table - never trust comments!