On Sat, Jul 1, 2017 at 8:13 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Monday, June 26, 2017 4:22:35 PM PDT Ian Romanick wrote: > > From: Jason Ekstrand <jason.ekstr...@intel.com> > > > > It's a bit rare, but blorp can trigger a urb reconfiguration. When that > > happens, we need to re-upload the URB config. Fortunately, this isn't as > > bad as it looks because gen7_upload_urb will not re-emit the packet if it > > would end up being a no-op so this doesn't mean that running blorp always > > triggers a URB reconfig. > > > > v2 (idr): Sort BRW_NEW_ tokens to match brw_recalculate_urb_fence and > > gen6_urb. > > > > v3 (idr): Don't whack BRW_NEW_URB_SIZE in blorp. Suggested by Jason. > > --- > > src/mesa/drivers/dri/i965/gen7_urb.c | 3 ++- > > src/mesa/drivers/dri/i965/genX_blorp_exec.c | 2 -- > > 2 files changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/gen7_urb.c > b/src/mesa/drivers/dri/i965/gen7_urb.c > > index 525c9c4..c4b479c 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_urb.c > > +++ b/src/mesa/drivers/dri/i965/gen7_urb.c > > @@ -236,7 +236,8 @@ gen7_upload_urb(struct brw_context *brw, unsigned > vs_size, > > const struct brw_tracked_state gen7_urb = { > > .dirty = { > > .mesa = 0, > > - .brw = BRW_NEW_CONTEXT | > > + .brw = BRW_NEW_BLORP | > > + BRW_NEW_CONTEXT | > > BRW_NEW_URB_SIZE | > > BRW_NEW_GS_PROG_DATA | > > BRW_NEW_TCS_PROG_DATA | > > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c > > index 8fd17fb..af3d609 100644 > > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c > > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c > > @@ -183,8 +183,6 @@ blorp_emit_urb_config(struct blorp_batch *batch, > > brw->urb.vsize >= vs_entry_size) > > return; > > > > - brw->ctx.NewDriverState |= BRW_NEW_URB_SIZE; > > - > > gen7_upload_urb(brw, vs_entry_size, false, false); > > #elif GEN_GEN == 6 > > gen6_upload_urb(brw, vs_entry_size, false, 0); > > > > The commit message is a bit confusing. It makes it sound like we were > failing to re-emit 3DSTATE_URB_* from the main drawing path after BLORP > trashed it. However, the brw->ctx.NewDriverState |= BRW_NEW_URB_SIZE > line that you deleted here guaranteed that it would happen. > > The real benefit appears to be that flagging BRW_NEW_URB_SIZE ("the > total size of the URB portion of the L3 cache has changed") causes > BLORP to think it needs to re-configure it again, so say, back-to-back > BLORP operations would configure it every time. > You are correct, sir. Originally the patch didn't have the second hunk and the commit message made sense even though it was wrong. With the hunk added, a new commit message would be good. > Using BRW_NEW_BLORP is definitely better - that's what it's there for. > > Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev