[Mesa-dev] [PATCH v2] main: buffer array variables can have array size of 0 if they are unsized

[Samuel Iglesias Gonsálvez]

On 07/10/15 01:19, Timothy Arceri wrote:
> On Tue, 2015-10-06 at 15:07 +0200, Samuel Iglesias Gonsálvez wrote:
>> On 06/10/15 12:59, Timothy Arceri wrote:
>>> On Tue, 2015-10-06 at 10:08 +0200, Samuel Iglesias Gonsalvez wrote:
>>>> From ARB_program_query_interface:
>>>>
>>>>   For the property ARRAY_SIZE, a single integer identifying the
>>>> number of
>>>>   active array elements of an active variable is written to
>>>> <params>.
>>>> The
>>>>   array size returned is in units of the type associated with the
>>>> property
>>>>   TYPE. For active variables not corresponding to an array of
>>>> basic
>>>> types,
>>>>   the value one is written to <params>. If the variable is a
>>>> shader
>>>>   storage block member in an array with no declared size, the
>>>> value
>>>> zero
>>>>   is written to <params>.
>>>>
>>>> v2:
>>>>
>>>> - Unsized arrays of arrays have an array size different than zero
>>>>
>>>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>>> ---
>>>>  src/mesa/main/shader_query.cpp | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/mesa/main/shader_query.cpp
>>>> b/src/mesa/main/shader_query.cpp
>>>> index dfc39f6..17076b8 100644
>>>> --- a/src/mesa/main/shader_query.cpp
>>>> +++ b/src/mesa/main/shader_query.cpp
>>>> @@ -1304,8 +1304,12 @@ _mesa_program_resource_prop(struct
>>>> gl_shader_program *shProg,
>>>>        switch (res->Type) {
>>>>        case GL_UNIFORM:
>>>>        case GL_BUFFER_VARIABLE:
>>>> +         if (RESOURCE_UNI(res)->is_shader_storage &&
>>>> +             RESOURCE_UNI(res)->is_unsized_array)
>>>> +            *val = RESOURCE_UNI(res)->array_elements;
>>>
>>> You don't need RESOURCE_UNI(res)->is_unsized_array here anymore
>>> right? 
>>>
>>
>> I still needed it here: this rule only applies to unsized arrays. The
>> rest of variables need the MAX2(...) because active variables not
>> corresponding to an array of basic types should write the value one
>> in
>> '*val'.
> 
> Right I forgot about types not being an array. My concern is adding yet
> another field to the uniform storage struct. IMO it already contains
> too many fields, since this is something that hangs around in memory
> for every uniform we should probably make at least some effort to keep
> it as small as possible.
> 
> I haven't tested it but I think you could use (RESOURCE_UNI(res)
> ->array_stride > 0) as a test of whether the buffer is an array. So
> here you could have:
> 
>   if (RESOURCE_UNI(res)->is_shader_storage &&                   
>       RESOURCE_UNI(res)->array_stride > 0)
> 
> And in the patch you linked to [0] you would have:
> 
>    case GL_BUFFER_VARIABLE:
>       if (RESOURCE_UNI(res)->array_stride > 0 &&
>           RESOURCE_UNI(res)->array_elements == 0)
>          return 1;
>       else
>          return RESOURCE_UNI(res)->array_elements;
> 
> You also might want some comments to go with it but what do you think
> about this approach?
> 

Yes, I like it. Using array_stride > 0 to detect if the buffer variable
is an array seems to be a good idea. I will prepare a new version of the
patches with that changed.

Thanks,

Sam

>>
>> If I remove it from here (just for testing), hundreds of dEQP GLES31
>> tests start to fail
>> (dEQP-GLES31.functional.ssbo.layout.single_basic_type.*.*).
>>
>> RESOURCE_UNI(res)->is_unsized_array is also needed in "main: consider
>> that unsized arrays have at least one active element" [0].
>>
>> Sam
>>
>> [0] http://lists.freedesktop.org/archives/mesa-dev/2015-October/09603
>> 8.html
>>
>>> RESOURCE_UNI(res)->is_shader_storage is all you need so you can
>>> also
>>> drop the patch that adds is_unsized_array to the uniform storage
>>> struct
>>>
>>> With that change: Reviewed-by: Timothy Arceri <
>>> t_arceri at yahoo.com.au>
>>>
>>>
>>>> +         else
>>>>              *val = MAX2(RESOURCE_UNI(res)->array_elements, 1);
>>>> -            return 1;
>>>> +         return 1;
>>>>        case GL_PROGRAM_INPUT:
>>>>        case GL_PROGRAM_OUTPUT:
>>>>           *val = MAX2(_mesa_program_resource_array_size(res), 1);
>>>
>