[Mesa-dev] [PATCH V3 1/9] mesa: create _mesa_attach_renderbuffer_without_ref() helper

[Timothy Arceri]

On 11/04/17 03:11, Brian Paul wrote:
> On 04/07/2017 09:21 PM, Timothy Arceri wrote:
>> This will be used to take ownership of freashly created renderbuffers,
>> avoiding the need to call the reference function which requires
>> locking.
>>
>> V2: dereference any existing fb attachments and actually attach the
>>      new rb.
>>
>> v3: split out validation and attachment type/complete setting into
>>      a shared static function.
>> ---
>>   src/mesa/main/renderbuffer.c | 43
>> +++++++++++++++++++++++++++++++++++--------
>>   src/mesa/main/renderbuffer.h |  5 +++++
>>   2 files changed, 40 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/mesa/main/renderbuffer.c b/src/mesa/main/renderbuffer.c
>> index 4375b5b..627bdca 100644
>> --- a/src/mesa/main/renderbuffer.c
>> +++ b/src/mesa/main/renderbuffer.c
>> @@ -99,28 +99,24 @@ _mesa_new_renderbuffer(struct gl_context *ctx,
>> GLuint name)
>>    * free the object in the end.
>>    */
>>   void
>>   _mesa_delete_renderbuffer(struct gl_context *ctx, struct
>> gl_renderbuffer *rb)
>>   {
>>      mtx_destroy(&rb->Mutex);
>>      free(rb->Label);
>>      free(rb);
>>   }
>>
>> -
>> -/**
>> - * Attach a renderbuffer to a framebuffer.
>> - * \param bufferName  one of the BUFFER_x tokens
>> - */
>> -void
>> -_mesa_add_renderbuffer(struct gl_framebuffer *fb,
>> -                       gl_buffer_index bufferName, struct
>> gl_renderbuffer *rb)
>> +static void
>> +validate_and_init_renderbuffer_attachment(struct gl_framebuffer *fb,
>> +                                          gl_buffer_index bufferName,
>> +                                          struct gl_renderbuffer *rb)
>>   {
>>      assert(fb);
>>      assert(rb);
>>      assert(bufferName < BUFFER_COUNT);
>>
>>      /* There should be no previous renderbuffer on this attachment
>> point,
>>       * with the exception of depth/stencil since the same
>> renderbuffer may
>>       * be used for both.
>>       */
>>      assert(bufferName == BUFFER_DEPTH ||
>> @@ -130,20 +126,51 @@ _mesa_add_renderbuffer(struct gl_framebuffer *fb,
>>      /* winsys vs. user-created buffer cross check */
>>      if (_mesa_is_user_fbo(fb)) {
>>         assert(rb->Name);
>>      }
>>      else {
>>         assert(!rb->Name);
>>      }
>>
>>      fb->Attachment[bufferName].Type = GL_RENDERBUFFER_EXT;
>>      fb->Attachment[bufferName].Complete = GL_TRUE;
>> +}
>> +
>> +
>> +/**
>> + * Attach a renderbuffer to a framebuffer.
>> + * \param bufferName  one of the BUFFER_x tokens
>> + *
>> + * This function avoids adding a reference and is therefore intended
>> to be
>> + * used with a freashly created renderbuffer.
>
> "freshly"
>
>
>> + */
>> +void
>> +_mesa_add_renderbuffer_without_ref(struct gl_framebuffer *fb,
>> +                                   gl_buffer_index bufferName,
>> +                                   struct gl_renderbuffer *rb)
>
> I see you've already pushed this.

Yes the previous leak was causing issue in Gnome so I wanted to push 
these in case they were causing problems also.

> Still, I'd like to suggest a
> different name such as _mesa_own_renderbuffer() that stresses the
> transfer of ownership of the renderbuffer.

I pushed the other two trivial suggestions, as for the name I struggled 
a bit with this because we still want it to be obvious that it is a 
variant of _mesa_add_renderbuffer().

How about _mesa_add_and_own_renderbuffer() ??


IMO these would make more sense being called _mesa_attach_* rather than 
add. I can change that also is you agree?

Maybe:

_mesa_attach_and_own_rb()
_mesa_attach_and_reference_rb()

??


>
>
>> +{
>
> If this function should only be used with a "freshly created"
> renderbuffer, can we assert that its RefCount is one here?
>
> -Brian
>
>> +   validate_and_init_renderbuffer_attachment(fb, bufferName, rb);
>> +
>> +
>> _mesa_reference_renderbuffer(&fb->Attachment[bufferName].Renderbuffer,
>> +                                NULL);
>> +   fb->Attachment[bufferName].Renderbuffer = rb;
>> +}
>> +
>> +/**
>> + * Attach a renderbuffer to a framebuffer.
>> + * \param bufferName  one of the BUFFER_x tokens
>> + */
>> +void
>> +_mesa_add_renderbuffer(struct gl_framebuffer *fb,
>> +                       gl_buffer_index bufferName, struct
>> gl_renderbuffer *rb)
>> +{
>> +   validate_and_init_renderbuffer_attachment(fb, bufferName, rb);
>>
>> _mesa_reference_renderbuffer(&fb->Attachment[bufferName].Renderbuffer,
>> rb);
>>   }
>>
>>
>>   /**
>>    * Remove the named renderbuffer from the given framebuffer.
>>    * \param bufferName  one of the BUFFER_x tokens
>>    */
>>   void
>>   _mesa_remove_renderbuffer(struct gl_framebuffer *fb,
>> diff --git a/src/mesa/main/renderbuffer.h b/src/mesa/main/renderbuffer.h
>> index aa83120..a6f1439 100644
>> --- a/src/mesa/main/renderbuffer.h
>> +++ b/src/mesa/main/renderbuffer.h
>> @@ -40,20 +40,25 @@ struct gl_renderbuffer;
>>   extern void
>>   _mesa_init_renderbuffer(struct gl_renderbuffer *rb, GLuint name);
>>
>>   extern struct gl_renderbuffer *
>>   _mesa_new_renderbuffer(struct gl_context *ctx, GLuint name);
>>
>>   extern void
>>   _mesa_delete_renderbuffer(struct gl_context *ctx, struct
>> gl_renderbuffer *rb);
>>
>>   extern void
>> +_mesa_add_renderbuffer_without_ref(struct gl_framebuffer *fb,
>> +                                   gl_buffer_index bufferName,
>> +                                   struct gl_renderbuffer *rb);
>> +
>> +extern void
>>   _mesa_add_renderbuffer(struct gl_framebuffer *fb,
>>                          gl_buffer_index bufferName, struct
>> gl_renderbuffer *rb);
>>
>>   extern void
>>   _mesa_remove_renderbuffer(struct gl_framebuffer *fb,
>>                             gl_buffer_index bufferName);
>>
>>   extern void
>>   _mesa_reference_renderbuffer_(struct gl_renderbuffer **ptr,
>>                                 struct gl_renderbuffer *rb);
>>
>