On 03/06/15 13:17, Emil Velikov wrote: > 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 ?
Yes, as far as I understood Ian and Timothy comments, they prefer to create a new patch, in order to solve the regression, but keeping the memory footprint gain. So yes, my patch(es) can be considered obsolete. Right now I don't have time to resume the work on this issue, but I would do that as soon as I have some time for that. Best regards -- Alejandro Piñeiro (apinhe...@igalia.com) _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
