[Mesa-dev] [PATCH v2 07/24] mesa: glGetProgramResourceLocation

[Martin Peres]

On 01/04/15 15:14, Tapani Pälli wrote:
> Patch adds required helper functions to shaderapi.h and
> the actual implementation.
>
> corresponding Piglit test:
>     arb_program_interface_query-resource-location
>
> The added functionality can be tested by tests for following
> functions that are refactored by later patches:
>
>     GetAttribLocation
>     GetUniformLocation
>     GetFragDataLocation
>
> v2: code cleanup, changes to array element
>      syntax checking (Ilia Mirkin)
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>   src/mesa/main/program_resource.c | 81 +++++++++++++++++++++++++++++++++++++++-
>   src/mesa/main/shader_query.cpp   | 67 +++++++++++++++++++++++++++++++++
>   src/mesa/main/shaderapi.h        |  4 ++
>   3 files changed, 151 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
> index 638f5f2..03801ac 100644
> --- a/src/mesa/main/program_resource.c
> +++ b/src/mesa/main/program_resource.c
> @@ -278,11 +278,90 @@ _mesa_GetProgramResourceiv(GLuint program, GLenum programInterface,
>   {
>   }
>   
> +/**
> + * Function verifies syntax of given name for GetProgramResourceLocation
> + * and GetProgramResourceLocationIndex for the following cases:
> + *
> + * "array element portion of a string passed to GetProgramResourceLocation
> + * or GetProgramResourceLocationIndex must not have, a "+" sign, extra
> + * leading zeroes, or whitespace".
> + *
> + * Check is written to be compatible with GL_ARB_array_of_arrays.
> + */
> +static bool
> +invalid_array_element_syntax(const GLchar *name)
> +{
> +   char *first = strchr(name, '[');
> +   char *last = strrchr(name, '[');
> +
> +   if (!first)
> +      return false;
> +
> +   /* No '+' or ' ' allowed anywhere. */
> +   if (strchr(first, '+') || strchr(first, ' '))
> +      return true;
> +
> +   /* Check that last array index is 0. */
> +   if (last[1] == '0' && last[2] != ']')
> +      return true;
> +
> +   return false;
> +}
> +
> +static struct gl_shader_program *
> +lookup_linked_program(GLuint program, const char *caller)
> +{
> +   GET_CURRENT_CONTEXT(ctx);
Why not take ctx as a parameter? Performance-wise it should not matter 
but taking ctx as a parameter makes it very obvious that this is an 
internal function. Your call.
> +   struct gl_shader_program *prog =
> +      _mesa_lookup_shader_program_err(ctx, program, caller);
> +
> +   if (!prog)
> +      return NULL;
> +
> +   if (prog->LinkStatus == GL_FALSE) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not linked)",
> +                  caller);
> +      return NULL;
> +   }
> +   return prog;
> +}
> +
>   GLint GLAPIENTRY
>   _mesa_GetProgramResourceLocation(GLuint program, GLenum programInterface,
>                                    const GLchar *name)
>   {
> -   return -1;
> +   GET_CURRENT_CONTEXT(ctx);
> +   struct gl_shader_program *shProg =
> +      lookup_linked_program(program, "glGetProgramResourceLocation");
> +
> +   if (!shProg || !name || invalid_array_element_syntax(name))
> +      return -1;
> +
> +   /* Validate programInterface. */
> +   switch (programInterface) {
> +   case GL_UNIFORM:
> +   case GL_PROGRAM_INPUT:
> +   case GL_PROGRAM_OUTPUT:
> +      break;
> +
> +   /* For reference valid cases requiring additional extension support:
> +    * GL_ARB_shader_subroutine
> +    * GL_ARB_tessellation_shader
> +    * GL_ARB_compute_shader
> +    */
> +   case GL_VERTEX_SUBROUTINE_UNIFORM:
> +   case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
> +   case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
> +   case GL_GEOMETRY_SUBROUTINE_UNIFORM:
> +   case GL_FRAGMENT_SUBROUTINE_UNIFORM:
> +   case GL_COMPUTE_SUBROUTINE_UNIFORM:
> +
> +   default:
> +      _mesa_error(ctx, GL_INVALID_ENUM, "glGetProgramResourceLocation(%s %s)",
> +                  _mesa_lookup_enum_by_nr(programInterface), name);
> +   }
> +
> +   return _mesa_program_resource_location(shProg, programInterface, name);
>   }
>   
>   GLint GLAPIENTRY
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index ab61be9..f50ceb6 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -754,3 +754,70 @@ _mesa_get_program_resource_name(struct gl_shader_program *shProg,
>      }
>      return true;
>   }
> +
> +static GLint
> +program_resource_location(struct gl_shader_program *shProg,
> +                          struct gl_program_resource *res, const char *name)
> +{
> +   unsigned index, offset;
> +   int array_index = -1;
> +
> +   if (res->Type == GL_PROGRAM_INPUT || res->Type == GL_PROGRAM_OUTPUT) {
> +      array_index = array_index_of_resource(res, name);
> +      if (array_index < 0)
> +         return -1;
> +   }
> +
> +   /* VERT_ATTRIB_GENERIC0 and FRAG_RESULT_DATA0 are decremented as these
> +    * offsets are used internally to differentiate between built-in attributes
> +    * and user-defined attributes.
> +    */
> +   switch (res->Type) {
> +   case GL_PROGRAM_INPUT:
> +      return RESOURCE_VAR(res)->data.location + array_index - VERT_ATTRIB_GENERIC0;
> +   case GL_PROGRAM_OUTPUT:
> +      return RESOURCE_VAR(res)->data.location + array_index - FRAG_RESULT_DATA0;
> +   case GL_UNIFORM:
> +      index = _mesa_get_uniform_location(shProg, name, &offset);
> +
> +      if (index == GL_INVALID_INDEX)
> +         return -1;
> +
> +      /* From the GL_ARB_uniform_buffer_object spec:
> +       *
> +       *     "The value -1 will be returned if <name> does not correspond to an
> +       *     active uniform variable name in <program>, if <name> is associated
> +       *     with a named uniform block, or if <name> starts with the reserved
> +       *     prefix "gl_"."
> +       */
> +      if (RESOURCE_UNI(res)->block_index != -1 ||
> +          RESOURCE_UNI(res)->atomic_buffer_index != -1)
> +         return -1;
> +
> +      /* location in remap table + array element offset */
> +      return RESOURCE_UNI(res)->remap_location + offset;
> +
> +   default:
> +      return -1;
> +   }
> +}
> +
> +/**
> + * Function implements following location queries:
> + *    glGetAttribLocation
> + *    glGetFragDataLocation
> + *    glGetUniformLocation
> + */
> +GLint
> +_mesa_program_resource_location(struct gl_shader_program *shProg,
> +                                GLenum interface, const char *name)
> +{
> +   struct gl_program_resource *res =
> +      _mesa_program_resource_find_name(shProg, interface, name);
> +
> +   /* Resource not found. */
> +   if (!res)
> +      return -1;
> +
> +   return program_resource_location(shProg, res, name);
> +}
> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
> index 7a7e3e9..73ebf60 100644
> --- a/src/mesa/main/shaderapi.h
> +++ b/src/mesa/main/shaderapi.h
> @@ -244,6 +244,10 @@ _mesa_get_program_resource_name(struct gl_shader_program *shProg,
>                                   GLsizei bufSize, GLsizei *length,
>                                   GLchar *name, const char *caller);
>   
> +extern GLint
> +_mesa_program_resource_location(struct gl_shader_program *shProg,
> +                                GLenum interface, const char *name);
> +
>   #ifdef __cplusplus
>   }
>   #endif

Looks good to me:

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>