[Mesa-dev] [PATCH v2] glsl: properly setting var->data.binding if explicit_binding is true

[Alejandro Piñeiro]

On 30/04/15 03:15, Timothy Arceri wrote:
> On Tue, 2015-04-28 at 10:07 +0200, Alejandro Piñeiro wrote:
>> There was a typo on commit c0cd5b, doing it when explicit_binding
>> was false. This prevented to use any binding point different to 0.
>>
>> Cc: 10.4, 10.5 <mesa-stable at lists.freedesktop.org>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
>> ---
>>
>> Piglit test to catch this error proposed here:
>> http://lists.freedesktop.org/archives/piglit/2015-April/015877.html
>>
>>
>>  src/glsl/link_atomics.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
>> index 603873a..9528e42 100644
>> --- a/src/glsl/link_atomics.cpp
>> +++ b/src/glsl/link_atomics.cpp
>> @@ -201,7 +201,7 @@ link_assign_atomic_counter_resources(struct gl_context *ctx,
>>           gl_uniform_storage *const storage = &prog->UniformStorage[id];
>>  
>>           mab.Uniforms[j] = id;
>> -         if (!var->data.explicit_binding)
>> +         if (var->data.explicit_binding)
>>              var->data.binding = i;
>>  
>>           storage->atomic_buffer_index = i;
> I'm no expert on atomic counters (so someone correct me if I'm wrong)
> but I've taken a look into this and I think the correct solution may be
> to remove the if statement completely. This would match the original
> implementation.
>
> The reason for this is that currently Mesa requires all atomic counters
> to have an explicit binding point (so currently this is always true),
> although the spec doesn't say that this is a requirement. Bindings can
> be implied, it's likely that once Mesa does support these implied
> bindings the new if statement is probably not going to make sense for
> that case either.

This is a good point. For example, even with the current code, you need
to call glBindBufferBase with binding 0 in order to work, even when the
initial value of data.binding is zero. I made that change, and run again
a full run of piglit, without finding any regression. In any case, I
will not provide a third version of the patch until someone confirms
definitely that the if statement should be removed.

Thanks for the suggestion

Best regards


-- 
Alejandro Piñeiro (apinheiro at igalia.com)