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

[Alejandro Piñeiro]

On 20/05/15 23:39, Timothy Arceri wrote:
> On Thu, 2015-05-14 at 22:49 +0200, Alejandro Piñeiro wrote:
>> On 14/05/15 20:38, Ian Romanick wrote:
>>>
>>> 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.
> The correct binding is stored in the gl_active_atomic_buffer struct
> which queried by glGetActiveAtomicCounterBufferiv()

Ok.


>>>   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?
> For clarity its probably a good idea to implement Ian's suggestion.
> Changing the binding value to the atomic buffer index (even though it
> seems to be unused elsewhere after this point) is a bit confusing. This
> email conversation is a good enough example of the confusion doing this
> can cause.
>
> If you implement Ian's suggestion you can probably remove the if
> statement too as the binding value is not used after this point. 

Probably I'm missing a global view of the situation, but taking into
account that one of the conclusions of this email conversation is that
the index and the binding are different, and removing one of the
variables from ir_variable::data.atomic is causing some confusion (and
the regression), shouldn't it easier to just revert the change? Commit
c0cd5b explains that the removal helped to reduce memory consumption,
but as far as I understand, it was done because it was concluded that
was not needed.

> Although I did notice this in the glsl to nir nir code:
>
>    /* XXX Get rid of buffer_index */
>    var->data.atomic.buffer_index = ir->data.binding;
>
> So this may be a problem too..

Well, if the change is reverted, it is just about removing that comment
(assuming that the comment was added for the same reason commit c0cd5b
was done).

BR

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