[Intel gfx][i-g-t PATCH 1/4] lib: Clean the batch buffer store after reset

[Intel gfx][i-g-t PATCH 1/4] lib: Clean the batch buffer store after reset

[Xiang, Haihao]

From: "Xiang, Haihao" <haihao.xiang@intel.com>

Otherwise the stale data in the buffer

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
---
 lib/intel_batchbuffer.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 06a5437..9ce7424 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -50,6 +50,8 @@ intel_batchbuffer_reset(struct intel_batchbuffer *batch)
 	batch->bo = drm_intel_bo_alloc(batch->bufmgr, "batchbuffer",
 				       BATCH_SZ, 4096);
 
+	memset(batch->buffer, 0, sizeof(batch->buffer));
+
 	batch->ptr = batch->buffer;
 }
 
-- 
1.7.9.5

[Intel gfx][i-g-t PATCH 2/4] rendercopy/bdw: Set Instruction Buffer size Modify Enable to 1

[Xiang, Haihao]

From: "Xiang, Haihao" <haihao.xiang@intel.com>

Otherwise it may result in GPU hang

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
---
 lib/rendercopy_gen8.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
index 43e962c..1a137dd 100644
--- a/lib/rendercopy_gen8.c
+++ b/lib/rendercopy_gen8.c
@@ -526,7 +526,7 @@ gen8_emit_state_base_address(struct intel_batchbuffer *batch) {
 	/* indirect object buffer size */
 	OUT_BATCH(0xfffff000 | 1);
 	/* intruction buffer size */
-	OUT_BATCH(1 << 12);
+	OUT_BATCH(1 << 12 | 1);
 }
 
 static void
-- 
1.7.9.5

Re: [Intel gfx][i-g-t PATCH 2/4] rendercopy/bdw: Set Instruction Buffer size Modify Enable to 1

[Kenneth Graunke]

On 12/06/2013 12:54 AM, Xiang, Haihao wrote:
> From: "Xiang, Haihao" <haihao.xiang@intel.com>
> 
> Otherwise it may result in GPU hang
> 
> Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
> ---
>  lib/rendercopy_gen8.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
> index 43e962c..1a137dd 100644
> --- a/lib/rendercopy_gen8.c
> +++ b/lib/rendercopy_gen8.c
> @@ -526,7 +526,7 @@ gen8_emit_state_base_address(struct intel_batchbuffer *batch) {
>  	/* indirect object buffer size */
>  	OUT_BATCH(0xfffff000 | 1);
>  	/* intruction buffer size */
> -	OUT_BATCH(1 << 12);
> +	OUT_BATCH(1 << 12 | 1);
>  }
>  
>  static void
> 

Thanks a ton for finding this!

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Re: [Intel gfx][i-g-t PATCH 2/4] rendercopy/bdw: Set Instruction Buffer size Modify Enable to 1

[Damien Lespiau]

On Fri, Dec 06, 2013 at 02:15:18AM -0800, Kenneth Graunke wrote:
> On 12/06/2013 12:54 AM, Xiang, Haihao wrote:
> > From: "Xiang, Haihao" <haihao.xiang@intel.com>
> > 
> > Otherwise it may result in GPU hang
> > 
> > Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
> > ---
> >  lib/rendercopy_gen8.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
> > index 43e962c..1a137dd 100644
> > --- a/lib/rendercopy_gen8.c
> > +++ b/lib/rendercopy_gen8.c
> > @@ -526,7 +526,7 @@ gen8_emit_state_base_address(struct intel_batchbuffer *batch) {
> >  	/* indirect object buffer size */
> >  	OUT_BATCH(0xfffff000 | 1);
> >  	/* intruction buffer size */
> > -	OUT_BATCH(1 << 12);
> > +	OUT_BATCH(1 << 12 | 1);
> >  }
> >  
> >  static void
> > 
> 
> Thanks a ton for finding this!
> 
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Nice catch! Thanks for the patch and review, pushed.

-- 
Damien

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

[Xiang, Haihao]

From: "Xiang, Haihao" <haihao.xiang@intel.com>

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 <haihao.xiang@intel.com>
---
 lib/rendercopy_gen8.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

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);
 }
