Bug 90175

Summary:	[hsw bisected][PATCH] atomic counters doesn't work for a binding point different to zero
Product:	Mesa	Reporter:	Alejandro Piñeiro (freenode IRC: apinheiro) <apinheiro>
Component:	glsl-compiler	Assignee:	Ian Romanick <idr>
Status:	RESOLVED FIXED	QA Contact:	Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity:	normal
Priority:	medium	CC:	t_arceri
Version:	git
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	Simple program to test the bug Fixes the bug

Description Alejandro Piñeiro (freenode IRC: apinheiro) 2015-04-25 16:37:08 UTC

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

Comment 1 Alejandro Piñeiro (freenode IRC: apinheiro) 2015-04-25 16:40:52 UTC

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.

Comment 2 Alejandro Piñeiro (freenode IRC: apinheiro) 2015-04-25 16:42:26 UTC

(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

Comment 3 Alejandro Piñeiro (freenode IRC: apinheiro) 2015-04-28 08:09:10 UTC

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

Comment 4 Alejandro Piñeiro (freenode IRC: apinheiro) 2015-08-03 10:04:47 UTC

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

Comment 5 Timothy Arceri 2015-11-08 22:45:22 UTC

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.