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" <fred...@kde.org> a écrit : > On Thursday 22 June 2017, Timothy Arceri wrote: > > From: Gregory Hainaut <gregory.hain...@gmail.com> > > > > 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 <gregory.hain...@gmail.com> > > --- > > 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> > > > > <!-- Buffer object functions --> > > > > - <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; > > > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev