<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Jul 1, 2017 at 8:13 PM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Monday, June 26, 2017 4:22:35 PM PDT Ian Romanick wrote:<br>
> From: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>><br>
><br>
> It's a bit rare, but blorp can trigger a urb reconfiguration.  When that<br>
> happens, we need to re-upload the URB config.  Fortunately, this isn't as<br>
> bad as it looks because gen7_upload_urb will not re-emit the packet if it<br>
> would end up being a no-op so this doesn't mean that running blorp always<br>
> triggers a URB reconfig.<br>
><br>
> v2 (idr): Sort BRW_NEW_ tokens to match brw_recalculate_urb_fence and<br>
> gen6_urb.<br>
><br>
> v3 (idr): Don't whack BRW_NEW_URB_SIZE in blorp.  Suggested by Jason.<br>
> ---<br>
>  src/mesa/drivers/dri/i965/<wbr>gen7_urb.c        | 3 ++-<br>
>  src/mesa/drivers/dri/i965/<wbr>genX_blorp_exec.c | 2 --<br>
>  2 files changed, 2 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/<wbr>gen7_urb.c b/src/mesa/drivers/dri/i965/<wbr>gen7_urb.c<br>
> index 525c9c4..c4b479c 100644<br>
> --- a/src/mesa/drivers/dri/i965/<wbr>gen7_urb.c<br>
> +++ b/src/mesa/drivers/dri/i965/<wbr>gen7_urb.c<br>
> @@ -236,7 +236,8 @@ gen7_upload_urb(struct brw_context *brw, unsigned vs_size,<br>
>  const struct brw_tracked_state gen7_urb = {<br>
>     .dirty = {<br>
>        .mesa = 0,<br>
> -      .brw = BRW_NEW_CONTEXT |<br>
> +      .brw = BRW_NEW_BLORP |<br>
> +             BRW_NEW_CONTEXT |<br>
>               BRW_NEW_URB_SIZE |<br>
>               BRW_NEW_GS_PROG_DATA |<br>
>               BRW_NEW_TCS_PROG_DATA |<br>
> diff --git a/src/mesa/drivers/dri/i965/<wbr>genX_blorp_exec.c b/src/mesa/drivers/dri/i965/<wbr>genX_blorp_exec.c<br>
> index 8fd17fb..af3d609 100644<br>
> --- a/src/mesa/drivers/dri/i965/<wbr>genX_blorp_exec.c<br>
> +++ b/src/mesa/drivers/dri/i965/<wbr>genX_blorp_exec.c<br>
> @@ -183,8 +183,6 @@ blorp_emit_urb_config(struct blorp_batch *batch,<br>
>         brw->urb.vsize >= vs_entry_size)<br>
>        return;<br>
><br>
> -   brw->ctx.NewDriverState |= BRW_NEW_URB_SIZE;<br>
> -<br>
>     gen7_upload_urb(brw, vs_entry_size, false, false);<br>
>  #elif GEN_GEN == 6<br>
>     gen6_upload_urb(brw, vs_entry_size, false, 0);<br>
><br>
<br>
</div></div>The commit message is a bit confusing.  It makes it sound like we were<br>
failing to re-emit 3DSTATE_URB_* from the main drawing path after BLORP<br>
trashed it.  However, the brw->ctx.NewDriverState |= BRW_NEW_URB_SIZE<br>
line that you deleted here guaranteed that it would happen.<br>
<br>
The real benefit appears to be that flagging BRW_NEW_URB_SIZE ("the<br>
total size of the URB portion of the L3 cache has changed") causes<br>
BLORP to think it needs to re-configure it again, so say, back-to-back<br>
BLORP operations would configure it every time.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Using BRW_NEW_BLORP is definitely better - that's what it's there for.<br>
<br>
Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>></blockquote></div><br></div></div>