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

[Francisco Jerez]

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_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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150929/e50ff39e/attachment.sig>