[Mesa-dev] [PATCH 03/23] mesa: glGetProgramInterfaceiv
Tapani Pälli
tapani.palli at intel.com
Sun Mar 15 23:24:41 PDT 2015
On 03/13/2015 09:57 PM, Ilia Mirkin wrote:
> 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.
The value is undefined in error conditions. Reason I do it there is that
I did not want to repeat initialization of it for every for loop since
the loop declaration already is quite long but I can move it there.
>> +
>> + /* 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.
OK, for some reason I like it more minimal but can change.
>> + 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.)
OK
>> + }
>> + 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.
Sure. I really wish these were all just C++ classes as the logic is not
that different.
>> + 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).
thanks, will fix
>> + 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?
It is simply the size of array if resource is an array. We might need
some tests for the cases you mentioned as did not encounter any issues
with current tests we have.
>> + 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