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

[Iago Toral]

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.

> +       *
> +       * 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).

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

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