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

[Francisco Jerez]

Marek Olšák <maraeo at gmail.com> writes:

> 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.
>
Yes, I'd expect that to work on i965.

> Marek
-------------- 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/20160119/a6ba6753/attachment.sig>