[Mesa-dev] [PATCH 09/20] glsl: add support for initialising sampler AoA

[Tapani Pälli]

On 08/04/2015 05:10 PM, Timothy Arceri wrote:
> On Tue, 2015-08-04 at 14:54 +0300, Tapani Pälli wrote:
>> Hi;
>>
>> I've tried to understand more about AoA to review the linker changes.
>>
>> Now I'm testing your patches (and taking currently closer look at 9/20).
>> Overall it looks fine, calling itself recursively for each array level.
>> However, with fs-initializer-const-index.shader_test it seems to set
>> bindings to 4 samplers (0,1,2,3). This is true also for
>> fs-initializer-non-const-index.shader_test. I don't understand why this
>> happens as const test has array like this:
>>
>> layout(binding = 0) uniform sampler2D tex[2][2][2];
>>
>> and non-const:
>>
>> layout(binding = 0) uniform sampler2D tex[2][2];
>>
>> Shouldn't first case set bindings to more samplers, am I missing something?
> Where are you checking what bindings are set? My guess is because the test
> only uses a max of 4 array elements [0][1][1] if I change this to [1][1][1] it
> seems to be correctly set it to 7.

I'm just printing stuff in gdb and saw that loop setting the binding to 
uniform storage set it only 4 times. Right, it could happen due to 
optimizations kicking in, they will remove unused slots from array end.

>
>>
>> On 07/29/2015 04:56 PM, Timothy Arceri wrote:
>>> ---
>>>    src/glsl/link_uniform_initializers.cpp | 68 ++++++++++++++++++++--------
>>> ------
>>>    1 file changed, 41 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/src/glsl/link_uniform_initializers.cpp
>>> b/src/glsl/link_uniform_initializers.cpp
>>> index 0cc79d9..d6a6ab7 100644
>>> --- a/src/glsl/link_uniform_initializers.cpp
>>> +++ b/src/glsl/link_uniform_initializers.cpp
>>> @@ -101,42 +101,54 @@ copy_constant_to_storage(union gl_constant_value
>>> *storage,
>>>    }
>>>
>>>    void
>>> -set_sampler_binding(gl_shader_program *prog, const char *name, int
>>> binding)
>>> +set_sampler_binding(void *mem_ctx, gl_shader_program *prog,
>>> +                    const glsl_type *type, const char *name, int
>>> *binding)
>>>    {
>>> -   struct gl_uniform_storage *const storage =
>>> -      get_storage(prog->UniformStorage, prog->NumUniformStorage, name);
>>>
>>> -   if (storage == NULL) {
>>> -      assert(storage != NULL);
>>> -      return;
>>> -   }
>>> +   if (type->is_array() && type->fields.array->is_array()) {
>>> +      const glsl_type *const element_type = type->fields.array;
>>>
>>> -   const unsigned elements = MAX2(storage->array_elements, 1);
>>> +      for (unsigned int i = 0; i < type->length; i++) {
>>> +	 const char *element_name = ralloc_asprintf(mem_ctx, "%s[%d]",
>>> name, i);
>>>
>>> -   /* Section 4.4.4 (Opaque-Uniform Layout Qualifiers) of the GLSL 4.20
>>> spec
>>> -    * says:
>>> -    *
>>> -    *     "If the binding identifier is used with an array, the first
>>> element
>>> -    *     of the array takes the specified unit and each subsequent
>>> element
>>> -    *     takes the next consecutive unit."
>>> -    */
>>> -   for (unsigned int i = 0; i < elements; i++) {
>>> -      storage->storage[i].i = binding + i;
>>> -   }
>>> +	 set_sampler_binding(mem_ctx, prog, element_type,
>>> +                             element_name, binding);
>>> +      }
>>> +   } else {
>>> +      struct gl_uniform_storage *const storage =
>>> +         get_storage(prog->UniformStorage, prog->NumUniformStorage,
>>> name);
>>>
>>> -   for (int sh = 0; sh < MESA_SHADER_STAGES; sh++) {
>>> -      gl_shader *shader = prog->_LinkedShaders[sh];
>>> +      if (storage == NULL) {
>>> +         assert(storage != NULL);
>>> +         return;
>>> +      }
>>> +
>>> +      const unsigned elements = MAX2(storage->array_elements, 1);
>>> +
>>> +      /* Section 4.4.4 (Opaque-Uniform Layout Qualifiers) of the GLSL
>>> 4.20 spec
>>> +       * says:
>>> +       *
>>> +       *     "If the binding identifier is used with an array, the first
>>> element
>>> +       *     of the array takes the specified unit and each subsequent
>>> element
>>> +       *     takes the next consecutive unit."
>>> +       */
>>> +      for (unsigned int i = 0; i < elements; i++) {
>>> +         storage->storage[i].i = (*binding)++;
>>> +      }
>>>
>>> -      if (shader && storage->sampler[sh].active) {
>>> -         for (unsigned i = 0; i < elements; i++) {
>>> -            unsigned index = storage->sampler[sh].index + i;
>>> +      for (int sh = 0; sh < MESA_SHADER_STAGES; sh++) {
>>> +        gl_shader *shader = prog->_LinkedShaders[sh];
>>>
>>> -            shader->SamplerUnits[index] = storage->storage[i].i;
>>> +         if (shader && storage->sampler[sh].active) {
>>> +            for (unsigned i = 0; i < elements; i++) {
>>> +               unsigned index = storage->sampler[sh].index + i;
>>> +
>>> +               shader->SamplerUnits[index] = storage->storage[i].i;
>>> +            }
>>>             }
>>>          }
>>> +      storage->initialized = true;
>>>       }
>>> -
>>> -   storage->initialized = true;
>>>    }
>>>
>>>    void
>>> @@ -270,7 +282,9 @@ link_set_uniform_initializers(struct gl_shader_program
>>> *prog,
>>>                const glsl_type *const type = var->type;
>>>
>>>                if (type->without_array()->is_sampler()) {
>>> -               linker::set_sampler_binding(prog, var->name, var
>>> ->data.binding);
>>> +               int binding = var->data.binding;
>>> +               linker::set_sampler_binding(mem_ctx, prog, var->type,
>>> +                                           var->name, &binding);
>>>                } else if (var->is_in_buffer_block()) {
>>>                   const glsl_type *const iface_type = var
>>> ->get_interface_type();
>>>
>>>