[Mesa-dev] [PATCH 1/4] mesa: only lock framebuffer in compat profile

[Timothy Arceri]

On 24/04/17 20:06, Nicolai Hähnle wrote:
> On 24.04.2017 07:28, 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(-)
> [snip]
>> 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) {
> 
> When does !ctx happen, and does it really guarantee not having a 
> compatibility context?

When _mesa_make_current() is called with a NULL ctx and during some 
destruction calls. Looking at this again I think you are right it's 
probably more safe to do:

if (ctx && ctx->API != API_OPENGL_COMPAT)

I'll change this.

> 
> BTW, what are the rules for having contexts of different GL versions / 
> different compatibility vs. core type in the same share group?

Hmm ... it doesn't seem to say anything about this, maybe this isn't 
going to work.

> 
> Cheers,
> Nicolai
> 
> 
>> +      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.
>>   *
>>
> 
>