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

[Brian Paul]

On 10/16/2011 04:37 PM, vlj wrote:
> ---
>   src/mesa/main/config.h          |    5 ++++
>   src/mesa/main/mtypes.h          |   51 +++++++++++++++++++++++++++++++++++++++
>   src/mesa/main/shaderobj.c       |   10 +++++++
>   src/mesa/program/prog_uniform.h |    1 +
>   4 files changed, 67 insertions(+), 0 deletions(-)
>
> diff --git a/src/mesa/main/config.h b/src/mesa/main/config.h
> index fffb1a7..4221521 100644
> --- a/src/mesa/main/config.h
> +++ b/src/mesa/main/config.h
> @@ -354,5 +354,10 @@
>    */
>   #define MAX_CLIPPED_VERTICES ((2 * (6 + MAX_CLIP_PLANES))+1)
>
> +/**
> + * UBO Variables
> + */
> +#define MAX_UBO_IN_SHADER 8
> +#define MAX_VARIABLES_IN_UBO 8
>
>   #endif /* MESA_CONFIG_H_INCLUDED */
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index f2eb889..4ef097f 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
> +{
> +    struct gl_buffer_object *UniformObj;
> +    GLint ActiveUBO;
> +};
>
>   /**
>    * Feedback buffer state
> @@ -2139,6 +2147,37 @@ struct gl_sl_pragmas
>   };
>
>
> +struct UBOVariableInfo

This structure name isn't consistent with others.  I'd call it 
gl_ubo_variable_info.


> +{
> +   char* Name;
> +   const struct glsl_type* Type;
> +   GLuint Offset;
> +   GLuint Stride;

Offset and Stride fields should probably have comments to indicate 
what the units are (bytes or ???).


> +   GLuint IndexInUBO;
> +   struct ubo* UBO;
> +};
> +
> +enum UBOLayout {
> +   packed,
> +   shared,
> +   std140,
> +};
> +
> +enum UBOMatrixLayout {
> +   rowmajor,
> +   columnmajor,
> +};

Enum values should be in all caps and have some kind of prefix.  For 
example:

enum gl_ubo_matrix_layout
{
    UBO_MATRIX_LAYOUT_ROW_MAJOR,
    UBO_MATRIX_LAYOUT_COLUMN_MAJOR
};

I know it's more typing, but it'll save time for all the people who 
will read this in the future.


> +struct ubo {
> +   char* Name;
> +   GLuint Index;
> +   struct UBOVariableInfo* StorageLayout;
> +   GLuint Layout; /** packed, shared or std140 */
> +   GLuint MatrixLayout; /** rowmajor or columnmajor */
> +   GLuint NumberOfVariables; /**<  number of UBOVariableInfo in StorageLayout */
> +   GLuint BoundBuffer;
> +};


The struct above should have a comment explaining exactly what it is 
since it's not immediately obvious.  Also, it should be called gl_ubo 
to be consistent.


> +
>   /**
>    * A GLSL vertex or fragment shader object.
>    */
> @@ -2163,6 +2202,8 @@ 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;
> +   unsigned UBOCount;
>   };
>
>
> @@ -2203,6 +2244,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;
> +   struct UBOVariableInfo **IndexedUniformsInUBO; /**<  Flat array that indexes all variables from UBO */
> +   unsigned UBOVariableCount;
>      struct gl_program_parameter_list *Varying;
>      GLboolean LinkStatus;   /**<  GL_LINK_STATUS */
>      GLboolean Validated;
> @@ -2219,6 +2262,11 @@ struct gl_shader_program
>       * \c NULL.
>       */
>      struct gl_shader *_LinkedShaders[MESA_SHADER_TYPES];
> +   /**
> +    * Program scope Uniform Buffer Object
> +    */
> +   struct ubo* UniformBufferObject[MAX_UBO_IN_SHADER];
> +   unsigned UBOCount;
>   };
>
>
> @@ -3285,6 +3333,9 @@ struct gl_context
>      struct gl_pixelstore_attrib	DefaultPacking;	/**<  Default params */
>      /*@}*/
>
> +   /** \name Attributes for UBO */
> +   struct gl_ubo_attribs UniformBufferObject;
> +
>      /** \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..a98fef7 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -123,8 +123,18 @@ _mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type)
>   static void
>   _mesa_delete_shader(struct gl_context *ctx, struct gl_shader *sh)
>   {
> +   unsigned i,j;
>      if (sh->Source)
>         free((void *) sh->Source);
> +   for (i = 0; i<  sh->UBOCount; i++) {
> +      free(sh->UniformBufferObjects[i].Name);
> +      for (j = 0; j<  sh->UniformBufferObjects[i].NumberOfVariables;j++) {
> +         free(sh->UniformBufferObjects[i].StorageLayout[j].Name);
> +      }
> +      free(sh->UniformBufferObjects[i].StorageLayout);
> +   }
> +   if(sh->UniformBufferObjects)

Please put a space between 'if' and '('.


> +      free(sh->UniformBufferObjects);
>      _mesa_reference_program(ctx,&sh->Program, NULL);
>      ralloc_free(sh);
>   }
> diff --git a/src/mesa/program/prog_uniform.h b/src/mesa/program/prog_uniform.h
> index 67f7800..cf00410 100644
> --- a/src/mesa/program/prog_uniform.h
> +++ b/src/mesa/program/prog_uniform.h
> @@ -52,6 +52,7 @@ struct gl_uniform
>      GLint GeomPos;
>      GLboolean Initialized;   /**<  For debug.  Has this uniform been set? */
>      const struct glsl_type *Type;
> +   struct UBOVariableInfo* UBOInformation;
>   };
>
>