[Mesa-dev,12/19] mesa: Refactor set_ubo_binding()

Submitted by Fredrik Höglund on April 21, 2014, 9:57 p.m.

Details

Message ID 1398117477-10313-13-git-send-email-fredrik@kde.org
State New
Headers show

Not browsing as part of any series.

Commit Message

Fredrik Höglund April 21, 2014, 9:57 p.m.
Make set_ubo_binding() just update the binding, and move the code
that does validation, flushes the vertices etc. into a new
bind_uniform_buffer() function.
---

v2: Document the difference between set_ubo_binding() and
    bind_uniform_buffer().

 src/mesa/main/bufferobj.c | 63 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 19 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
index 9a59a5a..856b246 100644
--- a/src/mesa/main/bufferobj.c
+++ b/src/mesa/main/bufferobj.c
@@ -2612,17 +2612,45 @@  _mesa_GetObjectParameterivAPPLE(GLenum objectType, GLuint name, GLenum pname,
    }
 }
 
+/**
+ * Binds a buffer object to a uniform buffer binding point.
+ *
+ * The caller is responsible for flushing vertices and updating
+ * NewDriverState.
+ */
 static void
 set_ubo_binding(struct gl_context *ctx,
-		int index,
-		struct gl_buffer_object *bufObj,
-		GLintptr offset,
-		GLsizeiptr size,
-		GLboolean autoSize)
+                struct gl_uniform_buffer_binding *binding,
+                struct gl_buffer_object *bufObj,
+                GLintptr offset,
+                GLsizeiptr size,
+                GLboolean autoSize)
+{
+   _mesa_reference_buffer_object(ctx, &binding->BufferObject, bufObj);
+
+   binding->Offset = offset;
+   binding->Size = size;
+   binding->AutomaticSize = autoSize;
+}
+
+/**
+ * Binds a buffer object to a uniform buffer binding point.
+ *
+ * Unlike set_ubo_binding(), this function also flushes vertices
+ * and updates NewDriverState.  It also checks if the binding
+ * has actually changed before updating it.
+ */
+static void
+bind_uniform_buffer(struct gl_context *ctx,
+                    GLuint index,
+                    struct gl_buffer_object *bufObj,
+                    GLintptr offset,
+                    GLsizeiptr size,
+                    GLboolean autoSize)
 {
-   struct gl_uniform_buffer_binding *binding;
+   struct gl_uniform_buffer_binding *binding =
+      &ctx->UniformBufferBindings[index];
 
-   binding = &ctx->UniformBufferBindings[index];
    if (binding->BufferObject == bufObj &&
        binding->Offset == offset &&
        binding->Size == size &&
@@ -2633,10 +2661,7 @@  set_ubo_binding(struct gl_context *ctx,
    FLUSH_VERTICES(ctx, 0);
    ctx->NewDriverState |= ctx->DriverFlags.NewUniformBuffer;
 
-   _mesa_reference_buffer_object(ctx, &binding->BufferObject, bufObj);
-   binding->Offset = offset;
-   binding->Size = size;
-   binding->AutomaticSize = autoSize;
+   set_ubo_binding(ctx, binding, bufObj, offset, size, autoSize);
 }
 
 /**
@@ -2665,13 +2690,12 @@  bind_buffer_range_uniform_buffer(struct gl_context *ctx,
       return;
    }
 
-   if (bufObj == ctx->Shared->NullBufferObj) {
-      offset = -1;
-      size = -1;
-   }
-
    _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, bufObj);
-   set_ubo_binding(ctx, index, bufObj, offset, size, GL_FALSE);
+
+   if (bufObj == ctx->Shared->NullBufferObj)
+      bind_uniform_buffer(ctx, index, bufObj, -1, -1, GL_TRUE);
+   else
+      bind_uniform_buffer(ctx, index, bufObj, offset, size, GL_FALSE);
 }
 
 
@@ -2690,10 +2714,11 @@  bind_buffer_base_uniform_buffer(struct gl_context *ctx,
    }
 
    _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, bufObj);
+
    if (bufObj == ctx->Shared->NullBufferObj)
-      set_ubo_binding(ctx, index, bufObj, -1, -1, GL_TRUE);
+      bind_uniform_buffer(ctx, index, bufObj, -1, -1, GL_TRUE);
    else
-      set_ubo_binding(ctx, index, bufObj, 0, 0, GL_TRUE);
+      bind_uniform_buffer(ctx, index, bufObj, 0, 0, GL_TRUE);
 }
 
 /**

Comments

On 04/21/2014 02:57 PM, Fredrik Höglund wrote:
> Make set_ubo_binding() just update the binding, and move the code
> that does validation, flushes the vertices etc. into a new
> bind_uniform_buffer() function.
> ---
> 
> v2: Document the difference between set_ubo_binding() and
>     bind_uniform_buffer().
> 
>  src/mesa/main/bufferobj.c | 63 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> index 9a59a5a..856b246 100644
> --- a/src/mesa/main/bufferobj.c
> +++ b/src/mesa/main/bufferobj.c
> @@ -2612,17 +2612,45 @@ _mesa_GetObjectParameterivAPPLE(GLenum objectType, GLuint name, GLenum pname,
>     }
>  }
>  
> +/**
> + * Binds a buffer object to a uniform buffer binding point.
> + *
> + * The caller is responsible for flushing vertices and updating
> + * NewDriverState.
> + */
>  static void
>  set_ubo_binding(struct gl_context *ctx,
> -		int index,
> -		struct gl_buffer_object *bufObj,
> -		GLintptr offset,
> -		GLsizeiptr size,
> -		GLboolean autoSize)
> +                struct gl_uniform_buffer_binding *binding,
> +                struct gl_buffer_object *bufObj,
> +                GLintptr offset,
> +                GLsizeiptr size,
> +                GLboolean autoSize)

Either in this patch or in a follow-up patch, I think autoSize (and
gl_uniform_buffer_binding::AutomaticSize) should be bool.  It's not
directly visible to the API.

> +{
> +   _mesa_reference_buffer_object(ctx, &binding->BufferObject, bufObj);
> +
> +   binding->Offset = offset;
> +   binding->Size = size;
> +   binding->AutomaticSize = autoSize;
> +}
> +
> +/**
> + * Binds a buffer object to a uniform buffer binding point.
> + *
> + * Unlike set_ubo_binding(), this function also flushes vertices
> + * and updates NewDriverState.  It also checks if the binding
> + * has actually changed before updating it.
> + */
> +static void
> +bind_uniform_buffer(struct gl_context *ctx,
> +                    GLuint index,
> +                    struct gl_buffer_object *bufObj,
> +                    GLintptr offset,
> +                    GLsizeiptr size,
> +                    GLboolean autoSize)
>  {
> -   struct gl_uniform_buffer_binding *binding;
> +   struct gl_uniform_buffer_binding *binding =
> +      &ctx->UniformBufferBindings[index];
>  
> -   binding = &ctx->UniformBufferBindings[index];
>     if (binding->BufferObject == bufObj &&
>         binding->Offset == offset &&
>         binding->Size == size &&
> @@ -2633,10 +2661,7 @@ set_ubo_binding(struct gl_context *ctx,
>     FLUSH_VERTICES(ctx, 0);
>     ctx->NewDriverState |= ctx->DriverFlags.NewUniformBuffer;
>  
> -   _mesa_reference_buffer_object(ctx, &binding->BufferObject, bufObj);
> -   binding->Offset = offset;
> -   binding->Size = size;
> -   binding->AutomaticSize = autoSize;
> +   set_ubo_binding(ctx, binding, bufObj, offset, size, autoSize);
>  }
>  
>  /**
> @@ -2665,13 +2690,12 @@ bind_buffer_range_uniform_buffer(struct gl_context *ctx,
>        return;
>     }
>  
> -   if (bufObj == ctx->Shared->NullBufferObj) {
> -      offset = -1;
> -      size = -1;
> -   }
> -
>     _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, bufObj);
> -   set_ubo_binding(ctx, index, bufObj, offset, size, GL_FALSE);
> +
> +   if (bufObj == ctx->Shared->NullBufferObj)
> +      bind_uniform_buffer(ctx, index, bufObj, -1, -1, GL_TRUE);
> +   else
> +      bind_uniform_buffer(ctx, index, bufObj, offset, size, GL_FALSE);

This is a change.  Why is autoSize sometimes true?

>  }
>  
>  
> @@ -2690,10 +2714,11 @@ bind_buffer_base_uniform_buffer(struct gl_context *ctx,
>     }
>  
>     _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, bufObj);
> +
>     if (bufObj == ctx->Shared->NullBufferObj)
> -      set_ubo_binding(ctx, index, bufObj, -1, -1, GL_TRUE);
> +      bind_uniform_buffer(ctx, index, bufObj, -1, -1, GL_TRUE);
>     else
> -      set_ubo_binding(ctx, index, bufObj, 0, 0, GL_TRUE);
> +      bind_uniform_buffer(ctx, index, bufObj, 0, 0, GL_TRUE);
>  }
>  
>  /**
>
On Monday 28 April 2014, Ian Romanick wrote:
> On 04/21/2014 02:57 PM, Fredrik Höglund wrote:
> > Make set_ubo_binding() just update the binding, and move the code
> > that does validation, flushes the vertices etc. into a new
> > bind_uniform_buffer() function.
> > ---
> > 
> > v2: Document the difference between set_ubo_binding() and
> >     bind_uniform_buffer().
> > 
> >  src/mesa/main/bufferobj.c | 63 +++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 44 insertions(+), 19 deletions(-)
> > 
> > diff --git a/src/mesa/main/bufferobj.c b/src/mesa/main/bufferobj.c
> > index 9a59a5a..856b246 100644
> > --- a/src/mesa/main/bufferobj.c
> > +++ b/src/mesa/main/bufferobj.c
> > @@ -2612,17 +2612,45 @@ _mesa_GetObjectParameterivAPPLE(GLenum objectType, GLuint name, GLenum pname,
> >     }
> >  }
> >  
> > +/**
> > + * Binds a buffer object to a uniform buffer binding point.
> > + *
> > + * The caller is responsible for flushing vertices and updating
> > + * NewDriverState.
> > + */
> >  static void
> >  set_ubo_binding(struct gl_context *ctx,
> > -		int index,
> > -		struct gl_buffer_object *bufObj,
> > -		GLintptr offset,
> > -		GLsizeiptr size,
> > -		GLboolean autoSize)
> > +                struct gl_uniform_buffer_binding *binding,
> > +                struct gl_buffer_object *bufObj,
> > +                GLintptr offset,
> > +                GLsizeiptr size,
> > +                GLboolean autoSize)
> 
> Either in this patch or in a follow-up patch, I think autoSize (and
> gl_uniform_buffer_binding::AutomaticSize) should be bool.  It's not
> directly visible to the API.

Yeah, that's a good point.  I think I'll make that change in
a follow-up patch since it touches mtypes.h as well.

> > +{
> > +   _mesa_reference_buffer_object(ctx, &binding->BufferObject, bufObj);
> > +
> > +   binding->Offset = offset;
> > +   binding->Size = size;
> > +   binding->AutomaticSize = autoSize;
> > +}
> > +
> > +/**
> > + * Binds a buffer object to a uniform buffer binding point.
> > + *
> > + * Unlike set_ubo_binding(), this function also flushes vertices
> > + * and updates NewDriverState.  It also checks if the binding
> > + * has actually changed before updating it.
> > + */
> > +static void
> > +bind_uniform_buffer(struct gl_context *ctx,
> > +                    GLuint index,
> > +                    struct gl_buffer_object *bufObj,
> > +                    GLintptr offset,
> > +                    GLsizeiptr size,
> > +                    GLboolean autoSize)
> >  {
> > -   struct gl_uniform_buffer_binding *binding;
> > +   struct gl_uniform_buffer_binding *binding =
> > +      &ctx->UniformBufferBindings[index];
> >  
> > -   binding = &ctx->UniformBufferBindings[index];
> >     if (binding->BufferObject == bufObj &&
> >         binding->Offset == offset &&
> >         binding->Size == size &&
> > @@ -2633,10 +2661,7 @@ set_ubo_binding(struct gl_context *ctx,
> >     FLUSH_VERTICES(ctx, 0);
> >     ctx->NewDriverState |= ctx->DriverFlags.NewUniformBuffer;
> >  
> > -   _mesa_reference_buffer_object(ctx, &binding->BufferObject, bufObj);
> > -   binding->Offset = offset;
> > -   binding->Size = size;
> > -   binding->AutomaticSize = autoSize;
> > +   set_ubo_binding(ctx, binding, bufObj, offset, size, autoSize);
> >  }
> >  
> >  /**
> > @@ -2665,13 +2690,12 @@ bind_buffer_range_uniform_buffer(struct gl_context *ctx,
> >        return;
> >     }
> >  
> > -   if (bufObj == ctx->Shared->NullBufferObj) {
> > -      offset = -1;
> > -      size = -1;
> > -   }
> > -
> >     _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, bufObj);
> > -   set_ubo_binding(ctx, index, bufObj, offset, size, GL_FALSE);
> > +
> > +   if (bufObj == ctx->Shared->NullBufferObj)
> > +      bind_uniform_buffer(ctx, index, bufObj, -1, -1, GL_TRUE);
> > +   else
> > +      bind_uniform_buffer(ctx, index, bufObj, offset, size, GL_FALSE);
> 
> This is a change.  Why is autoSize sometimes true?

It's supposed to indicate whether the driver should use the Size field
in the gl_uniform_buffer_binding or compute the size automatically.

I don't remember why I made that change, but AutomaticSize is only
used when a non-null buffer object is bound, so it doesn't matter
what it's set to when unbinding a buffer.

I'll restore the previous behavior before I push the patch.

> >  }
> >  
> >  
> > @@ -2690,10 +2714,11 @@ bind_buffer_base_uniform_buffer(struct gl_context *ctx,
> >     }
> >  
> >     _mesa_reference_buffer_object(ctx, &ctx->UniformBuffer, bufObj);
> > +
> >     if (bufObj == ctx->Shared->NullBufferObj)
> > -      set_ubo_binding(ctx, index, bufObj, -1, -1, GL_TRUE);
> > +      bind_uniform_buffer(ctx, index, bufObj, -1, -1, GL_TRUE);
> >     else
> > -      set_ubo_binding(ctx, index, bufObj, 0, 0, GL_TRUE);
> > +      bind_uniform_buffer(ctx, index, bufObj, 0, 0, GL_TRUE);
> >  }
> >  
> >  /**
> > 
> 
>