[Mesa-dev] [PATCH 1/4] mesa: only lock framebuffer in compat profile
Timothy Arceri
tarceri at itsqueeze.com
Mon Apr 24 23:10:12 UTC 2017
On 24/04/17 22:51, Fredrik Höglund wrote:
> 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.
Hmm. This is a good point. I think I'll try this again using the bind
functions to tell the difference as intended and use a hash in the ctx
rather than shared state for the ARB version.
Thanks.
>
>> + } 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.
>> *
>>
>
More information about the mesa-dev
mailing list