| Message ID | 1439378761-9625-1-git-send-email-arun.siluvery@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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
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
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
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
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
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
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
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);
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(-)