[Mesa-dev] [PATCH 10/22] i965: Make sure constants re-sent after constant buffer reallocation.

Wed Aug 28 17:38:39 PDT 2013

On 08/26/2013 03:12 PM, Paul Berry wrote:
> The hardware requires that after constant buffers for a stage are
> allocated using a 3DSTATE_PUSH_CONSTANT_ALLOC_{VS,HS,DS,GS,PS}
> command, and prior to execution of a 3DPRIMITIVE, the corresponding
> stage's constant buffers must be reprogrammed using a
> 3DSTATE_CONSTANT_{VS,HS,DS,GS,PS} command.
>
> Previously we didn't need to worry about this, because we only
> programmed 3DSTATE_PUSH_CONSTANT_ALLOC_{VS,HS,DS,GS,PS} once on
> startup.  But now that we reallocate the constant buffers whenever
> geometry shaders are switched on and off, we need to make sure the
> constant buffers are reprogrammed.

Not exactly.  The change to do PUSH_CONSTANT_ALLOC once at startup is 
very recent - I only committed it on June 10th (fc800f0c60a2) 
Previously, we had a state atom which did PUSH_CONSTANT_ALLOC whenever 
BRW_NEW_CONTEXT was flagged.

That's still vaguely once at startup, but keep in mind that before 
hardware contexts were mandatory, BRW_NEW_CONTEXT got flagged on every 
batch.

The atoms list looked like this:

    &gen7_push_constant_alloc,
    ...
    &gen7_vs_state,
    ...
    &gen7_ps_state,

Both VS and PS state listen to BRW_NEW_BATCH, so on every batch, we'd 
end up doing:

3DSTATE_PUSH_CONSTANT_ALLOC_VS (if hw_ctx == NULL)
3DSTATE_PUSH_CONSTANT_ALLOC_PS (if hw_ctx == NULL)
3DSTATE_CONSTANT_VS
3DSTATE_CONSTANT_PS

which meant that we always obeyed this rule, even when we didn't do the 
allocation once at startup and never again.

But this only worked because we always allocated push constant space at 
the start of a batch.  Your previous patch cause reallocations to happen 
mid-batch whenever the geometry program changes.  This makes the old 
tricks quit working, and we do need a new flag.

So, I was pretty skeptical of this patch, but on further review, it does 
appear to be necessary and looks fine as is.

