[Mesa-stable] [Mesa-dev] [PATCH] glsl: set the binding value regardless explicit_binding

[Timothy Arceri]

On Wed, 2015-05-13 at 09:58 -0700, Ian Romanick wrote:
> On 05/11/2015 03:37 AM, Alejandro Piñeiro wrote:
> > Since commit c0cd5b var->data.binding was set only when explicit_binding
> > was false, thas was wrong, should be a test to true. This prevented
> > to use any binding point different to 0.
> > 
> > In any case, that if statement is not needed. Right now mesa requires
> > all atomic counters to have an explicit binding point. This would match
> > the original implementation.
> > 
> > Cc: 10.4, 10.5 <mesa-stable at lists.freedesktop.org>
>       ^^^^^^^^^^ You should put quotes around this, or it treats it as
> two separate e-mail addresses.  One is 10.4 (invalid) and the other use
> a user named "10.5" at the address mesa-stable at lists.freedesktop.org
> (valid).
> 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
> > ---
> > 
> > New version based on Timothy Arceri suggestion at the list. Also
> > gentle ping for a formal review of the patch.
> > 
> >  src/glsl/link_atomics.cpp | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
> > index 603873a..2cede91 100644
> > --- a/src/glsl/link_atomics.cpp
> > +++ b/src/glsl/link_atomics.cpp
> > @@ -201,8 +201,7 @@ link_assign_atomic_counter_resources(struct gl_context *ctx,
> >           gl_uniform_storage *const storage = &prog->UniformStorage[id];
> >  
> >           mab.Uniforms[j] = id;
> > -         if (!var->data.explicit_binding)
> > -            var->data.binding = i;
> > +         var->data.binding = i;
> 
> I don't think this is correct.  If the user specified an explicit
> binding, which you correctly assert is required, this will overwrite
> that value with some other value.  See src/glsl/ast_to_hir.cpp around
> line 2663.  I suspect the value of ir_variable::data.binding is just
> getting lost somewhere (probably not being copied), and your change just
> fixes it by luck.

I thought the same thing when I first looked at this but running it in
the debugger showed the value in ir_variable::data.binding was correct
and it wasn't just working by luck.

find_active_atomic_counters() finds all the active counters which are
then packed into prog->AtomicBuffers. The code seems to expect
data.binding to be changed to point to its new packed location in
prog->AtomicBuffers.

> 
> What happens if you change your test to use binding 3 instead of 1?

It works correctly with the patch and fails without.

> 
> >           storage->atomic_buffer_index = i;
> >           storage->offset = var->data.atomic.offset;
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev