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

[Timothy Arceri]

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.


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