[Mesa-dev] [PATCH 03/23] mesa: glGetProgramInterfaceiv
Ilia Mirkin
imirkin at alum.mit.edu
Fri Mar 13 12:57:10 PDT 2015
On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
> Patch adds required helper functions to shaderapi.h and
> the actual implementation.
>
> corresponding Piglit test:
> arb_program_interface_query-getprograminterfaceiv
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
> src/mesa/main/program_resource.c | 79 ++++++++++++++++++++++++++++++++++++++++
> src/mesa/main/shader_query.cpp | 58 +++++++++++++++++++++++++++++
> src/mesa/main/shaderapi.h | 7 ++++
> 3 files changed, 144 insertions(+)
>
> diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
> index b3b93aa..a9194d1 100644
> --- a/src/mesa/main/program_resource.c
> +++ b/src/mesa/main/program_resource.c
> @@ -23,12 +23,91 @@
> *
> */
>
> +#include "main/enums.h"
> +#include "main/macros.h"
> +#include "main/mtypes.h"
> +#include "main/shaderapi.h"
> +#include "main/shaderobj.h"
> #include "program_resource.h"
>
> void GLAPIENTRY
> _mesa_GetProgramInterfaceiv(GLuint program, GLenum programInterface,
> GLenum pname, GLint *params)
> {
> + GET_CURRENT_CONTEXT(ctx);
> + struct gl_program_resource *res;
> + unsigned i;
> + struct gl_shader_program *shProg =
> + _mesa_lookup_shader_program_err(ctx, program,
> + "glGetProgramInterfaceiv");
> + if (!shProg)
> + return;
*params not modified in this case...
> +
> + if (!params) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetProgramInterfaceiv(params NULL)");
> + return;
> + }
Or this one...
> +
> + res = shProg->ProgramResourceList;
> + *params = 0;
I glanced at the spec but didn't notice anything saying this one way
or the other... should this be set on error conditions? Perhaps
there's precedent in some GL spec? Either way, you should do it
consistently, either always set it or always not set it.
> +
> + /* Validate pname against interface. */
> + switch(pname) {
> + case GL_ACTIVE_RESOURCES:
> + for (i = 0; i < shProg->NumProgramResourceList; i++, res++)
While this works, I believe the common way to handle this is with
res[i]. Here and elsewhere.
> + if (res->Type == programInterface)
> + (*params)++;
> + break;
> + case GL_MAX_NAME_LENGTH:
> + if (programInterface == GL_ATOMIC_COUNTER_BUFFER) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetProgramInterfaceiv(%s pname %s)",
> + _mesa_lookup_enum_by_nr(programInterface),
> + _mesa_lookup_enum_by_nr(pname));
> + return;
> + }
> + /* Name length consists of base name, 3 additional chars '[0]' if
> + * resource is an array and finally 1 char for string terminator.
> + */
> + for (i = 0; i < shProg->NumProgramResourceList; i++, res++)
Please use braces... I find it jarring for a something that doesn't
have braces to contain something that does. Not sure if others feel
similarly.
> + if (res->Type == programInterface) {
> + const char *name = _mesa_program_resource_name(res);
> + unsigned array_size = _mesa_program_resource_array_size(res);
> + *params = MAX2(*params, strlen(name) +
> + (array_size > 0 ? 3 : 0) + 1);
You could just check for array_size... I think it becomes less awkward
to read that way. (It can't be negative since it's unsigned.)
> + }
> + break;
> + case GL_MAX_NUM_ACTIVE_VARIABLES:
> + switch (programInterface) {
> + case GL_UNIFORM_BLOCK:
> + case GL_ATOMIC_COUNTER_BUFFER:
> + for (i = 0; i < shProg->NumProgramResourceList; i++, res++) {
Same comment about res
> + if (res->Type == programInterface) {
> + if (res->Type == GL_UNIFORM_BLOCK) {
So you have completely different logic for 2 different case statements
jumbled into a single block. I'd just split it up.
> + struct gl_uniform_block *block =
> + (struct gl_uniform_block *) res->Data;
> + *params = MAX2(*params, block->NumUniforms);
> + } else {
> + struct gl_active_atomic_buffer *buffer =
> + (struct gl_active_atomic_buffer *) res->Data;
> + *params = MAX2(*params, buffer->NumUniforms);
> + }
> + }
> + }
> + break;
> + default:
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glGetProgramInterfaceiv(%s pname %s)",
> + _mesa_lookup_enum_by_nr(programInterface),
> + _mesa_lookup_enum_by_nr(pname));
> + };
> + break;
> + default:
> + _mesa_error(ctx, GL_INVALID_ENUM,
> + "glGetProgramInterfaceiv(pname %s)",
> + _mesa_lookup_enum_by_nr(pname));
> + }
> }
>
> GLuint GLAPIENTRY
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index df9081b..9df793e 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -34,11 +34,29 @@
> #include "shaderobj.h"
> #include "program/hash_table.h"
> #include "../glsl/program.h"
> +#include "uniforms.h"
> +#include "main/enums.h"
>
> extern "C" {
> #include "shaderapi.h"
> }
>
> +/**
> + * Declare convenience functions to return resource data in a given type.
> + * Warning! this is not type safe so be *very* careful when using these.
> + */
> +#define DECL_RESOURCE_FUNC(name, type)\
Space before \ is customary.
> + const type * RESOURCE_ ##name (gl_program_resource *res) {\
This should be at column 0 (or everything else indented more). Also
either have spaces on both sides of ## or neither side -- it's a
binary operator, not unary (like # is, for example).
> + assert(res->Data);\
> + return (type *) res->Data;\
> +}
> +
> +DECL_RESOURCE_FUNC(VAR, ir_variable);
> +DECL_RESOURCE_FUNC(UBO, gl_uniform_block);
> +DECL_RESOURCE_FUNC(UNI, gl_uniform_storage);
> +DECL_RESOURCE_FUNC(ATC, gl_active_atomic_buffer);
> +DECL_RESOURCE_FUNC(XFB, gl_transform_feedback_varying_info);
> +
> void GLAPIENTRY
> _mesa_BindAttribLocation(GLhandleARB program, GLuint index,
> const GLcharARB *name)
> @@ -498,3 +516,43 @@ _mesa_GetFragDataLocation(GLuint program, const GLchar *name)
>
> return -1;
> }
> +
> +const char*
> +_mesa_program_resource_name(struct gl_program_resource *res)
> +{
> + switch (res->Type) {
> + case GL_UNIFORM_BLOCK:
> + return RESOURCE_UBO(res)->Name;
> + case GL_TRANSFORM_FEEDBACK_VARYING:
> + return RESOURCE_XFB(res)->Name;
> + case GL_PROGRAM_INPUT:
> + case GL_PROGRAM_OUTPUT:
> + return RESOURCE_VAR(res)->name;
> + case GL_UNIFORM:
> + return RESOURCE_UNI(res)->name;
> + default:
> + assert(!"support for resource type not implemented");
> + }
> + return NULL;
> +}
> +
> +
> +unsigned
> +_mesa_program_resource_array_size(struct gl_program_resource *res)
> +{
> + switch (res->Type) {
> + case GL_TRANSFORM_FEEDBACK_VARYING:
> + return RESOURCE_XFB(res)->Size;
> + case GL_PROGRAM_INPUT:
> + case GL_PROGRAM_OUTPUT:
> + return RESOURCE_VAR(res)->data.max_array_access;
> + case GL_UNIFORM:
> + return RESOURCE_UNI(res)->array_elements;
> + case GL_ATOMIC_COUNTER_BUFFER:
> + case GL_UNIFORM_BLOCK:
Hm... you can definitely have a UBO array (and with ARB_gs5, dynamic
indexing into that array). Not sure about atomic counters but I think
you can as well. Or are they not treated as arrays by this extension?
Perhaps add a comment exactly what array size it's trying to
determine?
> + return 0;
> + default:
> + assert(!"support for resource type not implemented");
> + }
> + return 0;
> +}
> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
> index 047d256..6db52f7 100644
> --- a/src/mesa/main/shaderapi.h
> +++ b/src/mesa/main/shaderapi.h
> @@ -219,6 +219,13 @@ extern GLuint GLAPIENTRY
> _mesa_CreateShaderProgramv(GLenum type, GLsizei count,
> const GLchar* const *strings);
>
> +/* GL_ARB_program_resource_query */
> +extern const char*
> +_mesa_program_resource_name(struct gl_program_resource *res);
> +
> +extern unsigned
> +_mesa_program_resource_array_size(struct gl_program_resource *res);
> +
> #ifdef __cplusplus
> }
> #endif
> --
> 2.1.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list