[Mesa-dev] [PATCH v4 (part2) 04/56] i965: set ARB_shader_storage_buffer_object related constant values

[Tapani Pälli]

On 09/03/2015 04:56 PM, Samuel Iglesias Gonsálvez wrote:
>
> On 03/09/15 14:08, Tapani Pälli wrote:
>>
>> On 09/03/2015 03:05 PM, Iago Toral wrote:
>>> On Thu, 2015-09-03 at 13:52 +0300, Tapani Pälli wrote:
>>>> On 09/03/2015 01:40 PM, Samuel Iglesias Gonsálvez wrote:
>>>>>
>>>>> On 03/09/15 12:30, Tapani Pälli wrote:
>>>>>> Hi;
>>>>>>
>>>>>> On 07/23/2015 09:42 AM, Samuel Iglesias Gonsalvez wrote:
>>>>>>> v2:
>>>>>>> - Add tessellation shader constants assignment
>>>>>>>
>>>>>>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>>>>>> ---
>>>>>>>      src/mesa/drivers/dri/i965/brw_context.c | 12 ++++++++++++
>>>>>>>      1 file changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>>>>>>> b/src/mesa/drivers/dri/i965/brw_context.c
>>>>>>> index b08a53b..a5c7b91 100644
>>>>>>> --- a/src/mesa/drivers/dri/i965/brw_context.c
>>>>>>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
>>>>>>> @@ -551,6 +551,18 @@ brw_initialize_context_constants(struct
>>>>>>> brw_context *brw)
>>>>>>>         ctx->Const.TextureBufferOffsetAlignment = 16;
>>>>>>>         ctx->Const.MaxTextureBufferSize = 128 * 1024 * 1024;
>>>>>>>
>>>>>>> +   /* FIXME: Tessellation stages are not yet supported in i965, so
>>>>>>> +    * MaxCombinedShaderStorageBlocks doesn't take them into account.
>>>>>>> +    */
>>>>>>> +   ctx->Const.Program[MESA_SHADER_VERTEX].MaxShaderStorageBlocks
>>>>>>> = 12;
>>>>>>> +
>>>>>>> ctx->Const.Program[MESA_SHADER_GEOMETRY].MaxShaderStorageBlocks = 12;
>>>>>>> +
>>>>>>> ctx->Const.Program[MESA_SHADER_TESS_EVAL].MaxShaderStorageBlocks = 0;
>>>>>>> +
>>>>>>> ctx->Const.Program[MESA_SHADER_TESS_CTRL].MaxShaderStorageBlocks = 0;
>>>>>>> +
>>>>>>> ctx->Const.Program[MESA_SHADER_FRAGMENT].MaxShaderStorageBlocks = 12;
>>>>>>> +   ctx->Const.Program[MESA_SHADER_COMPUTE].MaxShaderStorageBlocks
>>>>>>> = 12;
>>>>>>> +   ctx->Const.MaxCombinedShaderStorageBlocks = 12 * 3;
>>>>>>> +   ctx->Const.MaxShaderStorageBufferBindings = 48;
>>>>>> I think there is something funny with MaxShaderStorageBufferBindings
>>>>>> value calculation. Commit 28ef0f83 adds 12 to it and then this commit
>>>>>> overwrites it as 48. Without compute shaders I guess this value should
>>>>>> be 48 - 12?
>>>>>>
>>>>>> I guess earlier '+12' should be removed and this part should be
>>>>>> modified
>>>>>> to calculate based on what is supported. For me '48 -12' fixes a few
>>>>>> crashes I'm getting with
>>>>>> "igalia/wip/siglesias/ARB_shader_storage_buffer_object-v4.3" branch
>>>>>> I've
>>>>>> used for some testing.
>>>>>>
>>>>> I see. We are going to review the MaxShaderStorageBufferBindings
>>>>> calculation, thanks for the report and the ideas.
>>>>>
>>>>> Can you share with us the tests that crashed because of this? You can
>>>>> send them privately to me, if needed.
>>>> These are some Piglit tests that use buffer objects. Crash happens at
>>>> _mesa_free_buffer_objects(), it's not quite clear yet to me why (maybe
>>>> memory gets trashed) but I bisected it to this commit changing
>>>> MaxShaderStorageBufferBindings. For example
>>>> 'arb_framebuffer_no_attachments-atomic' segfaults when destroying the
>>>> context and calling _mesa_free_buffer_objects().
>>>>
>>>> Here's example backtrace:
>>>> (gdb) bt
>>>> #0  0x00007ffff3dae830 in pthread_mutex_lock () from
>>>> /lib64/libpthread.so.0
>>>> #1  0x00007ffff1da8681 in mtx_lock (mtx=0xffffffffffffffff) at
>>>> ../../include/c11/threads_posix.h:192
>>>> #2  _mesa_reference_buffer_object_ (ctx=0x7ffff7fd0040,
>>>> ptr=0x7ffff7ff13d0, bufObj=0x0) at main/bufferobj.c:450
>>>> #3  0x00007ffff1da9173 in _mesa_reference_buffer_object (bufObj=0x0,
>>>> ptr=<optimized out>, ctx=0x7ffff7fd0040) at main/bufferobj.h:123
>>>> #4  _mesa_free_buffer_objects (ctx=ctx at entry=0x7ffff7fd0040) at
>>>> main/bufferobj.c:907
>>>> #5  0x00007ffff1db553b in _mesa_free_context_data
>>>> (ctx=ctx at entry=0x7ffff7fd0040) at main/context.c:1342
>>>> #6  0x00007ffff2077a3e in intelDestroyContext (driContextPriv=0x6392f0)
>>>> at brw_context.c:996
>>>>
>>>> it crashes because 'oldObj' in _mesa_reference_buffer_object_ points to
>>>> 0xffffffffffffffff.
>>> Thanks for reporting this, the problem with the crash is that there is a
>>> mismatch between the number of bindings used by the driver and the
>>> maximum declared by Mesa, this was actually a silly mistake we
>>> introduced after updating some of our code during reviews. To fix this
>>> you need to set MAX_SHADER_STORAGE_BUFFERS in src/mesa/main/config.h to
>>> 15 (instead of the current value of 7, the issue comes from 7*6 = 42 <
>>> 48).
>>>
>>> Samuel is going to look into the +12 issue you also mentioned.
>> OK thanks!
>>
> You were right: we were overwriting MaxShaderStorageBufferBindings and
> the +12 was lost. I modified this patch to set
> MaxShaderStorageBufferBindings = 36 and later add the +12 if
> ARB_compute_shader is enabled.

This sounds a proper solution (I have same locally), you have my r-b for 
that.

> Jordan, this patch already had your R-b but we modified it a little bit
> to fix this bug. Once we have a final version of the patch series with
> all changes and fixes, we will contact you to talk about what to do with
> this patch (keep your R-b or not) and others, in order to facilitate the
> review.
>
> Thanks!
>
> Sam
>
>> // Tapani
>>