<div dir="auto">Hello Fredrik,<div dir="auto"> </div><div dir="auto">Yes the shadow hash feels useless now. I will update the patch in a couple of days (vacation currently).</div><div dir="auto"> </div><div dir="auto">Cheers,</div><div dir="auto">Gregory</div></div><div class="gmail_extra"> <div class="gmail_quote">Le 22 juin 2017 2:24 PM, "Fredrik Höglund" <<a href="mailto:fredrik@kde.org">fredrik@kde.org</a>> a écrit : <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thursday 22 June 2017, Timothy Arceri wrote: > From: Gregory Hainaut <<a href="mailto:gregory.hainaut@gmail.com">gregory.hainaut@gmail.com</a>> > > 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 <<a href="mailto:gregory.hainaut@gmail.com">gregory.hainaut@gmail.com</a>> > --- > 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 @@ > > <function name="GetTransformFeedbacki64_v"> > <param name="xfb" type="GLuint" /> > <param name="pname" type="GLenum" /> > <param name="index" type="GLuint" /> > <param name="param" type="GLint64 *" /> > </function> > >  > > - <function name="CreateBuffers"> > + <function name="CreateBuffers" marshal="custom"> > <param name="n" type="GLsizei" /> > <param name="buffers" type="GLuint *" /> > </function> > > <function name="NamedBufferStorage" no_error="true"> > <param name="buffer" type="GLuint" /> > <param name="size" type="GLsizeiptr" /> > <param name="data" type="const GLvoid *" /> > <param name="flags" type="GLbitfield" /> > </function> > 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 @@ > > <function name="BufferSubData" es1="1.1" es2="2.0" marshal="custom" > no_error="true"> > <param name="target" type="GLenum"/> > <param name="offset" type="GLintptr"/> > <param name="size" type="GLsizeiptr" counter="true"/> > <param name="data" type="const GLvoid *" count="size"/> > <glx ignore="true"/> > </function> > > - <function name="DeleteBuffers" es1="1.1" es2="2.0"> > + <function name="DeleteBuffers" es1="1.1" es2="2.0" marshal="custom"> > <param name="n" type="GLsizei" counter="true"/> > <param name="buffer" type="const GLuint *" count="n"/> > <glx ignore="true"/> > </function> > > - <function name="GenBuffers" es1="1.1" es2="2.0"> > + <function name="GenBuffers" es1="1.1" es2="2.0" marshal="custom"> > <param name="n" type="GLsizei" counter="true"/> > <param name="buffer" type="GLuint *" output="true" count="n"/> > <glx ignore="true"/> > </function> > > <function name="GetBufferParameteriv" es1="1.1" es2="2.0"> > <param name="target" type="GLenum"/> > <param name="pname" type="GLenum"/> > <param name="params" type="GLint *" output="true" variable_param="pname"/> > <glx ignore="true"/> > 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. > */ > > #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) > @@ -187,20 +188,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 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, > + const struct marshal_cmd_GenBuffers *cmd) > +{ > + assert(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(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; > }; > > diff --git a/src/mesa/main/marshal.h b/src/mesa/main/marshal.h > index ddfa834..8a8b8da 100644 > --- a/src/mesa/main/marshal.h > +++ b/src/mesa/main/marshal.h > @@ -174,20 +174,23 @@ _mesa_glthread_is_compat_bind_vertex_array(const struct gl_context *ctx) > return ctx->API != API_OPENGL_CORE; > } > > struct marshal_cmd_Enable; > struct marshal_cmd_ShaderSource; > struct marshal_cmd_Flush; > struct marshal_cmd_BindBuffer; > struct marshal_cmd_BufferData; > struct marshal_cmd_BufferSubData; > struct marshal_cmd_ClearBufferfv; > +struct marshal_cmd_CreateBuffers; > +struct marshal_cmd_GenBuffers; > +struct marshal_cmd_DeleteBuffers; > > void > _mesa_unmarshal_Enable(struct gl_context *ctx, > const struct marshal_cmd_Enable *cmd); > > void GLAPIENTRY > _mesa_marshal_Enable(GLenum cap); > > void GLAPIENTRY > _mesa_marshal_ShaderSource(GLuint shader, GLsizei count, > @@ -228,11 +231,32 @@ _mesa_marshal_BufferSubData(GLenum target, GLintptr offset, GLsizeiptr size, > const GLvoid * data); > > void > _mesa_unmarshal_ClearBufferfv(struct gl_context *ctx, > const struct marshal_cmd_ClearBufferfv *cmd); > > void GLAPIENTRY > _mesa_marshal_ClearBufferfv(GLenum buffer, GLint drawbuffer, > const GLfloat *value); > > +void GLAPIENTRY > +_mesa_marshal_CreateBuffers(GLsizei n, GLuint * buffers); > + > +void > +_mesa_unmarshal_CreateBuffers(struct gl_context *ctx, > + const struct marshal_cmd_CreateBuffers *cmd); > + > +void GLAPIENTRY > +_mesa_marshal_GenBuffers(GLsizei n, GLuint * buffer); > + > +void > +_mesa_unmarshal_GenBuffers(struct gl_context *ctx, > + const struct marshal_cmd_GenBuffers *cmd); > + > +void GLAPIENTRY > +_mesa_marshal_DeleteBuffers(GLsizei n, const GLuint * buffer); > + > +void > +_mesa_unmarshal_DeleteBuffers(struct gl_context *ctx, > + const struct marshal_cmd_DeleteBuffers *cmd); > + > #endif /* MARSHAL_H */ > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > index 0cb0024..7b9334c 100644 > --- a/src/mesa/main/mtypes.h > +++ b/src/mesa/main/mtypes.h > @@ -3249,20 +3249,25 @@ struct gl_shared_state > struct gl_program *DefaultVertexProgram; > struct gl_program *DefaultFragmentProgram; > /*@}*/ > > /* GL_ATI_fragment_shader */ > struct _mesa_HashTable *ATIShaders; > struct ati_fragment_shader *DefaultFragmentShader; > > struct _mesa_HashTable *BufferObjects; > > + /**< shadow of BufferObjects to track buffer in application thread when > + * glthread is enabled > + */ > + struct _mesa_HashTable *ShadowBufferObjects; > + The hash value is never used, so this should be a set, not a hash table. However, seeing as this patch makes Create/GenBuffers and DeleteBuffers synchronous, do we need a shadow hash table? When is it not in sync with Shared->BufferObjects? > /** Table of both gl_shader and gl_shader_program objects */ > struct _mesa_HashTable *ShaderObjects; > > /* GL_EXT_framebuffer_object */ > struct _mesa_HashTable *RenderBuffers; > struct _mesa_HashTable *FrameBuffers; > > /* GL_ARB_sync */ > struct set *SyncObjects; > > diff --git a/src/mesa/main/shared.c b/src/mesa/main/shared.c > index 6926d40..5766a9f 100644 > --- a/src/mesa/main/shared.c > +++ b/src/mesa/main/shared.c > @@ -75,20 +75,22 @@ _mesa_alloc_shared_state(struct gl_context *ctx) > shared->DefaultFragmentProgram = > ctx->Driver.NewProgram(ctx, GL_FRAGMENT_PROGRAM_ARB, 0, true); > > shared->ATIShaders = _mesa_NewHashTable(); > shared->DefaultFragmentShader = _mesa_new_ati_fragment_shader(ctx, 0); > > shared->ShaderObjects = _mesa_NewHashTable(); > > shared->BufferObjects = _mesa_NewHashTable(); > > + shared->ShadowBufferObjects = _mesa_NewHashTable(); > + > /* GL_ARB_sampler_objects */ > shared->SamplerObjects = _mesa_NewHashTable(); > > /* GL_ARB_bindless_texture */ > _mesa_init_shared_handles(shared); > > /* Allocate the default buffer object */ > shared->NullBufferObj = ctx->Driver.NewBufferObject(ctx, 0); > > /* Create default texture objects */ > @@ -290,20 +292,29 @@ delete_renderbuffer_cb(GLuint id, void *data, void *userData) > static void > delete_sampler_object_cb(GLuint id, void *data, void *userData) > { > struct gl_context *ctx = (struct gl_context *) userData; > struct gl_sampler_object *sampObj = (struct gl_sampler_object *) data; > _mesa_reference_sampler_object(ctx, &sampObj, NULL); > } > > > /** > + * Dummy Callback. Called by _mesa_HashDeleteAll() > + */ > +static void > +delete_dummy_cb(GLuint id, void *data, void *userData) > +{ > +} > + > + > +/** > * Deallocate a shared state object and all children structures. > * > * \param ctx GL context. > * \param shared shared state pointer. > * > * Frees the display lists, the texture objects (calling the driver texture > * deletion callback to free its private data) and the vertex programs, as well > * as their hash tables. > * > * \sa alloc_shared_state(). > @@ -337,20 +348,23 @@ free_shared_state(struct gl_context *ctx, struct gl_shared_state *shared) > _mesa_reference_program(ctx, &shared->DefaultVertexProgram, NULL); > _mesa_reference_program(ctx, &shared->DefaultFragmentProgram, NULL); > > _mesa_HashDeleteAll(shared->ATIShaders, delete_fragshader_cb, ctx); > _mesa_DeleteHashTable(shared->ATIShaders); > _mesa_delete_ati_fragment_shader(ctx, shared->DefaultFragmentShader); > > _mesa_HashDeleteAll(shared->BufferObjects, delete_bufferobj_cb, ctx); > _mesa_DeleteHashTable(shared->BufferObjects); > > + _mesa_HashDeleteAll(shared->ShadowBufferObjects, delete_dummy_cb, ctx); > + _mesa_DeleteHashTable(shared->ShadowBufferObjects); > + > _mesa_HashDeleteAll(shared->FrameBuffers, delete_framebuffer_cb, ctx); > _mesa_DeleteHashTable(shared->FrameBuffers); > _mesa_HashDeleteAll(shared->RenderBuffers, delete_renderbuffer_cb, ctx); > _mesa_DeleteHashTable(shared->RenderBuffers); > > _mesa_reference_buffer_object(ctx, &shared->NullBufferObj, NULL); > > { > struct set_entry *entry; > > </blockquote></div></div>