diff mbox

[2/2] drm/i915/hsw/bdw: Enable resource streamer bits on MI_BATCH_BUFFER_START

Message ID 1431504804-2811-3-git-send-email-abdiel.janulgue@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Abdiel Janulgue May 13, 2015, 8:13 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>)

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        | 3 ++-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++--
 3 files changed, 7 insertions(+), 3 deletions(-)

Comments

Chris Wilson May 13, 2015, 10:22 a.m. UTC | #1 
On Wed, May 13, 2015 at 11:13:24AM +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>)
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>

I would have liked to have seen the comments for HSW_CTX_TOTAL_SIZE
updated to include the resource streamer requirements.

Also

/* These flags are for resource streamer on HSW+ */
if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);

implies to me that we should be setting something for hsw to
save/restore RS that we do not currently. So either the comment needs
fixing, or we have a lack of code.

Note that intel_lrc.c has duplicate functions for gen8. :|
-Chris
Abdiel Janulgue May 15, 2015, 9:16 a.m. UTC | #2 
Hi,

On 05/13/2015 01:22 PM, Chris Wilson wrote:
> On Wed, May 13, 2015 at 11:13:24AM +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>)
>>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> 
> I would have liked to have seen the comments for HSW_CTX_TOTAL_SIZE
> updated to include the resource streamer requirements.

Comment block mention HSW_CTX_TOTAL_SIZE as 70720 bytes which already
implicitly include RS dwords.
I can add "70720 bytes include resource streamer dwords" to the comments
or do you want me to break this down further still?

> 
> Also
> 
> /* These flags are for resource streamer on HSW+ */
> if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> 	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> 
> implies to me that we should be setting something for hsw to
> save/restore RS that we do not currently. So either the comment needs
> fixing, or we have a lack of code.

I checked, the current code does disable context save and restore for RS
in HSW (commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2)

But I've now modified this to:

if (IS_HASWELL(ring->dev))
	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);


so that MI_SET_CONTEXT includes RS context on HSW.

Anyway, this seems to work perfectly fine in HSW as well. I'll include
that in the next version.

> 
> Note that intel_lrc.c has duplicate functions for gen8. :|
> -Chris
>
Chris Wilson May 15, 2015, 11:39 a.m. UTC | #3 
On Fri, May 15, 2015 at 12:16:06PM +0300, Abdiel Janulgue wrote:
> Hi,
> 
> On 05/13/2015 01:22 PM, Chris Wilson wrote:
> > On Wed, May 13, 2015 at 11:13:24AM +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>)
> >>
> >> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > 
> > I would have liked to have seen the comments for HSW_CTX_TOTAL_SIZE
> > updated to include the resource streamer requirements.
> 
> Comment block mention HSW_CTX_TOTAL_SIZE as 70720 bytes which already
> implicitly include RS dwords.
> I can add "70720 bytes include resource streamer dwords" to the comments
> or do you want me to break this down further still?

I am just unnerved by the "full size is 70720, but we use 66944 bytes".
So perhaps changing the wording there to illustrate the boundaries would
be useful. However, I would be happy with just changing the wording to 
"...so the final size, including the extra state required for the
Resource Streamer, is 66944 bytes"

The important part here (for me at least) is that we do have something
in the git history to double check later on (i.e. we know that when we
did the RS enabling, that we did check the context size). Having clear
comments is a boon.

> > Also
> > 
> > /* These flags are for resource streamer on HSW+ */
> > if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> > 	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > 
> > implies to me that we should be setting something for hsw to
> > save/restore RS that we do not currently. So either the comment needs
> > fixing, or we have a lack of code.
> 
> I checked, the current code does disable context save and restore for RS
> in HSW (commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2)
> 
> But I've now modified this to:
> 
> if (IS_HASWELL(ring->dev))
> 	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> 
> 
> so that MI_SET_CONTEXT includes RS context on HSW.

Now that makes more sense, I'd been puzzled by that comment and line of
code for ages :)
-Chris
diff mbox

Patch

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..d523494 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1172,7 +1172,8 @@  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..9045144 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2385,7 +2385,8 @@  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 +2409,8 @@  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);