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

[Alejandro Piñeiro]

On 14/05/15 20:38, Ian Romanick wrote:
> 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.

With the test I uploaded today, so with binding points 3 and 6, and
using index 0 and 1, calling glGetActiveAtomicCounterBufferiv properly
returns 3 and 6. Both with and without the patch of this thread.

>   This would be a good addition to the test.

Ok.

> 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?

Taking into account that glGetActiveAtomicCounterBufferiv is already
working, do you think that this is still needed?

BR

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