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

[Timothy Arceri]

On Thu, 2015-05-14 at 22:49 +0200, Alejandro Piñeiro wrote:
> 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.

The correct binding is stored in the gl_active_atomic_buffer struct
which queried by glGetActiveAtomicCounterBufferiv()


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

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

> 
> BR
>