> We do this by adding a new bit, BRW_NEW_PUSH_CONSTANT_ALLOCATION, to
> brw->state.dirty.brw.
> ---
>   src/mesa/drivers/dri/i965/brw_context.h   |  2 ++
>   src/mesa/drivers/dri/i965/gen6_gs_state.c |  2 +-
>   src/mesa/drivers/dri/i965/gen6_vs_state.c |  3 ++-
>   src/mesa/drivers/dri/i965/gen6_wm_state.c |  3 ++-
>   src/mesa/drivers/dri/i965/gen7_urb.c      | 13 +++++++++++++
>   src/mesa/drivers/dri/i965/gen7_vs_state.c |  3 ++-
>   src/mesa/drivers/dri/i965/gen7_wm_state.c |  3 ++-
>   7 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 95f9bb2..35193a6 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -158,6 +158,7 @@ enum brw_state_id {
>      BRW_STATE_UNIFORM_BUFFER,
>      BRW_STATE_META_IN_PROGRESS,
>      BRW_STATE_INTERPOLATION_MAP,
> +   BRW_STATE_PUSH_CONSTANT_ALLOCATION,
>      BRW_NUM_STATE_BITS
>   };
>
> @@ -194,6 +195,7 @@ enum brw_state_id {
>   #define BRW_NEW_UNIFORM_BUFFER          (1 << BRW_STATE_UNIFORM_BUFFER)
>   #define BRW_NEW_META_IN_PROGRESS        (1 << BRW_STATE_META_IN_PROGRESS)
>   #define BRW_NEW_INTERPOLATION_MAP       (1 << BRW_STATE_INTERPOLATION_MAP)
> +#define BRW_NEW_PUSH_CONSTANT_ALLOCATION (1 << BRW_STATE_PUSH_CONSTANT_ALLOCATION)
>
>   struct brw_state_flags {
>      /** State update flags signalled by mesa internals */
> diff --git a/src/mesa/drivers/dri/i965/gen6_gs_state.c b/src/mesa/drivers/dri/i965/gen6_gs_state.c
> index ac78286..9648fb7 100644
> --- a/src/mesa/drivers/dri/i965/gen6_gs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_gs_state.c
> @@ -81,7 +81,7 @@ upload_gs_state(struct brw_context *brw)
>   const struct brw_tracked_state gen6_gs_state = {
>      .dirty = {
>         .mesa  = _NEW_TRANSFORM,
> -      .brw   = BRW_NEW_CONTEXT,
> +      .brw   = BRW_NEW_CONTEXT | BRW_NEW_PUSH_CONSTANT_ALLOCATION,
>         .cache = CACHE_NEW_FF_GS_PROG
>      },
>      .emit = upload_gs_state,
> diff --git a/src/mesa/drivers/dri/i965/gen6_vs_state.c b/src/mesa/drivers/dri/i965/gen6_vs_state.c
> index c099342..9f99db8 100644
> --- a/src/mesa/drivers/dri/i965/gen6_vs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_vs_state.c
> @@ -206,7 +206,8 @@ const struct brw_tracked_state gen6_vs_state = {
>         .mesa  = _NEW_TRANSFORM | _NEW_PROGRAM_CONSTANTS,
>         .brw   = (BRW_NEW_CONTEXT |
>   		BRW_NEW_VERTEX_PROGRAM |
> -		BRW_NEW_BATCH),
> +		BRW_NEW_BATCH |
> +                BRW_NEW_PUSH_CONSTANT_ALLOCATION),
>         .cache = CACHE_NEW_VS_PROG | CACHE_NEW_SAMPLER
>      },
>      .emit = upload_vs_state,
> diff --git a/src/mesa/drivers/dri/i965/gen6_wm_state.c b/src/mesa/drivers/dri/i965/gen6_wm_state.c
> index e286785..6725805 100644
> --- a/src/mesa/drivers/dri/i965/gen6_wm_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_wm_state.c
> @@ -229,7 +229,8 @@ const struct brw_tracked_state gen6_wm_state = {
>   		_NEW_POLYGON |
>                   _NEW_MULTISAMPLE),
>         .brw   = (BRW_NEW_FRAGMENT_PROGRAM |
> -		BRW_NEW_BATCH),
> +		BRW_NEW_BATCH |
> +                BRW_NEW_PUSH_CONSTANT_ALLOCATION),
>         .cache = (CACHE_NEW_SAMPLER |
>   		CACHE_NEW_WM_PROG)
>      },
> diff --git a/src/mesa/drivers/dri/i965/gen7_urb.c b/src/mesa/drivers/dri/i965/gen7_urb.c
> index 4dc8f6e..1bca9cd 100644
> --- a/src/mesa/drivers/dri/i965/gen7_urb.c
> +++ b/src/mesa/drivers/dri/i965/gen7_urb.c
> @@ -81,6 +81,19 @@ gen7_allocate_push_constants(struct brw_context *brw)
>
>      gen7_emit_push_constant_state(brw, multiplier * vs_size,
>                                    multiplier * gs_size, multiplier * fs_size);
> +
> +   /* From p115 of the Ivy Bridge PRM (3.2.1.4 3DSTATE_PUSH_CONSTANT_ALLOC_VS):
> +    *
> +    *     Programming Restriction:
> +    *
> +    *     The 3DSTATE_CONSTANT_VS must be reprogrammed prior to the next
> +    *     3DPRIMITIVE command after programming the
> +    *     3DSTATE_PUSH_CONSTANT_ALLOC_VS.
> +    *
> +    * Similar text exists for the other 3DSTATE_PUSH_CONSTANT_ALLOC_*
> +    * commands.
> +    */
> +   brw->state.dirty.brw |= BRW_NEW_PUSH_CONSTANT_ALLOCATION;
>   }
>
>   void
> diff --git a/src/mesa/drivers/dri/i965/gen7_vs_state.c b/src/mesa/drivers/dri/i965/gen7_vs_state.c
> index 36ab229..36fccf7 100644
> --- a/src/mesa/drivers/dri/i965/gen7_vs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_vs_state.c
> @@ -116,7 +116,8 @@ const struct brw_tracked_state gen7_vs_state = {
>         .brw   = (BRW_NEW_CONTEXT |
>   		BRW_NEW_VERTEX_PROGRAM |
>   		BRW_NEW_VS_BINDING_TABLE |
> -		BRW_NEW_BATCH),
> +		BRW_NEW_BATCH |
> +                BRW_NEW_PUSH_CONSTANT_ALLOCATION),
>         .cache = CACHE_NEW_VS_PROG | CACHE_NEW_SAMPLER
>      },
>      .emit = upload_vs_state,
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> index e88db78..e5691fb 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> @@ -227,7 +227,8 @@ const struct brw_tracked_state gen7_ps_state = {
>   		_NEW_COLOR),
>         .brw   = (BRW_NEW_FRAGMENT_PROGRAM |
>   		BRW_NEW_PS_BINDING_TABLE |
> -		BRW_NEW_BATCH),
> +		BRW_NEW_BATCH |
> +                BRW_NEW_PUSH_CONSTANT_ALLOCATION),
>         .cache = (CACHE_NEW_SAMPLER |
>   		CACHE_NEW_WM_PROG)
>      },
>