Created attachment 115325 [details]
Simple program to test the bug
When using atomic counters on a shader, the spec  says that the binding point may be bound using BindBufferBase or BindBufferRange. The binding point will be referenced on the shader. Even if you use the default 0, you should call this method.
At this moment, only zero works. Im attaching a small test showing this. The example is using 1 for the binding. If the number is both replaced on the code and on the shader to zero, it works properly.
It fails with current master, but works with mesa installed on my system (10.3.2). After bisecting, this is the commit that caused the issue:
Author: Ian Romanick <email@example.com>
Date: Mon Jul 14 15:48:36 2014 -0700
glsl: Eliminate ir_variable::data.atomic.buffer_index
Just use ir_variable::data.binding... because that's the where the
binding is stored for everything else that can use layout(binding=).
Valgrind massif results for a trimmed apitrace of dota2:
n time(i) total(B) useful-heap(B) extra-heap(B) stacks(B)
Before (32-bit): 50 40,564,927,443 69,185,408 63,683,871 5,501,537 0
After (32-bit): 74 40,580,119,657 69,186,544 63,506,327 5,680,217 0
Before (64-bit): 59 36,822,048,449 96,526,888 89,113,000 7,413,888 0
After (64-bit): 89 36,822,971,897 96,526,616 88,735,296 7,791,320 0
A real savings of 173KiB on 32-bit and 368KiB on 64-bit.
Signed-off-by: Ian Romanick <firstname.lastname@example.org>
Reviewed-by: Kenneth Graunke <email@example.com>
Created attachment 115326 [details] [review]
Fixes the bug
Taking into account the explanation on the header about the variable binding (ast.h:553)
* Binding specified via GL_ARB_shading_language_420pack's "binding" keyword.
* This field is only valid if \c explicit_binding is set.
The binding is correct (and should be updated) if explicit_binding is true. But the correct behaviour was updating it if it was false.
This was not detected by piglit because all the calls to glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0.
I tested this patch by running all piglit on my system, and I didn't detect regression. I also runned make check without issues.
(In reply to Alejandro Piñeiro (freenode IRC: apinheiro) from comment #1)
> The binding is correct (and should be updated) if explicit_binding is true.
> But the correct behaviour was updating it if it was false.
Forgot to mention that I sent the patch to the mailing list. Second version already available here:
During the review of the first version of the patch, it was suggested to add a piglit test to catch this problem. First version of a piglit test here:
(In reply to Alejandro Piñeiro (freenode IRC: apinheiro) from comment #3)
> Forgot to mention that I sent the patch to the mailing list. Second version
> already available here:
Timothy Arceri is providing an alternative solution as part of his work of ARB_arrays_of_arrays GLSL ES. Last patch here:
> During the review of the first version of the patch, it was suggested to add
> a piglit test to catch this problem. First version of a piglit test here:
The fourth version of the same patch got reviewed and pushed by Timothy Arceri to piglit:
Fix pushed as part of commit a3d0359aff7a
glsl: keep track of intra-stage indices for atomics
This is more optimal as it means we no longer have to upload the same set
of ABO surfaces to all stages in the program.
This also fixes a bug where since commit c0cd5b var->data.binding was
being used as a replacement for atomic buffer index, but they don't have
to be the same value they just happened to end up the same when binding is 0.
Reviewed-by: Francisco Jerez <firstname.lastname@example.org>
Reviewed-by: Samuel Iglesias Gonsálvez <email@example.com>
Cc: Ilia Mirkin <firstname.lastname@example.org>
Cc: Alejandro Piñeiro <email@example.com>