[Mesa-dev] [PATCH] glsl: set the binding value regardless explicit_binding

[Alejandro Piñeiro]

On 14/05/15 13:40, Timothy Arceri wrote:
> 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-stable at 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-stable at 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.

I agree with Timothy analysis. As far as I see, the code that Timothy
points  seems to find which atomic counters and buffers are active, in
order to only take them into account, and the code that the patch
changes, seems to be doing the mapping with the final binding point.

In any case, it is true that the current commit description is wrong, as
the reasoning of why this change is needed is what Timothy has just
said, not what I initially thought.

>> What happens if you change your test to use binding 3 instead of 1?
> It works correctly with the patch and fails without.

I just attached a example similar to the one I attached on the bugzilla.
It uses two different binding points, 3 and 6. I moved the atomic
counter read to the fragment shader so it would be visually easier to
check. One counter for the red value, another counter for the green
value. The program asks on command line the values for the red and green
counter (0 and 1 are the only one valid but the program don't validate it).

It works correctly with the patch (and my system mesa 10.3.2), but fails
without it.

BR

-- 
Alejandro Piñeiro (apinheiro at igalia.com)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: simple-opengl-test.tar.gz
Type: application/gzip
Size: 3583 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150514/2d302e5f/attachment.bin>