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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev