[Mesa-dev] [PATCH 2/2] glsl: fix GL_BUFFER_DATA_SIZE value for shader storage blocks with unsize arrays

[Samuel Iglesias Gonsálvez]

On 28/10/15 10:32, Iago Toral wrote:
> On Thu, 2015-10-22 at 11:01 +0200, Samuel Iglesias Gonsalvez wrote:
>> From ARB_program_interface_query:
>>
>> "For the property of BUFFER_DATA_SIZE, then the implementation-dependent
>>  minimum total buffer object size, in basic machine units, required to hold
>>  all active variables associated with an active uniform block, shader
>>  storage block, or atomic counter buffer is written to <params>.  If the
>>  final member of an active shader storage block is array with no declared
>>  size, the minimum buffer size is computed assuming the array was declared
>>  as an array with one element."
>>
>> Fixes the following dEQP-GLES31 tests:
>>
>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.buffer_data_size.named_block
>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.buffer_data_size.unnamed_block
>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.buffer_data_size.block_array
>>
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>> ---
>>  src/glsl/link_uniform_blocks.cpp | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glsl/link_uniform_blocks.cpp b/src/glsl/link_uniform_blocks.cpp
>> index 5285d8d..a10b44b 100644
>> --- a/src/glsl/link_uniform_blocks.cpp
>> +++ b/src/glsl/link_uniform_blocks.cpp
>> @@ -130,13 +130,22 @@ private:
>>  
>>        unsigned alignment = 0;
>>        unsigned size = 0;
>> -
>> +      /* From ARB_program_interface_query:
> Add a blank line here
>> +       * "If the final member of an active shader storage block is array with
>> +       * no declared size, the minimum buffer size is computed assuming the
>> +       * array was declared as an array with one element."
> 
> Align the two lines above to the If in the first line and indent the
> block in quotes like it is done for other similar comments in the same
> function just below this.
> 

OK

>> +       *
>> +       * For that reason, we use the base type of the unsized array to calculate
>> +       * its size.
>> +       */
> 
> I was wondering if we should also check that this is the last member
> explicitly (there is an unused bool parameter in this function that
> informs us about that). My understanding is that only SSBOs can have
> that, and the parser should ensure that they are last  in the SSBO
> definition, so maybe it is redundant... if we don't want to add that
> check, then maybe it is worth amending the comment to explain why though
> (and even in that case maybe we want to add an assert).
> 

It is redundant because that check is done in the parser as you said. I
will amend the comment to explain it and add the assert too.

> With these changes:
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> 

Thanks,

Sam

>> +      const glsl_type *type_for_size =
>> +         type->is_unsized_array() ? type->without_array() : type;
>>        if (packing == GLSL_INTERFACE_PACKING_STD430) {
>>           alignment = type->std430_base_alignment(v->RowMajor);
>> -         size = type->std430_size(v->RowMajor);
>> +         size = type_for_size->std430_size(v->RowMajor);
>>        } else {
>>           alignment = type->std140_base_alignment(v->RowMajor);
>> -         size = type->std140_size(v->RowMajor);
>> +         size = type_for_size->std140_size(v->RowMajor);
>>        }
>>  
>>        this->offset = glsl_align(this->offset, alignment);
> 
> 
>