On Friday, August 18, 2017 4:21:39 PM PDT Jason Ekstrand wrote: > Looking at NewDriverState is not safe in general. The state atom system > is set up to ensure that new bits that get added to NewDriverState get > accumulated into the set of bits used when emitting atoms but it doesn't > go the other way. If we read NewDriverState, we may not get the full > picture because the per-pipeline state (3D or compute) does not get > added to NewDriverState before state emit is done. It's especially > dangerous to do this from BLORP (either explicitly or implicitly when > BLORP calls gen7_upload_urb) because that does not happen during one of > the normal state upload paths. > > This commit solves the problem by whacking all of the per-shader-stage > URB sizes to zero whenever we change the total URB size. We still have > to flag BRW_NEW_URB_SIZE to ensure that the gen7_urb atom triggers but > the actual decision in gen7_upload_urb can now be based entirely on URB > sizes rather than on state atoms. This also makes BLORP correct because > it just asks for a new URB config whenever the vsize is too small and so > any change to the total URB size will trigger blorp to re-emit as well > because 0 < vs_entry_size. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102289 > Cc: Kenneth Graunke <[email protected]> > Cc: [email protected] > --- > src/mesa/drivers/dri/i965/gen7_l3_state.c | 9 +++++++++ > src/mesa/drivers/dri/i965/gen7_urb.c | 4 +--- > src/mesa/drivers/dri/i965/genX_blorp_exec.c | 3 +-- > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/gen7_l3_state.c > b/src/mesa/drivers/dri/i965/gen7_l3_state.c > index 536c00c..53638eb 100644 > --- a/src/mesa/drivers/dri/i965/gen7_l3_state.c > +++ b/src/mesa/drivers/dri/i965/gen7_l3_state.c > @@ -204,6 +204,15 @@ update_urb_size(struct brw_context *brw, const struct > gen_l3_config *cfg) > if (brw->urb.size != sz) { > brw->urb.size = sz; > brw->ctx.NewDriverState |= BRW_NEW_URB_SIZE; > + > + /* If we change the total URB size, reset the individual stage sizes to > + * zero so that, even if there is no URB size change, gen7_upload_urb > + * still re-emits 3DSTATE_URB_*. > + */ > + brw->urb.vsize = 0; > + brw->urb.gsize = 0; > + brw->urb.hsize = 0; > + brw->urb.dsize = 0; > } > } > > diff --git a/src/mesa/drivers/dri/i965/gen7_urb.c > b/src/mesa/drivers/dri/i965/gen7_urb.c > index d5b03ef..06113fa 100644 > --- a/src/mesa/drivers/dri/i965/gen7_urb.c > +++ b/src/mesa/drivers/dri/i965/gen7_urb.c > @@ -201,9 +201,7 @@ gen7_upload_urb(struct brw_context *brw, unsigned vs_size, > /* If we're just switching between programs with the same URB > requirements, > * skip the rest of the logic. > */ > - if (!(brw->ctx.NewDriverState & BRW_NEW_CONTEXT) && > - !(brw->ctx.NewDriverState & BRW_NEW_URB_SIZE) && > - brw->urb.vsize == entry_size[MESA_SHADER_VERTEX] && > + if (brw->urb.vsize == entry_size[MESA_SHADER_VERTEX] && > brw->urb.gs_present == gs_present && > brw->urb.gsize == entry_size[MESA_SHADER_GEOMETRY] && > brw->urb.tess_present == tess_present && > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c > index c6eee4c..62d5c4a 100644 > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c > @@ -179,8 +179,7 @@ blorp_emit_urb_config(struct blorp_batch *batch, > struct brw_context *brw = batch->driver_batch; > > #if GEN_GEN >= 7 > - if (!(brw->ctx.NewDriverState & (BRW_NEW_CONTEXT | BRW_NEW_URB_SIZE)) && > - brw->urb.vsize >= vs_entry_size) > + if (brw->urb.vsize >= vs_entry_size) > return; > > gen7_upload_urb(brw, vs_entry_size, false, false); >
Reviewed-by: Kenneth Graunke <[email protected]>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
