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

[Ian Romanick]

On 05/14/2015 04:30 AM, 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 the active counters which are
> then packed into prog->AtomicBuffers. The code seems to expect
> data.binding to be changed to point to its new packed location in
> prog->AtomicBuffers.
> 
>>
>> What happens if you change your test to use binding 3 instead of 1?
> 
> It works correctly with the patch and fails without.

I think I see what's happening.  I don't think I understood
atomic.buffer_index well enough when I removed it. :(  In binding=3
case, what value does

    glGetActiveAtomicCounterBufferiv(prog,
                                     0,
                                     GL_ATOMIC_COUNTER_BUFFER_BINDING,
                                     param);

give?  My guess is that it gives 0 instead of the binding specified in
the shader.  This would be a good addition to the test.

The index and the binding are different.  The index is "which atomic is
this in the shader," and the binding is "which binding point is used to
attach a buffer to this atomic."  Thanks to c0cd5b, I think somewhere
we're using one value when we actually want the other... probably the
last hunk:

--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -2294,7 +2294,7 @@
vec4_visitor::visit_atomic_counter_intrinsic(ir_call *ir)
       ir->actual_parameters.get_head());
    ir_variable *location = deref->variable_referenced();
    unsigned surf_index = (prog_data->base.binding_table.abo_start +
-                          location->data.atomic.buffer_index);
+                          location->data.binding);

    /* Calculate the surface offset */
    src_reg offset(this, glsl_type::uint_type);

Maybe try adding a utility function that will search
gl_shader_program::UniformStorage for location->name and use the
atomic_buffer_index stored there for use in the i965 driver?

>>>           storage->atomic_buffer_index = i;
>>>           storage->offset = var->data.atomic.offset;
>>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
>