Created attachment 115325 [details] Simple program to test the bug When using atomic counters on a shader, the spec [1] 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: commit c0cd5bedf66887e958e140c047afc5bc26160000 Author: Ian Romanick <ian.d.romanick@intel.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 <ian.d.romanick@intel.com> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> [1] https://www.opengl.org/registry/specs/ARB/shader_atomic_counters.txt
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. * * \note * This field is only valid if \c explicit_binding is set. */ int binding; 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. ^^^^^^ typo: current
Forgot to mention that I sent the patch to the mailing list. Second version already available here: http://lists.freedesktop.org/archives/mesa-dev/2015-April/082917.html 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: http://lists.freedesktop.org/archives/piglit/2015-April/015877.html
Update: (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: > > http://lists.freedesktop.org/archives/mesa-dev/2015-April/082917.html Timothy Arceri is providing an alternative solution as part of his work of ARB_arrays_of_arrays GLSL ES. Last patch here: http://lists.freedesktop.org/archives/mesa-dev/2015-July/090103.html > 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: > > http://lists.freedesktop.org/archives/piglit/2015-April/015877.html The fourth version of the same patch got reviewed and pushed by Timothy Arceri to piglit: http://cgit.freedesktop.org/piglit/commit/?id=25f58b2ef0ef80a9dbcb43e6976927fdcb28b617
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 <currojerez@riseup.net> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com> Cc: Ilia Mirkin <imirkin@alum.mit.edu> Cc: Alejandro Piñeiro <apinheiro@igalia.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.