[PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Arun Siluvery]

From Gen9, by default push constant command is not committed to the shader unit
untill the corresponding shader's BTP_* command is parsed. This is the
behaviour when set shader is enabled. This patch updates the batch to follow
this requirement otherwise it results in gpu hang.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959

Set shader need to be disabled if legacy behaviour is required.

Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---

If this patch is applied then we don't need to disable set shader bit
in kernel and the following can be discarded.

http://lists.freedesktop.org/archives/intel-gfx/2015-August/073425.html

 lib/rendercopy_gen9.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
index 4a4a604..9537480 100644
--- a/lib/rendercopy_gen9.c
+++ b/lib/rendercopy_gen9.c
@@ -590,13 +590,9 @@ gen8_emit_multisample(struct intel_batchbuffer *batch) {
 
 static void
 gen8_emit_vs(struct intel_batchbuffer *batch) {
-	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_VS);
+	OUT_BATCH(GEN6_3DSTATE_CONSTANT_VS | (11-2));
 	OUT_BATCH(0);
-
-	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_VS);
 	OUT_BATCH(0);
-
-	OUT_BATCH(GEN6_3DSTATE_CONSTANT_VS | (11-2));
 	OUT_BATCH(0);
 	OUT_BATCH(0);
 	OUT_BATCH(0);
@@ -605,7 +601,11 @@ gen8_emit_vs(struct intel_batchbuffer *batch) {
 	OUT_BATCH(0);
 	OUT_BATCH(0);
 	OUT_BATCH(0);
+
+	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_VS);
 	OUT_BATCH(0);
+
+	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_VS);
 	OUT_BATCH(0);
 
 	OUT_BATCH(GEN6_3DSTATE_VS | (9-2));
@@ -998,14 +998,14 @@ void gen9_render_copyfunc(struct intel_batchbuffer *batch,
 
 	gen8_emit_sf(batch);
 
+	gen8_emit_ps(batch, ps_kernel_off);
+
 	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_PS);
 	OUT_BATCH(ps_binding_table);
 
 	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_PS);
 	OUT_BATCH(ps_sampler_state);
 
-	gen8_emit_ps(batch, ps_kernel_off);
-
 	OUT_BATCH(GEN6_3DSTATE_SCISSOR_STATE_POINTERS);
 	OUT_BATCH(scissor_state);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Joonas Lahtinen]

On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> From Gen9, by default push constant command is not committed to the 
> shader unit
> untill the corresponding shader's BTP_* command is parsed. This is 
> the
> behaviour when set shader is enabled. This patch updates the batch to 
> follow
> this requirement otherwise it results in gpu hang.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
> 
> Set shader need to be disabled if legacy behaviour is required.
> 
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
> 
> If this patch is applied then we don't need to disable set shader bit
> in kernel and the following can be discarded.
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/073425.ht
> ml
> 
>  lib/rendercopy_gen9.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> index 4a4a604..9537480 100644
> --- a/lib/rendercopy_gen9.c
> +++ b/lib/rendercopy_gen9.c
> @@ -590,13 +590,9 @@ gen8_emit_multisample(struct intel_batchbuffer 
> *batch) {
>  
>  static void
>  gen8_emit_vs(struct intel_batchbuffer *batch) {
> -	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_VS);
> +	OUT_BATCH(GEN6_3DSTATE_CONSTANT_VS | (11-2));
>  	OUT_BATCH(0);
> -
> -	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_VS);
>  	OUT_BATCH(0);
> -
> -	OUT_BATCH(GEN6_3DSTATE_CONSTANT_VS | (11-2));
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
> @@ -605,7 +601,11 @@ gen8_emit_vs(struct intel_batchbuffer *batch) {
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
> +
> +	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_VS);
>  	OUT_BATCH(0);
> +
> +	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_VS);
>  	OUT_BATCH(0);
>  
>  	OUT_BATCH(GEN6_3DSTATE_VS | (9-2));
> @@ -998,14 +998,14 @@ void gen9_render_copyfunc(struct 
> intel_batchbuffer *batch,
>  
>  	gen8_emit_sf(batch);
>  
> +	gen8_emit_ps(batch, ps_kernel_off);
> +
>  	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_PS);
>  	OUT_BATCH(ps_binding_table);
>  
>  	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_PS);
>  	OUT_BATCH(ps_sampler_state);
>  
> -	gen8_emit_ps(batch, ps_kernel_off);
> -
>  	OUT_BATCH(GEN6_3DSTATE_SCISSOR_STATE_POINTERS);
>  	OUT_BATCH(scissor_state);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Ben Widawsky]

