[Intel,gfx,i-g-t,3/4] rendercopy/bdw: A workaround for 3D pipeline

Commit Message

Xiang, Haihao Dec. 6, 2013, 8:54 a.m. UTC

From: "Xiang, Haihao" <[e-mail]>

Emit PIPELINE_SELECT twice and make sure the commands in
the first batch buffer have been done.

However I don't know why this works !!!

Signed-off-by: Xiang, Haihao <[e-mail]>
---
 lib/rendercopy_gen8.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Lespiau, Damien Dec. 6, 2013, 1:30 p.m. UTC | #1

On Fri, Dec 06, 2013 at 04:54:46PM +0800, Xiang, Haihao wrote:
> From: "Xiang, Haihao" <[e-mail]>
> 
> Emit PIPELINE_SELECT twice and make sure the commands in
> the first batch buffer have been done.
> 
> However I don't know why this works !!!

Hum :) on one hand, it's great that you found this w/a, on the other
hand, I'm not comfortable with not understanding why this works. So far
what we know (I don't have Silicon that can't test anything):

 - Ken was saying that mesa doesn't need this.
 - There are a bunch of W/A around FF units clock gating, might worth
   checking that we're not hiting WaDisableFfDopClockGating or one of
   those 3D Vs GPGPU pipelines ones.
   This could happen to you but not to Ken because you have been
   switching between 3D and media pipeline with the 2 igt tests.
 - In any case, doing a pass on the W/A sounds like a good idea
 - I'd be interested to know if there a even more minimal batch that
   works (say an empty batch), or if the active ingredient is the
   pipeline switch.

If people want to push the patch to make progress on other parts, I
guess that's fine, but we'll need to dig deeper here.

Xiang, Haihao Dec. 9, 2013, 4:47 a.m. UTC | #2

On Fri, 2013-12-06 at 13:30 +0000, Damien Lespiau wrote: 
> On Fri, Dec 06, 2013 at 04:54:46PM +0800, Xiang, Haihao wrote:
> > From: "Xiang, Haihao" <[e-mail]>
> > 
> > Emit PIPELINE_SELECT twice and make sure the commands in
> > the first batch buffer have been done.
> > 
> > However I don't know why this works !!!
> 
> Hum :) on one hand, it's great that you found this w/a, on the other
> hand, I'm not comfortable with not understanding why this works.

Thanks for the comments, actually I am not comfortable with it too.
gem_render_copy passed after I happened to run gem_media_fill first, so
I am curious which setting in gem_media_fill impact the result. Finally
I found it works if I emit PIPELINE_SELECT in a separated batch first. 

> So far
> what we know (I don't have Silicon that can't test anything):
> 
>  - Ken was saying that mesa doesn't need this.
>  - There are a bunch of W/A around FF units clock gating, might worth
>    checking that we're not hiting WaDisableFfDopClockGating or one of
>    those 3D Vs GPGPU pipelines ones.
>    This could happen to you but not to Ken because you have been
>    switching between 3D and media pipeline with the 2 igt tests.
>  - In any case, doing a pass on the W/A sounds like a good idea
>  - I'd be interested to know if there a even more minimal batch that
>    works (say an empty batch), or if the active ingredient is the
>    pipeline switch.

Oh, it works even with an batch which has only MI_BATCH_BUFFER_END.

> 
> If people want to push the patch to make progress on other parts, I
> guess that's fine, but we'll need to dig deeper here.

Agree, we should look into the issue to find the real root cause.

>

Daniel Vetter Dec. 9, 2013, 6:53 a.m. UTC | #3

On Mon, Dec 9, 2013 at 5:47 AM, Xiang, Haihao <[e-mail]> wrote:
> Oh, it works even with an batch which has only MI_BATCH_BUFFER_END.

This sounds like we're missing a tlb/cache invalidate before launching
the batch in the kernel. But the big flush we unconditionally do after
each batch is good enough to fix things. This could also explain some
of the other i-g-t failures ...
-Daniel

Patch

diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
index 1a137dd..6eb1051 100644
--- a/lib/rendercopy_gen8.c
+++ b/lib/rendercopy_gen8.c
@@ -148,7 +148,8 @@  batch_copy(struct intel_batchbuffer *batch, const void *ptr, uint32_t size, uint
 
 static void
 gen6_render_flush(struct intel_batchbuffer *batch,
-		  drm_intel_context *context, uint32_t batch_end)
+		drm_intel_context *context, uint32_t batch_end,
+		int waiting)
 {
 	int ret;
 
@@ -157,6 +158,11 @@  gen6_render_flush(struct intel_batchbuffer *batch,
 		ret = drm_intel_gem_bo_context_exec(batch->bo, context,
 						    batch_end, 0);
 	assert(ret == 0);
+
+	if (waiting) {
+		dri_bo_map(batch->bo, 0);
+		dri_bo_unmap(batch->bo);
+	}
 }
 
 /* Mostly copy+paste from gen6, except height, width, pitch moved */
@@ -880,6 +886,15 @@  void gen8_render_copyfunc(struct intel_batchbuffer *batch,
 
 	intel_batchbuffer_flush_with_context(batch, context);
 
+	/* I don't know why it works !!! */
+	batch->ptr = batch->buffer;
+	OUT_BATCH(GEN6_PIPELINE_SELECT | PIPELINE_SELECT_3D);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	batch_end = batch_align(batch, 8);
+	assert(batch_end < BATCH_STATE_SPLIT);
+	gen6_render_flush(batch, context, batch_end, 1);
+	intel_batchbuffer_reset(batch);
+
 	batch_align(batch, 8);
 
 	batch->ptr = &batch->buffer[BATCH_STATE_SPLIT];
@@ -968,6 +983,6 @@  void gen8_render_copyfunc(struct intel_batchbuffer *batch,
 
 	annotation_flush(&aub_annotations, batch);
 
-	gen6_render_flush(batch, context, batch_end);
+	gen6_render_flush(batch, context, batch_end, 0);
 	intel_batchbuffer_reset(batch);
 }