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

[Joonas Lahtinen]

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?

Regards, Joonas

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