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

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> 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;

Or even better so you can be sure that the value the gather pool offset
is incremented by matches the number of push constants actually used:

|    brw->gather_pool.next_offset += ALIGN(stage_state->push_const_size *
|                                          REG_SIZE, 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/e34ed389/attachment.sig>