[Mesa-dev] [PATCH V6 10/27] i965: fix atomic buffer index for bindings other than 0

Tue Sep 29 07:06:07 PDT 2015

On Tue, 2015-09-29 at 16:08 +0300, Francisco Jerez wrote:
> Ilia Mirkin <imirkin at alum.mit.edu> writes:
> 
> > On Tue, Sep 29, 2015 at 3:32 AM, Timothy Arceri <
> > t_arceri at 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_arceri at 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_arceri at 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_arceri at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev