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