[Mesa-dev] [PATCH] Make generation of framebuffer and renderbuffer id's threadsafe

[Brian Paul]

Hi Samuel,

The subject line should probably be something like:

"mesa: add locking in create_render_buffers() and create_framebuffers()"

Then, the comment body:

"""
This makes generation of framebuffer and renderbuffer id's threadsafe

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92633
"""


On 11/13/2015 03:57 AM, Samuel Maroy wrote:
> This should fix the issue described in
> https://bugs.freedesktop.org/show_bug.cgi?id=92633
>
> ---
>   src/mesa/main/fbobject.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index fe6bdc2..6398ff6 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1637,6 +1637,8 @@ create_render_buffers(struct gl_context *ctx, GLsizei n, GLuint *renderbuffers,
>      if (!renderbuffers)
>         return;
>
> +   mtx_lock(&ctx->Shared->Mutex);
> +
>      first = _mesa_HashFindFreeKeyBlock(ctx->Shared->RenderBuffers, n);
>
>      for (i = 0; i < n; i++) {
> @@ -1647,11 +1649,10 @@ create_render_buffers(struct gl_context *ctx, GLsizei n, GLuint *renderbuffers,
>            allocate_renderbuffer(ctx, name, func);

This function also calls mtx_lock(ctx->Shared->Mutex) so it will be a 
recursive lock.

In shared.c we init the mutex with:

    mtx_init(&shared->Mutex, mtx_plain);

So we should probably use (mtx_plain | mtx_recursive) there.

I think someone else spotted this a while ago but it looks like nothing 
was changed.  We should probably do an audit of our locking to check for 
other places where this might happen.



>         } else {
>            /* insert a dummy renderbuffer into the hash table */
> -         mtx_lock(&ctx->Shared->Mutex);
>            _mesa_HashInsert(ctx->Shared->RenderBuffers, name, &DummyRenderbuffer);
> -         mtx_unlock(&ctx->Shared->Mutex);
>         }
>      }
> +   mtx_unlock(&ctx->Shared->Mutex);

I'd insert one blank line before the new mtx_unlock().

>   }
>
>
> @@ -2650,6 +2651,7 @@ create_framebuffers(GLsizei n, GLuint *framebuffers, bool dsa)
>      if (!framebuffers)
>         return;
>
> +   mtx_lock(&ctx->Shared->Mutex);
>      first = _mesa_HashFindFreeKeyBlock(ctx->Shared->FrameBuffers, n);
>
>      for (i = 0; i < n; i++) {
> @@ -2660,16 +2662,17 @@ create_framebuffers(GLsizei n, GLuint *framebuffers, bool dsa)
>            fb = ctx->Driver.NewFramebuffer(ctx, framebuffers[i]);
>            if (!fb) {
>               _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s", func);
> -            return;
> +            goto beach;

Cute, but I think some people would prefer "goto cleanup" or something 
like that.

>            }
>         }
>         else
>            fb = &DummyFramebuffer;
>
> -      mtx_lock(&ctx->Shared->Mutex);
>         _mesa_HashInsert(ctx->Shared->FrameBuffers, name, fb);
> -      mtx_unlock(&ctx->Shared->Mutex);
>      }
> +
> +beach:
> +   mtx_unlock(&ctx->Shared->Mutex);
>   }
>

-Brian