On 28 August 2013 17:38, Kenneth Graunke <kenn...@whitecape.org> wrote:
> 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.
>
Ok, I've added a parenthetical comment to the commit message to clarify
this. It now says:
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 (or, previous to that, whenever BRW_NEW_CONTEXT was flagged).
>
> 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)
>> },
>>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
