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



_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to