[Mesa-dev] [PATCH 02/12] i965/urb: Trigger upload_urb on NEW_BLORP

[Jason Ekstrand]

On Sat, Jul 1, 2017 at 8:13 PM, Kenneth Graunke <kenneth at whitecape.org>
wrote:

> On Monday, June 26, 2017 4:22:35 PM PDT Ian Romanick wrote:
> > From: Jason Ekstrand <jason.ekstrand at 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 <kenneth at whitecape.org>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170702/da3c6594/attachment-0001.html>