[Mesa-dev] [PATCH v8 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue v7: rebase (update const handling) s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 24 -- src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 34 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 0c34b63854..c555727682 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -367,79 +367,79 @@ - + - + - + - + - + - + @@ -516,30 +516,30 @@ - + - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0a74..6e1ac09ce0 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -75,21 +75,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250777..447b03a41d 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -115,28 +115,30 @@ param: img_send_null - boolean flag to determine if blank pixel data should be sent when a NULL pointer is passed. This is only used by TexImage1D and TexImage2D. img_null_flag - boolean flag to determine if an extra flag is used to determine if a NULL pixel pointer was passed. This is used by TexSubImage1D, TexSubImage2D, TexImage3D and others. img_pad_dimensions - boolean flag to determine if dimension data and offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't want to track state for. glx: rop - Opcode value for "render" commands sop - Opcode value for "single&qu
[Mesa-dev] [PATCH v8 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers to be valid. v4: update comments based on Nicolai review v7: s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Drop the ShadowBufferObjects hash. Synchronous creation/destruction gives us enough guarantee to lookup the BufferObjects hash directly. Signed-off-by: Gregory Hainaut --- src/mesa/main/marshal.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 1914b5..285a46457d 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -237,21 +237,34 @@ _mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, /* BindBufferBase: marshalled asynchronously */ struct marshal_cmd_BindBufferBase { struct marshal_cmd_base cmd_base; GLenum target; GLuint index; GLuint buffer; }; -/** Tracks the current bindings for the vertex array and index array buffers. +/** + * Check that buffer is a valid buffer handle + * Always return false for ID 0. + */ +static bool +is_bufferobj(struct gl_context *ctx, GLuint buffer) +{ + if (buffer == 0) + return false; + else + return _mesa_HashLookup(ctx->Shared->BufferObjects, buffer) != NULL; +} + +/** Tracks the current bindings of GL buffer targets * * This is part of what we need to enable glthread on compat-GL contexts that * happen to use VBOs, without also supporting the full tracking of VBO vs * user vertex array bindings per attribute on each vertex array for * determining what to upload at draw call time. * * Note that GL core makes it so that a buffer binding with an invalid handle * in the "buffer" parameter will throw an error, and then a * glVertexAttribPointer() that followsmight not end up pointing at a VBO. * However, in GL core the draw call would throw an error as well, so we don't @@ -259,37 +272,54 @@ struct marshal_cmd_BindBufferBase * marshal user data for draw calls, and the unmarshal will just generate an * error or not as appropriate. * * For compatibility GL, we do need to accurately know whether the draw call * on the unmarshal side will dereference a user pointer or load data from a * VBO per vertex. That would make it seem like we need to track whether a * "buffer" is valid, so that we can know when an error will be generated * instead of updating the binding. However, compat GL has the ridiculous * feature that if you pass a bad name, it just gens a buffer object for you, * so we escape without having to know if things are valid or not. + * + * Pixel buffers are tracked to decide whether pixel transfer goes to a user + * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike + * for VBOs, we do need accurate tracking, since user pointers can be used in + * GL core contexts. */ static void -track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) +track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer) { struct glthread_state *glthread = ctx->GLThread; switch (target) { case GL_ARRAY_BUFFER: glthread->vertex_array_is_vbo = (buffer != 0); break; case GL_ELEMENT_ARRAY_BUFFER: /* The current element array buffer binding is actually tracked in the * vertex array object instead of the context, so this would need to * change on vertex array object updates. */ glthread->element_array_is_vbo = (buffer != 0); break; + case GL_PIXEL_UNPACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->bound_pixel_unpack_buffer = buffer; + else + glthread->bound_pixel_unpack_buffer = 0; + break; + case GL_PIXEL_PACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->bound_pixel_pack_buffer = buffer; + else + glthread->bound_pixel_pack_buffer = 0; + break; } } struct marshal_cmd_BindBuffer { struct marshal_cmd_base cmd_base; GLenum target; GLuint buffer; }; @@ -307,21 +337,21 @@ _mesa_unmarshal_BindBuffer(struct gl_context *ctx, CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer)); } void GLAPIENTRY _mesa_marshal_BindBuffer(GLenum target, GLuint buffer) { GET_CURRENT_CONTEXT(ctx); size_t cmd_size = sizeof(struct marshal_cmd_BindBuffer); struct marshal_cmd_BindBuffer *cmd; debug_print_marshal("BindBuffer"); - track_vbo_binding(ctx, target, buffer); + track_buffers_binding(ctx, target, buffer); if (cmd_size <= MARSHAL_MAX_CMD_SIZE) { cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer, cmd_size); cmd->target = target; cmd->buffer = buffer; _mesa_post_marshal_hook(ctx); } else { _mesa_
[Mesa-dev] [PATCH v8 1/3] mesa/glthread: track buffer destruction
It would be used in following commits to allow asynchronous PBO transfer. The tracking saves the buffer name into a hash. Saving pointer will be more complex as the buffer is created in BindBuffer due to IsBuffer insanity. Perf wise DeleteBuffers is now synchronous for robustness. v5: properly delete hash element with the help of _mesa_HashDeleteAll v6: rebase v7: rebase s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Drop the ShadowBufferObjects hash. Synchronous creation/destruction gives us enough guarantee to lookup the BufferObjects hash directly. Drop custom code for GenBuffers/CreateBuffers v8: rebase Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/gl_API.xml | 2 +- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c | 40 src/mesa/main/marshal.h | 8 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index 18839ec70c..4b01ca552f 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -5055,21 +5055,21 @@ - + diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h index 306246ca1c..e5c8b79a97 100644 --- a/src/mesa/main/glthread.h +++ b/src/mesa/main/glthread.h @@ -88,20 +88,30 @@ struct glthread_state * Tracks on the main thread side whether the current vertex array binding * is in a VBO. */ bool vertex_array_is_vbo; /** * Tracks on the main thread side whether the current element array (index * buffer) binding is in a VBO. */ bool element_array_is_vbo; + + /** +* Tracks on the main thread side the bound unpack pixel buffer +*/ + GLuint bound_pixel_unpack_buffer; + + /** +* Tracks on the main thread side the bound pack pixel buffer +*/ + GLuint bound_pixel_pack_buffer; }; void _mesa_glthread_init(struct gl_context *ctx); void _mesa_glthread_destroy(struct gl_context *ctx); void _mesa_glthread_restore_dispatch(struct gl_context *ctx); void _mesa_glthread_flush_batch(struct gl_context *ctx); void _mesa_glthread_finish(struct gl_context *ctx); #endif /* _GLTHREAD_H*/ diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 8f8e8c78ed..1914b5 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -25,20 +25,21 @@ * * Custom functions for marshalling GL calls from the main thread to a worker * thread when automatic code generation isn't appropriate. */ #include "main/enums.h" #include "main/macros.h" #include "marshal.h" #include "dispatch.h" #include "marshal_generated.h" +#include "hash.h" struct marshal_cmd_Flush { struct marshal_cmd_base cmd_base; }; void _mesa_unmarshal_Flush(struct gl_context *ctx, const struct marshal_cmd_Flush *cmd) @@ -188,20 +189,59 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, _mesa_post_marshal_hook(ctx); } else { _mesa_glthread_finish(ctx); CALL_ShaderSource(ctx->CurrentServerDispatch, (shader, count, string, length_tmp)); } free(length_tmp); } +static void track_buffers_destruction(struct gl_context *ctx, + GLsizei n, const GLuint * buffers) +{ + GLsizei i; + struct glthread_state *glthread = ctx->GLThread; + + if (n < 0 || !buffers) + return; + + for (i = 0; i < n ; i++) { + if (buffers[i] == glthread->bound_pixel_pack_buffer) + glthread->bound_pixel_pack_buffer = 0; + + if (buffers[i] == glthread->bound_pixel_unpack_buffer) + glthread->bound_pixel_unpack_buffer = 0; + } +} + +/* DeleteBuffers: custom marshal to track buffers destruction */ +void GLAPIENTRY +_mesa_marshal_DeleteBuffers(GLsizei n, const GLuint * buffer) +{ + GET_CURRENT_CONTEXT(ctx); + _mesa_glthread_finish(ctx); + debug_print_sync("DeleteBuffers"); + + // It is done before CALL_DeleteBuffers to avoid any ABA multithread issue. + track_buffers_destruction(ctx, n, buffer); + + CALL_DeleteBuffers(ctx->CurrentServerDispatch, (n, buffer)); +} + +void +_mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, + const struct marshal_cmd_DeleteBuffers *cmd) +{ + assert(0); +} + /* BindBufferBase: marshalled asynchronously */ struct marshal_cmd_BindBufferBase { struct marshal_cmd_base cmd_base; GLenum target; GLuint index; GLuint buffer; }; /** Tracks the current bindings for the vertex array and index array buffers. diff --git a/src/mesa/main/marshal.h b/src/mesa/main/marshal.h index 63e0295576..e7f681213c 100644 --- a/src/mesa/main/marshal.h +++ b/src/mesa/main/marshal.h @@ -180,20 +180,2
[Mesa-dev] [PATCH v8 0/3] asynchronous pbo transfer with glthread
Hello Mesa developers, > Please find a new version to handle invalid buffer handles. > > Allow to handle this kind of case: >genBuffer(&pbo); >BindBuffer(pbo) >DeleteBuffer(pbo); >BindBuffer(rand_pbo) >TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous > > There are various subtely to handle multi threaded shared context. In order to > keep the code sane, I've considered a buffer invalid when it is deleted by a > context even it is still bound to others contexts. It will force a synchronous > transfer which is always safe. > > An example could be >Ctx A: glGenBuffers(1, &pbo); >Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); >Ctx B: glDeleteBuffers(1, &pbo); >Ctx A: glTexSubImage2D(...); // will be synchronous, even though it >_could_ be asynchronous (because the PBO that was generated first is >still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback V5: Properly delete element of the new hash (first patch) v6: Rebase on latest master v7: Fredrik's suggestion (remove shadow hash table and rename pixel_*pack_buffer_bound variables) I rebased the code on latest master, it got extra conflict (gl_marshal.py) since the resent from Timothy v8: rebase on latest master Best regards, Gregory Hainaut (3): mesa/glthread: track buffer destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++--- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 ++-- src/mapi/glapi/gen/gl_API.xml | 30 +- src/mapi/glapi/gen/gl_marshal.py | 24 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +-- src/mesa/main/glthread.h | 10 src/mesa/main/marshal.c| 76 +- src/mesa/main/marshal.h| 8 +++ 9 files changed, 159 insertions(+), 38 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v7 0/3] asynchronous pbo transfer with glthread
Hello Mesa developers, > Please find a new version to handle invalid buffer handles. > > Allow to handle this kind of case: >genBuffer(&pbo); >BindBuffer(pbo) >DeleteBuffer(pbo); >BindBuffer(rand_pbo) >TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous > > There are various subtely to handle multi threaded shared context. In order to > keep the code sane, I've considered a buffer invalid when it is deleted by a > context even it is still bound to others contexts. It will force a synchronous > transfer which is always safe. > > An example could be >Ctx A: glGenBuffers(1, &pbo); >Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); >Ctx B: glDeleteBuffers(1, &pbo); >Ctx A: glTexSubImage2D(...); // will be synchronous, even though it >_could_ be asynchronous (because the PBO that was generated first is >still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback V5: Properly delete element of the new hash (first patch) v6: Rebase on latest master v7: Fredrik's suggestion (remove shadow hash table and rename pixel_*pack_buffer_bound variables) I rebased the code on latest master, it got extra conflict (gl_marshal.py) since the resent from Timothy Best regards, Gregory Hainaut (3): mesa/glthread: track buffer destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++--- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 ++-- src/mapi/glapi/gen/gl_API.xml | 30 +- src/mapi/glapi/gen/gl_marshal.py | 24 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +-- src/mesa/main/glthread.h | 10 src/mesa/main/marshal.c| 76 +- src/mesa/main/marshal.h| 8 +++ 9 files changed, 159 insertions(+), 38 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v7 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue v7: rebase (update const handling) s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 24 -- src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 34 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index d848f78d62..1474ae3d9a 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -367,79 +367,79 @@ - + - + - + - + - + - + @@ -516,30 +516,30 @@ - + - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0a74..6e1ac09ce0 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -75,21 +75,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250777..447b03a41d 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -115,28 +115,30 @@ param: img_send_null - boolean flag to determine if blank pixel data should be sent when a NULL pointer is passed. This is only used by TexImage1D and TexImage2D. img_null_flag - boolean flag to determine if an extra flag is used to determine if a NULL pixel pointer was passed. This is used by TexSubImage1D, TexSubImage2D, TexImage3D and others. img_pad_dimensions - boolean flag to determine if dimension data and offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't want to track state for. glx: rop - Opcode value for "render" commands sop - Opcode value for "single&qu
[Mesa-dev] [PATCH v7 1/3] mesa/glthread: track buffer destruction
It would be used in following commits to allow asynchronous PBO transfer. The tracking saves the buffer name into a hash. Saving pointer will be more complex as the buffer is created in BindBuffer due to IsBuffer insanity. Perf wise DeleteBuffers is now synchronous for robustness. v5: properly delete hash element with the help of _mesa_HashDeleteAll v6: rebase v7: rebase s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Drop the ShadowBufferObjects hash. Synchronous creation/destruction gives us enough guarantee to lookup the BufferObjects hash directly. Drop custom code for GenBuffers/CreateBuffers Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/gl_API.xml | 2 +- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c | 40 src/mesa/main/marshal.h | 8 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index 85083a428d..38123dc85a 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -5055,21 +5055,21 @@ - + diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h index 306246ca1c..e5c8b79a97 100644 --- a/src/mesa/main/glthread.h +++ b/src/mesa/main/glthread.h @@ -88,20 +88,30 @@ struct glthread_state * Tracks on the main thread side whether the current vertex array binding * is in a VBO. */ bool vertex_array_is_vbo; /** * Tracks on the main thread side whether the current element array (index * buffer) binding is in a VBO. */ bool element_array_is_vbo; + + /** +* Tracks on the main thread side the bound unpack pixel buffer +*/ + GLuint bound_pixel_unpack_buffer; + + /** +* Tracks on the main thread side the bound pack pixel buffer +*/ + GLuint bound_pixel_pack_buffer; }; void _mesa_glthread_init(struct gl_context *ctx); void _mesa_glthread_destroy(struct gl_context *ctx); void _mesa_glthread_restore_dispatch(struct gl_context *ctx); void _mesa_glthread_flush_batch(struct gl_context *ctx); void _mesa_glthread_finish(struct gl_context *ctx); #endif /* _GLTHREAD_H*/ diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 8db4531440..abb565191a 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -25,20 +25,21 @@ * * Custom functions for marshalling GL calls from the main thread to a worker * thread when automatic code generation isn't appropriate. */ #include "main/enums.h" #include "main/macros.h" #include "marshal.h" #include "dispatch.h" #include "marshal_generated.h" +#include "hash.h" struct marshal_cmd_Flush { struct marshal_cmd_base cmd_base; }; void _mesa_unmarshal_Flush(struct gl_context *ctx, const struct marshal_cmd_Flush *cmd) @@ -188,20 +189,59 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, _mesa_post_marshal_hook(ctx); } else { _mesa_glthread_finish(ctx); CALL_ShaderSource(ctx->CurrentServerDispatch, (shader, count, string, length_tmp)); } free(length_tmp); } +static void track_buffers_destruction(struct gl_context *ctx, + GLsizei n, const GLuint * buffers) +{ + GLsizei i; + struct glthread_state *glthread = ctx->GLThread; + + if (n < 0 || !buffers) + return; + + for (i = 0; i < n ; i++) { + if (buffers[i] == glthread->bound_pixel_pack_buffer) + glthread->bound_pixel_pack_buffer = 0; + + if (buffers[i] == glthread->bound_pixel_unpack_buffer) + glthread->bound_pixel_unpack_buffer = 0; + } +} + +/* DeleteBuffers: custom marshal to track buffers destruction */ +void GLAPIENTRY +_mesa_marshal_DeleteBuffers(GLsizei n, const GLuint * buffer) +{ + GET_CURRENT_CONTEXT(ctx); + _mesa_glthread_finish(ctx); + debug_print_sync("DeleteBuffers"); + + // It is done before CALL_DeleteBuffers to avoid any ABA multithread issue. + track_buffers_destruction(ctx, n, buffer); + + CALL_DeleteBuffers(ctx->CurrentServerDispatch, (n, buffer)); +} + +void +_mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, + const struct marshal_cmd_DeleteBuffers *cmd) +{ + assert(0); +} + /* BindBufferBase: marshalled asynchronously */ struct marshal_cmd_BindBufferBase { struct marshal_cmd_base cmd_base; GLenum target; GLuint index; GLuint buffer; }; /** Tracks the current bindings for the vertex array and index array buffers. diff --git a/src/mesa/main/marshal.h b/src/mesa/main/marshal.h index 999c75e3ea..b155a86012 100644 --- a/src/mesa/main/marshal.h +++ b/src/mesa/main/marshal.h @@ -176,20 +176,21 @@ _mesa_glt
[Mesa-dev] [PATCH v7 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers to be valid. v4: update comments based on Nicolai review v7: s/GLint pixel_pack_buffer_bound/GLuint bound_pixel_pack_buffer/ Drop the ShadowBufferObjects hash. Synchronous creation/destruction gives us enough guarantee to lookup the BufferObjects hash directly. Signed-off-by: Gregory Hainaut --- src/mesa/main/marshal.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index abb565191a..ba1841064b 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -237,21 +237,34 @@ _mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, /* BindBufferBase: marshalled asynchronously */ struct marshal_cmd_BindBufferBase { struct marshal_cmd_base cmd_base; GLenum target; GLuint index; GLuint buffer; }; -/** Tracks the current bindings for the vertex array and index array buffers. +/** + * Check that buffer is a valid buffer handle + * Always return false for ID 0. + */ +static bool +is_bufferobj(struct gl_context *ctx, GLuint buffer) +{ + if (buffer == 0) + return false; + else + return _mesa_HashLookup(ctx->Shared->BufferObjects, buffer) != NULL; +} + +/** Tracks the current bindings of GL buffer targets * * This is part of what we need to enable glthread on compat-GL contexts that * happen to use VBOs, without also supporting the full tracking of VBO vs * user vertex array bindings per attribute on each vertex array for * determining what to upload at draw call time. * * Note that GL core makes it so that a buffer binding with an invalid handle * in the "buffer" parameter will throw an error, and then a * glVertexAttribPointer() that followsmight not end up pointing at a VBO. * However, in GL core the draw call would throw an error as well, so we don't @@ -259,37 +272,54 @@ struct marshal_cmd_BindBufferBase * marshal user data for draw calls, and the unmarshal will just generate an * error or not as appropriate. * * For compatibility GL, we do need to accurately know whether the draw call * on the unmarshal side will dereference a user pointer or load data from a * VBO per vertex. That would make it seem like we need to track whether a * "buffer" is valid, so that we can know when an error will be generated * instead of updating the binding. However, compat GL has the ridiculous * feature that if you pass a bad name, it just gens a buffer object for you, * so we escape without having to know if things are valid or not. + * + * Pixel buffers are tracked to decide whether pixel transfer goes to a user + * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike + * for VBOs, we do need accurate tracking, since user pointers can be used in + * GL core contexts. */ static void -track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) +track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer) { struct glthread_state *glthread = ctx->GLThread; switch (target) { case GL_ARRAY_BUFFER: glthread->vertex_array_is_vbo = (buffer != 0); break; case GL_ELEMENT_ARRAY_BUFFER: /* The current element array buffer binding is actually tracked in the * vertex array object instead of the context, so this would need to * change on vertex array object updates. */ glthread->element_array_is_vbo = (buffer != 0); break; + case GL_PIXEL_UNPACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->bound_pixel_unpack_buffer = buffer; + else + glthread->bound_pixel_unpack_buffer = 0; + break; + case GL_PIXEL_PACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->bound_pixel_pack_buffer = buffer; + else + glthread->bound_pixel_pack_buffer = 0; + break; } } struct marshal_cmd_BindBuffer { struct marshal_cmd_base cmd_base; GLenum target; GLuint buffer; }; @@ -307,21 +337,21 @@ _mesa_unmarshal_BindBuffer(struct gl_context *ctx, CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer)); } void GLAPIENTRY _mesa_marshal_BindBuffer(GLenum target, GLuint buffer) { GET_CURRENT_CONTEXT(ctx); size_t cmd_size = sizeof(struct marshal_cmd_BindBuffer); struct marshal_cmd_BindBuffer *cmd; debug_print_marshal("BindBuffer"); - track_vbo_binding(ctx, target, buffer); + track_buffers_binding(ctx, target, buffer); if (cmd_size <= MARSHAL_MAX_CMD_SIZE) { cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer, cmd_size); cmd->target = target; cmd->buffer = buffer; _mesa_post_marshal_hook(ctx); } else { _mesa_
Re: [Mesa-dev] [PATCH 1/3] mesa/glthread: track buffer creation/destruction
Hello Fredrik, Yes the shadow hash feels useless now. I will update the patch in a couple of days (vacation currently). Cheers, Gregory Le 22 juin 2017 2:24 PM, "Fredrik Höglund" a écrit : > On Thursday 22 June 2017, Timothy Arceri wrote: > > From: Gregory Hainaut > > > > It would be used in next commit to allow asynchronous PBO transfer. > > > > The tracking saves the buffer name into a hash. Saving pointer > > will be more complex as the buffer is created in BindBuffer due to > IsBuffer > > insanity. > > > > Perf wise DeleteBuffers is now synchronous for robustness. > > > > v5: properly delete hash element with the help of _mesa_HashDeleteAll > > > > v6: rebase > > Signed-off-by: Gregory Hainaut > > --- > > src/mapi/glapi/gen/ARB_direct_state_access.xml | 2 +- > > src/mapi/glapi/gen/gl_API.xml | 4 +- > > src/mesa/main/glthread.h | 10 +++ > > src/mesa/main/marshal.c| 113 > + > > src/mesa/main/marshal.h| 24 ++ > > src/mesa/main/mtypes.h | 5 ++ > > src/mesa/main/shared.c | 14 +++ > > 7 files changed, 169 insertions(+), 3 deletions(-) > > > > diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml > b/src/mapi/glapi/gen/ARB_direct_state_access.xml > > index cb24d79..b75c772a 100644 > > --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml > > +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml > > @@ -42,21 +42,21 @@ > > > > > > > > > > > > > > > > > > > > > > - > > + > > > > > > > > > > > > > > > > > > > > > > diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API. > xml > > index 8784f05..aa34c04 100644 > > --- a/src/mapi/glapi/gen/gl_API.xml > > +++ b/src/mapi/glapi/gen/gl_API.xml > > @@ -5053,27 +5053,27 @@ > > > > >no_error="true"> > > > > > > > > > > > > > > > > - > > + > > > > > > > > > > > > - > > + > > > > > > > > > > > > > > > > > > variable_param="pname"/> > > > > diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h > > index dd65931..351be6c 100644 > > --- a/src/mesa/main/glthread.h > > +++ b/src/mesa/main/glthread.h > > @@ -89,20 +89,30 @@ struct glthread_state > > * Tracks on the main thread side whether the current vertex array > binding > > * is in a VBO. > > */ > > bool vertex_array_is_vbo; > > > > /** > > * Tracks on the main thread side whether the current element array > (index > > * buffer) binding is in a VBO. > > */ > > bool element_array_is_vbo; > > + > > + /** > > +* Tracks on the main thread side the bound unpack pixel buffer > > +*/ > > + GLint pixel_unpack_buffer_bound; > > + > > + /** > > +* Tracks on the main thread side the bound pack pixel buffer > > +*/ > > + GLint pixel_pack_buffer_bound; > > }; > > I suggest naming these bound_pixel_unpack_buffer and > bound_pixel_pack_buffer, respectively. The name pixel_pack_buffer_bound > suggests that this is a boolean value. > > The type should also be GLuint. > > One more comment below: > > > > > void _mesa_glthread_init(struct gl_context *ctx); > > void _mesa_glthread_destroy(struct gl_context *ctx); > > > > void _mesa_glthread_restore_dispatch(struct gl_context *ctx); > > void _mesa_glthread_flush_batch(struct gl_context *ctx); > > void _mesa_glthread_finish(struct gl_context *ctx); > > > > #endif /* _GLTHREAD_H*/ > > diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c > > index 4840f32..b1731ab 100644 > > --- a/src/mesa/main/marshal.c > > +++ b/src/mesa/main/marshal.c > > @@ -25,20 +25,21 @@ > > * > > * Custom functions for marshalling GL calls from the main thread to a > worker > > * thread when automatic code generation isn't appropriate. > > */ > >
Re: [Mesa-dev] [PATCH v5 0/4] Disable glthread if libX11 isn't thread-safe
On Mon, 29 May 2017 17:12:05 +0100 Emil Velikov wrote: > On 29 May 2017 at 15:45, Dieter Nützel wrote: > > Hi Gregory, > > > > there isn't currently a copy of this on Mesa-Patchwork. > > Can you please send one over there? > > > > And maybe an updated version of: > > [PATCH v5 0/3] asynchronous pbo transfer with glthread > > > > Would be awesome. > > Isn't Mesa-Patchwork a bot that parse the mail from the mailing list ? Reading Mesa doc, there is only a note that "patches must be marked superseeded manually" > The series is in master now, so it should be a bit easier ;-) > > Gregory, thanks again for the work. Please keep an eye open for > reports. it's highly unlikely but still. > > -Emil Thanks you for the review and the push :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 0/4] Disable glthread if libX11 isn't thread-safe
Hello Mesa developers, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. v2: based on Nicolai/Matt reviews Add a check on DRI extension version Use C comments :) v3: based on Emil reviews Split the initial first patch into 3 sub patches dri extension / glx / egl Improve error message Improve code readability Just include libX11 on EGL protected by ifdef v4: based on Eric feedback, I marked DRI3 as always thread safe v5: Fix the null pointer check on patch 4. I added Daniel comment on patch 3 but I'm not sure I got it right. Thanks you for all the review comments. Best regards, Gregory Hainaut (4): dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety glx: implement __DRIbackgroundCallableExtension.isThreadSafe egl: implement __DRIbackgroundCallableExtension.isThreadSafe glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 13 +++ src/egl/drivers/dri2/egl_dri2.c | 34 +++- src/gallium/state_trackers/dri/dri_context.c | 21 + src/glx/dri2_glx.c | 15 +++- src/glx/dri3_glx.c | 12 +- 5 files changed, 88 insertions(+), 7 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 4/4] glthread/gallium: require safe_glthread to start glthread
Print an error message for the user if the requirement isn't met, or we're not thread safe. v2: based on Nicolai feedbacks Check the DRI extension version v3: based on Emil feedbacks improve commit and error messages. use backgroundCallable variable to improve readability v5: based on Emil feedbacks Properly check the function pointer Signed-off-by: Gregory Hainaut --- src/gallium/state_trackers/dri/dri_context.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/gallium/state_trackers/dri/dri_context.c b/src/gallium/state_trackers/dri/dri_context.c index 92d79849c4..ec555e44d7 100644 --- a/src/gallium/state_trackers/dri/dri_context.c +++ b/src/gallium/state_trackers/dri/dri_context.c @@ -51,20 +51,22 @@ dri_create_context(gl_api api, const struct gl_config * visual, { __DRIscreen *sPriv = cPriv->driScreenPriv; struct dri_screen *screen = dri_screen(sPriv); struct st_api *stapi = screen->st_api; struct dri_context *ctx = NULL; struct st_context_iface *st_share = NULL; struct st_context_attribs attribs; enum st_context_error ctx_err = 0; unsigned allowed_flags = __DRI_CTX_FLAG_DEBUG | __DRI_CTX_FLAG_FORWARD_COMPATIBLE; + const __DRIbackgroundCallableExtension *backgroundCallable = + screen->sPriv->dri2.backgroundCallable; if (screen->has_reset_status_query) allowed_flags |= __DRI_CTX_FLAG_ROBUST_BUFFER_ACCESS; if (flags & ~allowed_flags) { *error = __DRI_CTX_ERROR_UNKNOWN_FLAG; goto fail; } if (!screen->has_reset_status_query && notify_reset) { @@ -151,24 +153,35 @@ dri_create_context(gl_api api, const struct gl_config * visual, ctx->st->st_manager_private = (void *) ctx; ctx->stapi = stapi; if (ctx->st->cso_context) { ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled, ctx->st->cso_context); ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context); } /* Do this last. */ if (ctx->st->start_thread && - /* the driver loader must implement this */ - screen->sPriv->dri2.backgroundCallable && - driQueryOptionb(&screen->optionCache, "mesa_glthread")) - ctx->st->start_thread(ctx->st); + driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (backgroundCallable && backgroundCallable->base.version >= 2 && +backgroundCallable->isThreadSafe) { + + if (backgroundCallable->isThreadSafe(cPriv->loaderPrivate)) +ctx->st->start_thread(ctx->st); + else +fprintf(stderr, "dri_create_context: glthread isn't thread safe " + "- missing call XInitThreads\n"); + } else { + fprintf(stderr, "dri_create_context: requested glthread but driver " + "is missing backgroundCallable V2 extension\n"); + } + } *error = __DRI_CTX_ERROR_SUCCESS; return GL_TRUE; fail: if (ctx && ctx->st) ctx->st->destroy(ctx->st); free(ctx); return GL_FALSE; -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 3/4] egl: implement __DRIbackgroundCallableExtension.isThreadSafe
v2: bump version v3: Add code comment s/IsGlThread/IsThread/ (and variation) Include X11/Xlibint.h protected by ifdef v5: based on Daniel feedback Move non X11 code outside of X11 define Always return true for Wayland Signed-off-by: Gregory Hainaut --- src/egl/drivers/dri2/egl_dri2.c | 34 +- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 0be7132ac5..8891771e3f 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -48,20 +48,24 @@ #include #include "GL/mesa_glinterop.h" #include #include #ifdef HAVE_WAYLAND_PLATFORM #include "wayland-drm.h" #include "wayland-drm-client-protocol.h" #endif +#ifdef HAVE_X11_PLATFORM +#include "X11/Xlibint.h" +#endif + #include "egl_dri2.h" #include "loader/loader.h" #include "util/u_atomic.h" /* The kernel header drm_fourcc.h defines the DRM formats below. We duplicate * some of the definitions here so that building Mesa won't bleeding-edge * kernel headers. */ #ifndef DRM_FORMAT_R8 #define DRM_FORMAT_R8fourcc_code('R', '8', ' ', ' ') /* [7:0] R */ @@ -85,24 +89,52 @@ static void dri_set_background_context(void *loaderPrivate) { _EGLContext *ctx = _eglGetCurrentContext(); _EGLThreadInfo *t = _eglGetCurrentThread(); _eglBindContextToThread(ctx, t); } +static GLboolean +dri_is_thread_safe(void *loaderPrivate) +{ + struct dri2_egl_surface *dri2_surf = loaderPrivate; + _EGLDisplay *display = dri2_surf->base.Resource.Display; + +#ifdef HAVE_X11_PLATFORM + Display *xdpy = (Display*)display->PlatformDisplay; + + /* Check Xlib is running in thread safe mode when running on EGL/X11-xlib +* platform +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + if (display->Platform == _EGL_PLATFORM_X11 && xdpy && !xdpy->lock_fns) + return false; +#endif + +#ifdef HAVE_WAYLAND_PLATFORM + if (display->Platform == _EGL_PLATFORM_WAYLAND) + return true; +#endif + + return true; +} + const __DRIbackgroundCallableExtension background_callable_extension = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isThreadSafe = dri_is_thread_safe, }; const __DRIuseInvalidateExtension use_invalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const EGLint dri2_to_egl_attribute_map[__DRI_ATTRIB_MAX] = { [__DRI_ATTRIB_BUFFER_SIZE ] = EGL_BUFFER_SIZE, [__DRI_ATTRIB_LEVEL] = EGL_LEVEL, [__DRI_ATTRIB_RED_SIZE] = EGL_RED_SIZE, -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 2/4] glx: implement __DRIbackgroundCallableExtension.isThreadSafe
v2: bump version v3: Add code comment s/IsGlThread/IsThread/ (and variation) v4: DRI3 doesn't hit X through GL call so it is always safe Signed-off-by: Gregory Hainaut --- src/glx/dri2_glx.c | 15 ++- src/glx/dri3_glx.c | 12 +++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index 145f44d6e8..4f163688f2 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -946,20 +946,32 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw) return priv->swap_interval; } static void driSetBackgroundContext(void *loaderPrivate) { struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +driIsThreadSafe(void *loaderPrivate) +{ + struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; + /* Check Xlib is running in thread safe mode +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + return pcp->base.psc->dpy->lock_fns != NULL; +} + static const __DRIdri2LoaderExtension dri2LoaderExtension = { .base = { __DRI_DRI2_LOADER, 3 }, .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= dri2GetBuffersWithFormat, }; static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .base = { __DRI_DRI2_LOADER, 3 }, @@ -967,23 +979,24 @@ static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= NULL, }; static const __DRIuseInvalidateExtension dri2UseInvalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const __DRIbackgroundCallableExtension driBackgroundCallable = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext= driSetBackgroundContext, + .isThreadSafe= driIsThreadSafe, }; _X_HIDDEN void dri2InvalidateBuffers(Display *dpy, XID drawable) { __GLXDRIdrawable *pdraw = dri2GetGlxDrawableFromXDrawableId(dpy, drawable); struct dri2_screen *psc; struct dri2_drawable *pdp = (struct dri2_drawable *) pdraw; diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c index e1dc5aa4a8..d07968e3c5 100644 --- a/src/glx/dri3_glx.c +++ b/src/glx/dri3_glx.c @@ -496,37 +496,47 @@ dri3_flush_front_buffer(__DRIdrawable *driDrawable, void *loaderPrivate) loader_dri3_wait_gl(draw); } static void dri_set_background_context(void *loaderPrivate) { struct dri3_context *pcp = (struct dri3_context *)loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +dri_is_thread_safe(void *loaderPrivate) +{ + /* Unlike DRI2, DRI3 doesn't call GetBuffers/GetBuffersWithFormat +* during draw so we're safe here. +*/ + return true; +} + /* The image loader extension record for DRI3 */ static const __DRIimageLoaderExtension imageLoaderExtension = { .base = { __DRI_IMAGE_LOADER, 1 }, .getBuffers = loader_dri3_get_buffers, .flushFrontBuffer= dri3_flush_front_buffer, }; const __DRIuseInvalidateExtension dri3UseInvalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const __DRIbackgroundCallableExtension driBackgroundCallable = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isThreadSafe = dri_is_thread_safe, }; static const __DRIextension *loader_extensions[] = { &imageLoaderExtension.base, &dri3UseInvalidate.base, &driBackgroundCallable.base, NULL }; /** dri3_swap_buffers -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 1/4] dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety
DRI-drivers could call Xlib functions, for example to allocate a new back buffer. When glthread is enabled, the driver runs mostly on a separate thread. Therefore we need to guarantee the thread safety between libX11 calls from the applications (not aware of the extra thread) and the ones from the driver. See discussion thread: https://lists.freedesktop.org/archives/mesa-dev/2017-April/152547.html Fortunately, Xlib allows to lock display to ensure thread safety but XInitThreads must be called first by the application to initialize the lock function pointer. This patch will allow to check XInitThreads was called to allow glthread on GLX or EGL platform. Note: a tentative was done to port libX11 code to XCB but it didn't solve fully thread safety. See discussion thread: https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html Note: Nvidia forces the driver to call XInitThreads. Quoting their manpage: "The NVIDIA OpenGL driver will automatically attempt to enable Xlib thread-safe mode if needed. However, it might not be possible in some situations, such as when the NVIDIA OpenGL driver library is dynamically loaded after Xlib has been loaded and initialized. If that is the case, threaded optimizations will stay disabled unless the application is modified to call XInitThreads() before initializing Xlib or to link directly against the NVIDIA OpenGL driver library. Alternatively, using the LD_PRELOAD environment variable to include the NVIDIA OpenGL driver library should also achieve the desired result." v2: based on Nicolai and Matt feedback Use C style comment v3: based on Emil feedback split the patch in 3 s/isGlThreadSafe/isThreadSafe/ v5: based on Marek comment Add a comment that isThreadSafe is supported by extension v2 Signed-off-by: Gregory Hainaut --- include/GL/internal/dri_interface.h | 13 + 1 file changed, 13 insertions(+) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index c83056aa70..ffe99499fc 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1714,13 +1714,26 @@ struct __DRIbackgroundCallableExtensionRec { * non-background thread (i.e. a thread that has already been bound to a * context using __DRIcoreExtensionRec::bindContext()); when this happens, * the \c loaderPrivate pointer must be equal to the pointer that was * passed to the driver when the currently bound context was created. * * This call should execute quickly enough that the driver can call it with * impunity whenever a background thread starts performing drawing * operations (e.g. it should just set a thread-local variable). */ void (*setBackgroundContext)(void *loaderPrivate); + + /** +* Indicate that it is multithread safe to use glthread. For GLX/EGL +* platforms using Xlib, that involves calling XInitThreads, before +* opening an X display. +* +* Note: only supported if extension version is at least 2. +* +* \param loaderPrivate is the value that was passed to to the driver when +* the context was created. This can be used by the loader to identify +* which context any callbacks are associated with. +*/ + GLboolean (*isThreadSafe)(void *loaderPrivate); }; #endif -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 1/4] dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety
DRI-drivers could call Xlib functions, for example to allocate a new back buffer. When glthread is enabled, the driver runs mostly on a separate thread. Therefore we need to guarantee the thread safety between libX11 calls from the applications (not aware of the extra thread) and the ones from the driver. See discussion thread: https://lists.freedesktop.org/archives/mesa-dev/2017-April/152547.html Fortunately, Xlib allows to lock display to ensure thread safety but XInitThreads must be called first by the application to initialize the lock function pointer. This patch will allow to check XInitThreads was called to allow glthread on GLX or EGL platform. Note: a tentative was done to port libX11 code to XCB but it didn't solve fully thread safety. See discussion thread: https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html Note: Nvidia forces the driver to call XInitThreads. Quoting their manpage: "The NVIDIA OpenGL driver will automatically attempt to enable Xlib thread-safe mode if needed. However, it might not be possible in some situations, such as when the NVIDIA OpenGL driver library is dynamically loaded after Xlib has been loaded and initialized. If that is the case, threaded optimizations will stay disabled unless the application is modified to call XInitThreads() before initializing Xlib or to link directly against the NVIDIA OpenGL driver library. Alternatively, using the LD_PRELOAD environment variable to include the NVIDIA OpenGL driver library should also achieve the desired result." v2: based on Nicolai and Matt feedback Use C style comment v3: based on Emil feedback split the patch in 3 s/isGlThreadSafe/isThreadSafe/ Signed-off-by: Gregory Hainaut --- include/GL/internal/dri_interface.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index c83056aa70..8381f704e4 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1714,13 +1714,24 @@ struct __DRIbackgroundCallableExtensionRec { * non-background thread (i.e. a thread that has already been bound to a * context using __DRIcoreExtensionRec::bindContext()); when this happens, * the \c loaderPrivate pointer must be equal to the pointer that was * passed to the driver when the currently bound context was created. * * This call should execute quickly enough that the driver can call it with * impunity whenever a background thread starts performing drawing * operations (e.g. it should just set a thread-local variable). */ void (*setBackgroundContext)(void *loaderPrivate); + + /** +* Indicate that it is multithread safe to use glthread. For GLX/EGL +* platforms using Xlib, that involves calling XInitThreads, before +* opening an X display. +* +* \param loaderPrivate is the value that was passed to to the driver when +* the context was created. This can be used by the loader to identify +* which context any callbacks are associated with. +*/ + GLboolean (*isThreadSafe)(void *loaderPrivate); }; #endif -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 4/4] glthread/gallium: require safe_glthread to start glthread
Print an error message for the user if the requirement isn't met, or we're not thread safe. v2: based on Nicolai feedbacks Check the DRI extension version v3: based on Emil feedbacks improve commit and error messages. use backgroundCallable variable to improve readability Signed-off-by: Gregory Hainaut --- src/gallium/state_trackers/dri/dri_context.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/gallium/state_trackers/dri/dri_context.c b/src/gallium/state_trackers/dri/dri_context.c index 92d79849c4..8cbab5359f 100644 --- a/src/gallium/state_trackers/dri/dri_context.c +++ b/src/gallium/state_trackers/dri/dri_context.c @@ -51,20 +51,22 @@ dri_create_context(gl_api api, const struct gl_config * visual, { __DRIscreen *sPriv = cPriv->driScreenPriv; struct dri_screen *screen = dri_screen(sPriv); struct st_api *stapi = screen->st_api; struct dri_context *ctx = NULL; struct st_context_iface *st_share = NULL; struct st_context_attribs attribs; enum st_context_error ctx_err = 0; unsigned allowed_flags = __DRI_CTX_FLAG_DEBUG | __DRI_CTX_FLAG_FORWARD_COMPATIBLE; + const __DRIbackgroundCallableExtension *backgroundCallable = + screen->sPriv->dri2.backgroundCallable; if (screen->has_reset_status_query) allowed_flags |= __DRI_CTX_FLAG_ROBUST_BUFFER_ACCESS; if (flags & ~allowed_flags) { *error = __DRI_CTX_ERROR_UNKNOWN_FLAG; goto fail; } if (!screen->has_reset_status_query && notify_reset) { @@ -151,24 +153,35 @@ dri_create_context(gl_api api, const struct gl_config * visual, ctx->st->st_manager_private = (void *) ctx; ctx->stapi = stapi; if (ctx->st->cso_context) { ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled, ctx->st->cso_context); ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context); } /* Do this last. */ if (ctx->st->start_thread && - /* the driver loader must implement this */ - screen->sPriv->dri2.backgroundCallable && - driQueryOptionb(&screen->optionCache, "mesa_glthread")) - ctx->st->start_thread(ctx->st); + driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (backgroundCallable && backgroundCallable->base.version >= 2 && +driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (backgroundCallable->isThreadSafe(cPriv->loaderPrivate)) +ctx->st->start_thread(ctx->st); + else +fprintf(stderr, "dri_create_context: glthread isn't thread safe " + "- missing call XInitThreads\n"); + } else { + fprintf(stderr, "dri_create_context: requested glthread but driver " + "is missing backgroundCallable V2 extension\n"); + } + } *error = __DRI_CTX_ERROR_SUCCESS; return GL_TRUE; fail: if (ctx && ctx->st) ctx->st->destroy(ctx->st); free(ctx); return GL_FALSE; -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 2/4] glx: implement __DRIbackgroundCallableExtension.isThreadSafe
v2: bump version v3: Add code comment s/IsGlThread/IsThread/ (and variation) v4: DRI3 doesn't hit X through GL call so it is always safe Signed-off-by: Gregory Hainaut --- src/glx/dri2_glx.c | 15 ++- src/glx/dri3_glx.c | 12 +++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index 145f44d6e8..4f163688f2 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -946,20 +946,32 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw) return priv->swap_interval; } static void driSetBackgroundContext(void *loaderPrivate) { struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +driIsThreadSafe(void *loaderPrivate) +{ + struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; + /* Check Xlib is running in thread safe mode +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + return pcp->base.psc->dpy->lock_fns != NULL; +} + static const __DRIdri2LoaderExtension dri2LoaderExtension = { .base = { __DRI_DRI2_LOADER, 3 }, .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= dri2GetBuffersWithFormat, }; static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .base = { __DRI_DRI2_LOADER, 3 }, @@ -967,23 +979,24 @@ static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= NULL, }; static const __DRIuseInvalidateExtension dri2UseInvalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const __DRIbackgroundCallableExtension driBackgroundCallable = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext= driSetBackgroundContext, + .isThreadSafe= driIsThreadSafe, }; _X_HIDDEN void dri2InvalidateBuffers(Display *dpy, XID drawable) { __GLXDRIdrawable *pdraw = dri2GetGlxDrawableFromXDrawableId(dpy, drawable); struct dri2_screen *psc; struct dri2_drawable *pdp = (struct dri2_drawable *) pdraw; diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c index e1dc5aa4a8..d07968e3c5 100644 --- a/src/glx/dri3_glx.c +++ b/src/glx/dri3_glx.c @@ -496,37 +496,47 @@ dri3_flush_front_buffer(__DRIdrawable *driDrawable, void *loaderPrivate) loader_dri3_wait_gl(draw); } static void dri_set_background_context(void *loaderPrivate) { struct dri3_context *pcp = (struct dri3_context *)loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +dri_is_thread_safe(void *loaderPrivate) +{ + /* Unlike DRI2, DRI3 doesn't call GetBuffers/GetBuffersWithFormat +* during draw so we're safe here. +*/ + return true; +} + /* The image loader extension record for DRI3 */ static const __DRIimageLoaderExtension imageLoaderExtension = { .base = { __DRI_IMAGE_LOADER, 1 }, .getBuffers = loader_dri3_get_buffers, .flushFrontBuffer= dri3_flush_front_buffer, }; const __DRIuseInvalidateExtension dri3UseInvalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const __DRIbackgroundCallableExtension driBackgroundCallable = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isThreadSafe = dri_is_thread_safe, }; static const __DRIextension *loader_extensions[] = { &imageLoaderExtension.base, &dri3UseInvalidate.base, &driBackgroundCallable.base, NULL }; /** dri3_swap_buffers -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v6 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers to be valid. v4: update comments based on Nicolai review Signed-off-by: Gregory Hainaut --- src/mesa/main/marshal.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 4037b793ee..1167271607 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -312,21 +312,34 @@ _mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, /* BindBufferBase: marshalled asynchronously */ struct marshal_cmd_BindBufferBase { struct marshal_cmd_base cmd_base; GLenum target; GLuint index; GLuint buffer; }; -/** Tracks the current bindings for the vertex array and index array buffers. +/** + * Check that buffer is a valid buffer handle + * Always return false for ID 0. + */ +static bool +is_bufferobj(struct gl_context *ctx, GLuint buffer) +{ + if (buffer == 0) + return false; + else + return _mesa_HashLookup(ctx->Shared->ShadowBufferObjects, buffer) != NULL; +} + +/** Tracks the current bindings of GL buffer targets * * This is part of what we need to enable glthread on compat-GL contexts that * happen to use VBOs, without also supporting the full tracking of VBO vs * user vertex array bindings per attribute on each vertex array for * determining what to upload at draw call time. * * Note that GL core makes it so that a buffer binding with an invalid handle * in the "buffer" parameter will throw an error, and then a * glVertexAttribPointer() that followsmight not end up pointing at a VBO. * However, in GL core the draw call would throw an error as well, so we don't @@ -334,37 +347,54 @@ struct marshal_cmd_BindBufferBase * marshal user data for draw calls, and the unmarshal will just generate an * error or not as appropriate. * * For compatibility GL, we do need to accurately know whether the draw call * on the unmarshal side will dereference a user pointer or load data from a * VBO per vertex. That would make it seem like we need to track whether a * "buffer" is valid, so that we can know when an error will be generated * instead of updating the binding. However, compat GL has the ridiculous * feature that if you pass a bad name, it just gens a buffer object for you, * so we escape without having to know if things are valid or not. + * + * Pixel buffers are tracked to decide whether pixel transfer goes to a user + * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike + * for VBOs, we do need accurate tracking, since user pointers can be used in + * GL core contexts. */ static void -track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) +track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer) { struct glthread_state *glthread = ctx->GLThread; switch (target) { case GL_ARRAY_BUFFER: glthread->vertex_array_is_vbo = (buffer != 0); break; case GL_ELEMENT_ARRAY_BUFFER: /* The current element array buffer binding is actually tracked in the * vertex array object instead of the context, so this would need to * change on vertex array object updates. */ glthread->element_array_is_vbo = (buffer != 0); break; + case GL_PIXEL_UNPACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_unpack_buffer_bound = buffer; + else + glthread->pixel_unpack_buffer_bound = 0; + break; + case GL_PIXEL_PACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_pack_buffer_bound = buffer; + else + glthread->pixel_pack_buffer_bound = 0; + break; } } struct marshal_cmd_BindBuffer { struct marshal_cmd_base cmd_base; GLenum target; GLuint buffer; }; @@ -382,21 +412,21 @@ _mesa_unmarshal_BindBuffer(struct gl_context *ctx, CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer)); } void GLAPIENTRY _mesa_marshal_BindBuffer(GLenum target, GLuint buffer) { GET_CURRENT_CONTEXT(ctx); size_t cmd_size = sizeof(struct marshal_cmd_BindBuffer); struct marshal_cmd_BindBuffer *cmd; debug_print_marshal("BindBuffer"); - track_vbo_binding(ctx, target, buffer); + track_buffers_binding(ctx, target, buffer); if (cmd_size <= MARSHAL_MAX_CMD_SIZE) { cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer, cmd_size); cmd->target = target; cmd->buffer = buffer; _mesa_post_marshal_hook(ctx); } else { _mesa_glthread_finish(ctx); CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer)); -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 0/4] Disable glthread if libX11 isn't thread-safe
Hello Mesa developers, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. v2: based on Nicolai/Matt reviews Add a check on DRI extension version Use C comments :) v3: based on Emil reviews Split the initial first patch into 3 sub patches dri extension / glx / egl Improve error message Improve code readability Just include libX11 on EGL protected by ifdef v4: based on Eric feedback, I marked DRI3 as always thread safe Thanks you for all the review comments. Best regards, Gregory Hainaut (4): dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety glx: implement __DRIbackgroundCallableExtension.isThreadSafe egl: implement __DRIbackgroundCallableExtension.isThreadSafe glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 11 +++ src/egl/drivers/dri2/egl_dri2.c | 28 +++- src/gallium/state_trackers/dri/dri_context.c | 21 + src/glx/dri2_glx.c | 15 ++- src/glx/dri3_glx.c | 12 +++- 5 files changed, 80 insertions(+), 7 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v6 0/3] asynchronous pbo transfer with glthread
Hello Mesa developers, Please find a new version to handle invalid buffer handles. Allow to handle this kind of case: genBuffer(&pbo); BindBuffer(pbo) DeleteBuffer(pbo); BindBuffer(rand_pbo) TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous There are various subtely to handle multi threaded shared context. In order to keep the code sane, I've considered a buffer invalid when it is deleted by a context even it is still bound to others contexts. It will force a synchronous transfer which is always safe. An example could be Ctx A: glGenBuffers(1, &pbo); Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); Ctx B: glDeleteBuffers(1, &pbo); Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ be asynchronous (because the PBO that was generated first is still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback V5: Properly delete element of the new hash (first patch) v6: Rebase on latest master Note: crash related to unsafe X call will be handled by "Disable glthread if libX11 isn't thread-safe" series Best regards, Gregory Hainaut (3): mesa/glthread: track buffer creation/destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 +- src/mapi/glapi/gen/gl_API.xml | 32 +++--- src/mapi/glapi/gen/gl_marshal.py | 23 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +++- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c| 149 - src/mesa/main/marshal.h| 24 src/mesa/main/mtypes.h | 5 + src/mesa/main/shared.c | 14 +++ 11 files changed, 269 insertions(+), 39 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v6 1/3] mesa/glthread: track buffer creation/destruction
It would be used in next commit to allow asynchronous PBO transfer. The tracking saves the buffer name into a hash. Saving pointer will be more complex as the buffer is created in BindBuffer due to IsBuffer insanity. Perf wise DeleteBuffers is now synchronous for robustness. v5: properly delete hash element with the help of _mesa_HashDeleteAll v6: rebase Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 2 +- src/mapi/glapi/gen/gl_API.xml | 4 +- src/mesa/main/glthread.h | 10 +++ src/mesa/main/marshal.c| 113 + src/mesa/main/marshal.h| 24 ++ src/mesa/main/mtypes.h | 5 ++ src/mesa/main/shared.c | 14 +++ 7 files changed, 169 insertions(+), 3 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index b8780f75b3..8761920c91 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -42,21 +42,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index 762fb5a676..829f99b0f0 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -5053,27 +5053,27 @@ - + - + diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h index 50c1db2548..494e94270a 100644 --- a/src/mesa/main/glthread.h +++ b/src/mesa/main/glthread.h @@ -85,20 +85,30 @@ struct glthread_state * Tracks on the main thread side whether the current vertex array binding * is in a VBO. */ bool vertex_array_is_vbo; /** * Tracks on the main thread side whether the current element array (index * buffer) binding is in a VBO. */ bool element_array_is_vbo; + + /** +* Tracks on the main thread side the bound unpack pixel buffer +*/ + GLint pixel_unpack_buffer_bound; + + /** +* Tracks on the main thread side the bound pack pixel buffer +*/ + GLint pixel_pack_buffer_bound; }; /** * A single batch of commands queued up for later execution by a thread pool * task. */ struct glthread_batch { /** * Next batch of commands to execute after this batch, or NULL if this is diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index ae4efb5ecb..4037b793ee 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -25,20 +25,21 @@ * * Custom functions for marshalling GL calls from the main thread to a worker * thread when automatic code generation isn't appropriate. */ #include "main/enums.h" #include "main/macros.h" #include "marshal.h" #include "dispatch.h" #include "marshal_generated.h" +#include "hash.h" #ifdef HAVE_PTHREAD struct marshal_cmd_Flush { struct marshal_cmd_base cmd_base; }; void @@ -189,20 +190,132 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, } _mesa_post_marshal_hook(ctx); } else { _mesa_glthread_finish(ctx); CALL_ShaderSource(ctx->CurrentServerDispatch, (shader, count, string, length_tmp)); } free(length_tmp); } +/** + * Used as a placeholder for track_buffers_creation/track_buffers_destruction + * so we know if buffer really exists in track_buffers_binding. + */ +static struct gl_buffer_object DummyBufferObject; + +static void track_buffers_creation(GLsizei n, GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + _mesa_HashInsertLocked(ctx->Shared->ShadowBufferObjects, buffers[i], +&DummyBufferObject); + } + + _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects); +} + +static void track_buffers_destruction(GLsizei n, const GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + struct glthread_state *glthread = ctx->GLThread; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + if (buffers[i] == glthread->pixel_pack_buffer_bound) + glthread->pixel_pack_buffer_bound = 0; + + if (buffers[i] == glthread->pixel_unpack_buffer_bound) + glthread->pixel_unpack_buffer_bound = 0; + + /* Technically the buffer can still exist if it is bound to another + * context. It isn't important as nex
[Mesa-dev] [PATCH v6 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 23 - src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 33 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 8761920c91..6ae1a6c249 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -367,79 +367,79 @@ - + - + - + - + - + - + @@ -516,30 +516,30 @@ - + - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0a74..6e1ac09ce0 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -75,21 +75,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250777..447b03a41d 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -115,28 +115,30 @@ param: img_send_null - boolean flag to determine if blank pixel data should be sent when a NULL pointer is passed. This is only used by TexImage1D and TexImage2D. img_null_flag - boolean flag to determine if an extra flag is used to determine if a NULL pixel pointer was passed. This is used by TexSubImage1D, TexSubImage2D, TexImage3D and others. img_pad_dimensions - boolean flag to determine if dimension data and offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't want to track state for. glx: rop - Opcode value for "render" commands sop - Opcode value for "single" commands vendorpriv -
[Mesa-dev] [PATCH v4 3/4] egl: implement __DRIbackgroundCallableExtension.isThreadSafe
v2: bump version v3: Add code comment s/IsGlThread/IsThread/ (and variation) Include X11/Xlibint.h protected by ifdef Signed-off-by: Gregory Hainaut --- src/egl/drivers/dri2/egl_dri2.c | 28 +++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 0be7132ac5..3295d472f4 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -48,20 +48,24 @@ #include #include "GL/mesa_glinterop.h" #include #include #ifdef HAVE_WAYLAND_PLATFORM #include "wayland-drm.h" #include "wayland-drm-client-protocol.h" #endif +#ifdef HAVE_X11_PLATFORM +#include "X11/Xlibint.h" +#endif + #include "egl_dri2.h" #include "loader/loader.h" #include "util/u_atomic.h" /* The kernel header drm_fourcc.h defines the DRM formats below. We duplicate * some of the definitions here so that building Mesa won't bleeding-edge * kernel headers. */ #ifndef DRM_FORMAT_R8 #define DRM_FORMAT_R8fourcc_code('R', '8', ' ', ' ') /* [7:0] R */ @@ -85,24 +89,46 @@ static void dri_set_background_context(void *loaderPrivate) { _EGLContext *ctx = _eglGetCurrentContext(); _EGLThreadInfo *t = _eglGetCurrentThread(); _eglBindContextToThread(ctx, t); } +static GLboolean +dri_is_thread_safe(void *loaderPrivate) +{ +#ifdef HAVE_X11_PLATFORM + struct dri2_egl_surface *dri2_surf = loaderPrivate; + _EGLDisplay *display = dri2_surf->base.Resource.Display; + Display *xdpy = (Display*)display->PlatformDisplay; + + /* Check Xlib is running in thread safe mode when running on EGL/X11-xlib +* platform +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + if (display->Platform == _EGL_PLATFORM_X11 && xdpy && !xdpy->lock_fns) + return false; +#endif + + return true; +} + const __DRIbackgroundCallableExtension background_callable_extension = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isThreadSafe = dri_is_thread_safe, }; const __DRIuseInvalidateExtension use_invalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const EGLint dri2_to_egl_attribute_map[__DRI_ATTRIB_MAX] = { [__DRI_ATTRIB_BUFFER_SIZE ] = EGL_BUFFER_SIZE, [__DRI_ATTRIB_LEVEL] = EGL_LEVEL, [__DRI_ATTRIB_RED_SIZE] = EGL_RED_SIZE, -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe
On Tue, 09 May 2017 09:31:18 -0700 Eric Anholt wrote: > Gregory Hainaut writes: > > > On 5/8/17, Emil Velikov wrote: > [...] > > > > Hello Emil, > > > > Yes you're right. And potentially it can be back-ported to stable > > branch. Besides it would allow to know which applications aren't X > > thread safe. And potentially app owners can fix the issue too. > > > > By the way, I don't have commit access so feel free to push the series :) > > > > On 5/8/17, Eric Anholt wrote: > [...] > [...] > [...] > [...] > [...] > > > > Hello Eric, > > > > Indeed I saw this behavior on Nouveau. I got a crash on > > GetBuffersWithFormat on DRI2 but it was fine on DRI3. I still kept the > > check on GLX/DRI3 because I don't know if we have a strong guarantee > > that X11 is never used. > > I would be surprised if there was a path that hit X through GL calls as > opposed to the window system, with DRI3. GetBuffersWithFormat was the > problem. Tbh, I'm a noob. If you're sure, we can always return true for GLX/DRI3. Otherwise I will put a comment that it is likely fine. Your call. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe
On 5/8/17, Emil Velikov wrote: > Having a look at Xlib might be good indeed. > > Then again, the solution you've proposed looks perfectly reasonable, > IMHO. It handles the problem _now_ and should also work when/if we > address Xlib. > I'll take another look today/tomorrow, but I think the series is > perfectly fine to land as-is. > > Thanks > Emil Hello Emil, Yes you're right. And potentially it can be back-ported to stable branch. Besides it would allow to know which applications aren't X thread safe. And potentially app owners can fix the issue too. By the way, I don't have commit access so feel free to push the series :) On 5/8/17, Eric Anholt wrote: > gregory hainaut writes: > >> On Fri, 5 May 2017 17:45:01 +0200 >> Axel Davy wrote: >> >>> Hi, >>> >>> There should be very few X11 calls while rendering (basically only at >>> the beginning or end of a frame). >>> >>> Why not just always run these calls in the main thread (and wait for >>> glthread work to finish) ? >>> >>> That's basically what we do for gallium nine. >>> >>> Yours, >>> >>> Axel >> >> Hello Axel, >> >> Yes it is another possibility. It would requires to track gl calls that >> end up in X11. >> I'm not sure if there is an easy way to list all those gl functions. There >> are at least the >> draw calls and maybe the clear operations. Besides I'm afraid that we will >> need to handle >> various corner cases of the OpenGL API. It is doable but likely more >> complicated. > > General GL calls (draws, clears) won't call X11 except for DRI2's > GetBuffers. If you're in DRI3, I believe you won't need to worry about > that at all. > > I think this patch is a good start, though. > Hello Eric, Indeed I saw this behavior on Nouveau. I got a crash on GetBuffersWithFormat on DRI2 but it was fine on DRI3. I still kept the check on GLX/DRI3 because I don't know if we have a strong guarantee that X11 is never used. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe
On Fri, 5 May 2017 18:17:22 +0100 Emil Velikov wrote: > On 5 May 2017 at 17:58, gregory hainaut wrote: > > On Fri, 5 May 2017 17:45:01 +0200 > > Axel Davy wrote: > > > [...] > > > > Hello Axel, > > > > Yes it is another possibility. It would requires to track gl calls that end > > up in X11. > > I'm not sure if there is an easy way to list all those gl functions. There > > are at least the > > draw calls and maybe the clear operations. Besides I'm afraid that we will > > need to handle > > various corner cases of the OpenGL API. It is doable but likely more > > complicated. > > > > There is also the Nvidia way, i.e. forces the driver to XInitThreads X11. I > > think it > > can be implemented with the help of constructor attribute. > > > Did you trace the above behaviour? What would happen in the following > scenario: > - There is no link against libGL/libEGL > - User gets the dpy primitive w/o calling XInitThreads > - Then user dlopens libGL/libEGL, which in itself calls XInitThreads > At that point it's a bit too late isn't it? > > -Emil Hello, No I didn't trace it. You right, it is too late. You can/must use LD_PRELOAD to ensure that libGL/libEGL is loaded first. To be honest, the best solution will be to have a thread safe Xlib. No more hack everywhere. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe
On Fri, 5 May 2017 17:45:01 +0200 Axel Davy wrote: > Hi, > > There should be very few X11 calls while rendering (basically only at > the beginning or end of a frame). > > Why not just always run these calls in the main thread (and wait for > glthread work to finish) ? > > That's basically what we do for gallium nine. > > Yours, > > Axel Hello Axel, Yes it is another possibility. It would requires to track gl calls that end up in X11. I'm not sure if there is an easy way to list all those gl functions. There are at least the draw calls and maybe the clear operations. Besides I'm afraid that we will need to handle various corner cases of the OpenGL API. It is doable but likely more complicated. There is also the Nvidia way, i.e. forces the driver to XInitThreads X11. I think it can be implemented with the help of constructor attribute. Cheers, Gregory > On 05/05/2017 17:37, Gregory Hainaut wrote: > > Hello Mesa developers, > > > > Following the discussion from > > https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html > > > > A check was added to ensure that X11 display can be locked. It should be > > enough > > to ensure thread safety between X11 and glthread. > > > > I also did the check on DRI3 as I'm not 100% sure that it is really thread > > safe. > > > > > > > > v2: based on Nicolai/Matt reviews > > Add a check on DRI extension version > > Use C comments :) > > > > v3: based on Emil reviews > > Split the initial first patch into 3 sub patches dri extension / glx / egl > > Improve error message > > Improve code readability > > Just include libX11 on EGL protected by ifdef > > > > Thanks you for all the review comments. > > > > Best regards, > > > > Gregory Hainaut (4): > >dri: Extend __DRIbackgroundCallableExtensionRec to include a callback > > that checks for thread safety > >glx: implement __DRIbackgroundCallableExtension.isThreadSafe > >egl: implement __DRIbackgroundCallableExtension.isThreadSafe > >glthread/gallium: require safe_glthread to start glthread > > > > include/GL/internal/dri_interface.h | 11 +++ > > src/egl/drivers/dri2/egl_dri2.c | 28 > > +++- > > src/gallium/state_trackers/dri/dri_context.c | 21 + > > src/glx/dri2_glx.c | 15 ++- > > src/glx/dri3_glx.c | 15 ++- > > 5 files changed, 83 insertions(+), 7 deletions(-) > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3 2/4] glx: implement __DRIbackgroundCallableExtension.isThreadSafe
v2: bump version v3: Add code comment s/IsGlThread/IsThread/ (and variation) Signed-off-by: Gregory Hainaut --- src/glx/dri2_glx.c | 15 ++- src/glx/dri3_glx.c | 15 ++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index 145f44d6e8..4f163688f2 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -946,20 +946,32 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw) return priv->swap_interval; } static void driSetBackgroundContext(void *loaderPrivate) { struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +driIsThreadSafe(void *loaderPrivate) +{ + struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; + /* Check Xlib is running in thread safe mode +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + return pcp->base.psc->dpy->lock_fns != NULL; +} + static const __DRIdri2LoaderExtension dri2LoaderExtension = { .base = { __DRI_DRI2_LOADER, 3 }, .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= dri2GetBuffersWithFormat, }; static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .base = { __DRI_DRI2_LOADER, 3 }, @@ -967,23 +979,24 @@ static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= NULL, }; static const __DRIuseInvalidateExtension dri2UseInvalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const __DRIbackgroundCallableExtension driBackgroundCallable = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext= driSetBackgroundContext, + .isThreadSafe= driIsThreadSafe, }; _X_HIDDEN void dri2InvalidateBuffers(Display *dpy, XID drawable) { __GLXDRIdrawable *pdraw = dri2GetGlxDrawableFromXDrawableId(dpy, drawable); struct dri2_screen *psc; struct dri2_drawable *pdp = (struct dri2_drawable *) pdraw; diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c index e1dc5aa4a8..3b317fdf1b 100644 --- a/src/glx/dri3_glx.c +++ b/src/glx/dri3_glx.c @@ -496,37 +496,50 @@ dri3_flush_front_buffer(__DRIdrawable *driDrawable, void *loaderPrivate) loader_dri3_wait_gl(draw); } static void dri_set_background_context(void *loaderPrivate) { struct dri3_context *pcp = (struct dri3_context *)loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +dri_is_thread_safe(void *loaderPrivate) +{ + struct dri3_context *pcp = (struct dri3_context *) loaderPrivate; + /* Check Xlib is running in thread safe mode +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + return pcp->base.psc->dpy->lock_fns != NULL; +} + /* The image loader extension record for DRI3 */ static const __DRIimageLoaderExtension imageLoaderExtension = { .base = { __DRI_IMAGE_LOADER, 1 }, .getBuffers = loader_dri3_get_buffers, .flushFrontBuffer= dri3_flush_front_buffer, }; const __DRIuseInvalidateExtension dri3UseInvalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const __DRIbackgroundCallableExtension driBackgroundCallable = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isThreadSafe = dri_is_thread_safe, }; static const __DRIextension *loader_extensions[] = { &imageLoaderExtension.base, &dri3UseInvalidate.base, &driBackgroundCallable.base, NULL }; /** dri3_swap_buffers -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3 3/4] egl: implement __DRIbackgroundCallableExtension.isThreadSafe
v2: bump version v3: Add code comment s/IsGlThread/IsThread/ (and variation) Include X11/Xlibint.h protected by ifdef Signed-off-by: Gregory Hainaut --- src/egl/drivers/dri2/egl_dri2.c | 28 +++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 0be7132ac5..3295d472f4 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -48,20 +48,24 @@ #include #include "GL/mesa_glinterop.h" #include #include #ifdef HAVE_WAYLAND_PLATFORM #include "wayland-drm.h" #include "wayland-drm-client-protocol.h" #endif +#ifdef HAVE_X11_PLATFORM +#include "X11/Xlibint.h" +#endif + #include "egl_dri2.h" #include "loader/loader.h" #include "util/u_atomic.h" /* The kernel header drm_fourcc.h defines the DRM formats below. We duplicate * some of the definitions here so that building Mesa won't bleeding-edge * kernel headers. */ #ifndef DRM_FORMAT_R8 #define DRM_FORMAT_R8fourcc_code('R', '8', ' ', ' ') /* [7:0] R */ @@ -85,24 +89,46 @@ static void dri_set_background_context(void *loaderPrivate) { _EGLContext *ctx = _eglGetCurrentContext(); _EGLThreadInfo *t = _eglGetCurrentThread(); _eglBindContextToThread(ctx, t); } +static GLboolean +dri_is_thread_safe(void *loaderPrivate) +{ +#ifdef HAVE_X11_PLATFORM + struct dri2_egl_surface *dri2_surf = loaderPrivate; + _EGLDisplay *display = dri2_surf->base.Resource.Display; + Display *xdpy = (Display*)display->PlatformDisplay; + + /* Check Xlib is running in thread safe mode when running on EGL/X11-xlib +* platform +* +* 'lock_fns' is the XLockDisplay function pointer of the X11 display 'dpy'. +* It wll be NULL if XInitThreads wasn't called. +*/ + if (display->Platform == _EGL_PLATFORM_X11 && xdpy && !xdpy->lock_fns) + return false; +#endif + + return true; +} + const __DRIbackgroundCallableExtension background_callable_extension = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isThreadSafe = dri_is_thread_safe, }; const __DRIuseInvalidateExtension use_invalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const EGLint dri2_to_egl_attribute_map[__DRI_ATTRIB_MAX] = { [__DRI_ATTRIB_BUFFER_SIZE ] = EGL_BUFFER_SIZE, [__DRI_ATTRIB_LEVEL] = EGL_LEVEL, [__DRI_ATTRIB_RED_SIZE] = EGL_RED_SIZE, -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe
Hello Mesa developers, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. v2: based on Nicolai/Matt reviews Add a check on DRI extension version Use C comments :) v3: based on Emil reviews Split the initial first patch into 3 sub patches dri extension / glx / egl Improve error message Improve code readability Just include libX11 on EGL protected by ifdef Thanks you for all the review comments. Best regards, Gregory Hainaut (4): dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety glx: implement __DRIbackgroundCallableExtension.isThreadSafe egl: implement __DRIbackgroundCallableExtension.isThreadSafe glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 11 +++ src/egl/drivers/dri2/egl_dri2.c | 28 +++- src/gallium/state_trackers/dri/dri_context.c | 21 + src/glx/dri2_glx.c | 15 ++- src/glx/dri3_glx.c | 15 ++- 5 files changed, 83 insertions(+), 7 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3 4/4] glthread/gallium: require safe_glthread to start glthread
Print an error message for the user if the requirement isn't met, or we're not thread safe. v2: based on Nicolai feedbacks Check the DRI extension version v3: based on Emil feedbacks improve commit and error messages. use backgroundCallable variable to improve readability Signed-off-by: Gregory Hainaut --- src/gallium/state_trackers/dri/dri_context.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/gallium/state_trackers/dri/dri_context.c b/src/gallium/state_trackers/dri/dri_context.c index 92d79849c4..8cbab5359f 100644 --- a/src/gallium/state_trackers/dri/dri_context.c +++ b/src/gallium/state_trackers/dri/dri_context.c @@ -51,20 +51,22 @@ dri_create_context(gl_api api, const struct gl_config * visual, { __DRIscreen *sPriv = cPriv->driScreenPriv; struct dri_screen *screen = dri_screen(sPriv); struct st_api *stapi = screen->st_api; struct dri_context *ctx = NULL; struct st_context_iface *st_share = NULL; struct st_context_attribs attribs; enum st_context_error ctx_err = 0; unsigned allowed_flags = __DRI_CTX_FLAG_DEBUG | __DRI_CTX_FLAG_FORWARD_COMPATIBLE; + const __DRIbackgroundCallableExtension *backgroundCallable = + screen->sPriv->dri2.backgroundCallable; if (screen->has_reset_status_query) allowed_flags |= __DRI_CTX_FLAG_ROBUST_BUFFER_ACCESS; if (flags & ~allowed_flags) { *error = __DRI_CTX_ERROR_UNKNOWN_FLAG; goto fail; } if (!screen->has_reset_status_query && notify_reset) { @@ -151,24 +153,35 @@ dri_create_context(gl_api api, const struct gl_config * visual, ctx->st->st_manager_private = (void *) ctx; ctx->stapi = stapi; if (ctx->st->cso_context) { ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled, ctx->st->cso_context); ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context); } /* Do this last. */ if (ctx->st->start_thread && - /* the driver loader must implement this */ - screen->sPriv->dri2.backgroundCallable && - driQueryOptionb(&screen->optionCache, "mesa_glthread")) - ctx->st->start_thread(ctx->st); + driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (backgroundCallable && backgroundCallable->base.version >= 2 && +driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (backgroundCallable->isThreadSafe(cPriv->loaderPrivate)) +ctx->st->start_thread(ctx->st); + else +fprintf(stderr, "dri_create_context: glthread isn't thread safe " + "- missing call XInitThreads\n"); + } else { + fprintf(stderr, "dri_create_context: requested glthread but driver " + "is missing backgroundCallable V2 extension\n"); + } + } *error = __DRI_CTX_ERROR_SUCCESS; return GL_TRUE; fail: if (ctx && ctx->st) ctx->st->destroy(ctx->st); free(ctx); return GL_FALSE; -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v3 1/4] dri: Extend __DRIbackgroundCallableExtensionRec to include a callback that checks for thread safety
DRI-drivers could call Xlib functions, for example to allocate a new back buffer. When glthread is enabled, the driver runs mostly on a separate thread. Therefore we need to guarantee the thread safety between libX11 calls from the applications (not aware of the extra thread) and the ones from the driver. See discussion thread: https://lists.freedesktop.org/archives/mesa-dev/2017-April/152547.html Fortunately, Xlib allows to lock display to ensure thread safety but XInitThreads must be called first by the application to initialize the lock function pointer. This patch will allow to check XInitThreads was called to allow glthread on GLX or EGL platform. Note: a tentative was done to port libX11 code to XCB but it didn't solve fully thread safety. See discussion thread: https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html Note: Nvidia forces the driver to call XInitThreads. Quoting their manpage: "The NVIDIA OpenGL driver will automatically attempt to enable Xlib thread-safe mode if needed. However, it might not be possible in some situations, such as when the NVIDIA OpenGL driver library is dynamically loaded after Xlib has been loaded and initialized. If that is the case, threaded optimizations will stay disabled unless the application is modified to call XInitThreads() before initializing Xlib or to link directly against the NVIDIA OpenGL driver library. Alternatively, using the LD_PRELOAD environment variable to include the NVIDIA OpenGL driver library should also achieve the desired result." v2: based on Nicolai and Matt feedback Use C style comment v3: based on Emil feedback split the patch in 3 s/isGlThreadSafe/isThreadSafe/ Signed-off-by: Gregory Hainaut --- include/GL/internal/dri_interface.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index c83056aa70..8381f704e4 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1714,13 +1714,24 @@ struct __DRIbackgroundCallableExtensionRec { * non-background thread (i.e. a thread that has already been bound to a * context using __DRIcoreExtensionRec::bindContext()); when this happens, * the \c loaderPrivate pointer must be equal to the pointer that was * passed to the driver when the currently bound context was created. * * This call should execute quickly enough that the driver can call it with * impunity whenever a background thread starts performing drawing * operations (e.g. it should just set a thread-local variable). */ void (*setBackgroundContext)(void *loaderPrivate); + + /** +* Indicate that it is multithread safe to use glthread. For GLX/EGL +* platforms using Xlib, that involves calling XInitThreads, before +* opening an X display. +* +* \param loaderPrivate is the value that was passed to to the driver when +* the context was created. This can be used by the loader to identify +* which context any callbacks are associated with. +*/ + GLboolean (*isThreadSafe)(void *loaderPrivate); }; #endif -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 2/2] glthread/gallium: require safe_glthread to start glthread
Otherwise print a warning v2: based on Nicolai feedback Check the DRI extension version Signed-off-by: Gregory Hainaut --- src/gallium/state_trackers/dri/dri_context.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/gallium/state_trackers/dri/dri_context.c b/src/gallium/state_trackers/dri/dri_context.c index 92d79849c4..7c074be63f 100644 --- a/src/gallium/state_trackers/dri/dri_context.c +++ b/src/gallium/state_trackers/dri/dri_context.c @@ -153,22 +153,30 @@ dri_create_context(gl_api api, const struct gl_config * visual, if (ctx->st->cso_context) { ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled, ctx->st->cso_context); ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context); } /* Do this last. */ if (ctx->st->start_thread && /* the driver loader must implement this */ screen->sPriv->dri2.backgroundCallable && - driQueryOptionb(&screen->optionCache, "mesa_glthread")) - ctx->st->start_thread(ctx->st); + /* v2 gives us isGlThreadSafe to ensure thread safety */ + screen->sPriv->dri2.backgroundCallable->base.version >= 2 && + driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (ctx->sPriv->dri2.backgroundCallable->isGlThreadSafe(cPriv->loaderPrivate)) + ctx->st->start_thread(ctx->st); + else + fprintf(stderr, "MESA warning: glthread can't be enabled because " + "the application didn't call XInitThreads\n"); + } *error = __DRI_CTX_ERROR_SUCCESS; return GL_TRUE; fail: if (ctx && ctx->st) ctx->st->destroy(ctx->st); free(ctx); return GL_FALSE; -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 1/2] glx|egl: allow to test if glthread is safe enough on X11 platform
I extended the struct __DRIbackgroundCallableExtensionRec because the other function pointer is already related for glthread. DRI2/DRI3 glx code path check that display can be locked (basically XInitThread was called) EGL code path is more tricky as we don't want to pull X11 header. Instead the code will assume that it is safe if X11 isn't used or there is no display (i.e. 100% XCB) The new function will be used in the next commit v2: based on Nicolai and Matt feedback Use C style comment Bump __DRIbackgroundCallableExtension version Signed-off-by: Gregory Hainaut --- include/GL/internal/dri_interface.h | 10 ++ src/egl/drivers/dri2/egl_dri2.c | 34 +- src/glx/dri2_glx.c | 11 ++- src/glx/dri3_glx.c | 10 +- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index 86efd1bdc9..ef897b6483 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1713,13 +1713,23 @@ struct __DRIbackgroundCallableExtensionRec { * non-background thread (i.e. a thread that has already been bound to a * context using __DRIcoreExtensionRec::bindContext()); when this happens, * the \c loaderPrivate pointer must be equal to the pointer that was * passed to the driver when the currently bound context was created. * * This call should execute quickly enough that the driver can call it with * impunity whenever a background thread starts performing drawing * operations (e.g. it should just set a thread-local variable). */ void (*setBackgroundContext)(void *loaderPrivate); + + /** +* Indicate that it is multithread safe to use glthread. Typically +* XInitThread was called in GLX setup. +* +* \param loaderPrivate is the value that was passed to to the driver when +* the context was created. This can be used by the loader to identify +* which context any callbacks are associated with. +*/ + GLboolean (*isGlThreadSafe)(void *loaderPrivate); }; #endif diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 91456b025d..07cafee468 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -85,24 +85,56 @@ static void dri_set_background_context(void *loaderPrivate) { _EGLContext *ctx = _eglGetCurrentContext(); _EGLThreadInfo *t = _eglGetCurrentThread(); _eglBindContextToThread(ctx, t); } +static GLboolean +dri_is_glthread_safe(void *loaderPrivate) +{ +#ifdef HAVE_X11_PLATFORM + struct dri2_egl_surface *dri2_surf = loaderPrivate; + _EGLDisplay *display = dri2_surf->base.Resource.Display; + Display *dpy; + + /* Only the libX11 isn't safe */ + if (display->Platform != _EGL_PLATFORM_X11) + return true; + + /* Will use pure XCB so no libX11 here either */ + if (display->PlatformDisplay == NULL) + return true; + + /** +* In an ideal world we would check the X11 lock pointer +* (display->PlatformDisplay->lock_fns). Unfortunately it +* requires to know the full type. And we don't want to bring X11 +* headers here. +* +* So let's assume an unsafe behavior. Modern EGL code shouldn't use +* libX11 anyway. +*/ + return false; +#else + return true; +#endif +} + const __DRIbackgroundCallableExtension background_callable_extension = { - .base = { __DRI_BACKGROUND_CALLABLE, 1 }, + .base = { __DRI_BACKGROUND_CALLABLE, 2 }, .setBackgroundContext = dri_set_background_context, + .isGlThreadSafe = dri_is_glthread_safe, }; const __DRIuseInvalidateExtension use_invalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; static const EGLint dri2_to_egl_attribute_map[] = { [__DRI_ATTRIB_BUFFER_SIZE ] = EGL_BUFFER_SIZE, [__DRI_ATTRIB_LEVEL] = EGL_LEVEL, [__DRI_ATTRIB_RED_SIZE] = EGL_RED_SIZE, diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index 145f44d6e8..7a6511e2de 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -946,20 +946,28 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw) return priv->swap_interval; } static void driSetBackgroundContext(void *loaderPrivate) { struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +driIsGlThreadSafe(void *loaderPrivate) +{ + struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; + return pcp->base.psc->dpy->lock_fns != NULL; +} + + static const __DRIdri2LoaderExtension dri2LoaderExtension = { .base = { __DRI_DRI2_LOADER, 3 }, .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= dri2GetBuffersWithFormat, }; static const __DRIdri2LoaderExtensio
[Mesa-dev] [PATCH v2 0/2 Disable glthread is libX11 isn't thread-safe]
Hello, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. EGL case is more tricky so the pair (X11/libX11) is marked as unsafe. I think it is fine because modern EGL application should rely on XCB (on the X11 platform). v2: based on Nicolai/Matt reviews Add a check on DRI extension version Use C comments :) Best regards, Gregory Hainaut (2): glx|egl: allow to test if glthread is safe enough on X11 platform glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 10 src/egl/drivers/dri2/egl_dri2.c | 34 +++- src/gallium/state_trackers/dri/dri_context.c | 12 -- src/glx/dri2_glx.c | 11 - src/glx/dri3_glx.c | 10 +++- 5 files changed, 72 insertions(+), 5 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/2] glx|egl: allow to test if glthread is safe enough on X11 platform
I extended the struct __DRIbackgroundCallableExtensionRec because the other function pointer is already related for glthread. DRI2/DRI3 glx code path check that display can be locked (basically XInitThread was called) EGL code path is more tricky as we don't want to pull X11 header. Instead the code will assume that it is safe if X11 isn't used or there is no display (i.e. 100% XCB) The new function will be used in the next commit Signed-off-by: Gregory Hainaut --- include/GL/internal/dri_interface.h | 9 + src/egl/drivers/dri2/egl_dri2.c | 30 ++ src/glx/dri2_glx.c | 9 + src/glx/dri3_glx.c | 8 4 files changed, 56 insertions(+) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index 86efd1bdc9..28a52ccdb9 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1713,13 +1713,22 @@ struct __DRIbackgroundCallableExtensionRec { * non-background thread (i.e. a thread that has already been bound to a * context using __DRIcoreExtensionRec::bindContext()); when this happens, * the \c loaderPrivate pointer must be equal to the pointer that was * passed to the driver when the currently bound context was created. * * This call should execute quickly enough that the driver can call it with * impunity whenever a background thread starts performing drawing * operations (e.g. it should just set a thread-local variable). */ void (*setBackgroundContext)(void *loaderPrivate); + /** +* Indicate that it is multithread safe to use glthread. Typically +* XInitThread was called in GLX setup. +* +* \param loaderPrivate is the value that was passed to to the driver when +* the context was created. This can be used by the loader to identify +* which context any callbacks are associated with. +*/ + GLboolean (*isGlThreadSafe)(void *loaderPrivate); }; #endif diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 2cab7d00c1..df2db97bcf 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -85,24 +85,54 @@ static void dri_set_background_context(void *loaderPrivate) { _EGLContext *ctx = _eglGetCurrentContext(); _EGLThreadInfo *t = _eglGetCurrentThread(); _eglBindContextToThread(ctx, t); } +static GLboolean +dri_is_glthread_safe(void *loaderPrivate) +{ +#ifdef HAVE_X11_PLATFORM + struct dri2_egl_surface *dri2_surf = loaderPrivate; + _EGLDisplay *display = dri2_surf->base.Resource.Display; + Display *dpy; + + // Only the libX11 isn't safe + if (display->Platform != _EGL_PLATFORM_X11) + return true; + + // Will use pure XCB so no libX11 here either + if (display->PlatformDisplay == NULL) + return true; + + // In an ideal world we would check the X11 lock pointer + // (display->PlatformDisplay->lock_fns). Unfortunately it + // requires to know the full type. And we don't want to bring X11 + // headers here. + // + // So let's assume an unsafe behavior. Modern EGL code shouldn't use + // libX11 anyway. + return false; +#else + return true; +#endif +} + const __DRIbackgroundCallableExtension background_callable_extension = { .base = { __DRI_BACKGROUND_CALLABLE, 1 }, .setBackgroundContext = dri_set_background_context, + .isGlThreadSafe = dri_is_glthread_safe, }; const __DRIuseInvalidateExtension use_invalidate = { .base = { __DRI_USE_INVALIDATE, 1 } }; EGLint dri2_to_egl_attribute_map[] = { 0, EGL_BUFFER_SIZE,/* __DRI_ATTRIB_BUFFER_SIZE */ EGL_LEVEL,/* __DRI_ATTRIB_LEVEL */ diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index 145f44d6e8..8f4d2f027f 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -946,20 +946,28 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw) return priv->swap_interval; } static void driSetBackgroundContext(void *loaderPrivate) { struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; __glXSetCurrentContext(&pcp->base); } +static GLboolean +driIsGlThreadSafe(void *loaderPrivate) +{ + struct dri2_context *pcp = (struct dri2_context *) loaderPrivate; + return pcp->base.psc->dpy->lock_fns != NULL; +} + + static const __DRIdri2LoaderExtension dri2LoaderExtension = { .base = { __DRI_DRI2_LOADER, 3 }, .getBuffers = dri2GetBuffers, .flushFrontBuffer= dri2FlushFrontBuffer, .getBuffersWithFormat= dri2GetBuffersWithFormat, }; static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { .base = { __DRI_DRI2_LOADER, 3 }, @@ -970,20 +978,21 @@ static const __DRIdri2LoaderExtension dri2LoaderExtension_old = { }; static const __DRIuseInvalidateExtension dri2UseInvalidate
[Mesa-dev] [RFC 2/2] glthread/gallium: require safe_glthread to start glthread
Otherwise print a warning Signed-off-by: Gregory Hainaut --- src/gallium/state_trackers/dri/dri_context.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/gallium/state_trackers/dri/dri_context.c b/src/gallium/state_trackers/dri/dri_context.c index 92d79849c4..35b0c454be 100644 --- a/src/gallium/state_trackers/dri/dri_context.c +++ b/src/gallium/state_trackers/dri/dri_context.c @@ -153,22 +153,28 @@ dri_create_context(gl_api api, const struct gl_config * visual, if (ctx->st->cso_context) { ctx->pp = pp_init(ctx->st->pipe, screen->pp_enabled, ctx->st->cso_context); ctx->hud = hud_create(ctx->st->pipe, ctx->st->cso_context); } /* Do this last. */ if (ctx->st->start_thread && /* the driver loader must implement this */ screen->sPriv->dri2.backgroundCallable && - driQueryOptionb(&screen->optionCache, "mesa_glthread")) - ctx->st->start_thread(ctx->st); + driQueryOptionb(&screen->optionCache, "mesa_glthread")) { + + if (ctx->sPriv->dri2.backgroundCallable->isGlThreadSafe(cPriv->loaderPrivate)) + ctx->st->start_thread(ctx->st); + else + fprintf(stderr, "MESA warning: glthread can't be enabled because " + "the application didn't call XInitThreads\n"); + } *error = __DRI_CTX_ERROR_SUCCESS; return GL_TRUE; fail: if (ctx && ctx->st) ctx->st->destroy(ctx->st); free(ctx); return GL_FALSE; -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 0/2] Disable glthread is libX11 isn't thread-safe
Hello, Following the discussion from https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html A check was added to ensure that X11 display can be locked. It should be enough to ensure thread safety between X11 and glthread. I also did the check on DRI3 as I'm not 100% sure that it is really thread safe. EGL case is more tricky so the pair (X11/libX11) is marked as unsafe. I think it is fine because modern EGL application should rely on XCB (on the X11 platform). Best regards, Gregory Hainaut (2): glx|egl: allow to test if glthread is safe enough on X11 platform glthread/gallium: require safe_glthread to start glthread include/GL/internal/dri_interface.h | 9 + src/egl/drivers/dri2/egl_dri2.c | 30 src/gallium/state_trackers/dri/dri_context.c | 10 -- src/glx/dri2_glx.c | 9 + src/glx/dri3_glx.c | 8 5 files changed, 64 insertions(+), 2 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 0/1] Port dri2GetBuffersWithFormat/dri2GetBuffers to XCB
On Thu, 27 Apr 2017 17:11:30 +0900 Michel Dänzer wrote: > On 26/04/17 07:06 PM, Gregory Hainaut wrote: > > On 4/26/17, Michel Dänzer wrote: > [...] > [...] > [...] > > > > I didn't test it (yet). But I think it is safe to call XCB from > > various threads. However if one thread use XLIB, you're screwed (to > > access the same display). > > > [...] > [...] > [...] > > I far from an expert so maybe I misunderstand some stuffs. So, as far > > as I understand XLIB and XCB are mixed together. They likely share the > > same queues. > > > > Let's say you have an app that does Xlib operations on display (for > > example XGetGeometry). As XInitThread wasn't called, you're in YOLO > > mode. > > > > glthread (gallium->DRI2) will do operation (GetBuffer) on the same > > display through an XCB connection but it is still the same display. > > XCB might lock it properly but Xlib call is lock-free. > > I was hoping the lower XCB layer could be used in a thread-safe way even > if the higher libX11 layer isn't. But maybe that's just wishful thinking. Hello Michel, Yeah me too. Besides libX11 relies on (the thread-safe) XCB too. > > What happen in my case is that XCB reply was corrupted (count was huge > > which lead to memory bad access). My guess was that Xlib calls from > > app were the culprit. > > It would be nice to get some certainty, e.g. via the valgrind > helgrind/drd tools. So I tried drd. Unfortunately I'm afraid that my testcase (PCSX2) is way too complex for this kind of tool. However, I always got a huge value on the reply->count in GetBuffer. I investigated it further. Reply->count was 94373760 which can be read as 1440,1920 so my surface resolution. I think it is enough to prove that xcb_dri2_get_buffers_with_format_reply is stealing the reply of XGetGeometry. Note my 2nd kind of crash is XIOError due to a null reply on XGetGeometry. which make sense based on the above behavior. FWIW, the crash seem to be gone with this patch + my PCSX2-XCB patches. However, I need to check DRI3 behavior when I resize the window. As you said it could trigger buffer invalidation too. > > [...] > [...] > > On one hand, I don't like crash neither but in other hand, it would be > > nice to keep glthread for application that call XInitThread properly. > > We could check dpy->lock_fns and only enable glthread with DRI2 if it's > non-NULL. If it's NULL and the environment variable mesa_glthread=true > is set, we could print a warning to stderr explaining that glthread > can't be enabled because the application didn't call XInitThreads. > It is good idea. I will check, if I can manage to implement such a check. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 1/1] glx: port dri2GetBuffers/dri2GetBuffersWithFormat to XCB
On 4/26/17, Michel Dänzer wrote: > On 26/04/17 05:07 PM, Gregory Hainaut wrote: >> >> Note: those dri2* functions are typically called by gallium/mesa state >> tracker to handle new backbuffer allocation. When the old backbuffer was >> previously invalidated due to vsync. > > FWIW, DRI2 buffer invalidation isn't directly related to vsync. Ok. What happen is that I received DRI2_InvalidateBuffers event instead of DRI2_BufferSwapComplete when I enabled vsync on my application. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC 0/1] Port dri2GetBuffersWithFormat/dri2GetBuffers to XCB
On 4/26/17, Michel Dänzer wrote: > On 26/04/17 05:07 PM, Gregory Hainaut wrote: >> Following the discussion in "[PATCH v4 0/3] asynchronous pbo transfer with >> glthread" >> >> It will help apps that are ported to XCB. > > How so? I didn't test it (yet). But I think it is safe to call XCB from various threads. However if one thread use XLIB, you're screwed (to access the same display). > >> But Xlib (without XInitThread) apps will still crash when glthread is >> enabled on DRI2. > > Do the crashes provide information about where glthread is still calling > libX11 APIs? I far from an expert so maybe I misunderstand some stuffs. So, as far as I understand XLIB and XCB are mixed together. They likely share the same queues. Let's say you have an app that does Xlib operations on display (for example XGetGeometry). As XInitThread wasn't called, you're in YOLO mode. glthread (gallium->DRI2) will do operation (GetBuffer) on the same display through an XCB connection but it is still the same display. XCB might lock it properly but Xlib call is lock-free. What happen in my case is that XCB reply was corrupted (count was huge which lead to memory bad access). My guess was that Xlib calls from app were the culprit. I will test my XCB version of my app + this patch. Hopefully, it would be crash free. >> I only tested the patch on Nouveau >> >> There are 3 remaining possibilities >> * Won't fix. DRI3/XCB is the future >> * Enable thread safe Xlib by default (which would make sense in multicore >> CPU era) >> * Track gl call that will use X11 API to force a sync > > Until either of the latter two happens, glthread should only be enabled > with DRI3. On one hand, I don't like crash neither but in other hand, it would be nice to keep glthread for application that call XInitThread properly. At least, glthread isn't enabled by default. > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
On Wed, 26 Apr 2017 11:03:11 +0900 Michel Dänzer wrote: > On 25/04/17 06:14 PM, Gregory Hainaut wrote: > > Hello, > > > > I did more tests on my side. DRI3 + recent stack is fine. Older > > (Debian Jessie, ~2y old) XCB hangs/deadlock. So all DRI3 drivers > > should be fine (typically AMD). And all applications that called > > XInitThread soon enough. So yeah I think we can merge it. Note: I > > don't have commit access. > > Okay, I'm not blocking somebody else from pushing the series. > > > > Nouveau doesn't use DRI3 by default. So I'm looking to use XCB for > > dri2GetBuffer* operations. I copied the code from EGL/X11. So far it > > isn't a success. The dri2_drawable pdraw object is correctly populated > > with good value but I have tons of piglit failure. Debug is on-going > > but I'm afraid that it might not be possible to use XCB. > > Can you share the patch you're testing? > > Hello Michel, I found my issue, I forgot to set the *width, *height pointer. Piglit status is good. You can found the patch here https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC 1/1] glx: port dri2GetBuffers/dri2GetBuffersWithFormat to XCB
By default Xlib isn't thread safe so we better avoid it when gl thread is enabled. It will help applications that use XCB. But it will still crash if applications are still relying on Xlib (without XInitThread). Note: those dri2* functions are typically called by gallium/mesa state tracker to handle new backbuffer allocation. When the old backbuffer was previously invalidated due to vsync. Signed-off-by: Gregory Hainaut --- src/glx/dri2.c | 118 - src/glx/dri2.h | 25 src/glx/dri2_glx.c | 64 + 3 files changed, 47 insertions(+), 160 deletions(-) diff --git a/src/glx/dri2.c b/src/glx/dri2.c index f00b96525a..eed899e237 100644 --- a/src/glx/dri2.c +++ b/src/glx/dri2.c @@ -391,138 +391,20 @@ DRI2DestroyDrawable(Display * dpy, XID drawable) LockDisplay(dpy); GetReq(DRI2DestroyDrawable, req); req->reqType = info->codes->major_opcode; req->dri2ReqType = X_DRI2DestroyDrawable; req->drawable = drawable; UnlockDisplay(dpy); SyncHandle(); } -DRI2Buffer * -DRI2GetBuffers(Display * dpy, XID drawable, - int *width, int *height, - unsigned int *attachments, int count, int *outCount) -{ - XExtDisplayInfo *info = DRI2FindDisplay(dpy); - xDRI2GetBuffersReply rep; - xDRI2GetBuffersReq *req; - DRI2Buffer *buffers; - xDRI2Buffer repBuffer; - CARD32 *p; - int i; - - XextCheckExtension(dpy, info, dri2ExtensionName, False); - - LockDisplay(dpy); - GetReqExtra(DRI2GetBuffers, count * 4, req); - req->reqType = info->codes->major_opcode; - req->dri2ReqType = X_DRI2GetBuffers; - req->drawable = drawable; - req->count = count; - p = (CARD32 *) & req[1]; - for (i = 0; i < count; i++) - p[i] = attachments[i]; - - if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) { - UnlockDisplay(dpy); - SyncHandle(); - return NULL; - } - - *width = rep.width; - *height = rep.height; - *outCount = rep.count; - - buffers = malloc(rep.count * sizeof buffers[0]); - if (buffers == NULL) { - _XEatData(dpy, rep.count * sizeof repBuffer); - UnlockDisplay(dpy); - SyncHandle(); - return NULL; - } - - for (i = 0; i < rep.count; i++) { - _XReadPad(dpy, (char *) &repBuffer, sizeof repBuffer); - buffers[i].attachment = repBuffer.attachment; - buffers[i].name = repBuffer.name; - buffers[i].pitch = repBuffer.pitch; - buffers[i].cpp = repBuffer.cpp; - buffers[i].flags = repBuffer.flags; - } - - UnlockDisplay(dpy); - SyncHandle(); - - return buffers; -} - - -DRI2Buffer * -DRI2GetBuffersWithFormat(Display * dpy, XID drawable, - int *width, int *height, - unsigned int *attachments, int count, int *outCount) -{ - XExtDisplayInfo *info = DRI2FindDisplay(dpy); - xDRI2GetBuffersReply rep; - xDRI2GetBuffersReq *req; - DRI2Buffer *buffers; - xDRI2Buffer repBuffer; - CARD32 *p; - int i; - - XextCheckExtension(dpy, info, dri2ExtensionName, False); - - LockDisplay(dpy); - GetReqExtra(DRI2GetBuffers, count * (4 * 2), req); - req->reqType = info->codes->major_opcode; - req->dri2ReqType = X_DRI2GetBuffersWithFormat; - req->drawable = drawable; - req->count = count; - p = (CARD32 *) & req[1]; - for (i = 0; i < (count * 2); i++) - p[i] = attachments[i]; - - if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) { - UnlockDisplay(dpy); - SyncHandle(); - return NULL; - } - - *width = rep.width; - *height = rep.height; - *outCount = rep.count; - - buffers = malloc(rep.count * sizeof buffers[0]); - if (buffers == NULL) { - _XEatData(dpy, rep.count * sizeof repBuffer); - UnlockDisplay(dpy); - SyncHandle(); - return NULL; - } - - for (i = 0; i < rep.count; i++) { - _XReadPad(dpy, (char *) &repBuffer, sizeof repBuffer); - buffers[i].attachment = repBuffer.attachment; - buffers[i].name = repBuffer.name; - buffers[i].pitch = repBuffer.pitch; - buffers[i].cpp = repBuffer.cpp; - buffers[i].flags = repBuffer.flags; - } - - UnlockDisplay(dpy); - SyncHandle(); - - return buffers; -} - - void DRI2CopyRegion(Display * dpy, XID drawable, XserverRegion region, CARD32 dest, CARD32 src) { XExtDisplayInfo *info = DRI2FindDisplay(dpy); xDRI2CopyRegionReq *req; xDRI2CopyRegionReply rep; XextSimpleCheckExtension(dpy, info, dri2ExtensionName); diff --git a/src/glx/dri2.h b/src/glx/dri2.h index 4be5bf8eb8..c383e27123 100644 --- a/src/glx/dri2.h +++ b/src/glx/dri2.h @@ -30,29 +30,20 @@ * Kristian Høgsberg (k...@redhat.com) */ #ifndef _DRI2_H_ #define _DRI2_H_ #include #include #include -typedef struct -{ - unsigned int attachment; - unsigned int name; - unsigned int pitch; - unsigned int cpp; -
[Mesa-dev] [RFC 0/1] Port dri2GetBuffersWithFormat/dri2GetBuffers to XCB
Following the discussion in "[PATCH v4 0/3] asynchronous pbo transfer with glthread" It will help apps that are ported to XCB. But Xlib (without XInitThread) apps will still crash when glthread is enabled on DRI2. I only tested the patch on Nouveau There are 3 remaining possibilities * Won't fix. DRI3/XCB is the future * Enable thread safe Xlib by default (which would make sense in multicore CPU era) * Track gl call that will use X11 API to force a sync Note: I don't know if xcb_dri2_get_buffers_buffers can return a NULL so I added a check in doubt. Best Regards, Gregory Hainaut (1): glx: port dri2GetBuffers/dri2GetBuffersWithFormat to XCB src/glx/dri2.c | 118 - src/glx/dri2.h | 25 src/glx/dri2_glx.c | 64 + 3 files changed, 47 insertions(+), 160 deletions(-) -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
Hello, I did more tests on my side. DRI3 + recent stack is fine. Older (Debian Jessie, ~2y old) XCB hangs/deadlock. So all DRI3 drivers should be fine (typically AMD). And all applications that called XInitThread soon enough. So yeah I think we can merge it. Note: I don't have commit access. Nouveau doesn't use DRI3 by default. So I'm looking to use XCB for dri2GetBuffer* operations. I copied the code from EGL/X11. So far it isn't a success. The dri2_drawable pdraw object is correctly populated with good value but I have tons of piglit failure. Debug is on-going but I'm afraid that it might not be possible to use XCB. Best Regads, Gregory On 4/25/17, Dieter Nützel wrote: > Am 21.04.2017 12:11, schrieb Marek Olšák: >> FWIW, I think this series can land, because glthread is not enabled by >> default, and the libX11 issue is unrelated. >> >> Marek > > > Gregory? > > For the series: > > Tested-by: Dieter Nützel > > On Turks XT (6670). > > Dieter > >> On Apr 21, 2017 4:22 AM, "Michel Dänzer" wrote: >> >>> On 21/04/17 09:01 AM, Marek Olšák wrote: >>>> On Thu, Apr 20, 2017 at 9:44 PM, gregory hainaut >>>> wrote: >>>>> On Thu, 20 Apr 2017 20:01:00 +0200 >>>>> Marek Olšák wrote: >>>>> >>>>>> On Thu, Apr 20, 2017 at 6:53 PM, gregory hainaut >>>>>> wrote: >>>>>>> On Thu, 20 Apr 2017 11:57:08 +0200 >>>>>>> Marek Olšák wrote: >>>>>>> >>>>>>>> On Thu, Apr 20, 2017 at 10:28 AM, gregory hainaut >>>>>>>> wrote: >>>>>>>>> On Thu, 20 Apr 2017 12:29:11 +0900 >>>>>>>>> Michel Dänzer wrote: >>>>>>>>> >>>>>>>>>> On 20/04/17 01:54 AM, Gregory Hainaut wrote: >>>>>>>>>>> Hello, >>>>>>>>>>> >>>>>>>>>>> Please find the latest version that include a small fix for >>> hash deletion. I >>>>>>>>>>> think the series is good now. Please review/ack it. >>>>>>>>>> >>>>>>>>>> I'm afraid I have to NACK it. As discussed in the v4 cover >>> letter >>>>>>>>>> thread, Mesa's glthread cannot make any libX11 API calls. >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> Hello Michel, >>>>>>>>> >>>>>>>>> Just to be sure we are on the same line, let's me do a >>> summary. >>>>>>>>> >>>>>>>>> PCSX2 does the following bad pattern >>>>>>>>> 1/ no call to XInitThread >>>>>>>>> 2/ XGetGeometry to get the size of the surface >>>>>>>>> 3/ glDrawArray into the default framebuffer 0 (the window, >>> I'm not sure how to call it) >>>>>>>>> => which seem to call DRI2GetBuffersWithFormat under the >>> hood. I guess to get the >>>>>>>>> associated buffer of the window. >>>>>>>>> >>>>>>>>> >>>>>>>>> So far it was (kind of) working fine because PCSX2 does tons >>> of PBO transfer. So glthread >>>>>>>>> was mostly synchronous. >>>>>>>>> >>>>>>>>> This series removes the (useless) PBO transfer >>> synchronization. So now glthread is really >>>>>>>>> asynchronous and the above bad pattern crash as expected. >>>>>>>>> >>>>>>>>> I didn't add any libX11 API call on the patches. And I don't >>> think we can remvove the DRI stuff. >>>>>>>>> >>>>>>>>> Hum, I don't know what will be the impact on the perf but >>> potentially we can force a synchronization >>>>>>>>> when there is a draw to framebuffer 0. >>>>>>>> >>>>>>>> Can you send us the backtrace when DRI2GetBuffersWithFormat is >>> called >>>>>>>> by glDrawArrays? >>>>>>>> >>>>>>>> Marek >>>>>>> >>>>>>> Hello Marek, Michel, >>>>>>> >>>>>>> Here the full backtra
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
On Thu, 20 Apr 2017 20:01:00 +0200 Marek Olšák wrote: > On Thu, Apr 20, 2017 at 6:53 PM, gregory hainaut > wrote: > > On Thu, 20 Apr 2017 11:57:08 +0200 > > Marek Olšák wrote: > > > >> On Thu, Apr 20, 2017 at 10:28 AM, gregory hainaut > >> wrote: > >> > On Thu, 20 Apr 2017 12:29:11 +0900 > >> > Michel Dänzer wrote: > >> > > >> >> On 20/04/17 01:54 AM, Gregory Hainaut wrote: > >> >> > Hello, > >> >> > > >> >> > Please find the latest version that include a small fix for hash > >> >> > deletion. I > >> >> > think the series is good now. Please review/ack it. > >> >> > >> >> I'm afraid I have to NACK it. As discussed in the v4 cover letter > >> >> thread, Mesa's glthread cannot make any libX11 API calls. > >> >> > >> >> > >> > > >> > Hello Michel, > >> > > >> > Just to be sure we are on the same line, let's me do a summary. > >> > > >> > PCSX2 does the following bad pattern > >> > 1/ no call to XInitThread > >> > 2/ XGetGeometry to get the size of the surface > >> > 3/ glDrawArray into the default framebuffer 0 (the window, I'm not sure > >> > how to call it) > >> >=> which seem to call DRI2GetBuffersWithFormat under the hood. I > >> > guess to get the > >> > associated buffer of the window. > >> > > >> > > >> > So far it was (kind of) working fine because PCSX2 does tons of PBO > >> > transfer. So glthread > >> > was mostly synchronous. > >> > > >> > This series removes the (useless) PBO transfer synchronization. So now > >> > glthread is really > >> > asynchronous and the above bad pattern crash as expected. > >> > > >> > I didn't add any libX11 API call on the patches. And I don't think we > >> > can remvove the DRI stuff. > >> > > >> > Hum, I don't know what will be the impact on the perf but potentially we > >> > can force a synchronization > >> > when there is a draw to framebuffer 0. > >> > >> Can you send us the backtrace when DRI2GetBuffersWithFormat is called > >> by glDrawArrays? > >> > >> Marek > > > > Hello Marek, Michel, > > > > Here the full backtrace. > > > > #0 DRI2GetBuffersWithFormat (dpy=0xc6307e48, drawable=104857784, > > width=0xdef348c0, height=0xdef348c4, attachments=0xdcc15c88, count=1, > > outCount=0xdcc15c74) at dri2.c:464 > > #1 0xf05bac45 in dri2GetBuffersWithFormat (driDrawable=0xdef348a8, > > width=0xdef348c0, height=0xdef348c4, attachments=0xdcc15c88, count=1, > > out_count=0xdcc15c74, loaderPrivate=0xdefc6ef0) at dri2_glx.c:894 > > #2 0xe3ec3111 in dri2_drawable_get_buffers (count=, > > atts=0xdef252f8, drawable=0xdefc7b08) at dri2.c:285 > > #3 dri2_allocate_textures (ctx=0xdef15928, drawable=0xdefc7b08, > > statts=0xdef252f8, statts_count=2) at dri2.c:480 > > #4 0xe3ebcbb0 in dri_st_framebuffer_validate (stctx=0xdef4ab10, > > stfbi=0xdefc7b08, statts=0xdef252f8, count=2, out=0xdcc15d78) at > > dri_drawable.c:83 > > #5 0xe3d37afc in st_framebuffer_validate (stfb=stfb@entry=0xdef24f58, > > st=st@entry=0xdef4ab10) at state_tracker/st_manager.c:189 > > Note "print stfb->Base->Name" give me 0 > > #6 0xe3d38649 in st_manager_validate_framebuffers (st=0xdef4ab10) at > > state_tracker/st_manager.c:869 > > #7 0xe3cd0580 in st_validate_state (st=0xdef4ab10, > > pipeline=ST_PIPELINE_RENDER) at state_tracker/st_atom.c:174 > > #8 0xe3cf935d in st_draw_vbo (ctx=0xdef7f8f8, prims=0xdcc15f40, > > nr_prims=1, ib=0x0, index_bounds_valid=1 '\001', min_index=39911, > > max_index=39914, tfb_vertcount=0x0, stream=0, indirect=0x0) at > > state_tracker/st_draw.c:191 > > #9 0xe3caf35e in vbo_draw_arrays (baseInstance=0, numInstances=1, count=4, > > start=39911, mode=5, ctx=0xdef7f8f8) at vbo/vbo_exec_array.c:427 > > #10 vbo_exec_DrawArrays (mode=5, start=39911, count=4) at > > vbo/vbo_exec_array.c:575 > > #11 0xe3be4f07 in _mesa_unmarshal_DrawArrays (ctx=0xdef7f8f8, > > cmd=0xc41574b0) at main/marshal_generated.c:26644 > > #12 _mesa_unmarshal_dispatch_cmd (ctx=0xdef7f8f8, cmd=0xc41574b0) at > > main/marshal_generated.c:42457 > > #13 0xe3ba8af0 in glthread_unmarshal_batch (release_batch=true, > > batch=0xc4157410
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
On Thu, 20 Apr 2017 10:56:34 +0900 Michel Dänzer wrote: > On 20/04/17 01:43 AM, gregory hainaut wrote: > > Hello All, > > > > I ported PCSX2 to xcb (at least the non-glx part). Crash is gone :) > > So I can send the v5 with the hash delete fix. > > > > However, Mesa might receive crash bug report when glthread is enabled > > on a random app that doesn't use xcb/XinitThread properly. > > This means it's still a bug in Mesa, you're just working around it in > your application. > > As we've explained, Mesa's glthread cannot make any libX11 API calls. > Yes. And unfortunately the crash is still here (seem less often). > > Maybe it would be better to always enable the XInitThread mode by default > > on the X11 library. > > If performance of X11 is critical, it would be better to switch to xcb > > anyway. > > There has been talk about making that change, but there's not even a > specific plan yet for making it happen upstream. It doesn't change the > situation with currently shipping libX11. > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
On Thu, 20 Apr 2017 11:57:08 +0200 Marek Olšák wrote: > On Thu, Apr 20, 2017 at 10:28 AM, gregory hainaut > wrote: > > On Thu, 20 Apr 2017 12:29:11 +0900 > > Michel Dänzer wrote: > > > >> On 20/04/17 01:54 AM, Gregory Hainaut wrote: > >> > Hello, > >> > > >> > Please find the latest version that include a small fix for hash > >> > deletion. I > >> > think the series is good now. Please review/ack it. > >> > >> I'm afraid I have to NACK it. As discussed in the v4 cover letter > >> thread, Mesa's glthread cannot make any libX11 API calls. > >> > >> > > > > Hello Michel, > > > > Just to be sure we are on the same line, let's me do a summary. > > > > PCSX2 does the following bad pattern > > 1/ no call to XInitThread > > 2/ XGetGeometry to get the size of the surface > > 3/ glDrawArray into the default framebuffer 0 (the window, I'm not sure how > > to call it) > >=> which seem to call DRI2GetBuffersWithFormat under the hood. I guess > > to get the > > associated buffer of the window. > > > > > > So far it was (kind of) working fine because PCSX2 does tons of PBO > > transfer. So glthread > > was mostly synchronous. > > > > This series removes the (useless) PBO transfer synchronization. So now > > glthread is really > > asynchronous and the above bad pattern crash as expected. > > > > I didn't add any libX11 API call on the patches. And I don't think we can > > remvove the DRI stuff. > > > > Hum, I don't know what will be the impact on the perf but potentially we > > can force a synchronization > > when there is a draw to framebuffer 0. > > Can you send us the backtrace when DRI2GetBuffersWithFormat is called > by glDrawArrays? > > Marek Hello Marek, Michel, Here the full backtrace. #0 DRI2GetBuffersWithFormat (dpy=0xc6307e48, drawable=104857784, width=0xdef348c0, height=0xdef348c4, attachments=0xdcc15c88, count=1, outCount=0xdcc15c74) at dri2.c:464 #1 0xf05bac45 in dri2GetBuffersWithFormat (driDrawable=0xdef348a8, width=0xdef348c0, height=0xdef348c4, attachments=0xdcc15c88, count=1, out_count=0xdcc15c74, loaderPrivate=0xdefc6ef0) at dri2_glx.c:894 #2 0xe3ec3111 in dri2_drawable_get_buffers (count=, atts=0xdef252f8, drawable=0xdefc7b08) at dri2.c:285 #3 dri2_allocate_textures (ctx=0xdef15928, drawable=0xdefc7b08, statts=0xdef252f8, statts_count=2) at dri2.c:480 #4 0xe3ebcbb0 in dri_st_framebuffer_validate (stctx=0xdef4ab10, stfbi=0xdefc7b08, statts=0xdef252f8, count=2, out=0xdcc15d78) at dri_drawable.c:83 #5 0xe3d37afc in st_framebuffer_validate (stfb=stfb@entry=0xdef24f58, st=st@entry=0xdef4ab10) at state_tracker/st_manager.c:189 Note "print stfb->Base->Name" give me 0 #6 0xe3d38649 in st_manager_validate_framebuffers (st=0xdef4ab10) at state_tracker/st_manager.c:869 #7 0xe3cd0580 in st_validate_state (st=0xdef4ab10, pipeline=ST_PIPELINE_RENDER) at state_tracker/st_atom.c:174 #8 0xe3cf935d in st_draw_vbo (ctx=0xdef7f8f8, prims=0xdcc15f40, nr_prims=1, ib=0x0, index_bounds_valid=1 '\001', min_index=39911, max_index=39914, tfb_vertcount=0x0, stream=0, indirect=0x0) at state_tracker/st_draw.c:191 #9 0xe3caf35e in vbo_draw_arrays (baseInstance=0, numInstances=1, count=4, start=39911, mode=5, ctx=0xdef7f8f8) at vbo/vbo_exec_array.c:427 #10 vbo_exec_DrawArrays (mode=5, start=39911, count=4) at vbo/vbo_exec_array.c:575 #11 0xe3be4f07 in _mesa_unmarshal_DrawArrays (ctx=0xdef7f8f8, cmd=0xc41574b0) at main/marshal_generated.c:26644 #12 _mesa_unmarshal_dispatch_cmd (ctx=0xdef7f8f8, cmd=0xc41574b0) at main/marshal_generated.c:42457 #13 0xe3ba8af0 in glthread_unmarshal_batch (release_batch=true, batch=0xc4157410, ctx=0xdef7f8f8) at main/glthread.c:64 #14 glthread_worker (data=0xdef7f8f8) at main/glthread.c:110 > If this DRI2GetBuffersWithFormat call results in libX11 API calls on the > glthread, that's a bug which needs to be fixed, either by moving the > DRI2GetBuffersWithFormat call to the main thread or (if possible) by > changing DRI2GetBuffersWithFormat to use XCB directly. Ok. On one hand, moving DRI2GetBuffersWithFormat to main thread won't be easy. I think we need to keep track of the current bound framebuffer on the app thread. So we can force a sync on gl operation that will access the framebuffer 0. On the other hand, GLX/XLIB/XCB interactions are quite foggy for me. It seem there already some xcb codes in various place typically EGL and the xcb_dri2 extension. So maybe there is a reason that code wasn't ported here. In particular, I saw this option in configure --enable-glx[=dri|xlib|gallium-xlib] enable the GLX library and choose a
Re: [Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
On Thu, 20 Apr 2017 12:29:11 +0900 Michel Dänzer wrote: > On 20/04/17 01:54 AM, Gregory Hainaut wrote: > > Hello, > > > > Please find the latest version that include a small fix for hash deletion. I > > think the series is good now. Please review/ack it. > > I'm afraid I have to NACK it. As discussed in the v4 cover letter > thread, Mesa's glthread cannot make any libX11 API calls. > > Hello Michel, Just to be sure we are on the same line, let's me do a summary. PCSX2 does the following bad pattern 1/ no call to XInitThread 2/ XGetGeometry to get the size of the surface 3/ glDrawArray into the default framebuffer 0 (the window, I'm not sure how to call it) => which seem to call DRI2GetBuffersWithFormat under the hood. I guess to get the associated buffer of the window. So far it was (kind of) working fine because PCSX2 does tons of PBO transfer. So glthread was mostly synchronous. This series removes the (useless) PBO transfer synchronization. So now glthread is really asynchronous and the above bad pattern crash as expected. I didn't add any libX11 API call on the patches. And I don't think we can remvove the DRI stuff. Hum, I don't know what will be the impact on the perf but potentially we can force a synchronization when there is a draw to framebuffer 0. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 0/3] asynchronous pbo transfer with glthread
Hello, Please find the latest version that include a small fix for hash deletion. I think the series is good now. Please review/ack it. Allow to handle this kind of case: genBuffer(&pbo); BindBuffer(pbo) DeleteBuffer(pbo); BindBuffer(rand_pbo) TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous There are various subtely to handle multi threaded shared context. In order to keep the code sane, I've considered a buffer invalid when it is deleted by a context even it is still bound to others contexts. It will force a synchronous transfer which is always safe. An example could be Ctx A: glGenBuffers(1, &pbo); Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); Ctx B: glDeleteBuffers(1, &pbo); Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ be asynchronous (because the PBO that was generated first is still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback V5: Properly delete element of the new hash (first patch) Best regards, Gregory Hainaut (3): mesa/glthread: track buffer creation/destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 +- src/mapi/glapi/gen/gl_API.xml | 32 +++--- src/mapi/glapi/gen/gl_marshal.py | 23 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +++- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c| 149 - src/mesa/main/marshal.h| 24 src/mesa/main/mtypes.h | 5 + src/mesa/main/shared.c | 14 +++ 11 files changed, 269 insertions(+), 39 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 23 - src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 33 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 566f157..d842ebd 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -367,79 +367,79 @@ - + - + - + - + - + - + @@ -516,30 +516,30 @@ - + - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0..6e1ac09 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -75,21 +75,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250..447b03a 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -115,28 +115,30 @@ param: img_send_null - boolean flag to determine if blank pixel data should be sent when a NULL pointer is passed. This is only used by TexImage1D and TexImage2D. img_null_flag - boolean flag to determine if an extra flag is used to determine if a NULL pixel pointer was passed. This is used by TexSubImage1D, TexSubImage2D, TexImage3D and others. img_pad_dimensions - boolean flag to determine if dimension data and offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't want to track state for. glx: rop - Opcode value for "render" commands sop - Opcode value for "single" commands vendorp
[Mesa-dev] [PATCH v5 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers to be valid. v4: update comments based on Nicolai review Signed-off-by: Gregory Hainaut --- src/mesa/main/marshal.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 4037b79..1167271 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -312,21 +312,34 @@ _mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, /* BindBufferBase: marshalled asynchronously */ struct marshal_cmd_BindBufferBase { struct marshal_cmd_base cmd_base; GLenum target; GLuint index; GLuint buffer; }; -/** Tracks the current bindings for the vertex array and index array buffers. +/** + * Check that buffer is a valid buffer handle + * Always return false for ID 0. + */ +static bool +is_bufferobj(struct gl_context *ctx, GLuint buffer) +{ + if (buffer == 0) + return false; + else + return _mesa_HashLookup(ctx->Shared->ShadowBufferObjects, buffer) != NULL; +} + +/** Tracks the current bindings of GL buffer targets * * This is part of what we need to enable glthread on compat-GL contexts that * happen to use VBOs, without also supporting the full tracking of VBO vs * user vertex array bindings per attribute on each vertex array for * determining what to upload at draw call time. * * Note that GL core makes it so that a buffer binding with an invalid handle * in the "buffer" parameter will throw an error, and then a * glVertexAttribPointer() that followsmight not end up pointing at a VBO. * However, in GL core the draw call would throw an error as well, so we don't @@ -334,37 +347,54 @@ struct marshal_cmd_BindBufferBase * marshal user data for draw calls, and the unmarshal will just generate an * error or not as appropriate. * * For compatibility GL, we do need to accurately know whether the draw call * on the unmarshal side will dereference a user pointer or load data from a * VBO per vertex. That would make it seem like we need to track whether a * "buffer" is valid, so that we can know when an error will be generated * instead of updating the binding. However, compat GL has the ridiculous * feature that if you pass a bad name, it just gens a buffer object for you, * so we escape without having to know if things are valid or not. + * + * Pixel buffers are tracked to decide whether pixel transfer goes to a user + * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike + * for VBOs, we do need accurate tracking, since user pointers can be used in + * GL core contexts. */ static void -track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) +track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer) { struct glthread_state *glthread = ctx->GLThread; switch (target) { case GL_ARRAY_BUFFER: glthread->vertex_array_is_vbo = (buffer != 0); break; case GL_ELEMENT_ARRAY_BUFFER: /* The current element array buffer binding is actually tracked in the * vertex array object instead of the context, so this would need to * change on vertex array object updates. */ glthread->element_array_is_vbo = (buffer != 0); break; + case GL_PIXEL_UNPACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_unpack_buffer_bound = buffer; + else + glthread->pixel_unpack_buffer_bound = 0; + break; + case GL_PIXEL_PACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_pack_buffer_bound = buffer; + else + glthread->pixel_pack_buffer_bound = 0; + break; } } struct marshal_cmd_BindBuffer { struct marshal_cmd_base cmd_base; GLenum target; GLuint buffer; }; @@ -382,21 +412,21 @@ _mesa_unmarshal_BindBuffer(struct gl_context *ctx, CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer)); } void GLAPIENTRY _mesa_marshal_BindBuffer(GLenum target, GLuint buffer) { GET_CURRENT_CONTEXT(ctx); size_t cmd_size = sizeof(struct marshal_cmd_BindBuffer); struct marshal_cmd_BindBuffer *cmd; debug_print_marshal("BindBuffer"); - track_vbo_binding(ctx, target, buffer); + track_buffers_binding(ctx, target, buffer); if (cmd_size <= MARSHAL_MAX_CMD_SIZE) { cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer, cmd_size); cmd->target = target; cmd->buffer = buffer; _mesa_post_marshal_hook(ctx); } else { _mesa_glthread_finish(ctx); CALL_BindBuffer(ctx->CurrentServerDispatch, (target, buffer)); -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v5 1/3] mesa/glthread: track buffer creation/destruction
It would be used in next commit to allow asynchronous PBO transfer. The tracking saves the buffer name into a hash. Saving pointer will be more complex as the buffer is created in BindBuffer due to IsBuffer insanity. Perf wise DeleteBuffers is now synchronous for robustness. v5: properly delete hash element with the help of _mesa_HashDeleteAll Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 2 +- src/mapi/glapi/gen/gl_API.xml | 4 +- src/mesa/main/glthread.h | 10 +++ src/mesa/main/marshal.c| 113 + src/mesa/main/marshal.h| 24 ++ src/mesa/main/mtypes.h | 5 ++ src/mesa/main/shared.c | 14 +++ 7 files changed, 169 insertions(+), 3 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 43841bb..566f157 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -42,21 +42,21 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index c0ee2f2..ad841a1 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -5042,27 +5042,27 @@ - + - + diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h index 50c1db2..494e942 100644 --- a/src/mesa/main/glthread.h +++ b/src/mesa/main/glthread.h @@ -85,20 +85,30 @@ struct glthread_state * Tracks on the main thread side whether the current vertex array binding * is in a VBO. */ bool vertex_array_is_vbo; /** * Tracks on the main thread side whether the current element array (index * buffer) binding is in a VBO. */ bool element_array_is_vbo; + + /** +* Tracks on the main thread side the bound unpack pixel buffer +*/ + GLint pixel_unpack_buffer_bound; + + /** +* Tracks on the main thread side the bound pack pixel buffer +*/ + GLint pixel_pack_buffer_bound; }; /** * A single batch of commands queued up for later execution by a thread pool * task. */ struct glthread_batch { /** * Next batch of commands to execute after this batch, or NULL if this is diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index ae4efb5..4037b79 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -25,20 +25,21 @@ * * Custom functions for marshalling GL calls from the main thread to a worker * thread when automatic code generation isn't appropriate. */ #include "main/enums.h" #include "main/macros.h" #include "marshal.h" #include "dispatch.h" #include "marshal_generated.h" +#include "hash.h" #ifdef HAVE_PTHREAD struct marshal_cmd_Flush { struct marshal_cmd_base cmd_base; }; void @@ -189,20 +190,132 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, } _mesa_post_marshal_hook(ctx); } else { _mesa_glthread_finish(ctx); CALL_ShaderSource(ctx->CurrentServerDispatch, (shader, count, string, length_tmp)); } free(length_tmp); } +/** + * Used as a placeholder for track_buffers_creation/track_buffers_destruction + * so we know if buffer really exists in track_buffers_binding. + */ +static struct gl_buffer_object DummyBufferObject; + +static void track_buffers_creation(GLsizei n, GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + _mesa_HashInsertLocked(ctx->Shared->ShadowBufferObjects, buffers[i], +&DummyBufferObject); + } + + _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects); +} + +static void track_buffers_destruction(GLsizei n, const GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + struct glthread_state *glthread = ctx->GLThread; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + if (buffers[i] == glthread->pixel_pack_buffer_bound) + glthread->pixel_pack_buffer_bound = 0; + + if (buffers[i] == glthread->pixel_unpack_buffer_bound) + glthread->pixel_unpack_buffer_bound = 0; + + /* Technically the buffer can still exist if it is bound to another + * context. It isn't important as next Bin
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
Hello All, I ported PCSX2 to xcb (at least the non-glx part). Crash is gone :) So I can send the v5 with the hash delete fix. However, Mesa might receive crash bug report when glthread is enabled on a random app that doesn't use xcb/XinitThread properly. Maybe it would be better to always enable the XInitThread mode by default on the X11 library. If performance of X11 is critical, it would be better to switch to xcb anyway. Cheers, Gregory On Tue, 18 Apr 2017 15:35:59 +0200 Marek Olšák wrote: > All GL calls that might use libX11 must not be asynchronous within glthread. > > Marek > > On Apr 18, 2017 10:43 AM, "Gregory Hainaut" > wrote: > > Hello Michel, > > As yes, I completely forgot about XInitThreads that must be it. I > don't know how Nvidia manage to solve/force it. Anyway, I will fix my > application. > > Thanks you for the info. > > On 4/18/17, Michel Dänzer wrote: > > On 18/04/17 05:04 PM, gregory hainaut wrote: > >> On Tue, 18 Apr 2017 08:51:24 +0200 > >> gregory hainaut wrote: > >> > >>> On Mon, 17 Apr 2017 11:17:42 +0900 > >>> Michel Dänzer wrote: > >>> > >>>> On 15/04/17 05:08 PM, gregory hainaut wrote: > >>>>> On Sat, 15 Apr 2017 00:50:15 +0200 > >>>>> Dieter Nützel wrote: > >>>>> > >>>>>> Am 14.04.2017 07:53, schrieb gregory hainaut: > >>>>>>> On Fri, 14 Apr 2017 05:20:38 +0200 > >>>>>>> Dieter Nützel wrote: > >>>>>>> > >>>>>>>> Am 14.04.2017 02:06, schrieb Dieter Nützel: > >>>>>>>>> Hello Gregory, > >>>>>>>>> > >>>>>>>>> have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? > >>>>>>>>> It result in crazy numbers and do not 'return' (one core stays @ > >>>>>>>>> 100%). > >>>>>>>> > >>>>>>>> This is related to 'mesa_glthread=true'. > >>>>>>>> If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' > >>>>>>>> exit > >>>>>>>> with ESC as expeted. > >>>>>>>> Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) > >>>>>>>> > >>>>>>>> Hope that helps. > >>>>>>>> > >>>>>>>> Dieter > >>>>>>> > >>>>>>> Hello Dieter, > >>>>>>> > >>>>>>> I tested the demo. There is a pseudo unrelated bug on the exit of > >>>>>>> the > >>>>>>> application. > >>>>>>> > >>>>>>> Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, > >>>>>>> found non-freed data > >>>>>>> > >>>>>>> I will add a call to a _mesa_HashDeleteAll to fix it. > >>>>>>> i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, > >>>>>>> ctx); > >>>>>>> > >>>>>>> Now let's go back to the test behavior. The benchmarks will send 4s > >>>>>>> of > >>>>>>> asynchronous PBO transfer commands. And then will sync gl_thread > >>>>>>> which > >>>>>>> mean the application thread will be blocked until all PBO transfers > >>>>>>> are > >>>>>>> done. Gl_thread is faster to dispatch command so you will need to > >>>>>>> wait > >>>>>>> more before the thread goes back to real life. > >>>>>>> > >>>>>>> On my side, I need to wait around 45 seconds for 6 millions of > >>>>>>> commands. > >>>>>>> Result: 6,440,627 reads (gl thread on + PBO patches) > >>>>>>> Result:274,960 reads (gl thread off) > >>>>>>> > >>>>>>> In your case, "Result: 77,444,412 reads", I hope you're patient. > >>>>>>> I think you must wait at least 10 minutes. > >>>>>> > >>>>>> Now, I was patient... > >>>>>> Tried 2 times but after ~20 minutes I've killed it at first and > >>>>>> attac
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
Hello Michel, As yes, I completely forgot about XInitThreads that must be it. I don't know how Nvidia manage to solve/force it. Anyway, I will fix my application. Thanks you for the info. On 4/18/17, Michel Dänzer wrote: > On 18/04/17 05:04 PM, gregory hainaut wrote: >> On Tue, 18 Apr 2017 08:51:24 +0200 >> gregory hainaut wrote: >> >>> On Mon, 17 Apr 2017 11:17:42 +0900 >>> Michel Dänzer wrote: >>> >>>> On 15/04/17 05:08 PM, gregory hainaut wrote: >>>>> On Sat, 15 Apr 2017 00:50:15 +0200 >>>>> Dieter Nützel wrote: >>>>> >>>>>> Am 14.04.2017 07:53, schrieb gregory hainaut: >>>>>>> On Fri, 14 Apr 2017 05:20:38 +0200 >>>>>>> Dieter Nützel wrote: >>>>>>> >>>>>>>> Am 14.04.2017 02:06, schrieb Dieter Nützel: >>>>>>>>> Hello Gregory, >>>>>>>>> >>>>>>>>> have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? >>>>>>>>> It result in crazy numbers and do not 'return' (one core stays @ >>>>>>>>> 100%). >>>>>>>> >>>>>>>> This is related to 'mesa_glthread=true'. >>>>>>>> If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' >>>>>>>> exit >>>>>>>> with ESC as expeted. >>>>>>>> Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) >>>>>>>> >>>>>>>> Hope that helps. >>>>>>>> >>>>>>>> Dieter >>>>>>> >>>>>>> Hello Dieter, >>>>>>> >>>>>>> I tested the demo. There is a pseudo unrelated bug on the exit of >>>>>>> the >>>>>>> application. >>>>>>> >>>>>>> Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, >>>>>>> found non-freed data >>>>>>> >>>>>>> I will add a call to a _mesa_HashDeleteAll to fix it. >>>>>>> i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, >>>>>>> ctx); >>>>>>> >>>>>>> Now let's go back to the test behavior. The benchmarks will send 4s >>>>>>> of >>>>>>> asynchronous PBO transfer commands. And then will sync gl_thread >>>>>>> which >>>>>>> mean the application thread will be blocked until all PBO transfers >>>>>>> are >>>>>>> done. Gl_thread is faster to dispatch command so you will need to >>>>>>> wait >>>>>>> more before the thread goes back to real life. >>>>>>> >>>>>>> On my side, I need to wait around 45 seconds for 6 millions of >>>>>>> commands. >>>>>>> Result: 6,440,627 reads (gl thread on + PBO patches) >>>>>>> Result:274,960 reads (gl thread off) >>>>>>> >>>>>>> In your case, "Result: 77,444,412 reads", I hope you're patient. >>>>>>> I think you must wait at least 10 minutes. >>>>>> >>>>>> Now, I was patient... >>>>>> Tried 2 times but after ~20 minutes I've killed it at first and >>>>>> attached >>>>>> gdb at it during second run. >>>>>> >>>>>> 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from >>>>>> /lib64/libpthread.so.0 >>>>>> (gdb) bt >>>>>> #0 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from >>>>>> /lib64/libpthread.so.0 >>>>>> #1 0x7fbda5359453 in ?? () from /usr/local/lib/dri/r600_dri.so >>>>>> #2 0x7fbda53661f4 in ?? () from /usr/local/lib/dri/r600_dri.so >>>>>> #3 0x00401e18 in ?? () >>>>>> #4 0x004028c7 in ?? () >>>>>> #5 0x7fbda9925781 in fghRedrawWindow () from >>>>>> /usr/lib64/libglut.so.3 >>>>>> #6 0x7fbda9925c08 in ?? () from /usr/lib64/libglut.so.3 >>>>>> #7 0x7fbda9926cf9 in fgEnumWindows () from >>>>>> /usr/lib64/libglut.so.3 >>>>>> #8 0x7fbda9925ce4 in glutMainLoopEv
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
On Tue, 18 Apr 2017 08:51:24 +0200 gregory hainaut wrote: > On Mon, 17 Apr 2017 11:17:42 +0900 > Michel Dänzer wrote: > > > On 15/04/17 05:08 PM, gregory hainaut wrote: > > > On Sat, 15 Apr 2017 00:50:15 +0200 > > > Dieter Nützel wrote: > > > > > >> Am 14.04.2017 07:53, schrieb gregory hainaut: > > >>> On Fri, 14 Apr 2017 05:20:38 +0200 > > >>> Dieter Nützel wrote: > > >>> > > >>>> Am 14.04.2017 02:06, schrieb Dieter Nützel: > > >>>>> Hello Gregory, > > >>>>> > > >>>>> have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? > > >>>>> It result in crazy numbers and do not 'return' (one core stays @ > > >>>>> 100%). > > >>>> > > >>>> This is related to 'mesa_glthread=true'. > > >>>> If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' > > >>>> exit > > >>>> with ESC as expeted. > > >>>> Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) > > >>>> > > >>>> Hope that helps. > > >>>> > > >>>> Dieter > > >>> > > >>> Hello Dieter, > > >>> > > >>> I tested the demo. There is a pseudo unrelated bug on the exit of the > > >>> application. > > >>> > > >>> Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, > > >>> found non-freed data > > >>> > > >>> I will add a call to a _mesa_HashDeleteAll to fix it. > > >>> i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, ctx); > > >>> > > >>> Now let's go back to the test behavior. The benchmarks will send 4s of > > >>> asynchronous PBO transfer commands. And then will sync gl_thread which > > >>> mean the application thread will be blocked until all PBO transfers are > > >>> done. Gl_thread is faster to dispatch command so you will need to wait > > >>> more before the thread goes back to real life. > > >>> > > >>> On my side, I need to wait around 45 seconds for 6 millions of > > >>> commands. > > >>> Result: 6,440,627 reads (gl thread on + PBO patches) > > >>> Result:274,960 reads (gl thread off) > > >>> > > >>> In your case, "Result: 77,444,412 reads", I hope you're patient. > > >>> I think you must wait at least 10 minutes. > > >> > > >> Now, I was patient... > > >> Tried 2 times but after ~20 minutes I've killed it at first and attached > > >> gdb at it during second run. > > >> > > >> 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from > > >> /lib64/libpthread.so.0 > > >> (gdb) bt > > >> #0 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from > > >> /lib64/libpthread.so.0 > > >> #1 0x7fbda5359453 in ?? () from /usr/local/lib/dri/r600_dri.so > > >> #2 0x7fbda53661f4 in ?? () from /usr/local/lib/dri/r600_dri.so > > >> #3 0x00401e18 in ?? () > > >> #4 0x004028c7 in ?? () > > >> #5 0x7fbda9925781 in fghRedrawWindow () from > > >> /usr/lib64/libglut.so.3 > > >> #6 0x7fbda9925c08 in ?? () from /usr/lib64/libglut.so.3 > > >> #7 0x7fbda9926cf9 in fgEnumWindows () from /usr/lib64/libglut.so.3 > > >> #8 0x7fbda9925ce4 in glutMainLoopEvent () from > > >> /usr/lib64/libglut.so.3 > > >> #9 0x7fbda9925d85 in glutMainLoop () from /usr/lib64/libglut.so.3 > > >> #10 0x004019fc in ?? () > > >> #11 0x7fbda957e541 in __libc_start_main () from /lib64/libc.so.6 > > >> #12 0x00401afa in ?? () > > >> > > >> Should I do more or not worth it? > > >> > > >> Dieter > > > > > > Hello Dieter, > > > > > > To be honest, I don't konw how much time you need to wait. 77 millions of > > > PBO transfer is quite huge. It depends on CPU/Memory/PCIe/VRAM/GPU speed. > > > > > > Hum based on the image size (194*188*4), you need to approximately > > > transfer > > > 10522 GB of data from your GPU... Which is likely around 20 minutes if > > > PCIe run at full speed. Honestly
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
On Mon, 17 Apr 2017 11:17:42 +0900 Michel Dänzer wrote: > On 15/04/17 05:08 PM, gregory hainaut wrote: > > On Sat, 15 Apr 2017 00:50:15 +0200 > > Dieter Nützel wrote: > > > >> Am 14.04.2017 07:53, schrieb gregory hainaut: > >>> On Fri, 14 Apr 2017 05:20:38 +0200 > >>> Dieter Nützel wrote: > >>> > >>>> Am 14.04.2017 02:06, schrieb Dieter Nützel: > >>>>> Hello Gregory, > >>>>> > >>>>> have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? > >>>>> It result in crazy numbers and do not 'return' (one core stays @ 100%). > >>>> > >>>> This is related to 'mesa_glthread=true'. > >>>> If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' > >>>> exit > >>>> with ESC as expeted. > >>>> Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) > >>>> > >>>> Hope that helps. > >>>> > >>>> Dieter > >>> > >>> Hello Dieter, > >>> > >>> I tested the demo. There is a pseudo unrelated bug on the exit of the > >>> application. > >>> > >>> Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, > >>> found non-freed data > >>> > >>> I will add a call to a _mesa_HashDeleteAll to fix it. > >>> i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, ctx); > >>> > >>> Now let's go back to the test behavior. The benchmarks will send 4s of > >>> asynchronous PBO transfer commands. And then will sync gl_thread which > >>> mean the application thread will be blocked until all PBO transfers are > >>> done. Gl_thread is faster to dispatch command so you will need to wait > >>> more before the thread goes back to real life. > >>> > >>> On my side, I need to wait around 45 seconds for 6 millions of > >>> commands. > >>> Result: 6,440,627 reads (gl thread on + PBO patches) > >>> Result:274,960 reads (gl thread off) > >>> > >>> In your case, "Result: 77,444,412 reads", I hope you're patient. > >>> I think you must wait at least 10 minutes. > >> > >> Now, I was patient... > >> Tried 2 times but after ~20 minutes I've killed it at first and attached > >> gdb at it during second run. > >> > >> 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from > >> /lib64/libpthread.so.0 > >> (gdb) bt > >> #0 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from > >> /lib64/libpthread.so.0 > >> #1 0x7fbda5359453 in ?? () from /usr/local/lib/dri/r600_dri.so > >> #2 0x7fbda53661f4 in ?? () from /usr/local/lib/dri/r600_dri.so > >> #3 0x00401e18 in ?? () > >> #4 0x004028c7 in ?? () > >> #5 0x7fbda9925781 in fghRedrawWindow () from > >> /usr/lib64/libglut.so.3 > >> #6 0x7fbda9925c08 in ?? () from /usr/lib64/libglut.so.3 > >> #7 0x7fbda9926cf9 in fgEnumWindows () from /usr/lib64/libglut.so.3 > >> #8 0x7fbda9925ce4 in glutMainLoopEvent () from > >> /usr/lib64/libglut.so.3 > >> #9 0x7fbda9925d85 in glutMainLoop () from /usr/lib64/libglut.so.3 > >> #10 0x004019fc in ?? () > >> #11 0x7fbda957e541 in __libc_start_main () from /lib64/libc.so.6 > >> #12 0x00401afa in ?? () > >> > >> Should I do more or not worth it? > >> > >> Dieter > > > > Hello Dieter, > > > > To be honest, I don't konw how much time you need to wait. 77 millions of > > PBO transfer is quite huge. It depends on CPU/Memory/PCIe/VRAM/GPU speed. > > > > Hum based on the image size (194*188*4), you need to approximately transfer > > 10522 GB of data from your GPU... Which is likely around 20 minutes if > > PCIe run at full speed. Honestly I will let the application in background > > for a couple of hours. > > Basically, the application needs to be fixed not to emit an unlimited > number of PBO transfers without doing anything which requires > synchronizing to the transfers. > > Hello Michel, Timothy, Marek Yes, I think it should limit the number of transfer to a million. And also uses fence to measure the PBO transfer. However, I have found others crashes on PCSX2 with those patches. It seems related to synchronization issue with GLX/DRI/X11. Th
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
On Sat, 15 Apr 2017 00:50:15 +0200 Dieter Nützel wrote: > Am 14.04.2017 07:53, schrieb gregory hainaut: > > On Fri, 14 Apr 2017 05:20:38 +0200 > > Dieter Nützel wrote: > > > >> Am 14.04.2017 02:06, schrieb Dieter Nützel: > >> > Hello Gregory, > >> > > >> > have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? > >> > It result in crazy numbers and do not 'return' (one core stays @ 100%). > >> > >> This is related to 'mesa_glthread=true'. > >> If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' > >> exit > >> with ESC as expeted. > >> Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) > >> > >> Hope that helps. > >> > >> Dieter > > > > Hello Dieter, > > > > I tested the demo. There is a pseudo unrelated bug on the exit of the > > application. > > > > Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, > > found non-freed data > > > > I will add a call to a _mesa_HashDeleteAll to fix it. > > i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, ctx); > > > > Now let's go back to the test behavior. The benchmarks will send 4s of > > asynchronous PBO transfer commands. And then will sync gl_thread which > > mean the application thread will be blocked until all PBO transfers are > > done. Gl_thread is faster to dispatch command so you will need to wait > > more before the thread goes back to real life. > > > > On my side, I need to wait around 45 seconds for 6 millions of > > commands. > > Result: 6,440,627 reads (gl thread on + PBO patches) > > Result:274,960 reads (gl thread off) > > > > In your case, "Result: 77,444,412 reads", I hope you're patient. > > I think you must wait at least 10 minutes. > > Now, I was patient... > Tried 2 times but after ~20 minutes I've killed it at first and attached > gdb at it during second run. > > 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > (gdb) bt > #0 0x7fbda686e9a6 in pthread_cond_wait@@GLIBC_2.3.2 () from > /lib64/libpthread.so.0 > #1 0x7fbda5359453 in ?? () from /usr/local/lib/dri/r600_dri.so > #2 0x7fbda53661f4 in ?? () from /usr/local/lib/dri/r600_dri.so > #3 0x00401e18 in ?? () > #4 0x004028c7 in ?? () > #5 0x7fbda9925781 in fghRedrawWindow () from > /usr/lib64/libglut.so.3 > #6 0x7fbda9925c08 in ?? () from /usr/lib64/libglut.so.3 > #7 0x7fbda9926cf9 in fgEnumWindows () from /usr/lib64/libglut.so.3 > #8 0x7fbda9925ce4 in glutMainLoopEvent () from > /usr/lib64/libglut.so.3 > #9 0x7fbda9925d85 in glutMainLoop () from /usr/lib64/libglut.so.3 > #10 0x004019fc in ?? () > #11 0x7fbda957e541 in __libc_start_main () from /lib64/libc.so.6 > #12 0x00401afa in ?? () > > Should I do more or not worth it? > > Dieter Hello Dieter, To be honest, I don't konw how much time you need to wait. 77 millions of PBO transfer is quite huge. It depends on CPU/Memory/PCIe/VRAM/GPU speed. Hum based on the image size (194*188*4), you need to approximately transfer 10522 GB of data from your GPU... Which is likely around 20 minutes if PCIe run at full speed. Honestly I will let the application in background for a couple of hours. Backtrace without symbol is hard to read. But I'm pretty sure, it is waiting on the glError call. Cheers, Gregory > >> > mesa-demos/tests> ./pbo > >> > ATTENTION: default value of option mesa_glthread overridden by > >> > environment. > >> > GL_VERSION = 4.1 Mesa 17.1.0-devel (git-7c8fe31e1c) > >> > GL_RENDERER = Gallium 0.4 on AMD TURKS (DRM 2.49.0 / > >> > 4.11.0-rc6-1.g5a51416-default, LLVM 5.0.0) > >> > Loaded 194 by 188 image > >> > Converting RGB image to RGBA > >> > Benchmarking... > >> > Result: 7712 reads in 4.00 seconds = -383971576.00 > >> > pixels/sec > >> > > >> > top - 02:04:42 up 10:05, 4 users, load average: 1,03, 0,77, 0,71 > >> > Tasks: 265 total, 1 running, 264 sleeping, 0 stopped, 0 zombie > >> > %Cpu0 : 1,3 us, 0,3 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > >> > 0,0 st > >> > %Cpu1 : 1,3 us, 0,3 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > >> > 0,0 st > >> > %Cpu2 : 1,7 us, 0,0 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > >> > 0,0 st > >>
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
On Fri, 14 Apr 2017 07:53:06 +0200 gregory hainaut wrote: > On Fri, 14 Apr 2017 05:20:38 +0200 > Dieter Nützel wrote: > > > Am 14.04.2017 02:06, schrieb Dieter Nützel: > > > Hello Gregory, > > > > > > have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? > > > It result in crazy numbers and do not 'return' (one core stays @ 100%). > > > > This is related to 'mesa_glthread=true'. > > If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' exit > > with ESC as expeted. > > Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) > > > > Hope that helps. > > > > Dieter > > Hello Dieter, > > I tested the demo. There is a pseudo unrelated bug on the exit of the > application. > > Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, found > non-freed data > > I will add a call to a _mesa_HashDeleteAll to fix it. > i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, ctx); > > Now let's go back to the test behavior. The benchmarks will send 4s of > asynchronous PBO transfer commands. And then will sync gl_thread which > mean the application thread will be blocked until all PBO transfers are > done. Gl_thread is faster to dispatch command so you will need to wait > more before the thread goes back to real life. > > On my side, I need to wait around 45 seconds for 6 millions of commands. > Result: 6,440,627 reads (gl thread on + PBO patches) > Result:274,960 reads (gl thread off) > > In your case, "Result: 77,444,412 reads", I hope you're patient. > I think you must wait at least 10 minutes. > > Best regards, > Gregory And to complete my answer on the crazy number. Yes there is an integer overflow when reads is too big. It would be better to promote it to double. pixelsPerSecond = reads * ImgWidth * ImgHeight / seconds; That being said, pixelsPerSecond doesn't have any physical meaning. reads * ImgWidth * ImgHeight is the number of pixels that *will* be transfered not the number of pixels that *was* transfered. As you have notice the transfer will take more than the value of seconds (which is 4). > > > mesa-demos/tests> ./pbo > > > ATTENTION: default value of option mesa_glthread overridden by > > > environment. > > > GL_VERSION = 4.1 Mesa 17.1.0-devel (git-7c8fe31e1c) > > > GL_RENDERER = Gallium 0.4 on AMD TURKS (DRM 2.49.0 / > > > 4.11.0-rc6-1.g5a51416-default, LLVM 5.0.0) > > > Loaded 194 by 188 image > > > Converting RGB image to RGBA > > > Benchmarking... > > > Result: 7712 reads in 4.00 seconds = -383971576.00 > > > pixels/sec > > > > > > top - 02:04:42 up 10:05, 4 users, load average: 1,03, 0,77, 0,71 > > > Tasks: 265 total, 1 running, 264 sleeping, 0 stopped, 0 zombie > > > %Cpu0 : 1,3 us, 0,3 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu1 : 1,3 us, 0,3 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu2 : 1,7 us, 0,0 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu3 : 2,3 us, 0,3 sy, 0,0 ni, 97,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu4 : 1,7 us, 0,3 sy, 0,0 ni, 98,0 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu5 : 98,3 us, 1,7 sy, 0,0 ni, 0,0 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu6 : 2,0 us, 0,3 sy, 0,0 ni, 97,7 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > %Cpu7 : 1,7 us, 0,0 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > > 0,0 st > > > KiB Mem : 24680300 total, 8155356 free, 5751864 used, 10773080 > > > buff/cache > > > KiB Swap:0 total,0 free,0 used. 18437888 avail > > > Mem > > > > > > PID USER PR NIVIRTRESSHR S %CPU %MEM TIME+ > > > COMMAND > > > 19380 dieter20 0 3259764 2,911g 22472 S 100,3 12,37 2:28.48 > > > pbo > > > 27937 dieter20 0 4029572 570236 166116 S 5,980 2,310 9:45.53 > > > konqueror > > > 13432 dieter20 0 1922820 269892 129152 S 5,648 1,094 4:33.80 > > > Web Content > > > > > > Other than that: > > > > > > For the series: > > > > > > Tested-by: Dieter Nützel > > > r600g, Turks XT (6670) > > > > > > Dieter > > > > > > Am 13.04.2017 19:32, schrieb Gregory Hainaut: > > >> H
Re: [Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
On Fri, 14 Apr 2017 05:20:38 +0200 Dieter Nützel wrote: > Am 14.04.2017 02:06, schrieb Dieter Nützel: > > Hello Gregory, > > > > have you tested this with Mesa-demos/tests/pbo 'b' (benchmark)? > > It result in crazy numbers and do not 'return' (one core stays @ 100%). > > This is related to 'mesa_glthread=true'. > If I disable (unset) it, all is fine after 'b' benchmark and 'pbo' exit > with ESC as expeted. > Crazy numbers stay, maybe counter overrun due to BIG numbers? ;-) > > Hope that helps. > > Dieter Hello Dieter, I tested the demo. There is a pseudo unrelated bug on the exit of the application. Mesa 17.1.0-devel implementation error: In _mesa_DeleteHashTable, found non-freed data I will add a call to a _mesa_HashDeleteAll to fix it. i.e. _mesa_HashDeleteAll(shared->ShadowBufferObjects, dummy_cb, ctx); Now let's go back to the test behavior. The benchmarks will send 4s of asynchronous PBO transfer commands. And then will sync gl_thread which mean the application thread will be blocked until all PBO transfers are done. Gl_thread is faster to dispatch command so you will need to wait more before the thread goes back to real life. On my side, I need to wait around 45 seconds for 6 millions of commands. Result: 6,440,627 reads (gl thread on + PBO patches) Result:274,960 reads (gl thread off) In your case, "Result: 77,444,412 reads", I hope you're patient. I think you must wait at least 10 minutes. Best regards, Gregory > > mesa-demos/tests> ./pbo > > ATTENTION: default value of option mesa_glthread overridden by > > environment. > > GL_VERSION = 4.1 Mesa 17.1.0-devel (git-7c8fe31e1c) > > GL_RENDERER = Gallium 0.4 on AMD TURKS (DRM 2.49.0 / > > 4.11.0-rc6-1.g5a51416-default, LLVM 5.0.0) > > Loaded 194 by 188 image > > Converting RGB image to RGBA > > Benchmarking... > > Result: 7712 reads in 4.00 seconds = -383971576.00 > > pixels/sec > > > > top - 02:04:42 up 10:05, 4 users, load average: 1,03, 0,77, 0,71 > > Tasks: 265 total, 1 running, 264 sleeping, 0 stopped, 0 zombie > > %Cpu0 : 1,3 us, 0,3 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu1 : 1,3 us, 0,3 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu2 : 1,7 us, 0,0 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu3 : 2,3 us, 0,3 sy, 0,0 ni, 97,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu4 : 1,7 us, 0,3 sy, 0,0 ni, 98,0 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu5 : 98,3 us, 1,7 sy, 0,0 ni, 0,0 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu6 : 2,0 us, 0,3 sy, 0,0 ni, 97,7 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > %Cpu7 : 1,7 us, 0,0 sy, 0,0 ni, 98,3 id, 0,0 wa, 0,0 hi, 0,0 si, > > 0,0 st > > KiB Mem : 24680300 total, 8155356 free, 5751864 used, 10773080 > > buff/cache > > KiB Swap:0 total,0 free,0 used. 18437888 avail > > Mem > > > > PID USER PR NIVIRTRESSHR S %CPU %MEM TIME+ > > COMMAND > > 19380 dieter20 0 3259764 2,911g 22472 S 100,3 12,37 2:28.48 > > pbo > > 27937 dieter20 0 4029572 570236 166116 S 5,980 2,310 9:45.53 > > konqueror > > 13432 dieter20 0 1922820 269892 129152 S 5,648 1,094 4:33.80 > > Web Content > > > > Other than that: > > > > For the series: > > > > Tested-by: Dieter Nützel > > r600g, Turks XT (6670) > > > > Dieter > > > > Am 13.04.2017 19:32, schrieb Gregory Hainaut: > >> Hello, > >> > >> Please find a new version to handle invalid buffer handles. > >> > >> Allow to handle this kind of case: > >>genBuffer(&pbo); > >>BindBuffer(pbo) > >>DeleteBuffer(pbo); > >>BindBuffer(rand_pbo) > >>TexSubImage2D(user_memory_pointer); // Data transfer will be > >> synchronous > >> > >> There are various subtely to handle multi threaded shared context. In > >> order to > >> keep the code sane, I've considered a buffer invalid when it is > >> deleted by a > >> context even it is still bound to others contexts. It will force a > >> synchronous > >> transfer which is always safe. > >> > >> An example could be > >>Ctx A: glGenBuffers(1, &pbo); > >>Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); > >>Ctx B: glDeleteBuffers(1, &pbo); > >>Ctx A: glTexSubIm
[Mesa-dev] [PATCH v4 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers to be valid. v4: update comments based on Nicolai review Signed-off-by: Gregory Hainaut --- src/mesa/main/marshal.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 4037b79..1167271 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -319,7 +319,20 @@ struct marshal_cmd_BindBufferBase GLuint buffer; }; -/** Tracks the current bindings for the vertex array and index array buffers. +/** + * Check that buffer is a valid buffer handle + * Always return false for ID 0. + */ +static bool +is_bufferobj(struct gl_context *ctx, GLuint buffer) +{ + if (buffer == 0) + return false; + else + return _mesa_HashLookup(ctx->Shared->ShadowBufferObjects, buffer) != NULL; +} + +/** Tracks the current bindings of GL buffer targets * * This is part of what we need to enable glthread on compat-GL contexts that * happen to use VBOs, without also supporting the full tracking of VBO vs @@ -341,9 +354,14 @@ struct marshal_cmd_BindBufferBase * instead of updating the binding. However, compat GL has the ridiculous * feature that if you pass a bad name, it just gens a buffer object for you, * so we escape without having to know if things are valid or not. + * + * Pixel buffers are tracked to decide whether pixel transfer goes to a user + * pointer (must be synchronous) or a GL buffer (can be asynchronous). Unlike + * for VBOs, we do need accurate tracking, since user pointers can be used in + * GL core contexts. */ static void -track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) +track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer) { struct glthread_state *glthread = ctx->GLThread; @@ -358,6 +376,18 @@ track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) */ glthread->element_array_is_vbo = (buffer != 0); break; + case GL_PIXEL_UNPACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_unpack_buffer_bound = buffer; + else + glthread->pixel_unpack_buffer_bound = 0; + break; + case GL_PIXEL_PACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_pack_buffer_bound = buffer; + else + glthread->pixel_pack_buffer_bound = 0; + break; } } @@ -389,7 +419,7 @@ _mesa_marshal_BindBuffer(GLenum target, GLuint buffer) struct marshal_cmd_BindBuffer *cmd; debug_print_marshal("BindBuffer"); - track_vbo_binding(ctx, target, buffer); + track_buffers_binding(ctx, target, buffer); if (cmd_size <= MARSHAL_MAX_CMD_SIZE) { cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer, -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v4 1/3] mesa/glthread: track buffer creation/destruction
It would be used in next commit to allow asynchronous PBO transfer. The tracking saves the buffer name into a hash. Saving pointer will be more complex as the buffer is created in BindBuffer due to IsBuffer insanity. Perf wise DeleteBuffers is now synchronous for robustness. Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 2 +- src/mapi/glapi/gen/gl_API.xml | 4 +- src/mesa/main/glthread.h | 10 +++ src/mesa/main/marshal.c| 113 + src/mesa/main/marshal.h| 24 ++ src/mesa/main/mtypes.h | 5 ++ src/mesa/main/shared.c | 4 + 7 files changed, 159 insertions(+), 3 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 43841bb..566f157 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -49,7 +49,7 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index c0ee2f2..ad841a1 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -5049,13 +5049,13 @@ - + - + diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h index 50c1db2..494e942 100644 --- a/src/mesa/main/glthread.h +++ b/src/mesa/main/glthread.h @@ -92,6 +92,16 @@ struct glthread_state * buffer) binding is in a VBO. */ bool element_array_is_vbo; + + /** +* Tracks on the main thread side the bound unpack pixel buffer +*/ + GLint pixel_unpack_buffer_bound; + + /** +* Tracks on the main thread side the bound pack pixel buffer +*/ + GLint pixel_pack_buffer_bound; }; /** diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index ae4efb5..4037b79 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -32,6 +32,7 @@ #include "marshal.h" #include "dispatch.h" #include "marshal_generated.h" +#include "hash.h" #ifdef HAVE_PTHREAD @@ -196,6 +197,118 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, free(length_tmp); } +/** + * Used as a placeholder for track_buffers_creation/track_buffers_destruction + * so we know if buffer really exists in track_buffers_binding. + */ +static struct gl_buffer_object DummyBufferObject; + +static void track_buffers_creation(GLsizei n, GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + _mesa_HashInsertLocked(ctx->Shared->ShadowBufferObjects, buffers[i], +&DummyBufferObject); + } + + _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects); +} + +static void track_buffers_destruction(GLsizei n, const GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + struct glthread_state *glthread = ctx->GLThread; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + if (buffers[i] == glthread->pixel_pack_buffer_bound) + glthread->pixel_pack_buffer_bound = 0; + + if (buffers[i] == glthread->pixel_unpack_buffer_bound) + glthread->pixel_unpack_buffer_bound = 0; + + /* Technically the buffer can still exist if it is bound to another + * context. It isn't important as next BindBuffer will consider + * the buffer invalid and following PBO transfer will be synchronous + * which is always safe. + */ + _mesa_HashRemoveLocked(ctx->Shared->ShadowBufferObjects, buffers[i]); + } + + _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects); +} + + +/* CreateBuffers: custom marshal to track buffers creation */ +void GLAPIENTRY +_mesa_marshal_CreateBuffers(GLsizei n, GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + _mesa_glthread_finish(ctx); + debug_print_sync("CreateBuffers"); + CALL_CreateBuffers(ctx->CurrentServerDispatch, (n, buffers)); + + track_buffers_creation(n, buffers); +} + +void +_mesa_unmarshal_CreateBuffers(struct gl_context *ctx, + const struct marshal_cmd_CreateBuffers *cmd) +{ + assert(0); +} + +/* GenBuffers: custom marshal to track buffers creation */ +void GLAPIENTRY +_mesa_marshal_GenBuffers(GLsizei n, GLuint * buffer) +{ + GET_CURRENT_CONTEXT(ctx); + _mesa_glthread_finish(ctx); + debug_print_sync("GenBuffers"); + CALL_GenBuffers(ctx->CurrentServerDispatch, (n, buffer)); + + track_buffers_creation(n, buffer); +} + +void +_mesa_unmarshal_GenBuffers(struct gl_context *ctx, +
[Mesa-dev] [PATCH v4 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported v4: add Nicolai comment to explain why ppbo isn't impacted by the variable_params issue Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 23 - src/mapi/glapi/gen/marshal_XML.py | 21 ++- 6 files changed, 67 insertions(+), 33 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 566f157..d842ebd 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -374,7 +374,7 @@ - + @@ -384,7 +384,7 @@ - + @@ -396,7 +396,7 @@ - + @@ -410,7 +410,7 @@ - + @@ -420,7 +420,7 @@ - + @@ -432,7 +432,7 @@ - + @@ -523,7 +523,7 @@ - + @@ -532,7 +532,7 @@ - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0..6e1ac09 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -82,7 +82,7 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250..447b03a 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -122,14 +122,16 @@ param: offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index ad841a1..f436a95 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -2149,7 +2149,7 @@ - + @@ -2161,7 +2161,7 @@ - + @@ -2640,7 +2640,7 @@ - + @@ -3293,7 +3293,7 @@ - + @@ -3305,7 +3305,7 @@ - + @@ -4005,7 +4005,7 @@ - + @@ -4019,7 +4019,7 @@ - + @@ -4501,7 +4501,7 @@ - + @@ -4514,7 +4514,7 @@ - + @@ -4526,7 +4526,7 @@ - + @@ -4537,7 +4537,7 @@ - + @@ -4552,7 +4552,7 @@
[Mesa-dev] [PATCH v4 0/3] asynchronous pbo transfer with glthread
Hello, Please find a new version to handle invalid buffer handles. Allow to handle this kind of case: genBuffer(&pbo); BindBuffer(pbo) DeleteBuffer(pbo); BindBuffer(rand_pbo) TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous There are various subtely to handle multi threaded shared context. In order to keep the code sane, I've considered a buffer invalid when it is deleted by a context even it is still bound to others contexts. It will force a synchronous transfer which is always safe. An example could be Ctx A: glGenBuffers(1, &pbo); Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); Ctx B: glDeleteBuffers(1, &pbo); Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ be asynchronous (because the PBO that was generated first is still bound!) V3: I mixed up the number so I jumped right away to v4... V4: improve commments based on Nicolai feedback Best regards, Gregory Hainaut (3): mesa/glthread: track buffer creation/destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 +- src/mapi/glapi/gen/gl_API.xml | 32 +++--- src/mapi/glapi/gen/gl_marshal.py | 23 +++- src/mapi/glapi/gen/marshal_XML.py | 21 +++- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c| 149 - src/mesa/main/marshal.h| 24 src/mesa/main/mtypes.h | 5 + src/mesa/main/shared.c | 4 + 11 files changed, 259 insertions(+), 39 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/3] mapi/glthread: generate asynchronous code for PBO transfer
On Thu, 13 Apr 2017 18:31:06 +0200 Nicolai Hähnle wrote: > On 05.04.2017 12:30, Gregory Hainaut wrote: > > # Classify fixed and variable parameters. > > self.fixed_params = [] > > self.variable_params = [] > > for p in self.parameters: > > if p.is_padding: > > continue > > -if p.is_variable_length(): > > +if self.marshal == "upbo" and p.is_pointer(): > > +# Pixel buffer transfer API is tricky. By default > > it contains > > +# a pointer to user memory so a variable length > > parameter. > > +# When a pixel buffer is bound, the pointer > > becomes an offset. > > +# > > +# Non-PBO transfer will be synchronous so > > parameter type isn't > > +# important. PBO transfer will be asynchronous so > > the parameter > > +# must be marked as fixed > > +self.fixed_params.append(p) > > > > > >> If this is needed for upbo, shouldn't it also be needed for ppbo? > >> > >> Cheers, > >> Nicolai > > > > Hello Nicolai, > > > > It isn't symmetrical. In case of UPBO data ought to be copied from app > > thread to gl thread. You can see variable_length parameter as input > > pointer. Variable length will generate the memcpy code. > > > > However PPBO will copy from GPU to user pointer. There is no data > > associated with the pointer so the pointer isn't "used" by glthread, > > only transferred to GL. > > > > I think the code would love an extra comment. > > Okay, I get it now. But the comment could maybe use some re-wording, > especially because this whole issue only applies to some functions: > those where the marshaling code theoretically has a size, such as with > glCompressedTexImage2D. It would be good to clarify that. Yes. I added this comment. Though I'm not sure it enough. + # variable_params are meaningful when pointers are associated + # with a payload hence it only impacts upbo but not ppbo. Cheers, Gregory > > Thanks, > Nicolai ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 0/3] asynchronous pbo transfer with glthread
On Wed, 5 Apr 2017 12:52:03 +0200 Gregory Hainaut wrote: > > Still, I believe there is the following bug in the series: > > > glGenBuffers(1, &pbo); > > glBindBuffer(..., pbo); > > glDeleteBuffers(1, &pbo); > > glTexSubImage2D(...); // will be asynchronous, but refers to user memory > > because the pbo was implicitly unbound > > I unbound the buffer when it is deleted in the > track_buffers_destruction function. See code chunk below. So I think > it must work. Or did I miss something ? > > > + if (buffers[i] == glthread->pixel_pack_buffer_bound) > > + glthread->pixel_pack_buffer_bound = 0; > > + > > + if (buffers[i] == glthread->pixel_unpack_buffer_bound) > > + glthread->pixel_unpack_buffer_bound = 0; > Hello Nicolai, Do we agree that above pattern will work with my patches ? > > > Ctx A: glGenBuffers(1, &pbo); // assume that this gets the same buffer name > You can't have the same name because name is still bound in context A. Actually I'm wrong here. glDeleteBuffers makes the name available again. Which lead to interesting question if the buffer is bound again after the name is reused by a glGenBuffers call. Anyway it isn't important. Chapter 5.1.3 of 4.5 core spec "Since the name is marked unused, binding the name will create a new object with the same name, and attaching the name will generate an error." Cheers, Gregory > So maybe you mean something instead for your example ? > > > Ctx A: glGenBuffers(1, &pbo); > > Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); > > Ctx B: glDeleteBuffers(1, &pbo); > ... > > Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ > > be asynchronous (because the PBO that was generated first is still bound!) > > And yes I confirm, it isn't optimal. I did it on purpose because it is > much safer/easier and as you said it is a crazy case. > > > Cheers, > Gregory > > On 4/5/17, Nicolai Hähnle wrote: > > On 01.04.2017 11:42, Gregory Hainaut wrote: > >> Hello, > >> > >> Please find a new version to handle invalid buffer handles. > >> > >> Allow to handle this kind of case: > >>genBuffer(&pbo); > >>DeleteBuffer(pbo); > >>BindBuffer(pbo) > >>TexSubImage2D(user_memory_pointer); // Data transfer will be > >> synchronous > >> > >> There are various subtely to handle multi threaded shared context. In > >> order to > >> keep the code sane, I've considered a buffer invalid when it is deleted by > >> a > >> context even it is still bound to others contexts. It will force a > >> synchronous > >> transfer which is always safe. > > > > Thanks! I think you're right that tracking pointers is not actually > > needed as long as Create/Delete are synchronous. It's less accurate, but > > the inaccuracy doesn't necessarily lead to correctness problems. > > > > Still, I believe there is the following bug in the series: > > > > glGenBuffers(1, &pbo); > > glBindBuffer(..., pbo); > > glDeleteBuffers(1, &pbo); > > glTexSubImage2D(...); // will be asynchronous, but refers to user memory > > because the pbo was implicitly unbound > > > > Once you fix this bug by updating the current context's bindings in > > glDeleteBuffers, you'll have the following multi-context situation, > > which is sub-optimal but not a bug -- and applications which do that are > > crazy anyway, so let's not worry about them. I'm just mentioning it for > > completeness: > > > > Ctx A: glGenBuffers(1, &pbo); > > Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); > > Ctx B: glDeleteBuffers(1, &pbo); > > Ctx A: glGenBuffers(1, &pbo); // assume that this gets the same buffer name > > Ctx A: glDeleteBuffers(1, &pbo); > > Ctx A: glTexSubImage2D(...); // will be synchronous, even though it > > _could_ be asynchronous (because the PBO that was generated first is > > still bound!) > > > > Cheers, > > Nicolai > > > > > >> > >> Gregory Hainaut (3): > >> mesa/glthread: track buffer creation/destruction > >> mesa/glthread: add tracking of PBO binding > >> mapi/glthread: generate asynchronous code for PBO transfer > >> > >> src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- > >> src/mapi/glapi/gen/ARB_robustness.xml | 2 +- > >> src/mapi/glapi/gen/gl_API.dtd | 10 +- > >> src/mapi/glap
[Mesa-dev] [PATCH v4] glsl/blob: avoid NULL ptr in prog parameter name
Context: _mesa_add_parameter is sometimes[0] called with a NULL name as a mean of an unnamed parameter. Allowing NULL pointer as a name means that it must be NULL checked each access. So far it isn't always[1] true. Parameter name is only used for debug purpose (printf) and to lookup the index/location of the program by the application. Conclusion, there is no valid reason to use a NULL pointer instead of an empty string. So it was decided to use an empty string which avoid all issues related to NULL pointer [0]: texture gather offsets glsl opcode and st_init_atifs_prog [1]: at least shader cache, st_nir_lookup_parameter_index and some printfs Issue found by piglit 'texturegatheroffsets' tests on Nouveau v4: new patch based on Nicolai/Timothy/ilia discussion Signed-off-by: Gregory Hainaut --- src/mesa/program/prog_parameter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/program/prog_parameter.c b/src/mesa/program/prog_parameter.c index c294b00..6689c71 100644 --- a/src/mesa/program/prog_parameter.c +++ b/src/mesa/program/prog_parameter.c @@ -258,7 +258,7 @@ _mesa_add_parameter(struct gl_program_parameter_list *paramList, for (i = 0; i < sz4; i++) { struct gl_program_parameter *p = paramList->Parameters + oldNum + i; - p->Name = name ? strdup(name) : NULL; + p->Name = strdup(name ? name : ""); p->Type = type; p->Size = size; p->DataType = datatype; -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string
On Thu, 6 Apr 2017 00:21:19 +0200 gregory hainaut wrote: > On Wed, 5 Apr 2017 14:22:00 -0400 > Ilia Mirkin wrote: > > > On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut > > wrote: > > > Context: > > > Nouveau uses NULL strings for unnamed parameter of texture gather > > > offsets opcode. > > > > To be clear, this isn't a "nouveau" thing, as it is well downstream of > > all this GLSL stuff. And FWIW llvmpipe/softpipe also support the > > 4-offset TG4 (I think). It's a GLSL IR thing. If it needs changing, > > you can change how GLSL IR represents this stuff, instead of > > necessarily supporting this wart. > > > > Unfortunately I don't understand the problem clearly enough to tell > > what's going on and what the proper solution is. > > > > Ilia, > > Yes you're right it isn't limited to Nouveau. And actually it could appear > in future glsl code. Actually there is another call to _mesa_add_parameter > in st_init_atifs_prog from state_tracker/st_atifs_to_tgsi.c > > If I try to do a summary. > > The problem is "what is the expected name of an unnamed parameter ?" > The 2 possibles answer are NULL or emptry (AKA ""). > > Nicolai raise the valid point that code could handle NULL and empty > differently. > > I tried to do various grep without success. I don't think there is any > difference > between NULL and "" but easy to say hard to demonstrate. > > However I don't think that allowing NULL pointer in name is a good idea. > > For example st_nir_lookup_parameter_index (st_glsl_to_nir.cpp) use > this kind of code. I'm afraid that strncmp with a NULL pointer is an > undefined behavior. > > for (unsigned i = 0; i < params->NumParameters; i++) { > struct gl_program_parameter *p = ¶ms->Parameters[i]; > if ((strncmp(p->Name, name, namelen) == 0) && > > > The YOLO solution is appealing. We change _mesa_add_parameter NULL name > parameter to "". Then we run piglit. > > > Best regards, > Gregory Hello All, I commented the "Name" field in the struct to see the gcc error message. I did also some grep to be sure I didn't miss anything. I didn't find anything that differentiate a NULL pointer from an empty string. Actually "Name" is used inside printf and to lookup the index/location. Note: some code doesn't check for NULL pointer. copy_indirect_accessed_array (prog_parameter_layout.c) will use NULL on purpose to avoid a double-free (due to pointer memcpy). But it is only a temporary struct that will be freed by the caller (_mesa_layout_parameters) Conclusion, it would be enough to add a parameter with p->Name = strdup(name ? name : ""); Or we can keep the *string_optional function. It is up to you. Tell me what solution do you prefer and I will redo the patch accordingly. Best regards, Gregory > > > > > > Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau > > > > > > v3: Redo patch based on Nicolai/Timothy feedback > > > Create dedicated blob_write_string_optional/blob_read_string_optional to > > > handle null pointer string. > > > Add an assert in blob_write_string to ease debug > > > > > > Signed-off-by: Gregory Hainaut > > > --- > > > src/compiler/glsl/blob.c | 33 + > > > src/compiler/glsl/blob.h | 27 +++ > > > src/compiler/glsl/shader_cache.cpp | 4 ++-- > > > 3 files changed, 62 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c > > > index 769ebf1..62d6bf5 100644 > > > --- a/src/compiler/glsl/blob.c > > > +++ b/src/compiler/glsl/blob.c > > > @@ -176,9 +176,29 @@ blob_write_intptr(struct blob *blob, intptr_t value) > > > bool > > > blob_write_string(struct blob *blob, const char *str) > > > { > > > + // Please use blob_write_string_optional instead > > > + assert(str != NULL); > > > + > > > return blob_write_bytes(blob, str, strlen(str) + 1); > > > } > > > > > > +bool > > > +blob_write_string_optional(struct blob *blob, const char *str) > > > +{ > > > + bool ret; > > > + const uint8_t flag = str != NULL ? 1 : 0; > > > + > > > + ret = blob_write_bytes(blob, &flag, 1); > > > + > > > + if (!ret) > > > + return false; > > > + > > > + if (s
Re: [Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string
On Wed, 5 Apr 2017 14:22:00 -0400 Ilia Mirkin wrote: > On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut > wrote: > > Context: > > Nouveau uses NULL strings for unnamed parameter of texture gather > > offsets opcode. > > To be clear, this isn't a "nouveau" thing, as it is well downstream of > all this GLSL stuff. And FWIW llvmpipe/softpipe also support the > 4-offset TG4 (I think). It's a GLSL IR thing. If it needs changing, > you can change how GLSL IR represents this stuff, instead of > necessarily supporting this wart. > > Unfortunately I don't understand the problem clearly enough to tell > what's going on and what the proper solution is. > Ilia, Yes you're right it isn't limited to Nouveau. And actually it could appear in future glsl code. Actually there is another call to _mesa_add_parameter in st_init_atifs_prog from state_tracker/st_atifs_to_tgsi.c If I try to do a summary. The problem is "what is the expected name of an unnamed parameter ?" The 2 possibles answer are NULL or emptry (AKA ""). Nicolai raise the valid point that code could handle NULL and empty differently. I tried to do various grep without success. I don't think there is any difference between NULL and "" but easy to say hard to demonstrate. However I don't think that allowing NULL pointer in name is a good idea. For example st_nir_lookup_parameter_index (st_glsl_to_nir.cpp) use this kind of code. I'm afraid that strncmp with a NULL pointer is an undefined behavior. for (unsigned i = 0; i < params->NumParameters; i++) { struct gl_program_parameter *p = ¶ms->Parameters[i]; if ((strncmp(p->Name, name, namelen) == 0) && The YOLO solution is appealing. We change _mesa_add_parameter NULL name parameter to "". Then we run piglit. Best regards, Gregory > > > > Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau > > > > v3: Redo patch based on Nicolai/Timothy feedback > > Create dedicated blob_write_string_optional/blob_read_string_optional to > > handle null pointer string. > > Add an assert in blob_write_string to ease debug > > > > Signed-off-by: Gregory Hainaut > > --- > > src/compiler/glsl/blob.c | 33 + > > src/compiler/glsl/blob.h | 27 +++ > > src/compiler/glsl/shader_cache.cpp | 4 ++-- > > 3 files changed, 62 insertions(+), 2 deletions(-) > > > > diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c > > index 769ebf1..62d6bf5 100644 > > --- a/src/compiler/glsl/blob.c > > +++ b/src/compiler/glsl/blob.c > > @@ -176,9 +176,29 @@ blob_write_intptr(struct blob *blob, intptr_t value) > > bool > > blob_write_string(struct blob *blob, const char *str) > > { > > + // Please use blob_write_string_optional instead > > + assert(str != NULL); > > + > > return blob_write_bytes(blob, str, strlen(str) + 1); > > } > > > > +bool > > +blob_write_string_optional(struct blob *blob, const char *str) > > +{ > > + bool ret; > > + const uint8_t flag = str != NULL ? 1 : 0; > > + > > + ret = blob_write_bytes(blob, &flag, 1); > > + > > + if (!ret) > > + return false; > > + > > + if (str != NULL) > > + ret = blob_write_string(blob, str); > > + > > + return ret; > > +} > > + > > void > > blob_reader_init(struct blob_reader *blob, uint8_t *data, size_t size) > > { > > @@ -321,3 +341,16 @@ blob_read_string(struct blob_reader *blob) > > > > return ret; > > } > > + > > +char * > > +blob_read_string_optional(struct blob_reader *blob) > > +{ > > + uint8_t *flag; > > + flag = (uint8_t *)blob_read_bytes(blob, 1); > > + > > + if (flag == NULL || *flag == 0) { > > + return NULL; > > + } > > + > > + return blob_read_string(blob); > > +} > > diff --git a/src/compiler/glsl/blob.h b/src/compiler/glsl/blob.h > > index 940c81e..2f58cc3 100644 > > --- a/src/compiler/glsl/blob.h > > +++ b/src/compiler/glsl/blob.h > > @@ -203,6 +203,7 @@ blob_write_intptr(struct blob *blob, intptr_t value); > > > > /** > > * Add a NULL-terminated string to a blob, (including the NULL terminator). > > + * If str could be NULL, blob_write_string_optional must be used instead. > > * > > * \return True unless allocation failed. > > */ > > @@ -210,6 +211,15 @@ bool > > blob_write_string(struct blob *blob, const char *str); > >
[Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string
Context: Nouveau uses NULL strings for unnamed parameter of texture gather offsets opcode. Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau v3: Redo patch based on Nicolai/Timothy feedback Create dedicated blob_write_string_optional/blob_read_string_optional to handle null pointer string. Add an assert in blob_write_string to ease debug Signed-off-by: Gregory Hainaut --- src/compiler/glsl/blob.c | 33 + src/compiler/glsl/blob.h | 27 +++ src/compiler/glsl/shader_cache.cpp | 4 ++-- 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c index 769ebf1..62d6bf5 100644 --- a/src/compiler/glsl/blob.c +++ b/src/compiler/glsl/blob.c @@ -176,9 +176,29 @@ blob_write_intptr(struct blob *blob, intptr_t value) bool blob_write_string(struct blob *blob, const char *str) { + // Please use blob_write_string_optional instead + assert(str != NULL); + return blob_write_bytes(blob, str, strlen(str) + 1); } +bool +blob_write_string_optional(struct blob *blob, const char *str) +{ + bool ret; + const uint8_t flag = str != NULL ? 1 : 0; + + ret = blob_write_bytes(blob, &flag, 1); + + if (!ret) + return false; + + if (str != NULL) + ret = blob_write_string(blob, str); + + return ret; +} + void blob_reader_init(struct blob_reader *blob, uint8_t *data, size_t size) { @@ -321,3 +341,16 @@ blob_read_string(struct blob_reader *blob) return ret; } + +char * +blob_read_string_optional(struct blob_reader *blob) +{ + uint8_t *flag; + flag = (uint8_t *)blob_read_bytes(blob, 1); + + if (flag == NULL || *flag == 0) { + return NULL; + } + + return blob_read_string(blob); +} diff --git a/src/compiler/glsl/blob.h b/src/compiler/glsl/blob.h index 940c81e..2f58cc3 100644 --- a/src/compiler/glsl/blob.h +++ b/src/compiler/glsl/blob.h @@ -203,6 +203,7 @@ blob_write_intptr(struct blob *blob, intptr_t value); /** * Add a NULL-terminated string to a blob, (including the NULL terminator). + * If str could be NULL, blob_write_string_optional must be used instead. * * \return True unless allocation failed. */ @@ -210,6 +211,15 @@ bool blob_write_string(struct blob *blob, const char *str); /** + * Add a flag + a NULL-terminated string to a blob, (including the NULL + * terminator). The flag will be zero if str is NULL. + * + * \return True unless allocation failed. + */ +bool +blob_write_string_optional(struct blob *blob, const char *str); + +/** * Start reading a blob, (initializing the contents of \blob for reading). * * After this call, the caller can use the various blob_read_* functions to @@ -294,6 +304,23 @@ blob_read_intptr(struct blob_reader *blob); char * blob_read_string(struct blob_reader *blob); +/** + * Read a NULL-terminated string from the current location, (and update the + * current location to just past this string). + * + * \note Similar as blob_read_string but return NULL if written string was NULL. + * + * \note The memory returned belongs to the data underlying the blob reader. The + * caller must copy the string in order to use the string after the lifetime + * of the data underlying the blob reader. + * + * \return The string read (see note above about memory lifetime). However, if + * there is no NULL byte remaining within the blob, this function returns + * NULL. + */ +char * +blob_read_string_optional(struct blob_reader *blob); + #ifdef __cplusplus } #endif diff --git a/src/compiler/glsl/shader_cache.cpp b/src/compiler/glsl/shader_cache.cpp index f5e6a22..e07d2c4 100644 --- a/src/compiler/glsl/shader_cache.cpp +++ b/src/compiler/glsl/shader_cache.cpp @@ -1078,7 +1078,7 @@ write_shader_parameters(struct blob *metadata, struct gl_program_parameter *param = ¶ms->Parameters[i]; blob_write_uint32(metadata, param->Type); - blob_write_string(metadata, param->Name); + blob_write_string_optional(metadata, param->Name); blob_write_uint32(metadata, param->Size); blob_write_uint32(metadata, param->DataType); blob_write_bytes(metadata, param->StateIndexes, @@ -1104,7 +1104,7 @@ read_shader_parameters(struct blob_reader *metadata, _mesa_reserve_parameter_storage(params, num_parameters); while (i < num_parameters) { gl_register_file type = (gl_register_file) blob_read_uint32(metadata); - const char *name = blob_read_string(metadata); + const char *name = blob_read_string_optional(metadata); unsigned size = blob_read_uint32(metadata); unsigned data_type = blob_read_uint32(metadata); blob_copy_bytes(metadata, (uint8_t *) state_indexes, -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 0/3] asynchronous pbo transfer with glthread
> Still, I believe there is the following bug in the series: > glGenBuffers(1, &pbo); > glBindBuffer(..., pbo); > glDeleteBuffers(1, &pbo); > glTexSubImage2D(...); // will be asynchronous, but refers to user memory > because the pbo was implicitly unbound I unbound the buffer when it is deleted in the track_buffers_destruction function. See code chunk below. So I think it must work. Or did I miss something ? > + if (buffers[i] == glthread->pixel_pack_buffer_bound) > + glthread->pixel_pack_buffer_bound = 0; > + > + if (buffers[i] == glthread->pixel_unpack_buffer_bound) > + glthread->pixel_unpack_buffer_bound = 0; > Ctx A: glGenBuffers(1, &pbo); // assume that this gets the same buffer name You can't have the same name because name is still bound in context A. So maybe you mean something instead for your example ? > Ctx A: glGenBuffers(1, &pbo); > Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); > Ctx B: glDeleteBuffers(1, &pbo); ... > Ctx A: glTexSubImage2D(...); // will be synchronous, even though it _could_ > be asynchronous (because the PBO that was generated first is still bound!) And yes I confirm, it isn't optimal. I did it on purpose because it is much safer/easier and as you said it is a crazy case. Cheers, Gregory On 4/5/17, Nicolai Hähnle wrote: > On 01.04.2017 11:42, Gregory Hainaut wrote: >> Hello, >> >> Please find a new version to handle invalid buffer handles. >> >> Allow to handle this kind of case: >>genBuffer(&pbo); >>DeleteBuffer(pbo); >>BindBuffer(pbo) >>TexSubImage2D(user_memory_pointer); // Data transfer will be >> synchronous >> >> There are various subtely to handle multi threaded shared context. In >> order to >> keep the code sane, I've considered a buffer invalid when it is deleted by >> a >> context even it is still bound to others contexts. It will force a >> synchronous >> transfer which is always safe. > > Thanks! I think you're right that tracking pointers is not actually > needed as long as Create/Delete are synchronous. It's less accurate, but > the inaccuracy doesn't necessarily lead to correctness problems. > > Still, I believe there is the following bug in the series: > > glGenBuffers(1, &pbo); > glBindBuffer(..., pbo); > glDeleteBuffers(1, &pbo); > glTexSubImage2D(...); // will be asynchronous, but refers to user memory > because the pbo was implicitly unbound > > Once you fix this bug by updating the current context's bindings in > glDeleteBuffers, you'll have the following multi-context situation, > which is sub-optimal but not a bug -- and applications which do that are > crazy anyway, so let's not worry about them. I'm just mentioning it for > completeness: > > Ctx A: glGenBuffers(1, &pbo); > Ctx A: glBindBuffer(PIXEL_UNPACK_BUFFER, pbo); > Ctx B: glDeleteBuffers(1, &pbo); > Ctx A: glGenBuffers(1, &pbo); // assume that this gets the same buffer name > Ctx A: glDeleteBuffers(1, &pbo); > Ctx A: glTexSubImage2D(...); // will be synchronous, even though it > _could_ be asynchronous (because the PBO that was generated first is > still bound!) > > Cheers, > Nicolai > > >> >> Gregory Hainaut (3): >> mesa/glthread: track buffer creation/destruction >> mesa/glthread: add tracking of PBO binding >> mapi/glthread: generate asynchronous code for PBO transfer >> >> src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- >> src/mapi/glapi/gen/ARB_robustness.xml | 2 +- >> src/mapi/glapi/gen/gl_API.dtd | 10 +- >> src/mapi/glapi/gen/gl_API.xml | 32 +++--- >> src/mapi/glapi/gen/gl_marshal.py | 23 +++- >> src/mapi/glapi/gen/marshal_XML.py | 19 +++- >> src/mesa/main/glthread.h | 10 ++ >> src/mesa/main/marshal.c| 149 >> - >> src/mesa/main/marshal.h| 24 >> src/mesa/main/mtypes.h | 5 + >> src/mesa/main/shared.c | 4 + >> 11 files changed, 257 insertions(+), 39 deletions(-) >> > > > -- > Lerne, wie die Welt wirklich ist, > Aber vergiss niemals, wie sie sein sollte. > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/3] mapi/glthread: generate asynchronous code for PBO transfer
# Classify fixed and variable parameters. self.fixed_params = [] self.variable_params = [] for p in self.parameters: if p.is_padding: continue -if p.is_variable_length(): +if self.marshal == "upbo" and p.is_pointer(): +# Pixel buffer transfer API is tricky. By default it contains +# a pointer to user memory so a variable length parameter. +# When a pixel buffer is bound, the pointer becomes an offset. +# +# Non-PBO transfer will be synchronous so parameter type isn't +# important. PBO transfer will be asynchronous so the parameter +# must be marked as fixed +self.fixed_params.append(p) > If this is needed for upbo, shouldn't it also be needed for ppbo? > > Cheers, > Nicolai Hello Nicolai, It isn't symmetrical. In case of UPBO data ought to be copied from app thread to gl thread. You can see variable_length parameter as input pointer. Variable length will generate the memcpy code. However PPBO will copy from GPU to user pointer. There is no data associated with the pointer so the pointer isn't "used" by glthread, only transferred to GL. I think the code would love an extra comment. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] glsl/blob: handle NULL ptr in blob_write_string/blob_read_string
On Wed, 5 Apr 2017 09:09:45 +1000 Timothy Arceri wrote: > On 05/04/17 02:29, Gregory Hainaut wrote: > > Context: > > Nouveau uses NULL strings for unnamed parameter of texture gather > > offsets opcode. > > > > Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau > > > > v2: based on Nicolai feedback > > Hi Gregory, > > Nicolai suggested you change the caller of create a new helper function > for the case where string can be NULL. e.g blob_write_optional_string() > > The change below causes an extra read/write which is not required for > ever other use of this function. Please create the additional function > as a wrapper around blob_write_string() > > Thanks, > Tim > Hello Timothy, I will change it. However what do you want to do if blob_write_string is called with a null pointer ? Do you prefer * Crash * Assert * Report an internal error message I'm leading toward the latter but I'm afraid it might either corrupt the cache (if we skip the string write or use an empty string). Well we could just report an error before the crash too. Best regards, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 0/3] asynchronous pbo transfer with glthread
Hello, Please find a new version to handle invalid buffer handles. Allow to handle this kind of case: genBuffer(&pbo); DeleteBuffer(pbo); BindBuffer(pbo) TexSubImage2D(user_memory_pointer); // Data transfer will be synchronous There are various subtely to handle multi threaded shared context. In order to keep the code sane, I've considered a buffer invalid when it is deleted by a context even it is still bound to others contexts. It will force a synchronous transfer which is always safe. Gregory Hainaut (3): mesa/glthread: track buffer creation/destruction mesa/glthread: add tracking of PBO binding mapi/glthread: generate asynchronous code for PBO transfer src/mapi/glapi/gen/ARB_direct_state_access.xml | 18 +-- src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 +- src/mapi/glapi/gen/gl_API.xml | 32 +++--- src/mapi/glapi/gen/gl_marshal.py | 23 +++- src/mapi/glapi/gen/marshal_XML.py | 19 +++- src/mesa/main/glthread.h | 10 ++ src/mesa/main/marshal.c| 149 - src/mesa/main/marshal.h| 24 src/mesa/main/mtypes.h | 5 + src/mesa/main/shared.c | 4 + 11 files changed, 257 insertions(+), 39 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] glsl/blob: handle NULL ptr in blob_write_string/blob_read_string
Context: Nouveau uses NULL strings for unnamed parameter of texture gather offsets opcode. Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau v2: based on Nicolai feedback Adds an extra flag byte that will be null if the string is null. This way, null strings are handled transparently for Mesa. Signed-off-by: Gregory Hainaut --- src/compiler/glsl/blob.c | 20 +++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c index 769ebf1..b520044 100644 --- a/src/compiler/glsl/blob.c +++ b/src/compiler/glsl/blob.c @@ -176,7 +176,18 @@ blob_write_intptr(struct blob *blob, intptr_t value) bool blob_write_string(struct blob *blob, const char *str) { - return blob_write_bytes(blob, str, strlen(str) + 1); + bool ret = true; + const uint8_t flag = str != NULL ? 1 : 0; + + ret = blob_write_bytes(blob, &flag, 1); + + if (!ret) + return false; + + if (flag) + ret = blob_write_bytes(blob, str, strlen(str) + 1); + + return ret; } void @@ -293,8 +304,15 @@ blob_read_string(struct blob_reader *blob) { int size; char *ret; + uint8_t *flag; uint8_t *nul; + flag = (uint8_t *)blob_read_bytes(blob, 1); + + if (flag == NULL || *flag == 0) { + return NULL; + } + /* If we're already at the end, then this is an overrun. */ if (blob->current >= blob->end) { blob->overrun = true; -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 3/3] mapi/glthread: generate asynchronous code for PBO transfer
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) v3: split buffer tracking into separate patches. use 'goto fallback_to_sync' when asynchronous transfer isn't supported Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 23 - src/mapi/glapi/gen/marshal_XML.py | 19 - 6 files changed, 65 insertions(+), 33 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 566f157..d842ebd 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -374,7 +374,7 @@ - + @@ -384,7 +384,7 @@ - + @@ -396,7 +396,7 @@ - + @@ -410,7 +410,7 @@ - + @@ -420,7 +420,7 @@ - + @@ -432,7 +432,7 @@ - + @@ -523,7 +523,7 @@ - + @@ -532,7 +532,7 @@ - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0..6e1ac09 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -82,7 +82,7 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250..447b03a 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -122,14 +122,16 @@ param: offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index ce904a5..6e29d3f 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -2149,7 +2149,7 @@ - + @@ -2161,7 +2161,7 @@ - + @@ -2640,7 +2640,7 @@ - + @@ -3293,7 +3293,7 @@ - + @@ -3305,7 +3305,7 @@ - + @@ -4005,7 +4005,7 @@ - + @@ -4019,7 +4019,7 @@ - + @@ -4501,7 +4501,7 @@ - + @@ -4514,7 +4514,7 @@ - + @@ -4526,7 +4526,7 @@ - + @@ -4537,7 +4537,7 @@ - + @@ -4552,7 +4552,7 @@ - + @@ -4565,7 +4565,7 @@ - +
[Mesa-dev] [PATCH v2 1/3] mesa/glthread: track buffer creation/destruction
It would be used in next commit to allow asynchronous PBO transfer. The tracking saves the buffer name into a hash. Saving pointer will be more complex as the buffer is created in BindBuffer due to IsBuffer insanity. Perf wise DeleteBuffers is now synchronous for robustness. Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 2 +- src/mapi/glapi/gen/gl_API.xml | 4 +- src/mesa/main/glthread.h | 10 +++ src/mesa/main/marshal.c| 113 + src/mesa/main/marshal.h| 24 ++ src/mesa/main/mtypes.h | 5 ++ src/mesa/main/shared.c | 4 + 7 files changed, 159 insertions(+), 3 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 43841bb..566f157 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -49,7 +49,7 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index 8392e3a..ce904a5 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -5049,13 +5049,13 @@ - + - + diff --git a/src/mesa/main/glthread.h b/src/mesa/main/glthread.h index 50c1db2..494e942 100644 --- a/src/mesa/main/glthread.h +++ b/src/mesa/main/glthread.h @@ -92,6 +92,16 @@ struct glthread_state * buffer) binding is in a VBO. */ bool element_array_is_vbo; + + /** +* Tracks on the main thread side the bound unpack pixel buffer +*/ + GLint pixel_unpack_buffer_bound; + + /** +* Tracks on the main thread side the bound pack pixel buffer +*/ + GLint pixel_pack_buffer_bound; }; /** diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index b01c073..5a8354d 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -32,6 +32,7 @@ #include "marshal.h" #include "dispatch.h" #include "marshal_generated.h" +#include "hash.h" #ifdef HAVE_PTHREAD @@ -159,6 +160,118 @@ _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, free(length_tmp); } +/** + * Used as a placeholder for track_buffers_creation/track_buffers_destruction + * so we know if buffer really exists in track_buffers_binding. + */ +static struct gl_buffer_object DummyBufferObject; + +static void track_buffers_creation(GLsizei n, GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + _mesa_HashInsertLocked(ctx->Shared->ShadowBufferObjects, buffers[i], +&DummyBufferObject); + } + + _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects); +} + +static void track_buffers_destruction(GLsizei n, const GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + GLsizei i; + struct glthread_state *glthread = ctx->GLThread; + + if (n < 0 || !buffers) + return; + + _mesa_HashLockMutex(ctx->Shared->ShadowBufferObjects); + + for (i = 0; i < n ; i++) { + if (buffers[i] == glthread->pixel_pack_buffer_bound) + glthread->pixel_pack_buffer_bound = 0; + + if (buffers[i] == glthread->pixel_unpack_buffer_bound) + glthread->pixel_unpack_buffer_bound = 0; + + /* Technically the buffer can still exist if it is bound to another + * context. It isn't important as next BindBuffer will consider + * the buffer invalid and following PBO transfer will be synchronous + * which is always safe. + */ + _mesa_HashRemoveLocked(ctx->Shared->ShadowBufferObjects, buffers[i]); + } + + _mesa_HashUnlockMutex(ctx->Shared->ShadowBufferObjects); +} + + +/* CreateBuffers: custom marshal to track buffers creation */ +void GLAPIENTRY +_mesa_marshal_CreateBuffers(GLsizei n, GLuint * buffers) +{ + GET_CURRENT_CONTEXT(ctx); + _mesa_glthread_finish(ctx); + debug_print_sync("CreateBuffers"); + CALL_CreateBuffers(ctx->CurrentServerDispatch, (n, buffers)); + + track_buffers_creation(n, buffers); +} + +void +_mesa_unmarshal_CreateBuffers(struct gl_context *ctx, + const struct marshal_cmd_CreateBuffers *cmd) +{ + assert(0); +} + +/* GenBuffers: custom marshal to track buffers creation */ +void GLAPIENTRY +_mesa_marshal_GenBuffers(GLsizei n, GLuint * buffer) +{ + GET_CURRENT_CONTEXT(ctx); + _mesa_glthread_finish(ctx); + debug_print_sync("GenBuffers"); + CALL_GenBuffers(ctx->CurrentServerDispatch, (n, buffer)); + + track_buffers_creation(n, buffer); +} + +void +_mesa_unmarshal_GenBuffers(struct gl_context *ctx, +
[Mesa-dev] [PATCH v2 2/3] mesa/glthread: add tracking of PBO binding
In gl core, buffer must be reserved first by CreateBuffers/GenBuffers to be valid. Signed-off-by: Gregory Hainaut --- src/mesa/main/marshal.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c index 5a8354d..acbf6df 100644 --- a/src/mesa/main/marshal.c +++ b/src/mesa/main/marshal.c @@ -290,7 +290,20 @@ _mesa_unmarshal_BindBufferBase(struct gl_context *ctx, const struct marshal_cmd_ CALL_BindBufferBase(ctx->CurrentServerDispatch, (target, index, buffer)); } -/** Tracks the current bindings for the vertex array and index array buffers. +/** + * Check that buffer is a valid buffer handle + * Always return false for ID 0. + */ +static bool +is_bufferobj(struct gl_context *ctx, GLuint buffer) +{ + if (buffer == 0) + return false; + else + return _mesa_HashLookup(ctx->Shared->ShadowBufferObjects, buffer) != NULL; +} + +/** Tracks the current bindings of GL buffer targets * * This is part of what we need to enable glthread on compat-GL contexts that * happen to use VBOs, without also supporting the full tracking of VBO vs @@ -312,9 +325,14 @@ _mesa_unmarshal_BindBufferBase(struct gl_context *ctx, const struct marshal_cmd_ * instead of updating the binding. However, compat GL has the ridiculous * feature that if you pass a bad name, it just gens a buffer object for you, * so we escape without having to know if things are valid or not. + * + * Code was extended to track pixel buffers so you know if pixel transfer + * goes to an user pointer (must be synchronous) or an GL buffer (could + * be asynchronous). Unlike VBO we need a stronger guarantee that buffer + * was reserved by GenBuffers or CreateBuffers. */ static void -track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) +track_buffers_binding(struct gl_context *ctx, GLenum target, GLuint buffer) { struct glthread_state *glthread = ctx->GLThread; @@ -329,6 +347,18 @@ track_vbo_binding(struct gl_context *ctx, GLenum target, GLuint buffer) */ glthread->element_array_is_vbo = (buffer != 0); break; + case GL_PIXEL_UNPACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_unpack_buffer_bound = buffer; + else + glthread->pixel_unpack_buffer_bound = 0; + break; + case GL_PIXEL_PACK_BUFFER: + if (ctx->API == API_OPENGL_COMPAT || is_bufferobj(ctx, buffer)) + glthread->pixel_pack_buffer_bound = buffer; + else + glthread->pixel_pack_buffer_bound = 0; + break; } } @@ -360,7 +390,7 @@ _mesa_marshal_BindBuffer(GLenum target, GLuint buffer) struct marshal_cmd_BindBuffer *cmd; debug_print_marshal("BindBuffer"); - track_vbo_binding(ctx, target, buffer); + track_buffers_binding(ctx, target, buffer); if (cmd_size <= MARSHAL_MAX_CMD_SIZE) { cmd = _mesa_glthread_allocate_command(ctx, DISPATCH_CMD_BindBuffer, -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Fwd: [PATCH 1/1] glsl/blob: handle copy of NULL ptr in blob_write_string
On Fri, 31 Mar 2017 12:53:47 -0400 Ilia Mirkin wrote: > On Fri, Mar 31, 2017 at 6:12 AM, Gregory Hainaut > wrote: > >> Others have reported this crashing on Nouveau. I haven't seen the problem > >> on radeonsi or i965. > > > > Hello Timothy (sorry for the double mail, email is a complex tool:) ) > > > > Hum, tbh. I was quite surprised to hit this bug. I guess you save a > > pre-optimized shader in the cache. So it could depends on optimization > > passes. > > > > From the top of my head, I think the "offending" line is this one > > const ivec2 offsets[4] = {ivec2(...), ivec2(...), ivec2(...), ivec2(...)}; > > > > Strangely enough there are only 3 parameters without name in the > > parameter list (signature is int, size 2 and CONTANT). Maybe one was > > optimized away, I didn't look further. > > Note that nouveau is unique in that it can process > textureGatherOffsets() directly, without lowering it to 4x > textureGatherOffset. > > The relevant code is in st_glsl_to_tgsi.cpp > > if (!pscreen->get_param(pscreen, PIPE_CAP_TEXTURE_GATHER_OFFSETS)) > lower_offset_arrays(ir); > > So I think with nouveau, you're seeing glsl ir that you wouldn't see > otherwise. > > -ilia Hello ilia You're right. The issue appears in the texture gather 4 opcode. I can see this path (st_glsl_to_tgsi.cpp) in GDB. case ir_tg4: opcode = TGSI_OPCODE_TG4; Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/1] glsl/blob: handle copy of NULL ptr in blob_write_string
On Fri, 31 Mar 2017 19:16:10 +1100 Timothy Arceri wrote: > > > On 31/03/17 18:00, gregory hainaut wrote: > > On Fri, 31 Mar 2017 08:24:36 +0200 > > Nicolai Hähnle wrote: > > > > Hello Nicolai > > > >> On 30.03.2017 21:55, Gregory Hainaut wrote: > >>> Typically happen when we want to copy an unnamed shader parameter > >>> in the shader cache. > >> > >> So this happens only when blob_write_string is called from nouveau? > > > > Sorry, I poorly explain myself. I should have written reproduce & > > tested on Nouveau. I don't know for others drivers, they should be > > impacted. > > > > _mesa_add_parameter seems to allow to store a NULL pointer in p->Name. > > Which is later written by blob_write_string. I guess it could > > depends on the shader cache state. > > > > > > I got the crash with this piglit test: > > textureGather fs offsets r 0 float 2D repeat -auto -fb > > Others have reported this crashing on Nouveau. I haven't seen the > problem on radeonsi or i965. > > > > > > >> By the way, please setup send-mail so that it threads your mails. > >> That should be the default, so I'm not sure what happened here... > > Oh. I edited my email in the mailer queue which moved the email from my > > pop3 to my imap account. I guess it broke the threading link. I will > > be more careful next time. > > > > Thanks > > > > > >> Thanks, > >> Nicolai > > > >>> > >>> Note: it is safer to copy an empty string so we can read it back > >>> safely. > >>> > >>> Fix piglit crashes of the 'texturegatheroffsets' tests > >>> > >>> Signed-off-by: Gregory Hainaut > >>> --- > >>> src/compiler/glsl/blob.c | 5 - > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c > >>> index 769ebf1..f84d7f3 100644 > >>> --- a/src/compiler/glsl/blob.c > >>> +++ b/src/compiler/glsl/blob.c > >>> @@ -176,7 +176,10 @@ blob_write_intptr(struct blob *blob, intptr_t > >>> value) bool > >>> blob_write_string(struct blob *blob, const char *str) > >>> { > >>> - return blob_write_bytes(blob, str, strlen(str) + 1); > >>> + if (str == NULL) > >>> + return blob_write_bytes(blob, "", 1); > >>> + else > >>> + return blob_write_bytes(blob, str, strlen(str) + 1); > >>> } > >>> > >>> void > >>> > >> > >> Fwiw, I backtraced the origin of the NULL. As you can see _mesa_add_typed_unnamed_constant will set the name string to NULL instead of "". So it seems Intel/AMD don't use unnamed constant when the shader is linked. #0 _mesa_add_parameter (paramList=0x8126628, type=PROGRAM_CONSTANT, name=0x0, size=2, datatype=5124, values=0xc4d0, state=0x0) at mesa/program/prog_parameter.c:256 #1 0xf6d6224d in _mesa_add_typed_unnamed_constant (paramList=0x8126628, values=0xc4d0, size=2, datatype=5124, swizzleOut=0xc468) at mesa/program/prog_parameter.c:345 #2 0xf6d1d9ee in glsl_to_tgsi_visitor::add_constant (this=0x839b838, file=PROGRAM_CONSTANT, values=0xc4d0, size=2, datatype=5124, swizzle_out=0x83a0a98) at state_tracker/st_glsl_to_tgsi.cpp:1126 #3 0xf6d296af in glsl_to_tgsi_visitor::visit (this=0x839b838, ir=0x8395b60) at state_tracker/st_glsl_to_tgsi.cpp:3410 #4 0xf6e184c7 in ir_constant::accept (this=0x8395b60, v=0x839b838) at glsl/ir.h:2133 #5 0xf6d28b5d in glsl_to_tgsi_visitor::visit (this=0x839b838, ir=0x8395a98) at state_tracker/st_glsl_to_tgsi.cpp:3278 So we can do 3 different fixes (and potentially the 3). 1/ update _mesa_add_typed_unnamed_constant to use an empty string 2/ update _mesa_add_parameter "p->Name = name ? strdup(name) : NULL;" to use an empty string when name is null 3/ or my previous patch :) Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Fwd: [PATCH 1/1] glsl/blob: handle copy of NULL ptr in blob_write_string
> Others have reported this crashing on Nouveau. I haven't seen the problem on > radeonsi or i965. Hello Timothy (sorry for the double mail, email is a complex tool:) ) Hum, tbh. I was quite surprised to hit this bug. I guess you save a pre-optimized shader in the cache. So it could depends on optimization passes. From the top of my head, I think the "offending" line is this one const ivec2 offsets[4] = {ivec2(...), ivec2(...), ivec2(...), ivec2(...)}; Strangely enough there are only 3 parameters without name in the parameter list (signature is int, size 2 and CONTANT). Maybe one was optimized away, I didn't look further. Cheers ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/1] glsl/blob: handle copy of NULL ptr in blob_write_string
On Fri, 31 Mar 2017 08:24:36 +0200 Nicolai Hähnle wrote: Hello Nicolai > On 30.03.2017 21:55, Gregory Hainaut wrote: > > Typically happen when we want to copy an unnamed shader parameter > > in the shader cache. > > So this happens only when blob_write_string is called from nouveau? Sorry, I poorly explain myself. I should have written reproduce & tested on Nouveau. I don't know for others drivers, they should be impacted. _mesa_add_parameter seems to allow to store a NULL pointer in p->Name. Which is later written by blob_write_string. I guess it could depends on the shader cache state. I got the crash with this piglit test: textureGather fs offsets r 0 float 2D repeat -auto -fb > By the way, please setup send-mail so that it threads your mails. > That should be the default, so I'm not sure what happened here... Oh. I edited my email in the mailer queue which moved the email from my pop3 to my imap account. I guess it broke the threading link. I will be more careful next time. Thanks > Thanks, > Nicolai > > > > Note: it is safer to copy an empty string so we can read it back > > safely. > > > > Fix piglit crashes of the 'texturegatheroffsets' tests > > > > Signed-off-by: Gregory Hainaut > > --- > > src/compiler/glsl/blob.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c > > index 769ebf1..f84d7f3 100644 > > --- a/src/compiler/glsl/blob.c > > +++ b/src/compiler/glsl/blob.c > > @@ -176,7 +176,10 @@ blob_write_intptr(struct blob *blob, intptr_t > > value) bool > > blob_write_string(struct blob *blob, const char *str) > > { > > - return blob_write_bytes(blob, str, strlen(str) + 1); > > + if (str == NULL) > > + return blob_write_bytes(blob, "", 1); > > + else > > + return blob_write_bytes(blob, str, strlen(str) + 1); > > } > > > > void > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/1] NULL pointer in blob_write_string
Hello, Fix a crash on Nouveau + Shader cache. I don't know if it could impact current stable version As a remainder I don't have commit access. Best regards, Gregory Hainaut (1): glsl/blob: handle copy of NULL ptr in blob_write_string src/compiler/glsl/blob.c | 5 +- 1 files changed, 5 insertions(+), 5 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/1] glsl/blob: handle copy of NULL ptr in blob_write_string
Typically happen when we want to copy an unnamed shader parameter in the shader cache. Note: it is safer to copy an empty string so we can read it back safely. Fix piglit crashes of the 'texturegatheroffsets' tests Signed-off-by: Gregory Hainaut --- src/compiler/glsl/blob.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c index 769ebf1..f84d7f3 100644 --- a/src/compiler/glsl/blob.c +++ b/src/compiler/glsl/blob.c @@ -176,7 +176,10 @@ blob_write_intptr(struct blob *blob, intptr_t value) bool blob_write_string(struct blob *blob, const char *str) { - return blob_write_bytes(blob, str, strlen(str) + 1); + if (str == NULL) + return blob_write_bytes(blob, "", 1); + else + return blob_write_bytes(blob, str, strlen(str) + 1); } void -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa glthread: allow asynchronous pixel transfer operation when a buffer is bound
On Mon, 27 Mar 2017 16:10:32 +0200 Gregory Hainaut wrote: > Hello, > > Sorry I was in vacation. I will update my patch with a hash map to > trace buffer creation/destruction. > > Cheers, > Gregory > > On 3/20/17, Nicolai Hähnle wrote: > > On 20.03.2017 14:33, Markus Wick wrote: > >> Am 2017-03-20 14:21, schrieb Nicolai Hähnle: > >>> On 17.03.2017 18:59, gregory hainaut wrote: > >>>> If the application is badly/strangely coded, glthread will make it > >>>> worst. > >>>> The solution ought to be either fix the app or don't use glthread. > >>> > >>> It would be nice if glthread could handle this properly, but I don't > >>> currently see how. > >> > >> The dispatcher thread needs a map of all valid buffer objects. So we > >> need to update such a map on all glGenBuffers/glDeleteBuffers calls. So > >> the overhead will be the map lookup on all affected glBindBuffer calls. > > > > You're right. Ridiculous details of the OpenGL spec make this > > interesting, actually, because Section 6.1 (Creating and Binding Buffer > > Objects) of the OpenGL 4.5 (Compability Profile) spec says: > > > > "A buffer object is created by binding an unused name to a buffer > > target. The binding is effected by calling > > > > void BindBuffer( enum target, uint buffer ); > > > > target must be one of the targets listed in table 6.1. If the buffer > > object named buffer has not been previously bound, or has been deleted > > since the last binding, the GL creates a new state vector, initialized > > with a zero-sized memory buffer and comprising all the state and with > > the same initial values listed in table 6.2." > > > > But this language is different from that for core profiles, where a call > > to glGenBuffers is required. So in this case, the compatibility profile > > spec is actually better for performance :/ > > > > Cheers, > > Nicolai > > Hello, Actually I found another issue on this patch. A deleted buffer is unbound from the context. So validity must be updated accordingly. However I have a tricky question, I think that it is possible to share buffers between multiple contexts. Do we want to support it with glthread ? If yes, it will require either * to duplicate the reference counting logic * Or to use a synchronous delete and peep into the context to check if the buffer is still alive * Or any better idea ^^ Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa glthread: allow asynchronous pixel transfer operation when a buffer is bound
Hello, Sorry I was in vacation. I will update my patch with a hash map to trace buffer creation/destruction. Cheers, Gregory On 3/20/17, Nicolai Hähnle wrote: > On 20.03.2017 14:33, Markus Wick wrote: >> Am 2017-03-20 14:21, schrieb Nicolai Hähnle: >>> On 17.03.2017 18:59, gregory hainaut wrote: >>>> If the application is badly/strangely coded, glthread will make it >>>> worst. >>>> The solution ought to be either fix the app or don't use glthread. >>> >>> It would be nice if glthread could handle this properly, but I don't >>> currently see how. >> >> The dispatcher thread needs a map of all valid buffer objects. So we >> need to update such a map on all glGenBuffers/glDeleteBuffers calls. So >> the overhead will be the map lookup on all affected glBindBuffer calls. > > You're right. Ridiculous details of the OpenGL spec make this > interesting, actually, because Section 6.1 (Creating and Binding Buffer > Objects) of the OpenGL 4.5 (Compability Profile) spec says: > > "A buffer object is created by binding an unused name to a buffer > target. The binding is effected by calling > > void BindBuffer( enum target, uint buffer ); > > target must be one of the targets listed in table 6.1. If the buffer > object named buffer has not been previously bound, or has been deleted > since the last binding, the GL creates a new state vector, initialized > with a zero-sized memory buffer and comprising all the state and with > the same initial values listed in table 6.2." > > But this language is different from that for core profiles, where a call > to glGenBuffers is required. So in this case, the compatibility profile > spec is actually better for performance :/ > > Cheers, > Nicolai > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/glthread: add custom marshalling for ClearBufferfv()
Hello Timothy, 2 small questions: Will it work for DSA equivalent function, namely glClearNamedFramebufferfv ? Would it be interesting to also do the equivalent for glClearBufferiv/glClearBufferuiv ? Note the *uiv variant could be easier as the size is always 4 INT, so it can be done with a scale attribute on the XML. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Mesa (master): mesa/marshal: add custom BufferData/ BufferSubData marshalling
> > >>* + variable_data += size; > *>>* And here, and below. > * > This code is mostly a copy from the generated code, which adds these for > some reason (possibly a bug in generation, but it may be used by other > functions and just excess here). > > Hello, As I already looked into the python last week, it is due to loop unrolling behavior to prepare next variable parameter. Of course, most of the time, there is only a single variable parameter so it is useless. Actually it is useless for last variable parameter. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa glthread: allow asynchronous pixel transfer operation when a buffer is bound
On Fri, 17 Mar 2017 13:11:31 +0100 Markus Wick wrote: > Hi gregory, > > Am 2017-03-17 10:25, schrieb Gregory Hainaut: > > diff --git a/src/mesa/main/marshal.c b/src/mesa/main/marshal.c > > index f8cad30..43a70d4 100644 > > --- a/src/mesa/main/marshal.c > > +++ b/src/mesa/main/marshal.c > > @@ -214,6 +218,12 @@ track_vbo_binding(struct gl_context *ctx, GLenum > > target, GLuint buffer) > > */ > >glthread->element_array_is_vbo = (buffer != 0); > >break; > > + case GL_PIXEL_UNPACK_BUFFER: > > + glthread->pixel_unpack_buffer_bound = (buffer != 0); > > + break; > > + case GL_PIXEL_PACK_BUFFER: > > + glthread->pixel_pack_buffer_bound = (buffer != 0); > > + break; > > } > > } > > > > I wonder what shall happen if buffer is not 0, but neither a valid > buffer object. So the glBindBuffer call will fail, and there would still > be no buffer bound. > The comment above this function explains it very well. > > degasus Hello degasus, What shall happen ? => Honestly I don't know. Is the behavior 100% defined when you hit a gl error ? What will happen ? => It depends on the gl transfer operation that will follow the bind. Let's take some examples glBindBuffer(invalid_buffer); // if no buffer is bound, offset_to_pbo will be interpreted as a memory // pointer. Crash is expected (even with gl thread off) glTextureSubImage1D(., offset_to_pbo); // Now, if the application know that buffer isn't bound (hazardous dev or // using an is/get API to check the status). The application will use // a real memory pointer. It will be fine without glthread. But it will // crash with (the current) glthread. glTextureSubImage1D(., memory_ptr); If the application is badly/strangely coded, glthread will make it worst. The solution ought to be either fix the app or don't use glthread. Best regards, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] Faster glthread with persistant PBO
Hello, The patch avoids a glthread sync for pixel transfer operations from/to PBO. Initial patch was based on regular expression. This new version uses XML attributes as suggest by Timothy. Besides it marks the memory pointer as a constant parameter. Otherwise it will trigger a memcpy from PBO offset. PS: Timothy you can discard the patch that I sent you on your old collabora mail. I forgot to update my alias ;) Gregory Hainaut (1): mesa glthread: allow asynchronous pixel transfer operation when a buffer is bound src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 25 ++- src/mapi/glapi/gen/marshal_XML.py | 19 - src/mesa/main/glthread.h | 10 + src/mesa/main/marshal.c| 16 --- 8 files changed, 90 insertions(+), 36 deletions(-) -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa glthread: allow asynchronous pixel transfer operation when a buffer is bound
Improve speed on PCSX2 v2: Add ppbo/ubpo status in XML file Disable variable parameter (as the pointer would be a fixed offset) Signed-off-by: Gregory Hainaut --- src/mapi/glapi/gen/ARB_direct_state_access.xml | 16 +++ src/mapi/glapi/gen/ARB_robustness.xml | 2 +- src/mapi/glapi/gen/gl_API.dtd | 10 + src/mapi/glapi/gen/gl_API.xml | 28 +- src/mapi/glapi/gen/gl_marshal.py | 25 ++- src/mapi/glapi/gen/marshal_XML.py | 19 - src/mesa/main/glthread.h | 10 + src/mesa/main/marshal.c| 16 --- 8 files changed, 90 insertions(+), 36 deletions(-) diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml b/src/mapi/glapi/gen/ARB_direct_state_access.xml index 43841bb..8d98c2b 100644 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml @@ -374,7 +374,7 @@ - + @@ -384,7 +384,7 @@ - + @@ -396,7 +396,7 @@ - + @@ -410,7 +410,7 @@ - + @@ -420,7 +420,7 @@ - + @@ -432,7 +432,7 @@ - + @@ -523,7 +523,7 @@ - + @@ -532,7 +532,7 @@ - + diff --git a/src/mapi/glapi/gen/ARB_robustness.xml b/src/mapi/glapi/gen/ARB_robustness.xml index 9b2f2f0..6e1ac09 100644 --- a/src/mapi/glapi/gen/ARB_robustness.xml +++ b/src/mapi/glapi/gen/ARB_robustness.xml @@ -82,7 +82,7 @@ - + diff --git a/src/mapi/glapi/gen/gl_API.dtd b/src/mapi/glapi/gen/gl_API.dtd index b464250..447b03a 100644 --- a/src/mapi/glapi/gen/gl_API.dtd +++ b/src/mapi/glapi/gen/gl_API.dtd @@ -122,14 +122,16 @@ param: offset data should be padded to the next even number of dimensions. For example, this will insert an empty "height" field after the "width" field in the protocol for TexImage1D. - marshal - One of "sync", "async", "draw", or "custom", defaulting to -async unless one of the arguments is something we know we can't -codegen for. If "sync", we finish any queued glthread work and call + marshal - One of "sync", "async", "draw", "ppbo", "upbo" or "custom", +defaulting to async unless one of the arguments is something we know we +can't codegen for. If "sync", we finish any queued glthread work and call the Mesa implementation directly. If "async", we queue the function call to be performed by glthread. If "custom", the prototype will be generated but a custom implementation will be present in marshal.c. If "draw", it will follow the "async" rules except that "indices" are -ignored (since they may come from a VBO). +ignored (since they may come from a VBO). If "ppbo"/"upbo", it will +follow the "async" rules when a pack/unpack pixel buffer is bound +otherwise it will follow the "sync" rules. marshal_fail - an expression that, if it evaluates true, causes glthread to switch back to the Mesa implementation and call it directly. Used to disable glthread for GL compatibility interactions that we don't diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml index 15d7e4f..561f2b5 100644 --- a/src/mapi/glapi/gen/gl_API.xml +++ b/src/mapi/glapi/gen/gl_API.xml @@ -2149,7 +2149,7 @@ - + @@ -2161,7 +2161,7 @@ - + @@ -2640,7 +2640,7 @@ - + @@ -3293,7 +3293,7 @@ - + @@ -3305,7 +3305,7 @@ - + @@ -4005,7 +4005,7 @@ - + @@ -4019,7 +4019,7 @@ - + @@ -4501,7 +4501,7 @@ - + @@ -4514,7 +4514,7 @@ - + @@ -4526,7 +4526,7 @@ - + @@ -4537,7 +4537,7 @@ - + @@ -4552,7 +4552,7 @@ - + @@ -4565,7 +4565,7 @@ - +
Re: [Mesa-dev] [PATCH 07/25] glapi: Mark compressed teximage functions as sync.
> Without doing some additional tracking, we won't know whether the data > will be immediate user data, or will be loaded from a PBO. The normal > teximage functions will be sync by default because they don't know up > front what the size of their image data is. But for compressed teximage, > we have the count information, so they would end up async by default. > --- > src/mapi/glapi/gen/gl_API.xml | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml > index ce9ad17..e29825d 100644 > --- a/src/mapi/glapi/gen/gl_API.xml > +++ b/src/mapi/glapi/gen/gl_API.xml > @@ -4487,72 +4487,72 @@ > > > > > > > > > > > - > + > > > > > > > > > > > > > - > + > > > > > > > > > > > > - > + > > > > > > > > > > > - > + > > > > > > > > > > > > > > > - > + marshal="sync"> > > > > > > > > > > > -- > 2.9.3 Hello, It is expected/normal that CompressedTexSubImage1D got a default marshal value (async) instead of sync ? * CompressedTexSubImage1D: async * CompressedTexSubImage2D: sync * CompressedTexSubImage3D: sync * CompressedTexImage1D: sync * CompressedTexImage2D: sync * CompressedTexImage3D: sync Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Any updates on threaded GL dispatch?
Hello Tim, > Ignoring the attributes in XML files during generation just seems hacky to > me. Are you able to take you reg expression and apply it to sed in some way > to actually update the XML? Ok. I will update the XML files. I will try to send you a patch Thursday. Cheers, Gregory On 3/6/17, Timothy Arceri wrote: > > > On 04/03/17 04:41, gregory hainaut wrote: >> On Fri, 3 Mar 2017 16:46:24 +0100 >> Marek Olšák wrote: >> >>> On Fri, Mar 3, 2017 at 10:19 AM, Timothy Arceri >>> wrote: >>>> On 02/03/17 22:18, Marek Olšák wrote: >>>>> >>>>> The bad news is my involvement is currently on hold due to other >>>>> projects and responsibilities. >>>> >>>> >>>> I can probably spend some time on this. Seems like Gregory has taken >>>> care of >>>> most of the problems and it just needs someone to push it over the >>>> line. >>> >>> There are also plenty of unresolved review comments from Emil and >>> others. > > I've addressed all but the no trivial comments in patch 6 [1]. And > pushed to a glthread branch in my repo[2]. I've also sent out some of > the trivial standalone patches to the list that I will push with my > review in a couple of days. > > I'll try address the rest of the issue in patch 6 tomorrow and then I'll > resend the series. > > > [1] https://patchwork.freedesktop.org/patch/137755/ > [2] https://github.com/tarceri/Mesa/compare/glthread > >>> >>> Marek >> >> Yes I only fixed the piglit bad/crash regression on Nouveau. I added >> also the perf optimization for PCSX2 ;) I did a basic test of EGL >> (i.e. PCSX2) and it seems to work. >> >> By the way, I don't know how costly is the remaining synchronization >> but there are 2 potential optimizations >> * glUniform for double seems to sync whereas standard float >> are asynchronous. Maybe it misses the scale parameter in XML. >> I guess double was added after glthread. It might worth to check >> the behavior of glProgramUniform (introduce by SSO) too. >> * GL3 glClearBuffer functions are synchronous due to the pointer to >> the single pixel value. However there are maybe tricks to find the >> correct size of the pixel. >> >> Cheers, >> Gregory >> > > Hi Gregory, > > Just a comment on your patch: > > mesa glthread: allow asynchronous pixel transfer operation when a > buffer is bound > > Ignoring the attributes in XML files during generation just seems hacky > to me. Are you able to take you reg expression and apply it to sed in > some way to actually update the XML? > > Thanks, > Tim > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Any updates on threaded GL dispatch?
On Fri, 3 Mar 2017 16:46:24 +0100 Marek Olšák wrote: > On Fri, Mar 3, 2017 at 10:19 AM, Timothy Arceri wrote: > > On 02/03/17 22:18, Marek Olšák wrote: > >> > >> The bad news is my involvement is currently on hold due to other > >> projects and responsibilities. > > > > > > I can probably spend some time on this. Seems like Gregory has taken care of > > most of the problems and it just needs someone to push it over the line. > > There are also plenty of unresolved review comments from Emil and others. > > Marek Yes I only fixed the piglit bad/crash regression on Nouveau. I added also the perf optimization for PCSX2 ;) I did a basic test of EGL (i.e. PCSX2) and it seems to work. By the way, I don't know how costly is the remaining synchronization but there are 2 potential optimizations * glUniform for double seems to sync whereas standard float are asynchronous. Maybe it misses the scale parameter in XML. I guess double was added after glthread. It might worth to check the behavior of glProgramUniform (introduce by SSO) too. * GL3 glClearBuffer functions are synchronous due to the pointer to the single pixel value. However there are maybe tricks to find the correct size of the pixel. Cheers, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] doc: GL_ARB_buffer_storage is supported on llvmpipe/swr
Thanks you. Ok for the no commit access hint. On 3/2/17, Emil Velikov wrote: > On 26 February 2017 at 21:58, Gregory Hainaut > wrote: >> On 2/25/17, Edward O'Callaghan wrote: >>> Acked-by: Edward O'Callaghan >>> >>> On 02/25/2017 07:45 AM, Gregory Hainaut wrote: >>>> At least, the extension is exported (gallium capability >>>> PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT is 1) >>>> >>>> Signed-off-by: Gregory Hainaut >>>> --- >>>> docs/features.txt | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/docs/features.txt b/docs/features.txt >>>> index d9528e9..9d3a460 100644 >>>> --- a/docs/features.txt >>>> +++ b/docs/features.txt >>>> @@ -191,7 +191,7 @@ GL 4.3, GLSL 4.30 -- all DONE: i965/gen8+, nvc0, >>>> radeonsi >>>> GL 4.4, GLSL 4.40 -- all DONE: i965/gen8+, nvc0, radeonsi >>>> >>>>GL_MAX_VERTEX_ATTRIB_STRIDE DONE (all >>>> drivers) >>>> - GL_ARB_buffer_storage DONE (i965, >>>> nv50, >>>> r600) >>>> + GL_ARB_buffer_storage DONE (i965, >>>> nv50, >>>> r600, llvmpipe, swr) >>>>GL_ARB_clear_texture DONE (i965, >>>> nv50, >>>> r600, llvmpipe, softpipe) >>>>GL_ARB_enhanced_layouts DONE (i965, >>>> nv50, >>>> llvmpipe, softpipe) >>>>- compile-time constant expressions DONE >>>> >>> >>> >> >> Hello, >> >> I don't have git access. Could you push it too ? >> > Done. Gregory for the future do add a note after the --- line. We do > forget to has access and who doesn't ;-) > > -Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Any updates on threaded GL dispatch?
Hello Dieter, I sent all my patches to Marek. You can find latest code in his branch https://cgit.freedesktop.org/~mareko/mesa/?h=glthread Status: * all crashes are fixed * major piglit bug are fixed * asynchronous PBO transfer are done too (useful with persistent PBO) * I don't know if there is any remaining blocking point. Remain 2 minors issues that doesn't worth to be fixed IMHO. * CPU time measure of glxSwap is wrong. But I suspect the test is wrong as it would love a flush before starting the timer. * The error type could be wrong when you send too much uniform. Too much is count * sizeof(uniform data) >= INT_MAX. Best regards, Gregory On 3/2/17, Dieter Nützel wrote: > Hello Gregory and Marek, > > are there any updates on threaded GL dispatch? > I mean this: > > [-] >> As a quick summary: >> * there are now only 2 minors fail on piglit with my latest patches >> (sent >> to Marek) >> * I have a pending patch to allow asynchronous PBO transfer >> * Now that piglit is crash free I will give a try to both glxgear and >> glmark. Hopefully they will be both good. >> >> Gregory > > And this: >>> The number of tests run doesn't necessarily correspond to the amount >>> of test coverage. 10 tests doing different things can be more useful >>> than 1 tests doing the same thing. >>> >>> Marek >> >> Fair point. >> >> As a side note, I tested both glxgear and glmark2 which are now >> crash-free :) > > Where can I grep 'latest' code with these patches? > GREAT success! > > -Dieter > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] doc: GL_ARB_buffer_storage is supported on llvmpipe/swr
On 2/25/17, Edward O'Callaghan wrote: > Acked-by: Edward O'Callaghan > > On 02/25/2017 07:45 AM, Gregory Hainaut wrote: >> At least, the extension is exported (gallium capability >> PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT is 1) >> >> Signed-off-by: Gregory Hainaut >> --- >> docs/features.txt | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/docs/features.txt b/docs/features.txt >> index d9528e9..9d3a460 100644 >> --- a/docs/features.txt >> +++ b/docs/features.txt >> @@ -191,7 +191,7 @@ GL 4.3, GLSL 4.30 -- all DONE: i965/gen8+, nvc0, >> radeonsi >> GL 4.4, GLSL 4.40 -- all DONE: i965/gen8+, nvc0, radeonsi >> >>GL_MAX_VERTEX_ATTRIB_STRIDE DONE (all >> drivers) >> - GL_ARB_buffer_storage DONE (i965, nv50, >> r600) >> + GL_ARB_buffer_storage DONE (i965, nv50, >> r600, llvmpipe, swr) >>GL_ARB_clear_texture DONE (i965, nv50, >> r600, llvmpipe, softpipe) >>GL_ARB_enhanced_layouts DONE (i965, nv50, >> llvmpipe, softpipe) >>- compile-time constant expressions DONE >> > > Hello, I don't have git access. Could you push it too ? Thanks you, Gregory ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] doc: GL_ARB_buffer_storage is supported on llvmpipe/swr
At least, the extension is exported (gallium capability PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT is 1) Signed-off-by: Gregory Hainaut --- docs/features.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/features.txt b/docs/features.txt index d9528e9..9d3a460 100644 --- a/docs/features.txt +++ b/docs/features.txt @@ -191,7 +191,7 @@ GL 4.3, GLSL 4.30 -- all DONE: i965/gen8+, nvc0, radeonsi GL 4.4, GLSL 4.40 -- all DONE: i965/gen8+, nvc0, radeonsi GL_MAX_VERTEX_ATTRIB_STRIDE DONE (all drivers) - GL_ARB_buffer_storage DONE (i965, nv50, r600) + GL_ARB_buffer_storage DONE (i965, nv50, r600, llvmpipe, swr) GL_ARB_clear_texture DONE (i965, nv50, r600, llvmpipe, softpipe) GL_ARB_enhanced_layouts DONE (i965, nv50, llvmpipe, softpipe) - compile-time constant expressions DONE -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Time to merge threaded GL dispatch? (aka glthread)
On Mon, 13 Feb 2017 17:04:01 +0100 Marek Olšák wrote: > On Mon, Feb 13, 2017 at 4:14 PM, Gregory Hainaut > wrote: > > If I remember correctly I got something like 4K-8K fails (total is 40K > > tests) when I broke the GL1 restore dispatch function. So it is > > around 10%-20%. Maybe it was only the test below 3.1. I was afraid > > that various modern extension could still be tested with some GL1 > > drawing. > > The number of tests run doesn't necessarily correspond to the amount > of test coverage. 10 tests doing different things can be more useful > than 1 tests doing the same thing. > > Marek Fair point. As a side note, I tested both glxgear and glmark2 which are now crash-free :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev