[Mesa-dev] [PATCH 1/4] mesa: only lock framebuffer in compat profile
Fredrik Höglund
fredrik at kde.org
Mon Apr 24 12:51:36 UTC 2017
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.
> *
>
More information about the mesa-dev
mailing list