-- 
1.7.9.5

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

[Damien Lespiau]

On Fri, Dec 06, 2013 at 04:54:46PM +0800, Xiang, Haihao wrote:
> From: "Xiang, Haihao" <haihao.xiang@intel.com>
> 
> 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.

-- 
Damien

> 
> Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
> ---
>  lib/rendercopy_gen8.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> 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);
>  }
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Xiang, Haihao]

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" <haihao.xiang@intel.com>
> > 
> > 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.

> 

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

[Daniel Vetter]

On Mon, Dec 9, 2013 at 5:47 AM, Xiang, Haihao <haihao.xiang@intel.com> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Intel gfx][i-g-t PATCH 4/4] Revert "gen8 rendercpy: temporarily disable"

[Xiang, Haihao]

From: "Xiang, Haihao" <haihao.xiang@intel.com>

This reverts commit e41928e6c9bb3f24833a827903f1afeda83592d6.

Now the case no longer causes GPU hang on GEN
---
 lib/rendercopy_i830.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c
index 5dd67b2..73edcfa 100644
--- a/lib/rendercopy_i830.c
+++ b/lib/rendercopy_i830.c
@@ -241,10 +241,8 @@ render_copyfunc_t get_render_copyfunc(int devid)
 		copy = gen6_render_copyfunc;
 	else if (IS_GEN7(devid))
 		copy = gen7_render_copyfunc;
-	else if (IS_GEN8(devid)) {
-		fprintf(stderr, "Temporarily disabled\n");
-		//copy = gen8_render_copyfunc;
-	}
+	else if (IS_GEN8(devid))
+		copy = gen8_render_copyfunc;
 
 	return copy;
 }
-- 
1.7.9.5

Re: [Intel gfx][i-g-t PATCH 1/4] lib: Clean the batch buffer store after reset

[Kenneth Graunke]

On 12/06/2013 12:54 AM, Xiang, Haihao wrote:
> From: "Xiang, Haihao" <haihao.xiang@intel.com>
> 
> Otherwise the stale data in the buffer
> 
> Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
> ---
>  lib/intel_batchbuffer.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index 06a5437..9ce7424 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -50,6 +50,8 @@ intel_batchbuffer_reset(struct intel_batchbuffer *batch)
>  	batch->bo = drm_intel_bo_alloc(batch->bufmgr, "batchbuffer",
>  				       BATCH_SZ, 4096);
>  
> +	memset(batch->buffer, 0, sizeof(batch->buffer));
> +
>  	batch->ptr = batch->buffer;
>  }
>  
> 

I don't think that should be harmful, but this would definitely make
debugging nicer.  For intel-gpu-tools, I think it makes sense.

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Re: [Intel gfx][i-g-t PATCH 1/4] lib: Clean the batch buffer store after reset

[Damien Lespiau]

On Fri, Dec 06, 2013 at 02:14:52AM -0800, Kenneth Graunke wrote:
> On 12/06/2013 12:54 AM, Xiang, Haihao wrote:
> > From: "Xiang, Haihao" <haihao.xiang@intel.com>
> > 
> > Otherwise the stale data in the buffer
> > 
> > Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
> > ---
> >  lib/intel_batchbuffer.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> > index 06a5437..9ce7424 100644
> > --- a/lib/intel_batchbuffer.c
> > +++ b/lib/intel_batchbuffer.c
> > @@ -50,6 +50,8 @@ intel_batchbuffer_reset(struct intel_batchbuffer *batch)
> >  	batch->bo = drm_intel_bo_alloc(batch->bufmgr, "batchbuffer",
> >  				       BATCH_SZ, 4096);
> >  
> > +	memset(batch->buffer, 0, sizeof(batch->buffer));
> > +
> >  	batch->ptr = batch->buffer;
> >  }
> >  
> > 
> 
> I don't think that should be harmful, but this would definitely make
> debugging nicer.  For intel-gpu-tools, I think it makes sense.
> 
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Indeed, I think it makes sense as well, thanks for the patch and review, pushed.

-- 
Damien