[Mesa-dev] [PATCH 09/25] glsl: Add support for image binding qualifiers.

[Timothy Arceri]

On Wed, 2015-08-19 at 13:52 +0300, Francisco Jerez wrote:
> Timothy Arceri <t_arceri at yahoo.com.au> writes:
> 
> > On Mon, 2015-08-17 at 19:45 +0300, Francisco Jerez wrote:
> > > Support for binding an image to an image unit explicitly in the shader
> > > source is required by both GLSL 4.2 and GLSL ES 3.1, but not by the
> > > original ARB_shader_image_load_store extension.
> > > ---
> > >  src/glsl/ast_to_hir.cpp                | 12 +++++++++++-
> > >  src/glsl/link_uniform_initializers.cpp | 24 +++++++++++++++++-------
> > >  2 files changed, 28 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > > index 06cd6a5..0bf7a1f 100644
> > > --- a/src/glsl/ast_to_hir.cpp
> > > +++ b/src/glsl/ast_to_hir.cpp
> > > @@ -2181,10 +2181,20 @@ validate_binding_qualifier(struct 
> > > _mesa_glsl_parse_state *state,
> > >  
> > >           return false;
> > >        }
> > > +   } else if (state->is_version(420, 310) &&
> > > +              var->type->without_array()->is_image()) {
> > > +      assert(ctx->Const.MaxImageUnits <= MAX_IMAGE_UNITS);
> > > +      if (max_index >= ctx->Const.MaxImageUnits) {
> > > +         _mesa_glsl_error(loc, state, "Image binding %d exceeds the "
> > > +                          " maximum number of image units (%d)", 
> > > max_index,
> > > +                          ctx->Const.MaxImageUnits);
> > > +         return false;
> > > +      }
> > > +
> > >     } else {
> > >        _mesa_glsl_error(loc, state,
> > >                         "the \"binding\" qualifier only applies to 
> > > uniform "
> > > -                       "blocks, samplers, atomic counters, or arrays 
> > > thereof");
> > > +                       "blocks, opaque variables, or arrays thereof");
> > >        return false;
> > >     }
> > >  
> > > diff --git a/src/glsl/link_uniform_initializers.cpp 
> > > b/src/glsl/link_uniform_initializers.cpp
> > > index d61ae91..7c6269b 100644
> > > --- a/src/glsl/link_uniform_initializers.cpp
> > > +++ b/src/glsl/link_uniform_initializers.cpp
> > > @@ -101,7 +101,7 @@ copy_constant_to_storage(union gl_constant_value 
> > > *storage,
> > >  }
> > >  
> > >  void
> > > -set_sampler_binding(gl_shader_program *prog, const char *name, int 
> > > binding)
> > > +set_opaque_binding(gl_shader_program *prog, const char *name, int 
> > > binding)
> > 
> > I guess you could add a comment the atomics are handled elsewhere but not 
> > a
> > big deal.
> > 
> > Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>
> > 
> 
> > --- a/src/glsl/link_uniform_initializers.cpp
> > +++ b/src/glsl/link_uniform_initializers.cpp
> > @@ -100,6 +100,11 @@ copy_constant_to_storage(union gl_constant_value 
> > *storage,
> >     }
> >  }
> > 
> > +/**
> > + * Initialize an opaque uniform from the value of an explicit binding
> > + * qualifier specified in the shader.  Atomic counters are different 
> > because
> > + * they have no storage and should be handled elsewhere.
> > + */
> >  void
> >  set_opaque_binding(gl_shader_program *prog, const char *name, int 
> > binding)
> >  {
> 
> Does that look okay for you?

Looks good to me, thanks.


> 
> Thanks.
> 
> > 
> > >  {
> > >     struct gl_uniform_storage *const storage =
> > >        get_storage(prog->UniformStorage, prog->NumUniformStorage, name);
> > > @@ -127,11 +127,20 @@ set_sampler_binding(gl_shader_program *prog, const 
> > > char *name, int binding)
> > >     for (int sh = 0; sh < MESA_SHADER_STAGES; sh++) {
> > >        gl_shader *shader = prog->_LinkedShaders[sh];
> > >  
> > > -      if (shader && storage->sampler[sh].active) {
> > > -         for (unsigned i = 0; i < elements; i++) {
> > > -            unsigned index = storage->sampler[sh].index + i;
> > > +      if (shader) {
> > > +         if (storage->type->base_type == GLSL_TYPE_SAMPLER &&
> > > +             storage->sampler[sh].active) {
> > > +            for (unsigned i = 0; i < elements; i++) {
> > > +               const unsigned index = storage->sampler[sh].index + i;
> > > +               shader->SamplerUnits[index] = storage->storage[i].i;
> > > +            }
> > >  
> > > -            shader->SamplerUnits[index] = storage->storage[i].i;
> > > +         } else if (storage->type->base_type == GLSL_TYPE_IMAGE &&
> > > +                    storage->image[sh].active) {
> > > +            for (unsigned i = 0; i < elements; i++) {
> > > +               const unsigned index = storage->image[sh].index + i;
> > > +               shader->ImageUnits[index] = storage->storage[i].i;
> > > +            }
> > >           }
> > >        }
> > >     }
> > > @@ -267,8 +276,9 @@ link_set_uniform_initializers(struct 
> > > gl_shader_program 
> > > *prog,
> > >           if (var->data.explicit_binding) {
> > >              const glsl_type *const type = var->type;
> > >  
> > > -            if (type->without_array()->is_sampler()) {
> > > -               linker::set_sampler_binding(prog, var->name, var
> > > ->data.binding);
> > > +            if (type->without_array()->is_sampler() ||
> > > +                type->without_array()->is_image()) {
> > > +               linker::set_opaque_binding(prog, var->name, var
> > > ->data.binding);
> > >              } else if (var->is_in_buffer_block()) {
> > >                 const glsl_type *const iface_type = var
> > > ->get_interface_type();
> > >