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_atomics.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_atomics.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. > -ilia > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
