[Mesa-dev] [PATCH v4 (part2) 53/59] glapi: add ARB_shader_storage_block_buffer_object

[Samuel Iglesias Gonsálvez]

On 25/08/15 12:46, Emil Velikov wrote:
> On 25 August 2015 at 06:42, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com> wrote:
>> On 24/08/15 16:10, Emil Velikov wrote:
>>> Hi Samuel, Iago,
>>>
>>> On 05/08/15 09:30, Iago Toral Quiroga wrote:
>>>> From: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>>>
>>>> v2:
>>>> - Add ShaderStorageBlockBinding to static_data.py
>>>>
>>> Why (see comment below) ?
>>>
>>>
>>> v4:
>>>  - Ship ARB_shader_storage_buffer_object.xml in the tarball.
>>>
>>> Seems like other patches are also missing v4 revision history :|
>>>
>>
>> We usually send a new version of the patch series when we have not
>> received more reviews for several days/weeks
> How about pinging individual patches (or a list in the series cover letter) ?
> 
>> and we have enough number
>> of changes in different patches (because of reviews and/or major changes
>> in master that invalidate our code).
>>
> Just do a follow up revision of the said patch(es) ?
> 
>> For that reason, some of our patches don't have new comments in the
>> revision history.
>>
> Afaics not even one patch in v4 does has revision history. Please don't do that.
> 
> Another thing you can try is gradually sending patches rather than
> accumulating X months of work. This way the reviewer(s) will do
> (almost) all it in one go.
> 

Yeah, you are right. We are going to take it into account for future
submissions. Thanks for the advices!

>>>
>>>> --- a/src/mapi/glapi/gen/static_data.py
>>>> +++ b/src/mapi/glapi/gen/static_data.py
>>>> @@ -1305,6 +1305,7 @@ functions = [
>>>>     "ShaderBinary",
>>>>     "ShaderSource",
>>>>     "ShaderSourceARB",
>>>> +   "ShaderStorageBlockBinding",
>>> This list is for functions exported by libGL.so, which imho should not
>>> change (ever). If there is there a program that depends on this static
>>> symbol (and the binary drivers expose it), it should be noted in the
>>> commit message.
>>>
>>
>> This is a new function defined by ARB_shader_storage_buffer_object
>> specification [0].
>>
> The spec does not say that the function should be statically available
> (via libGL.so). The OpenGL ABI has some information as to which
> functions should be available statically/dynamically [1]
> 
> [1] https://www.opengl.org/registry/ABI/
> 
>> I have a test program that uses that function. If I remove the
>> static_data.py definition, I hit this linker error:
>>
>> <file>: undefined reference to `glShaderStorageBlockBinding'
>>
> Sounds like a bug in the app. As mentioned by Ian, use libepoxy or
> alike which will fetch all the symbols using the appropriate method.
> 
>> So following what how other functions are defined, I added it to
>> static_data.py.
> git log shows that the list of functions hasn't changed since day one,
> while at the same time other extensions were added/enabled in mesa.
> 
> Hope this answers things from a different angle rather then flocking
> up a dead horse :)
> 

Thank you very much for this explanation and for the link!

Sam

> Thanks
> Emil
>