On Tue, 2015-09-29 at 16:08 +0300, Francisco Jerez wrote: > Ilia Mirkin <imir...@alum.mit.edu> writes: > > > On Tue, Sep 29, 2015 at 3:32 AM, Timothy Arceri < > > t_arc...@yahoo.com.au> wrote: > > > On Tue, 2015-09-29 at 02:55 -0400, Ilia Mirkin wrote: > > > > On Tue, Sep 29, 2015 at 2:48 AM, Timothy Arceri < > > > > t_arc...@yahoo.com.au> wrote: > > > > > On Tue, 2015-09-29 at 02:33 -0400, Ilia Mirkin wrote: > > > > > > On Tue, Sep 29, 2015 at 2:26 AM, Timothy Arceri < > > > > > > t_arc...@yahoo.com.au> wrote: > > > > > > > On Tue, 2015-09-29 at 02:08 -0400, Ilia Mirkin wrote: > > > > > > > > On Tue, Sep 29, 2015 at 2:05 AM, Timothy Arceri < > > > > > > > > t_arc...@yahoo.com.au> wrote: > > > > > > > > > On Tue, 2015-09-29 at 01:05 -0400, Ilia Mirkin wrote: > > > > > > > > No. I mean the line: > > > > > > > > > > > > > > > > brw->vtbl.emit_buffer_surface_state(brw, > > > > > > > > &surf_offsets[i], > > > > > > > > bo, > > > > > > > > > > > > > > > > Shouldn't that look up the atomic buffer index in prog > > > > > > > > ->Uniforms > > > > > > > > instead of using 'i'? > > > > > > > > > > > > > > No that looks fine. The problem in the patch was we > > > > > > > didn't have > > > > > > > the > > > > > > > atomic buffex index and we used the binding as the offset > > > > > > > which > > > > > > > doesn't > > > > > > > have to be the same as the buffer index. > > > > > > > > > > > > > > In brw_upload_abo_surfaces its the opposite we have the > > > > > > > atomic > > > > > > > buffer > > > > > > > index which is i and we are looking up the binding to get > > > > > > > the > > > > > > > buffer > > > > > > > object. > > > > > > > > > > > > > > Does that make sense? > > > > > > > > > > > > i is the binding index though... > > > > > > > > > > i is the buffer index > > > > > > > > > > See find_active_atomic_counters() for the code the counts the > > > > > active > > > > > buffers. > > > > > > > > > > http://cgit.freedesktop.org/mesa/mesa/tree/src/glsl/link_atom > > > > > ics.cp > > > > > p#n9 > > > > > 9 > > > > > > > > > > This count is then used to set NumAtomicBuffers and to create > > > > > the > > > > > prog > > > > > ->AtomicBuffers array so the array is no bigger than it needs > > > > > to > > > > > be. > > > > > This is the reason the binding and atomic buffer index can > > > > > differ. > > > > > > > > > > http://cgit.freedesktop.org/mesa/mesa/tree/src/glsl/link_atom > > > > > ics.cp > > > > > p#n1 > > > > > 73 > > > > > > > > Do you agree that NumAtomicBuffers can go up to > > > > MaxCombinedAtomicBuffers? If so, that's problematic, when it's > > > > > > > > > MaxAtomicBuffers, right? > > > > > > Ok, so now we are talking about a new problem from what this > > > patch is > > > trying to solve. > > > > Well, the problem is "the indexes are all messed up". I think this > > patch fixes half the problem in a way that happens to work some of > > the > > time, much like the existing logic. (Admittedly your solution works > > *more* of the time...) > > > I think this patch is correct in the sense that it makes the atomic > buffer index used by the compiler consistent with the index of the > same > atomic counter buffer used by the state upload code, which was no > longer > the case since c0cd5bedf66887e958e140c047afc5bc26160000. > > > > > > I agree it can go up to MaxCombinedAtomicBuffers. > > > > > > I'm not sure what the answer is, maybe a question for curro. I > > > don't > > > know enough about this code to know if we want to upload > > > everything > > > here or just do a stage at a time. > > > > We need to keep track of an intra-stage index, accessible both at > > instruction emission time as well as at resource emission time. I > > thought that was the atomic_buffer_index. Sounds like it's not, but > > in > > that case it should be made to be that IMO. See > > _mesa_get_sampler_uniform_value for how this is handled for > > samplers... not sure if it'll be helpful or not. > > > You're right that gl_uniform_storage::atomic_buffer_index is per > -program > rather than per-stage, but the choice is not arbitrary, it's mandated > by > the spec: ARB_shader_atomic_counters defines a space of active atomic > counter buffer indices (e.g. the bufferIndex parameter passed to > glGetActiveAtomicCounterBufferiv()) with one entry for each > individual > atomic counter buffer actually referenced by the program. The state > required for each active atomic counter buffer in the program maps to > an > entry of the gl_shader_program::AtomicBuffers array. The purpose of > gl_uniform_storage::atomic_buffer_index is to map a given atomic > counter > uniform to the index of its BO in the same index space (i.e. it's the > same thing you query with the GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX > uniform param). > > I agree with you that some sort of intra-stage index (kind of like > gl_uniform_storage::sampler[], ::image[] and such) would be useful, > at > least for the i965 driver: The current i965 implementation uses the > active atomic counter buffer index as surface index for simplicity, > what > is definitely suboptimal since it imposes a limit on the sum of > distinct > atomic counter buffers that may be used in the whole program, because > it > means we have to upload the same set of ABO surfaces to all stages in > the program.
After figuring out what was going on I came here to say something similar. Its good to know I came to the right conclusion. I will have a go at implementing an intra-stage index I dont think it will be too difficult. > > > -ilia > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
