On Monday 24 April 2017, Timothy Arceri wrote: > From the EXT_framebuffer_object spec: > > "Framebuffer objects created with the commands defined > by the GL_EXT_framebuffer_object extension are defined > to be shared, while FBOs created with commands defined > by the OpenGL core or GL_ARB_framebuffer_object > extension are defined *not* to be shared. However, the > following functions are viewed as aliases (in particular > the opcodes for X are also the same) between the functions > of GL_EXT_framebuffer_object and > GL_ARB_framebuffer_object: > > ... > > Since the above pairs are aliases, the functions of a > pair are equivalent. Note that the functions > BindFramebuffer and BindFramebufferEXT are not aliases > and neither are the functions BindRenderbuffer and > BindRenderbufferEXT. Because object creation occurs > when the framebuffer object is bound for the first time, > a framebuffer object can be shared across contexts only > if it was first bound with BindFramebufferEXT. > Framebuffers first bound with BindFramebuffer may not > be shared across contexts. Framebuffer objects created > with BindFramebufferEXT may subsequently be bound using > BindFramebuffer. Framebuffer objects created with > BindFramebuffer may be bound with BindFramebufferEXT > provided they are bound to the same context they were > created on." > > So in theory we could have a flag that is set by the bind > functions to decide if to lock or not. However we only > expose GL_EXT_framebuffer_object in compat profile so this > change just uses that to decide if we should lock or not. > --- > src/mesa/main/fbobject.c | 45 ++++++++++++++++++++++++++-------- > src/mesa/main/framebuffer.c | 60 > +++++++++++++++++++++++++++++++-------------- > 2 files changed, 76 insertions(+), 29 deletions(-) > > diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c > index d486d01..0f2298d 100644 > --- a/src/mesa/main/fbobject.c > +++ b/src/mesa/main/fbobject.c > @@ -145,22 +145,28 @@ _mesa_lookup_renderbuffer_err(struct gl_context *ctx, > GLuint id, > * Helper routine for getting a gl_framebuffer. > */ > struct gl_framebuffer * > _mesa_lookup_framebuffer(struct gl_context *ctx, GLuint id) > { > struct gl_framebuffer *fb; > > if (id == 0) > return NULL; > > - fb = (struct gl_framebuffer *) > - _mesa_HashLookup(ctx->Shared->FrameBuffers, id); > + if (ctx->API != API_OPENGL_COMPAT) { > + fb = (struct gl_framebuffer *) > + _mesa_HashLookupLocked(ctx->Shared->FrameBuffers, id);
Aside from the issue of mixed core/compatibility share groups, the hash table is still in gl_shared_state, so it can still be accessed by multiple threads simultaneously. > + } else { > + fb = (struct gl_framebuffer *) > + _mesa_HashLookup(ctx->Shared->FrameBuffers, id); > + } > + > return fb; > } > > > /** > * A convenience function for direct state access that throws > * GL_INVALID_OPERATION if the framebuffer doesn't exist. > */ > struct gl_framebuffer * > _mesa_lookup_framebuffer_err(struct gl_context *ctx, GLuint id, > @@ -542,21 +548,22 @@ set_renderbuffer_attachment(struct gl_context *ctx, > * Attach a renderbuffer object to a framebuffer object. > */ > void > _mesa_FramebufferRenderbuffer_sw(struct gl_context *ctx, > struct gl_framebuffer *fb, > GLenum attachment, > struct gl_renderbuffer *rb) > { > struct gl_renderbuffer_attachment *att; > > - mtx_lock(&fb->Mutex); > + if (ctx->API == API_OPENGL_COMPAT) > + mtx_lock(&fb->Mutex); > > att = get_attachment(ctx, fb, attachment, NULL); > assert(att); > if (rb) { > set_renderbuffer_attachment(ctx, att, rb); > if (attachment == GL_DEPTH_STENCIL_ATTACHMENT) { > /* do stencil attachment here (depth already done above) */ > att = get_attachment(ctx, fb, GL_STENCIL_ATTACHMENT_EXT, NULL); > assert(att); > set_renderbuffer_attachment(ctx, att, rb); > @@ -568,21 +575,22 @@ _mesa_FramebufferRenderbuffer_sw(struct gl_context *ctx, > if (attachment == GL_DEPTH_STENCIL_ATTACHMENT) { > /* detach stencil (depth was detached above) */ > att = get_attachment(ctx, fb, GL_STENCIL_ATTACHMENT_EXT, NULL); > assert(att); > remove_attachment(ctx, att); > } > } > > invalidate_framebuffer(fb); > > - mtx_unlock(&fb->Mutex); > + if (ctx->API == API_OPENGL_COMPAT) > + mtx_unlock(&fb->Mutex); > } > > > /** > * Fallback for ctx->Driver.ValidateFramebuffer() > * Check if the renderbuffer's formats are supported by the software > * renderer. > * Drivers should probably override this. > */ > void > @@ -2583,21 +2591,28 @@ bind_framebuffer(GLenum target, GLuint framebuffer, > bool allow_user_names) > return; > } > > if (!newDrawFb) { > /* create new framebuffer object */ > newDrawFb = ctx->Driver.NewFramebuffer(ctx, framebuffer); > if (!newDrawFb) { > _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBindFramebufferEXT"); > return; > } > - _mesa_HashInsert(ctx->Shared->FrameBuffers, framebuffer, newDrawFb); > + > + if (ctx->API != API_OPENGL_COMPAT) { > + _mesa_HashInsertLocked(ctx->Shared->FrameBuffers, framebuffer, > + newDrawFb); > + } else { > + _mesa_HashInsert(ctx->Shared->FrameBuffers, framebuffer, > + newDrawFb); > + } > } > newReadFb = newDrawFb; > } > else { > /* Binding the window system framebuffer (which was originally set > * with MakeCurrent). > */ > newDrawFb = ctx->WinSysDrawBuffer; > newReadFb = ctx->WinSysReadBuffer; > } > @@ -2713,21 +2728,26 @@ _mesa_DeleteFramebuffers(GLsizei n, const GLuint > *framebuffers) > assert(fb->RefCount >= 2); > _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); > } > if (fb == ctx->ReadBuffer) { > /* bind default */ > assert(fb->RefCount >= 2); > _mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, 0); > } > > /* remove from hash table immediately, to free the ID */ > - _mesa_HashRemove(ctx->Shared->FrameBuffers, framebuffers[i]); > + if (ctx->API != API_OPENGL_COMPAT) { > + _mesa_HashRemoveLocked(ctx->Shared->FrameBuffers, > + framebuffers[i]); > + } else { > + _mesa_HashRemove(ctx->Shared->FrameBuffers, framebuffers[i]); > + } > > if (fb != &DummyFramebuffer) { > /* But the object will not be freed until it's no longer > * bound in any context. > */ > _mesa_reference_framebuffer(&fb, NULL); > } > } > } > } > @@ -2750,21 +2770,22 @@ create_framebuffers(GLsizei n, GLuint *framebuffers, > bool dsa) > const char *func = dsa ? "glCreateFramebuffers" : "glGenFramebuffers"; > > if (n < 0) { > _mesa_error(ctx, GL_INVALID_VALUE, "%s(n < 0)", func); > return; > } > > if (!framebuffers) > return; > > - _mesa_HashLockMutex(ctx->Shared->FrameBuffers); > + if (ctx->API == API_OPENGL_COMPAT) > + _mesa_HashLockMutex(ctx->Shared->FrameBuffers); > > first = _mesa_HashFindFreeKeyBlock(ctx->Shared->FrameBuffers, n); > > for (i = 0; i < n; i++) { > GLuint name = first + i; > framebuffers[i] = name; > > if (dsa) { > fb = ctx->Driver.NewFramebuffer(ctx, framebuffers[i]); > if (!fb) { > @@ -2772,21 +2793,22 @@ create_framebuffers(GLsizei n, GLuint *framebuffers, > bool dsa) > _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s", func); > return; > } > } > else > fb = &DummyFramebuffer; > > _mesa_HashInsertLocked(ctx->Shared->FrameBuffers, name, fb); > } > > - _mesa_HashUnlockMutex(ctx->Shared->FrameBuffers); > + if (ctx->API == API_OPENGL_COMPAT) > + _mesa_HashUnlockMutex(ctx->Shared->FrameBuffers); > } > > > void GLAPIENTRY > _mesa_GenFramebuffers(GLsizei n, GLuint *framebuffers) > { > create_framebuffers(n, framebuffers, false); > } > > > @@ -3215,21 +3237,23 @@ _mesa_framebuffer_texture(struct gl_context *ctx, > struct gl_framebuffer *fb, > } else { > _mesa_error(ctx, GL_INVALID_ENUM, > "%s(invalid attachment %s)", caller, > _mesa_enum_to_string(attachment)); > } > return; > } > > FLUSH_VERTICES(ctx, _NEW_BUFFERS); > > - mtx_lock(&fb->Mutex); > + if (ctx->API == API_OPENGL_COMPAT) > + mtx_lock(&fb->Mutex); > + > if (texObj) { > if (attachment == GL_DEPTH_ATTACHMENT && > texObj == fb->Attachment[BUFFER_STENCIL].Texture && > level == fb->Attachment[BUFFER_STENCIL].TextureLevel && > _mesa_tex_target_to_face(textarget) == > fb->Attachment[BUFFER_STENCIL].CubeMapFace && > layer == fb->Attachment[BUFFER_STENCIL].Zoffset) { > /* The texture object is already attached to the stencil attachment > * point. Don't create a new renderbuffer; just reuse the stencil > * attachment's. This is required to prevent a GL error in > @@ -3274,21 +3298,22 @@ _mesa_framebuffer_texture(struct gl_context *ctx, > struct gl_framebuffer *fb, > else { > remove_attachment(ctx, att); > if (attachment == GL_DEPTH_STENCIL_ATTACHMENT) { > assert(att == &fb->Attachment[BUFFER_DEPTH]); > remove_attachment(ctx, &fb->Attachment[BUFFER_STENCIL]); > } > } > > invalidate_framebuffer(fb); > > - mtx_unlock(&fb->Mutex); > + if (ctx->API == API_OPENGL_COMPAT) > + mtx_unlock(&fb->Mutex); > } > > > static void > framebuffer_texture_with_dims(int dims, GLenum target, > GLenum attachment, GLenum textarget, > GLuint texture, GLint level, GLint layer, > const char *caller) > { > GET_CURRENT_CONTEXT(ctx); > diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c > index 9002020..b42bfba 100644 > --- a/src/mesa/main/framebuffer.c > +++ b/src/mesa/main/framebuffer.c > @@ -234,42 +234,64 @@ _mesa_free_framebuffer_data(struct gl_framebuffer *fb) > > /** > * Set *ptr to point to fb, with refcounting and locking. > * This is normally only called from the _mesa_reference_framebuffer() macro > * when there's a real pointer change. > */ > void > _mesa_reference_framebuffer_(struct gl_framebuffer **ptr, > struct gl_framebuffer *fb) > { > - if (*ptr) { > - /* unreference old renderbuffer */ > - GLboolean deleteFlag = GL_FALSE; > - struct gl_framebuffer *oldFb = *ptr; > - > - mtx_lock(&oldFb->Mutex); > - assert(oldFb->RefCount > 0); > - oldFb->RefCount--; > - deleteFlag = (oldFb->RefCount == 0); > - mtx_unlock(&oldFb->Mutex); > + GET_CURRENT_CONTEXT(ctx); > + > + if (!ctx || ctx->API != API_OPENGL_COMPAT) { > + if (*ptr) { > + /* unreference old renderbuffer */ > + struct gl_framebuffer *oldFb = *ptr; > + > + assert(oldFb->RefCount > 0); > + oldFb->RefCount--; > > - if (deleteFlag) > - oldFb->Delete(oldFb); > + if (oldFb->RefCount == 0) > + oldFb->Delete(oldFb); > > - *ptr = NULL; > - } > + *ptr = NULL; > + } > > - if (fb) { > - mtx_lock(&fb->Mutex); > - fb->RefCount++; > - mtx_unlock(&fb->Mutex); > - *ptr = fb; > + if (fb) { > + fb->RefCount++; > + *ptr = fb; > + } > + } else { > + if (*ptr) { > + /* unreference old renderbuffer */ > + bool deleteFlag = false; > + struct gl_framebuffer *oldFb = *ptr; > + > + mtx_lock(&oldFb->Mutex); > + assert(oldFb->RefCount > 0); > + oldFb->RefCount--; > + deleteFlag = (oldFb->RefCount == 0); > + mtx_unlock(&oldFb->Mutex); > + > + if (deleteFlag) > + oldFb->Delete(oldFb); > + > + *ptr = NULL; > + } > + > + if (fb) { > + mtx_lock(&fb->Mutex); > + fb->RefCount++; > + mtx_unlock(&fb->Mutex); > + *ptr = fb; > + } > } > } > > > /** > * Resize the given framebuffer's renderbuffers to the new width and height. > * This should only be used for window-system framebuffers, not > * user-created renderbuffers (i.e. made with GL_EXT_framebuffer_object). > * This will typically be called directly from a device driver. > * > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev