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 <sigles...@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@entry=0x7ffff7fd0040) at >>> main/bufferobj.c:907 >>> #5 0x00007ffff1db553b in _mesa_free_context_data >>> (ctx=ctx@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. 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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
