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

[Timothy Arceri]

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.

> 
> BR
>