On Wed, Aug 12, 2015 at 03:10:18PM +0300, Joonas Lahtinen wrote:
> On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> > From Gen9, by default push constant command is not committed to the 
> > shader unit
> > untill the corresponding shader's BTP_* command is parsed. This is 
> > the
> > behaviour when set shader is enabled. This patch updates the batch to 
> > follow
> > this requirement otherwise it results in gpu hang.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
> > 
> > Set shader need to be disabled if legacy behaviour is required.
> > 
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 

Repeating what I said on the mesa thread:
Does anyone understand why this actually causes a hang on the IGT test? I
certainly don't. The docs are pretty clear that the constant command is not
committed until the BTP command, but I can't make any sense of how it related to
a GPU hang.

[snip]

---
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Joonas Lahtinen]

Hi,

On ke, 2015-08-12 at 18:35 -0700, Ben Widawsky wrote:
> On Wed, Aug 12, 2015 at 03:10:18PM +0300, Joonas Lahtinen wrote:
> > On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> > > From Gen9, by default push constant command is not committed to 
> > > the 
> > > shader unit
> > > untill the corresponding shader's BTP_* command is parsed. This 
> > > is 
> > > the
> > > behaviour when set shader is enabled. This patch updates the 
> > > batch to 
> > > follow
> > > this requirement otherwise it results in gpu hang.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
> > > 
> > > Set shader need to be disabled if legacy behaviour is required.
> > > 
> > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > 
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> 
> Repeating what I said on the mesa thread:
> Does anyone understand why this actually causes a hang on the IGT 
> test? I
> certainly don't. The docs are pretty clear that the constant command 
> is not
> committed until the BTP command, but I can't make any sense of how it 
> related to
> a GPU hang.
> 

Changing the order makes the hang go away and come back for sure, we've
all been experiencing that. System validation said it is a programming
restriction, so I'm not sure how relevant it is what goes wrong if it's
not followed. And the legacy mode bits were added to support the old
behavior of having the order like it has been previously, so I do not
see why question it without visibility to the actual RTL. And enabling
the legacy bits makes the hang go away, too.

If I had the RTL sources, then it would be more relevant to take
educated guesses as to why a set of hundreds of thousands of
transistors doesn't work as it should :) Without that, if it gets
stuck, it gets stuck.

Regards, Joonas

> [snip]
> 
> ---
> Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Ben Widawsky]

On Thu, Aug 13, 2015 at 10:33:00AM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> On ke, 2015-08-12 at 18:35 -0700, Ben Widawsky wrote:
> > On Wed, Aug 12, 2015 at 03:10:18PM +0300, Joonas Lahtinen wrote:
> > > On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> > > > From Gen9, by default push constant command is not committed to 
> > > > the 
> > > > shader unit
> > > > untill the corresponding shader's BTP_* command is parsed. This 
> > > > is 
> > > > the
> > > > behaviour when set shader is enabled. This patch updates the 
> > > > batch to 
> > > > follow
> > > > this requirement otherwise it results in gpu hang.
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
> > > > 
> > > > Set shader need to be disabled if legacy behaviour is required.
> > > > 
> > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > > 
> > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > 
> > 
> > Repeating what I said on the mesa thread:
> > Does anyone understand why this actually causes a hang on the IGT 
> > test? I
> > certainly don't. The docs are pretty clear that the constant command 
> > is not
> > committed until the BTP command, but I can't make any sense of how it 
> > related to
> > a GPU hang.
> > 
> 
> Changing the order makes the hang go away and come back for sure, we've
> all been experiencing that. System validation said it is a programming
> restriction, so I'm not sure how relevant it is what goes wrong if it's
> not followed. And the legacy mode bits were added to support the old
> behavior of having the order like it has been previously, so I do not
> see why question it without visibility to the actual RTL. And enabling
> the legacy bits makes the hang go away, too.
> 
> If I had the RTL sources, then it would be more relevant to take
> educated guesses as to why a set of hundreds of thousands of
> transistors doesn't work as it should :) Without that, if it gets
> stuck, it gets stuck.
> 
> Regards, Joonas
> 

