[Mesa-dev] [PATCH 01/10] tgsi: add MEMBAR opcode to handle memoryBarrier* GLSL intrinsics

[Marek Olšák]

On Tue, Jan 19, 2016 at 9:09 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Marek Olšák <maraeo at gmail.com> writes:
>
>> On Tue, Jan 19, 2016 at 3:25 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> On Mon, Jan 18, 2016 at 6:06 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>>> For 1-4,
>>>>
>>>> Reviewed-by: Marek Olšák <marek.olsak at amd.com>
>>>>
>>>> I'm not very familiar with the code in 2, but the changes seem reasonable.
>>>>
>>>> Also, and I know this is not your mistake, but still, mtypes.h has:
>>>>
>>>> struct gl_atomic_buffer_binding
>>>>       AtomicBufferBindings[MAX_COMBINED_ATOMIC_BUFFERS];
>>>>
>>>> But it should be:
>>>>
>>>> struct gl_atomic_buffer_binding
>>>>       AtomicBufferBindings[16 /* or use a proper definition here */];
>>>>
>>>> It's not possible to use more than MaxAtomicBufferBindings, because
>>>> the slots are shared among all shader stages.
>>>
>>> Besides the suboptimal name, I don't see what's so terribly wrong
>>> about this. They're saying there are 15*6 binding points (the value of
>>> MAX_COMBINED_ATOMIC_BUFFERS), and that's also the
>>> MaxCombinedAtomicBuffers thing. I guess MaxCombinedAtomicBuffers
>>> should be N * the max # of bindings? So this is a bit more restrictive
>>> than it could be, but... meh.
>>
>> I don't understand. AtomicBufferBindings are binding points. There is
>> a fixed number of binding points shared by all shader stages, for
>> example 15. It's not possible to bind a buffer above that. From the
>> spec:
>>
>> "BindBufferBase and BindBufferRange will generate an INVALID_VALUE
>> error if <index> is greater than or equal to the value of
>> MAX_ATOMIC_COUNTER_BUFFER_BINDINGS".
>>
>> This is why AtomicBufferBindings[MAX_COMBINED_ATOMIC_BUFFERS] wastes a
>> lot of memory.
>>
> It only wastes any memory if your driver exposes a
> GL_MAX_ATOMIC_COUNTER_BUFFER_BINDINGS less than the
> MAX_COMBINED_ATOMIC_BUFFERS macro, which might be necessary due to
> hardware restrictions, and even in that case the "waste" will be a
> constant amount of memory per context so I'm not particularly concerned
> about it, you could avoid it by allocating the array manually based on
> the MaxAtomicBufferBindings constant provided by the driver, but it
> would be kind of painful.
>
> I didn't choose the binding point array size to be
> MAX_COMBINED_ATOMIC_BUFFERS by accident, it's the maximum amount of
> atomic counter binding points that can potentially be useful, because
> anything greater than that is functionally equivalent to a context with
> MAX_COMBINED_ATOMIC_BUFFERS binding points since you won't be able to
> set up a pipeline using more than that many atomic buffers.  The i965
> driver exposes MAX_COMBINED_ATOMIC_BUFFERS binding points for that
> reason.

It looks like i965 is wrong even. It sets:
GL_MAX_ATOMIC_COUNTER_BINDINGS = 90

Therefore, a shader is allowed to do:
layout(binding = 89, offset = 0) uniform atomic_uint a;

Does that work on i965? It won't report any error as far as I can see.

Marek