[Mesa-dev] [PATCH 1/3] mesa/glthread: track buffer creation/destruction
Gregory Hainaut
gregory.hainaut at gmail.com
Sun Jun 25 15:19:22 UTC 2017
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" <fredrik at kde.org> a écrit :
> On Thursday 22 June 2017, Timothy Arceri wrote:
> > From: Gregory Hainaut <gregory.hainaut at 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.hainaut at 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;
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170625/cbf5d8b8/attachment-0001.html>
More information about the mesa-dev
mailing list