Hello gents, On 21 May 2015 at 14:03, Timothy Arceri <t_arc...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
