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-sta...@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-sta...@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 active counters which are then added to prog->AtomicBuffers. The code seems to expect data.binding be changed to point to its new 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
