[Mesa-dev] [PATCH] i915: BINDING_TABLE_POINTER_* after CONSTANT_* for SKL & BXT

[Joonas Lahtinen]

Hi,

This could then be applied with the correction s/i915/i965/ in the
commit message, right?

Regards, Joonas

On to, 2015-08-13 at 09:06 -0700, Ben Widawsky wrote:
> On Thu, Aug 13, 2015 at 10:38:43AM +0300, Joonas Lahtinen wrote:
> > Hi,
> > 
> > On ke, 2015-08-12 at 18:34 -0700, Ben Widawsky wrote:
> > > On Wed, Aug 12, 2015 at 03:09:44PM +0300, Joonas Lahtinen wrote:
> > > > Add a comment about reinforcing command order so that
> > > > 3DSTATE_BINDING_TABLE_POINTER_* commands are after
> > > > 3DSTATE_CONSTANT_* commands for SKL & BXT, otherwise the
> > > > GPU might hang.
> > > > 
> > > > Changing the BLORP code is not relevant (where the order
> > > > is "wrong"), as it is not used for GEN8 or up.
> > > > 
> > > > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > > > Cc: Arun Siluvery <arun.siluvery at linux.intel.com>
> > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com
> > > > >
> > > > ---
> > > >  src/mesa/drivers/dri/i965/brw_state_upload.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c 
> > > > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > index 9de42ce..9078e11 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > @@ -299,9 +299,9 @@ static const struct brw_tracked_state 
> > > > *gen8_render_atoms[] =
> > > >     &brw_wm_abo_surfaces,
> > > >     &gen6_renderbuffer_surfaces,
> > > >     &brw_texture_surfaces,
> > > > -   &brw_vs_binding_table,
> > > > -   &brw_gs_binding_table,
> > > > -   &brw_wm_binding_table,
> > > > +   &brw_vs_binding_table, /* Must come after vs_push_constants
> > > > for 
> > > > Skylake and Broxton. */
> > > > +   &brw_gs_binding_table, /* Must come after gs_push_constants
> > > > for 
> > > > Skylake and Broxton. */
> > > > +   &brw_wm_binding_table, /* Must come after wm_push_constants
> > > > for 
> > > > Skylake and Broxton. */
> > > >  
> > > >     &brw_fs_samplers,
> > > >     &brw_vs_samplers,
> > > 
> > > 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.
> > > 
> > 
> > Discussion about this continued in the driver list.
> > 
> > > In any event, I don't think the comments are super useful, but 
> > > they're not
> > > harmful either. I'd suggest one line instead:
> > > "NOTE: push_constant_ff must precede binding table pointer
> > > upload"
> > > 
> > 
> > The table previously seemed to contain per-line comments for other
> > ordering restrictions, so I just went with style that looks
> > consitent
> > with the rest. Also it makes some sense, as it's only the
> > respective
> > 3DSTATE_CONSTANT_* whose parsing is triggered by matching a
> > 3DSTATE_BINDING_TABLE_POINTER_* command. They could be interleaved
> > too.
> > 
> > I'll correct the s/i915/i965/ as noted by Matt. How about the
> > comments?
> 
> Whatever you like. Ideally we'd probably try to capture such things
> in the atom
> debugging, but I did look at that code, and it seems like it'd be a
> pain to add
> for no real gain.
> 
> > 
> > Regards, Joonas
> > 
> > > Reviewed-by: Ben Widawsky <ben at bwidawsk.net>