On 03/07/2014 12:55 PM, Tapani Pälli wrote:
> Patch adds a remap table for uniforms that is used to provide a mapping
> from application specified uniform location to actual location in the
> UniformStorage. Existing UniformLocationBaseScale usage is removed as
> table can be used to set sequential values for array uniform elements.
>
> This mapping helps to implement GL_ARB_explicit_uniform_location so that
> uniforms locations can be reorganized and handled in a more easy manner.
>
> Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
A couple minor comments below. With those fixed, this patch is
Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
Might give others a day or two to review too...
I look forward to seeing the patches to implement
GL_ARB_explicit_uniform_location. That will be a nice addition for our
next major release. :)
> ---
> src/glsl/ir_uniform.h | 6 ++++++
> src/glsl/link_uniforms.cpp | 38 +++++++++++++++++++++++++++-----------
> src/mesa/main/mtypes.h | 22 +++++++---------------
> src/mesa/main/shaderobj.c | 7 ++++++-
> src/mesa/main/uniform_query.cpp | 7 +++++++
> src/mesa/main/uniforms.h | 31 ++++++++++++++++++-------------
> 6 files changed, 71 insertions(+), 40 deletions(-)
>
> diff --git a/src/glsl/ir_uniform.h b/src/glsl/ir_uniform.h
> index 7508f79..3508509 100644
> --- a/src/glsl/ir_uniform.h
> +++ b/src/glsl/ir_uniform.h
> @@ -178,6 +178,12 @@ struct gl_uniform_storage {
> * an atomic counter.
> */
> int atomic_buffer_index;
> +
> + /**
> + * The 'base location' for this uniform in the uniform remap table. For
> + * arrays this is the first element in the array.
> + */
> + unsigned remap_location;
> };
>
> #ifdef __cplusplus
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index 1c451e7..1607555 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -799,6 +799,10 @@ link_assign_uniform_locations(struct gl_shader_program
> *prog)
> prog->UniformStorage = NULL;
> prog->NumUserUniformStorage = 0;
>
> + ralloc_free(prog->UniformRemapTable);
> + prog->UniformRemapTable = NULL;
> + prog->NumUniformRemapTable = 0;
> +
> if (prog->UniformHash != NULL) {
> prog->UniformHash->clear();
> } else {
> @@ -911,19 +915,31 @@ link_assign_uniform_locations(struct gl_shader_program
> *prog)
> sizeof(prog->_LinkedShaders[i]->SamplerTargets));
> }
>
> - /* Determine the size of the largest uniform array queryable via
> - * glGetUniformLocation. Using this as the location scale guarantees that
> - * there is enough "room" for the array index to be stored in the low
> order
> - * part of the uniform location. It also makes the locations be more
> - * tightly packed.
> - */
> - unsigned max_array_size = 1;
> + /* Build the uniform remap table that is used to set/get uniform
> locations */
> for (unsigned i = 0; i < num_user_uniforms; i++) {
> - if (uniforms[i].array_elements > max_array_size)
> - max_array_size = uniforms[i].array_elements;
> - }
>
> - prog->UniformLocationBaseScale = max_array_size;
> + /* how many new entries for this uniform? */
> + unsigned entries = 1;
> +
> + if (uniforms[i].array_elements > 0)
> + entries = uniforms[i].array_elements;
I'd do this as
const unsigned entries = MAX2(1, uniforms[i].array_elements);
> +
> + /* resize remap table to fit new entries */
> + prog->UniformRemapTable =
> + reralloc(prog,
> + prog->UniformRemapTable,
> + gl_uniform_storage *,
> + prog->NumUniformRemapTable + entries);
> +
> + /* set pointers for this uniform */
> + for (unsigned j = 0; j < entries; j++)
> + prog->UniformRemapTable[prog->NumUniformRemapTable+j] =
> &uniforms[i];
> +
> + /* set the base location in remap table for the uniform */
> + uniforms[i].remap_location = prog->NumUniformRemapTable;
> +
> + prog->NumUniformRemapTable += entries;
> + }
>
> #ifndef NDEBUG
> for (unsigned i = 0; i < num_user_uniforms; i++) {
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index d05649c..3133ea5 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2702,6 +2702,13 @@ struct gl_shader_program
> struct gl_uniform_storage *UniformStorage;
>
> /**
> + * This table is used to generate locations of uniforms
> + * (returned by \c glGetUniformLocation).
> + */
> + unsigned NumUniformRemapTable;
> + struct gl_uniform_storage **UniformRemapTable;
> +
> + /**
> * Size of the gl_ClipDistance array that is output from the last pipeline
> * stage before the fragment shader.
> */
> @@ -2711,21 +2718,6 @@ struct gl_shader_program
> unsigned NumUniformBlocks;
>
> /**
> - * Scale factor for the uniform base location
> - *
> - * This is used to generate locations (returned by \c
> glGetUniformLocation)
> - * of uniforms. The base location of the uniform is multiplied by this
> - * value, and the array index is added.
> - *
> - * \note
> - * Must be >= 1.
> - *
> - * \sa
> - * _mesa_uniform_merge_location_offset,
> _mesa_uniform_split_location_offset
> - */
> - unsigned UniformLocationBaseScale;
> -
> - /**
> * Indices into the _LinkedShaders's UniformBlocks[] array for each stage
> * they're used in, or -1.
> *
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index d5c3d80..b0f0bfa 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -285,7 +285,12 @@ _mesa_clear_shader_program_data(struct gl_context *ctx,
> ralloc_free(shProg->UniformStorage);
> shProg->NumUserUniformStorage = 0;
> shProg->UniformStorage = NULL;
> - shProg->UniformLocationBaseScale = 0;
> + }
> +
> + if (shProg->UniformRemapTable) {
> + ralloc_free(shProg->UniformRemapTable);
> + shProg->NumUniformRemapTable = 0;
> + shProg->UniformRemapTable = NULL;
> }
>
> if (shProg->UniformHash) {
> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
> index 8cc5da7..700a333 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -246,6 +246,13 @@ validate_uniform_parameters(struct gl_context *ctx,
> return false;
> }
>
> + /* Check that the given location is in bounds of uniform remap table. */
> + if (location >= (GLint) shProg->NumUniformRemapTable) {
> + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)",
> + caller, location);
> + return false;
> + }
> +
> _mesa_uniform_split_location_offset(shProg, location, loc, array_index);
>
> if (*loc >= shProg->NumUserUniformStorage) {
> diff --git a/src/mesa/main/uniforms.h b/src/mesa/main/uniforms.h
> index bd50fd9..d493d2d 100644
> --- a/src/mesa/main/uniforms.h
> +++ b/src/mesa/main/uniforms.h
> @@ -340,15 +340,14 @@ struct gl_builtin_uniform_desc {
> * element. We could insert dummy entries in the list for each array
> * element after [0] but that causes complications elsewhere.
> *
> - * We solve this problem by encoding two values in the location that's
> - * returned by glGetUniformLocation():
> - * a) index into gl_uniform_list::Uniforms[] for the uniform
> - * b) an array/field offset (0 for simple types)
> + * We solve this problem by creating multiple entries for uniform arrays
> + * in the UniformRemapTable so that their elements get sequential locations.
> + *
> + * Utility functions below offer functionality to split UniformRemapTable
> + * location in to location of the uniform in UniformStorage + offset to the
> + * array element (0 if not an array) and also merge it back again as the
> + * UniformRemapTable location.
> *
> - * These two values are encoded in the high and low halves of a GLint.
> - * By putting the uniform number in the high part and the offset in the
> - * low part, we can support the unofficial ability to index into arrays
> - * by adding offsets to the location value.
> */
> /*@{*/
> /**
> @@ -358,9 +357,8 @@ static inline GLint
> _mesa_uniform_merge_location_offset(const struct gl_shader_program *prog,
> unsigned base_location, unsigned offset)
^^^^^^
At least this parameter (and the offset parameter in
_mesa_uniform_split_location_offset) should be changed. It was a bad
name when I came up with it. :) Maybe uniform_array_index? We should
probably also change base_location too. Maybe storage_index?
> {
> - assert(prog->UniformLocationBaseScale >= 1);
> - assert(offset < prog->UniformLocationBaseScale);
> - return (base_location * prog->UniformLocationBaseScale) + offset;
> + /* location in remap table + array element offset */
> + return prog->UniformStorage[base_location].remap_location + offset;
> }
>
> /**
> @@ -371,8 +369,15 @@ _mesa_uniform_split_location_offset(const struct
> gl_shader_program *prog,
> GLint location, unsigned *base_location,
> unsigned *offset)
> {
> - *offset = location % prog->UniformLocationBaseScale;
> - *base_location = location / prog->UniformLocationBaseScale;
> + *base_location = prog->UniformRemapTable[location] - prog->UniformStorage;
> + *offset = location - prog->UniformRemapTable[location]->remap_location;
> +
> + /**
Don't need the Doxygen markup here. :)
> + * gl_uniform_storage in in UniformStorage with the calculated
> base_location
> + * must match with the entry in remap table
> + */
> + assert(&prog->UniformStorage[*base_location] ==
> + prog->UniformRemapTable[location]);
> }
> /*@}*/
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