Let me start by saying I do believe that questioning this shouldn't prevent
merging the patch.

<rant>
I absolutely disagree with you and think we should question these kind of things
and get out of the mindset of, "well, it fixes a hang, let's move on."
Understanding these kind of things is critical to writing stable drivers.  If
the programming guide/SV team said it can lead to a hang, that's one thing, but
AFAICT, we do not understand why it is hanging nor does any of the documentation
we do have suggest it should hang. Without clarification, next time we have a
similar hang signature we're going to be right back here where we started.

It was one thing when there were a handful of us working on the stuff and we
didn't have time to get to the bottom of bugs like this. I'm guilty of patches
like this myself. I really do not see any excuse for this any more though.
</rant>

Could you send me the reference for where SV said it was a "programming
restriction"? To me it all sounds very much like an implementation detail, and
I'd like to try to understand what I am missing.

> > [snip]
> > 
> > ---
> > Ben Widawsky, Intel Open Source Technology Center

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Daniel Vetter]

On Thu, Aug 13, 2015 at 03:49:35PM -0700, Ben Widawsky wrote:
> On Thu, Aug 13, 2015 at 10:33:00AM +0300, Joonas Lahtinen wrote:
> > Hi,
> > 
> > On ke, 2015-08-12 at 18:35 -0700, Ben Widawsky wrote:
> > > On Wed, Aug 12, 2015 at 03:10:18PM +0300, Joonas Lahtinen wrote:
> > > > On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> > > > > From Gen9, by default push constant command is not committed to 
> > > > > the 
> > > > > shader unit
> > > > > untill the corresponding shader's BTP_* command is parsed. This 
> > > > > is 
> > > > > the
> > > > > behaviour when set shader is enabled. This patch updates the 
> > > > > batch to 
> > > > > follow
> > > > > this requirement otherwise it results in gpu hang.
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
> > > > > 
> > > > > Set shader need to be disabled if legacy behaviour is required.
> > > > > 
> > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > > > 
> > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > 
> > > 
> > > Repeating what I said on the mesa thread:
> > > Does anyone understand why this actually causes a hang on the IGT 
> > > test? I
> > > certainly don't. The docs are pretty clear that the constant command 
> > > is not
> > > committed until the BTP command, but I can't make any sense of how it 
> > > related to
> > > a GPU hang.
> > > 
> > 
> > Changing the order makes the hang go away and come back for sure, we've
> > all been experiencing that. System validation said it is a programming
> > restriction, so I'm not sure how relevant it is what goes wrong if it's
> > not followed. And the legacy mode bits were added to support the old
> > behavior of having the order like it has been previously, so I do not
> > see why question it without visibility to the actual RTL. And enabling
> > the legacy bits makes the hang go away, too.
> > 
> > If I had the RTL sources, then it would be more relevant to take
> > educated guesses as to why a set of hundreds of thousands of
> > transistors doesn't work as it should :) Without that, if it gets
> > stuck, it gets stuck.
> > 
> > Regards, Joonas
> > 
> 
> Let me start by saying I do believe that questioning this shouldn't prevent
> merging the patch.
> 
> <rant>
> I absolutely disagree with you and think we should question these kind of things
> and get out of the mindset of, "well, it fixes a hang, let's move on."
> Understanding these kind of things is critical to writing stable drivers.  If
> the programming guide/SV team said it can lead to a hang, that's one thing, but
> AFAICT, we do not understand why it is hanging nor does any of the documentation
> we do have suggest it should hang. Without clarification, next time we have a
> similar hang signature we're going to be right back here where we started.
> 
> It was one thing when there were a handful of us working on the stuff and we
> didn't have time to get to the bottom of bugs like this. I'm guilty of patches
> like this myself. I really do not see any excuse for this any more though.
> </rant>
> 
> Could you send me the reference for where SV said it was a "programming
> restriction"? To me it all sounds very much like an implementation detail, and
> I'd like to try to understand what I am missing.

Fully agree, we can't just ignore hangs but have to bottom out on them.
And in the past we've had piles of examples where we discovered something
and didn't chase it correctly, then a few months later massive panic in
intel. At least this must be reflected in bspec.

