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

[Emil Velikov]

Hello gents,

On 21 May 2015 at 14:03, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> On Thu, 2015-05-21 at 12:22 +0200, Alejandro Piñeiro wrote:
>>
>> 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.
>
>
> It was part of a memory reduction series, so I'm not sure it should be
> reverted. I'm sure Ian would have suggested doing so if he thought that
> was the better option:
>
> http://lists.freedesktop.org/archives/mesa-dev/2014-July/063303.html
>
>
>>
>> > 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).
>
> If the change was to be reverted this would probably need to be changed
> to
>
> var->data.atomic.buffer_index = ir->data.atomic.buffer_index;
>
> as nir didn't exist when the change was made.
>
In light of the above discussion should we consider this patch as
obsolete (more of incomplete iiuc) or is it still applicable ? If the
latter can we get some formal r-bs and get this merged in master ?

Thanks
Emil