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

[Ben Widawsky]

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>