[Mesa-dev] [PATCH 4/9] mesa: initial data structures for Uniform Buffer Objects

[Brian Paul]

On 09/23/2011 07:53 AM, vlj wrote:
> ---
>   src/mesa/main/mtypes.h    |   47 ++++++++++++++++++++++++++++++++++++++++++++-
>   src/mesa/main/shaderobj.c |    7 ++++++
>   2 files changed, 53 insertions(+), 1 deletions(-)
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index f2eb889..49ae2fa 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1696,6 +1696,14 @@ struct gl_array_attrib
>      struct gl_buffer_object *ElementArrayBufferObj;
>   };
>
> +/**
> + * UBO state
> + */
> +struct gl_ubo_attribs {

Structures in mtypes.h have the opening brace on a separate line.


> +    struct gl_buffer_object *UniformObj;
> +    GLint ActiveUBO;
> +};
> +
>
>   /**
>    * Feedback buffer state
> @@ -1840,6 +1848,10 @@ struct gl_program_parameter_list;
>   struct gl_uniform_list;
>
>
> +struct gl_uniform_buffer_object_list {
> +    unsigned count;
> +};

s/count/Count/


> +
>   /**
>    * Base class for any kind of program object
>    */
> @@ -1873,6 +1885,7 @@ struct gl_program
>      struct gl_program_parameter_list *Varying;
>      /** Vertex program user-defined attributes */
>      struct gl_program_parameter_list *Attributes;
> +   struct gl_uniform_buffer_object_list *Uniform_buffer_objects;
>
>      /** Map from sampler unit to texture unit (set by glUniform1i()) */
>      GLubyte SamplerUnits[MAX_SAMPLERS];
> @@ -2138,6 +2151,26 @@ struct gl_sl_pragmas
>      GLboolean Debug;     /**<  defaults off */
>   };
>
> +#define MAX_UBO_IN_SHADER 8
> +#define MAX_VARIABLES_IN_UBO 8

#defines like this are typically put in the config.h file.


> +
> +struct UBOVariableInfo {
> +   char* name;
> +   unsigned index;
> +   const struct glsl_type* Type;
> +   unsigned location;
> +   unsigned size;
> +   unsigned offset;
> +   unsigned stride;
> +};

Please capitalize the first letter of those members to match the 
conventions of other structures.

Also, we typically use the GL types in these structures, so "GLuint 
Stride", for example.


> +
> +struct ubo {
> +   char* name;
> +   unsigned index;
> +   struct UBOVariableInfo* storage_layout;
> +   unsigned number_of_variables;

Does number_of_variables indicate the size of the array pointed to by 
the storage_layout member?  There should be a comment explaining that.


> +   unsigned bound_buffer;
> +};
>
>   /**
>    * A GLSL vertex or fragment shader object.
> @@ -2163,9 +2196,10 @@ struct gl_shader
>      /** Shaders containing built-in functions that are used for linking. */
>      struct gl_shader *builtins_to_link[16];
>      unsigned num_builtins_to_link;
> +   struct ubo UniformBufferObjects[MAX_UBO_IN_SHADER];
> +   unsigned UBOCount;
>   };
>
> -

Undo whitespace change.


>   /**
>    * A GLSL program object.
>    * Basically a linked collection of vertex and fragment shaders.
> @@ -2203,6 +2237,8 @@ struct gl_shader_program
>      struct gl_fragment_program *FragmentProgram; /**<  Linked fragment prog */
>      struct gl_geometry_program *GeometryProgram; /**<  Linked geometry prog */
>      struct gl_uniform_list *Uniforms;
> +
> +

Undo whitespace change.


>      struct gl_program_parameter_list *Varying;
>      GLboolean LinkStatus;   /**<  GL_LINK_STATUS */
>      GLboolean Validated;
> @@ -2219,6 +2255,10 @@ struct gl_shader_program
>       * \c NULL.
>       */
>      struct gl_shader *_LinkedShaders[MESA_SHADER_TYPES];
> +
> +

Needless whitespace.


> +   struct ubo* UniformBufferObjects[MAX_UBO_IN_SHADER];
> +   unsigned ubo_count;
>   };
>
>
> @@ -3285,6 +3325,11 @@ struct gl_context
>      struct gl_pixelstore_attrib	DefaultPacking;	/**<  Default params */
>      /*@}*/
>
> +   /** \name Attributes for UBO */
> +   /*@{*/
> +   struct gl_ubo_attribs Uniform_Buffer_Object;
> +   /*@}*/

You don't need the @-grouping stuff for a single member.
Remove underscores from the member.


> +
>      /** \name Other assorted state (not pushed/popped on attribute stack) */
>      /*@{*/
>      struct gl_pixelmaps          PixelMaps;
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index f128648..b7e9467 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -125,6 +125,13 @@ _mesa_delete_shader(struct gl_context *ctx, struct gl_shader *sh)
>   {
>      if (sh->Source)
>         free((void *) sh->Source);
> +   for(unsigned i = 0; i<  sh->UBOCount; i++) {
> +      free(sh->UniformBufferObjects[i].name);
> +      for(unsigned j = 0; j<  sh->UniformBufferObjects[i].number_of_variables;j++) {
> +         free(sh->UniformBufferObjects[i].storage_layout[j].name);
> +      }
> +      free(sh->UniformBufferObjects[i].storage_layout);
> +   }

Please use space: "for ("

Don't declare the loop counters inside the for-loop.  It won't compile 
with MSVC's C compiler.


>      _mesa_reference_program(ctx,&sh->Program, NULL);
>      ralloc_free(sh);
>   }