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

[Ilia Mirkin]

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 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