Joonas, can you please work together with Ben to chase this down? If you
don't get traction please escalate the shit out of this,  we really must
at least get docs to accurately reflect reality.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Joonas Lahtinen]

Hi,

On pe, 2015-08-14 at 10:58 +0200, Daniel Vetter wrote:
> On Thu, Aug 13, 2015 at 03:49:35PM -0700, Ben Widawsky wrote:
> > On Thu, Aug 13, 2015 at 10:33:00AM +0300, Joonas Lahtinen wrote:
> > > Hi,
> > > 
> > > On ke, 2015-08-12 at 18:35 -0700, Ben Widawsky wrote:
> > > > On Wed, Aug 12, 2015 at 03:10:18PM +0300, Joonas Lahtinen 
> > > > wrote:
> > > > > On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> > > > > > From Gen9, by default push constant command is not 
> > > > > > committed to 
> > > > > > the 
> > > > > > shader unit
> > > > > > untill the corresponding shader's BTP_* command is parsed. 
> > > > > > This 
> > > > > > is 
> > > > > > the
> > > > > > behaviour when set shader is enabled. This patch updates 
> > > > > > the 
> > > > > > batch to 
> > > > > > follow
> > > > > > this requirement otherwise it results in gpu hang.
> > > > > > 
> > > > > > Bugzilla: 
> > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=89959
> > > > > > 
> > > > > > Set shader need to be disabled if legacy behaviour is 
> > > > > > required.
> > > > > > 
> > > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com
> > > > > > >
> > > > > 
> > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com
> > > > > >
> > > > > 
> > > > 
> > > > Repeating what I said on the mesa thread:
> > > > Does anyone understand why this actually causes a hang on the 
> > > > IGT 
> > > > test? I
> > > > certainly don't. The docs are pretty clear that the constant 
> > > > command 
> > > > is not
> > > > committed until the BTP command, but I can't make any sense of 
> > > > how it 
> > > > related to
> > > > a GPU hang.
> > > > 
> > > 
> > > Changing the order makes the hang go away and come back for sure, 
> > > we've
> > > all been experiencing that. System validation said it is a 
> > > programming
> > > restriction, so I'm not sure how relevant it is what goes wrong 
> > > if it's
> > > not followed. And the legacy mode bits were added to support the 
> > > old
> > > behavior of having the order like it has been previously, so I do 
> > > not
> > > see why question it without visibility to the actual RTL. And 
> > > enabling
> > > the legacy bits makes the hang go away, too.
> > > 
> > > If I had the RTL sources, then it would be more relevant to take
> > > educated guesses as to why a set of hundreds of thousands of
> > > transistors doesn't work as it should :) Without that, if it gets
> > > stuck, it gets stuck.
> > > 
> > > Regards, Joonas
> > > 
> > 
> > Let me start by saying I do believe that questioning this shouldn't 
> > prevent
> > merging the patch.
> > 
> > <rant>
> > I absolutely disagree with you and think we should question these 
> > kind of things
> > and get out of the mindset of, "well, it fixes a hang, let's move 
> > on."
> > Understanding these kind of things is critical to writing stable 
> > drivers.  If
> > the programming guide/SV team said it can lead to a hang, that's 
> > one thing, but
> > AFAICT, we do not understand why it is hanging nor does any of the 
> > documentation
> > we do have suggest it should hang. Without clarification, next time 
> > we have a
> > similar hang signature we're going to be right back here where we 
> > started.
> > 
> > It was one thing when there were a handful of us working on the 
> > stuff and we
> > didn't have time to get to the bottom of bugs like this. I'm guilty 
> > of patches
> > like this myself. I really do not see any excuse for this any more 
> > though.
> > </rant>
> > 
> > Could you send me the reference for where SV said it was a 
> > "programming
> > restriction"? To me it all sounds very much like an implementation 
> > detail, and
> > I'd like to try to understand what I am missing.
> 
> Fully agree, we can't just ignore hangs but have to bottom out on 
> them.
> And in the past we've had piles of examples where we discovered 
> something
> and didn't chase it correctly, then a few months later massive panic 
> in
> intel. At least this must be reflected in bspec.
> 
> Joonas, can you please work together with Ben to chase this down? If 
> you
> don't get traction please escalate the shit out of this,  we really 
> must
> at least get docs to accurately reflect reality.
> 

