[Mesa-dev] [PATCH] i965: Stop looking at NewDriverState when emitting 3DSTATE_URB

[Kenneth Graunke]

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 <kenneth at whitecape.org>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  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 <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170818/ac380867/attachment-0001.sig>