Bug 90175 - [hsw bisected][PATCH] atomic counters doesn't work for a binding point different to zero
Summary: [hsw bisected][PATCH] atomic counters doesn't work for a binding point differ...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-25 16:37 UTC by Alejandro Piñeiro (freenode IRC: apinheiro)
Modified: 2015-11-08 22:45 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Simple program to test the bug (3.36 KB, application/gzip)
2015-04-25 16:37 UTC, Alejandro Piñeiro (freenode IRC: apinheiro)
Details
Fixes the bug (1009 bytes, patch)
2015-04-25 16:40 UTC, Alejandro Piñeiro (freenode IRC: apinheiro)
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this 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


bug/show.html.tmpl processed on Jan 16, 2017 at 21:52:27.
(provided by the Example extension).