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

[Tapani Pälli]

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.


> Sam
>
>>
>>> +
>>>       if (brw->gen >= 6) {
>>>          ctx->Const.MaxVarying = 32;
>>>          ctx->Const.Program[MESA_SHADER_VERTEX].MaxOutputComponents = 128;
>>>
>>
>> // Tapani
>>