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-sta...@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-sta...@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 (apinhe...@igalia.com) _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
