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.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