The latest reply from SV team to Arun's inquiry is that when the order
is not followed, the previous push constant values get used (which will
be all zeroes, so doesn't really make sense) and would lead to the hang
or corruption. I'm checking them if the same applies for the BTP itself
too, where the PS pointer keeps changing (and it's the only changing
thing), and could explain it better.

Regards, Joonas

> Thanks, Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Ben Widawsky]

On Tue, Aug 18, 2015 at 05:25:55PM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> On pe, 2015-08-14 at 10:58 +0200, Daniel Vetter wrote:
> > On Thu, Aug 13, 2015 at 03:49:35PM -0700, Ben Widawsky wrote:
> > > On Thu, Aug 13, 2015 at 10:33:00AM +0300, Joonas Lahtinen wrote:
> > > > Hi,
> > > > 
> > > > On ke, 2015-08-12 at 18:35 -0700, Ben Widawsky wrote:
> > > > > On Wed, Aug 12, 2015 at 03:10:18PM +0300, Joonas Lahtinen 
> > > > > wrote:
> > > > > > On ke, 2015-08-12 at 12:26 +0100, Arun Siluvery wrote:
> > > > > > > From Gen9, by default push constant command is not 
> > > > > > > committed to 
> > > > > > > the 
> > > > > > > shader unit
> > > > > > > untill the corresponding shader's BTP_* command is parsed. 
> > > > > > > This 
> > > > > > > is 
> > > > > > > the
> > > > > > > behaviour when set shader is enabled. This patch updates 
> > > > > > > the 
> > > > > > > batch to 
> > > > > > > follow
> > > > > > > this requirement otherwise it results in gpu hang.
> > > > > > > 
> > > > > > > Bugzilla: 
> > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=89959
> > > > > > > 
> > > > > > > Set shader need to be disabled if legacy behaviour is 
> > > > > > > required.
> > > > > > > 
> > > > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > > > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com
> > > > > > > >
> > > > > > 
> > > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com
> > > > > > >
> > > > > > 
> > > > > 
> > > > > Repeating what I said on the mesa thread:
> > > > > Does anyone understand why this actually causes a hang on the 
> > > > > IGT 
> > > > > test? I
> > > > > certainly don't. The docs are pretty clear that the constant 
> > > > > command 
> > > > > is not
> > > > > committed until the BTP command, but I can't make any sense of 
> > > > > how it 
> > > > > related to
> > > > > a GPU hang.
> > > > > 
> > > > 
> > > > Changing the order makes the hang go away and come back for sure, 
> > > > we've
> > > > all been experiencing that. System validation said it is a 
> > > > programming
> > > > restriction, so I'm not sure how relevant it is what goes wrong 
> > > > if it's
> > > > not followed. And the legacy mode bits were added to support the 
> > > > old
> > > > behavior of having the order like it has been previously, so I do 
> > > > not
> > > > see why question it without visibility to the actual RTL. And 
> > > > enabling
> > > > the legacy bits makes the hang go away, too.
> > > > 
> > > > If I had the RTL sources, then it would be more relevant to take
> > > > educated guesses as to why a set of hundreds of thousands of
> > > > transistors doesn't work as it should :) Without that, if it gets
> > > > stuck, it gets stuck.
> > > > 
> > > > Regards, Joonas
> > > > 
> > > 
> > > Let me start by saying I do believe that questioning this shouldn't 
> > > prevent
> > > merging the patch.
> > > 
> > > <rant>
> > > I absolutely disagree with you and think we should question these 
> > > kind of things
> > > and get out of the mindset of, "well, it fixes a hang, let's move 
> > > on."
> > > Understanding these kind of things is critical to writing stable 
> > > drivers.  If
> > > the programming guide/SV team said it can lead to a hang, that's 
> > > one thing, but
> > > AFAICT, we do not understand why it is hanging nor does any of the 
> > > documentation
> > > we do have suggest it should hang. Without clarification, next time 
> > > we have a
> > > similar hang signature we're going to be right back here where we 
> > > started.
> > > 
> > > It was one thing when there were a handful of us working on the 
> > > stuff and we
> > > didn't have time to get to the bottom of bugs like this. I'm guilty 
> > > of patches
> > > like this myself. I really do not see any excuse for this any more 
> > > though.
> > > </rant>
> > > 
> > > Could you send me the reference for where SV said it was a 
> > > "programming
> > > restriction"? To me it all sounds very much like an implementation 
> > > detail, and
> > > I'd like to try to understand what I am missing.
> > 
> > Fully agree, we can't just ignore hangs but have to bottom out on 
> > them.
> > And in the past we've had piles of examples where we discovered 
> > something
> > and didn't chase it correctly, then a few months later massive panic 
> > in
> > intel. At least this must be reflected in bspec.
> > 
> > Joonas, can you please work together with Ben to chase this down? If 
> > you
> > don't get traction please escalate the shit out of this,  we really 
> > must
> > at least get docs to accurately reflect reality.
> > 
> 
> The latest reply from SV team to Arun's inquiry is that when the order
> is not followed, the previous push constant values get used (which will
> be all zeroes, so doesn't really make sense) and would lead to the hang
> or corruption. I'm checking them if the same applies for the BTP itself
> too, where the PS pointer keeps changing (and it's the only changing
> thing), and could explain it better.
> 
> Regards, Joonas

Cool. It seems like an interesting conversation. I managed to convince myself
corruption can happen, maybe - but not a hang. Now that you've kicked off that
conversation, I'm fine with merging the patch (we've done our "due diligence"
/me shudders).

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> 
> > Thanks, Daniel

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib/rendercopy_gen9: Setup Push constant pointer before sending BTP commands

