[Mesa-dev] [PATCH 12/17] mesa/st: make sure the const buffer is large enough for packed uniforms

[Timothy Arceri]

On 26/06/17 19:27, Nicolai Hähnle wrote:

> On 25.06.2017 03:31, Timothy Arceri wrote:
>> ---
>>   src/mesa/state_tracker/st_atom_constbuf.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/state_tracker/st_atom_constbuf.c 
>> b/src/mesa/state_tracker/st_atom_constbuf.c
>> index 7f17546..eb8f6b3 100644
>> --- a/src/mesa/state_tracker/st_atom_constbuf.c
>> +++ b/src/mesa/state_tracker/st_atom_constbuf.c
>> @@ -83,21 +83,27 @@ void st_upload_constants(struct st_context *st, 
>> struct gl_program *prog)
>>        /* Make all bindless samplers/images bound texture/image units 
>> resident in
>>       * the context.
>>       */
>>      st_make_bound_samplers_resident(st, prog);
>>      st_make_bound_images_resident(st, prog);
>>        /* update constants */
>>      if (params && params->NumParameters) {
>>         struct pipe_constant_buffer cb;
>> -      const uint paramBytes = params->NumParameters * 
>> sizeof(GLfloat) * 4;
>> +
>> +      /* We need to align to 4 as the driver will expect to load 
>> four values
>> +       * regardless of how many values the last param has. It's safe 
>> to do the
>> +       * align as the param list always allocates extra memory.
>> +       */
>> +      const uint paramBytes =
>> +         align((params->NumParameterValues * sizeof(GLfloat)) + 1, 4);
>
> This looks broken.
>
> First of all, I would say that it shouldn't actually be required. A 
> driver that supports packed uniforms should only load as much as the 
> shader actually requests.

I recall you telling me on IRC that radeonsi needs to load vec4 chunks 
for efficiency. Maybe I misinterpreted what you were saying?

>
> Second, even if it were needed, the code doesn't do what it says.

:P I made some *simplifications* here at some point, not sure what I was 
thinking at the time.

>
> Cheers,
> Nicolai
>
>
>>           /* Update the constants which come from fixed-function 
>> state, such as
>>          * transformation matrices, fog factors, etc.  The rest of 
>> the values in
>>          * the parameters list are explicitly set by the user with 
>> glUniform,
>>          * glProgramParameter(), etc.
>>          */
>>         if (params->StateFlags)
>>            _mesa_load_state_parameters(st->ctx, params);
>>           _mesa_shader_write_subroutine_indices(st->ctx, stage);
>>
>
>