[Mesa-dev] [PATCH 04/20] i965: Allocate space on the gather pool for UBO entries

[Francisco Jerez]

Abdiel Janulgue <abdiel.janulgue at linux.intel.com> writes:

> If there are UBO constant entries, append them to stage_state->push_const_size.
> The gather pool contains the combined entries of both ordinary uniforms
> and UBO constants.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/gen6_vs_state.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen6_vs_state.c b/src/mesa/drivers/dri/i965/gen6_vs_state.c
> index b78166e..843df94 100644
> --- a/src/mesa/drivers/dri/i965/gen6_vs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_vs_state.c
> @@ -59,7 +59,9 @@ gen6_upload_push_constants(struct brw_context *brw,
>     struct gl_context *ctx = &brw->ctx;
>  
>     if (prog_data->nr_params == 0) {
> -      stage_state->push_const_size = 0;
> +      if (prog_data->nr_ubo_params == 0) {
> +         stage_state->push_const_size = 0;
> +      }

The 'if (prog_data->nr_ubo_params == 0)' conditional here only seems to
be required because you don't assign stage_state->push_const_size
consistently below depending on whether 'prog_data->nr_params == 0' or
not.  Why don't you drop this assignment, the
'stage_state->push_const_size = ALIGN(prog_data->nr_params, 8) / 8;'
assignment in the other branch of this if, and the
'stage_state->push_const_size = ALIGN(prog_data->nr_params +
prog_data->nr_ubo_params, 8) / 8;' assignment introduced in this patch
and instead assign it once, right before the ' if (brw->gather_pool.bo
!= NULL)' conditional, like:

|    stage_state->push_const_size = DIV_ROUND_UP(prog_data->nr_params +
|                                                prog_data->nr_ubo_params, 8);

>     } else {
>        /* Updates the ParamaterValues[i] pointers for all parameters of the
>         * basic type of PROGRAM_STATE_VAR.
> @@ -122,10 +124,24 @@ gen6_upload_push_constants(struct brw_context *brw,
>     }
>     /* Allocate gather pool space for uniform and UBO entries in 512-bit chunks*/
>     if (brw->gather_pool.bo != NULL) {
> +      unsigned gather_pool_next_offset = brw->gather_pool.next_offset;
> +

Set stage_state->push_const_offset to brw->gather_pool.next_offset up
front to avoid the need for this temporary.

>        if (prog_data->nr_params > 0) {
>           int num_consts = ALIGN(prog_data->nr_params, 4) / 4;
> +         gather_pool_next_offset += (ALIGN(num_consts, 4) / 4) * 64;
> +      }
> +
> +      if (prog_data->nr_ubo_params > 0) {
> +         stage_state->push_const_size = ALIGN(prog_data->nr_params + prog_data->nr_ubo_params, 8) / 8;
> +         uint32_t num_constants = ALIGN(prog_data->nr_ubo_params, 4) / 4;
> +         gather_pool_next_offset += (ALIGN(num_constants, 4) / 4) * 64;
> +      }

You set push_const_size to the 32B-aligned value of the sum of nr_params
and nr_ubo_params, but then increment gather_pool.next_offset by the sum
of the separately aligned values of both terms, which AFAICT may
overestimate the amount of space actually used from the pool.  I suggest
you drop the two conditionals above and instead do:

|    brw->gather_pool.next_offset += DIV_ROUND_UP(prog_data->nr + prog_data->nr_ubo_params, 16) * 64;

> +
> +      if (gather_pool_next_offset > brw->gather_pool.next_offset) {
>           stage_state->push_const_offset = brw->gather_pool.next_offset;
> -         brw->gather_pool.next_offset += (ALIGN(num_consts, 4) / 4) * 64;
> +         brw->gather_pool.next_offset = gather_pool_next_offset;
> +         assert(brw->gather_pool.next_offset < brw->gather_pool.bo->size);
> +         assert(stage_state->push_const_offset < brw->gather_pool.next_offset);
>        }

This block should also become unnecessary if you make the changes I
suggested earlier.

>     }
>  }
> -- 
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151023/ba4b51f0/attachment.sig>