[Mika Kuoppala]

Arun Siluvery <arun.siluvery@linux.intel.com> writes:

> From Gen9, by default push constant command is not committed to the shader unit
> untill the corresponding shader's BTP_* command is parsed. This is the
> behaviour when set shader is enabled. This patch updates the batch to follow
> this requirement otherwise it results in gpu hang.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89959
>
> Set shader need to be disabled if legacy behaviour is required.
>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>

with skl-y,

Tested-by: Mika Kuoppala <mika.kuoppala@intel.com>

> If this patch is applied then we don't need to disable set shader bit
> in kernel and the following can be discarded.
>
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/073425.html
>
>  lib/rendercopy_gen9.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/lib/rendercopy_gen9.c b/lib/rendercopy_gen9.c
> index 4a4a604..9537480 100644
> --- a/lib/rendercopy_gen9.c
> +++ b/lib/rendercopy_gen9.c
> @@ -590,13 +590,9 @@ gen8_emit_multisample(struct intel_batchbuffer *batch) {
>  
>  static void
>  gen8_emit_vs(struct intel_batchbuffer *batch) {
> -	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_VS);
> +	OUT_BATCH(GEN6_3DSTATE_CONSTANT_VS | (11-2));
>  	OUT_BATCH(0);
> -
> -	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_VS);
>  	OUT_BATCH(0);
> -
> -	OUT_BATCH(GEN6_3DSTATE_CONSTANT_VS | (11-2));
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
> @@ -605,7 +601,11 @@ gen8_emit_vs(struct intel_batchbuffer *batch) {
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
>  	OUT_BATCH(0);
> +
> +	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_VS);
>  	OUT_BATCH(0);
> +
> +	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_VS);
>  	OUT_BATCH(0);
>  
>  	OUT_BATCH(GEN6_3DSTATE_VS | (9-2));
> @@ -998,14 +998,14 @@ void gen9_render_copyfunc(struct intel_batchbuffer *batch,
>  
>  	gen8_emit_sf(batch);
>  
> +	gen8_emit_ps(batch, ps_kernel_off);
> +
>  	OUT_BATCH(GEN7_3DSTATE_BINDING_TABLE_POINTERS_PS);
>  	OUT_BATCH(ps_binding_table);
>  
>  	OUT_BATCH(GEN7_3DSTATE_SAMPLER_STATE_POINTERS_PS);
>  	OUT_BATCH(ps_sampler_state);
>  
> -	gen8_emit_ps(batch, ps_kernel_off);
> -
>  	OUT_BATCH(GEN6_3DSTATE_SCISSOR_STATE_POINTERS);
>  	OUT_BATCH(scissor_state);
>  
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx