[Mesa-dev] [PATCH 1/4] mesa: only lock framebuffer in compat profile
Fredrik Höglund
fredrik at kde.org
Tue Apr 25 18:10:18 UTC 2017
On Tuesday 25 April 2017, Timothy Arceri wrote:
> 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.
I've thought about that, but there are some problems with it:
1) ARB_framebuffer_object and EXT_framebuffer_object share the same
namespace.
2) Framebuffers are inserted into the hash table in GenFramebuffers,
and at that point we don't know if they will be shared or not.
3) Renderbuffers are always shared, so the _mesa_HashWalk in
_mesa_renderbuffer_storage needs to visit all framebuffers in all
contexts.
4) DeleteFramebuffers and DeleteFramebuffersEXT are aliases, and that
carries the implication that it is legal to delete a non-shared
framebuffer created in another context.
But it might be possible to keep inserting all framebuffers into the
shared hash table while having a second context-local hash table that
contains weak references to non-shared framebuffers.
> 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