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.kuopp...@linux.intel.com> > > > Cc: Arun Siluvery <arun.siluv...@linux.intel.com> > > > Signed-off-by: Joonas Lahtinen <joonas.lahti...@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 <b...@bwidawsk.net> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
