[Mesa-dev] [PATCH 08/23] mesa: implementation of glGetProgramResourceiv
Tapani Pälli
tapani.palli at intel.com
Wed Mar 18 03:24:37 PDT 2015
On 03/16/2015 08:30 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.
>>
>> The property query functionality can be tested with tests for
>> following functions that are refactored by later patches:
8<
>> +static GLint
>> +is_resource_referenced(struct gl_shader_program *shProg,
>> + struct gl_program_resource *res,
>> + GLuint index, uint8_t stage)
>> +{
>> + if (res->Type == GL_ATOMIC_COUNTER_BUFFER)
>> + return RESOURCE_ATC(res)->StageReferences[stage];
>> +
>> + if (res->Type == GL_UNIFORM_BLOCK)
>> + return shProg->UniformBlockStageIndex[stage][index] !=
>> + -1 ? 1 : 0;
>
> This is _really_ hard to read. On the bright side you can just change this to
>
> return foo != -1; and be done with it. This evaluates to 1/0.
ok, will change
>> +
>> + return (res->StageReferences & (1 << stage));
>
> return is not a function, no need for the parens around the returned value.
will fix
>> +}
>> +
>> +static bool
>> +get_buffer_property(struct gl_shader_program *shProg,
>> + struct gl_program_resource *res, const GLenum prop,
>> + GLint *val)
>> +{
>> + if (res->Type != GL_UNIFORM_BLOCK &&
>> + res->Type != GL_ATOMIC_COUNTER_BUFFER)
>> + return false;
>> +
>> + switch (prop) {
>> + case GL_BUFFER_BINDING:
>
> Gah! This function is disgusting (not really your fault). Would it
> look any less disgusting if you were to reverse the order, i.e.
>
> if (res->type == uniform) {
> ubo_type *ubo = RESOURCE_UBO(res);
> switch () {
> case foo:
> return ubo->bar;
> }
> } else {
> atc_type *atc = ...
> switch ...
> }
>
> There would be less annoying uniform/atomic repetition, and you'd lose
> the ambiguous if/elseif/no else case which can't logically happen but
> compiler might not notice situations.
>
> Anyways, this is *completely* optional, merely wanted to suggest it in
> case you liked it better.
I agree that it is far from pretty. I'll try your proposal and then
maybe just also have 2 separate functions for these, even though it kind
of sucks as these are very much alike, even the members could be renamed
to match so that it could be some kind of generic buffer resource struct.
>> + if (res->Type == GL_UNIFORM_BLOCK)
>> + *val = RESOURCE_UBO(res)->Binding;
>> + else if (res->Type == GL_ATOMIC_COUNTER_BUFFER)
>> + *val = RESOURCE_ATC(res)->Binding;
>> + return true;
>> + case GL_BUFFER_DATA_SIZE:
>> + if (res->Type == GL_UNIFORM_BLOCK)
>> + *val = RESOURCE_UBO(res)->UniformBufferSize;
>> + else if (res->Type == GL_ATOMIC_COUNTER_BUFFER)
>> + *val = RESOURCE_ATC(res)->MinimumSize;
>> + return true;
>> + case GL_NUM_ACTIVE_VARIABLES:
>> + if (res->Type == GL_UNIFORM_BLOCK)
>> + *val = RESOURCE_UBO(res)->NumUniforms;
>> + else if (res->Type == GL_ATOMIC_COUNTER_BUFFER)
>> + *val = RESOURCE_ATC(res)->NumUniforms;
>> + return true;
>> + case GL_ACTIVE_VARIABLES:
>> + if (res->Type == GL_ATOMIC_COUNTER_BUFFER) {
>> + int amount = RESOURCE_ATC(res)->NumUniforms;
>> + for (int i = 0; i < amount; i++)
>> + *val++ = RESOURCE_ATC(res)->Uniforms[i];
>> + return true;
>> + }
>> + if (res->Type == GL_UNIFORM_BLOCK) {
>> + int amount = RESOURCE_UBO(res)->NumUniforms;
>> + for (int i = 0; i < amount; i++) {
>> + const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName;
>> + *val = program_resource_location(shProg, res, iname);
>> + }
>> + return true;
>> + }
>> + default:
>> + assert(!"support for property type not implemented");
>> + }
>> + return false;
>> +}
>> +
>> +bool
>> +_mesa_program_resource_prop(struct gl_shader_program *shProg,
>> + struct gl_program_resource *res, GLuint index,
>> + const GLenum prop, GLint *val, const char *caller)
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> +
>> +#define VALIDATE_TYPE(type)\
>> + if (res->Type != type)\
>> + goto invalid_operation;
>> +
>> + switch(prop) {
>> + case GL_NAME_LENGTH:
>> + if (res->Type == GL_ATOMIC_COUNTER_BUFFER)
>> + goto invalid_operation;
>> + /* Base name +3 if array '[0]' + terminator. */
>> + *val = strlen(_mesa_program_resource_name(res)) +
>> + (_mesa_program_resource_array_size(res) > 0 ? 3 : 0) + 1;
>> + return true;
>> + case GL_TYPE:
>> + switch (res->Type) {
>> + case GL_UNIFORM:
>> + *val = RESOURCE_UNI(res)->type->gl_type;
>> + return true;
>> + case GL_PROGRAM_INPUT:
>> + case GL_PROGRAM_OUTPUT:
>> + *val = RESOURCE_VAR(res)->type->gl_type;
>> + return true;
>> + case GL_TRANSFORM_FEEDBACK_VARYING:
>> + *val = RESOURCE_XFB(res)->Type;
>> + return true;
>> + default:
>> + goto invalid_operation;
>> + }
>> + case GL_ARRAY_SIZE:
>> + switch (res->Type) {
>> + case GL_UNIFORM:
>> + *val = MAX2(RESOURCE_UNI(res)->array_elements, 1);
>> + return true;
>
> indent is funny here.
will fix
>> + case GL_PROGRAM_INPUT:
>> + case GL_PROGRAM_OUTPUT:
>> + *val = MAX2(RESOURCE_VAR(res)->type->length, 1);
>> + return true;
>> + case GL_TRANSFORM_FEEDBACK_VARYING:
>> + *val = MAX2(RESOURCE_XFB(res)->Size, 1);
>> + return true;
>> + default:
>> + goto invalid_operation;
>> + }
>> + case GL_OFFSET:
>> + VALIDATE_TYPE(GL_UNIFORM);
>> + *val = RESOURCE_UNI(res)->offset;
>> + return true;
>> + case GL_BLOCK_INDEX:
>> + VALIDATE_TYPE(GL_UNIFORM);
>> + *val = RESOURCE_UNI(res)->block_index;
>> + return true;
>> + case GL_ARRAY_STRIDE:
>> + VALIDATE_TYPE(GL_UNIFORM);
>> + *val = RESOURCE_UNI(res)->array_stride;
>> + return true;
>> + case GL_MATRIX_STRIDE:
>> + VALIDATE_TYPE(GL_UNIFORM);
>> + *val = RESOURCE_UNI(res)->matrix_stride;
>> + return true;
>> + case GL_IS_ROW_MAJOR:
>> + VALIDATE_TYPE(GL_UNIFORM);
>> + *val = RESOURCE_UNI(res)->row_major;
>> + return true;
>> + case GL_ATOMIC_COUNTER_BUFFER_INDEX:
>> + VALIDATE_TYPE(GL_UNIFORM);
>> + *val = RESOURCE_UNI(res)->atomic_buffer_index;
>> + return true;
>> + case GL_BUFFER_BINDING:
>> + case GL_BUFFER_DATA_SIZE:
>> + case GL_NUM_ACTIVE_VARIABLES:
>> + case GL_ACTIVE_VARIABLES:
>> + if (!get_buffer_property(shProg, res, prop, val))
>> + goto invalid_operation;
>> + return true;
>> + case GL_REFERENCED_BY_VERTEX_SHADER:
>> + case GL_REFERENCED_BY_GEOMETRY_SHADER:
>> + case GL_REFERENCED_BY_FRAGMENT_SHADER:
>> + switch (res->Type) {
>> + case GL_UNIFORM:
>> + case GL_PROGRAM_INPUT:
>> + case GL_PROGRAM_OUTPUT:
>> + case GL_UNIFORM_BLOCK:
>> + case GL_ATOMIC_COUNTER_BUFFER:
>> + *val = is_resource_referenced(shProg, res, index,
>> + stage_from_enum(prop));
>> + return true;
>> + default:
>> + goto invalid_operation;
>> + }
>> + case GL_LOCATION:
>> + switch (res->Type) {
>> + case GL_UNIFORM:
>> + case GL_PROGRAM_INPUT:
>> + case GL_PROGRAM_OUTPUT:
>> + *val = program_resource_location(shProg, res,
>> + _mesa_program_resource_name(res));
>> + return true;
>> + default:
>> + goto invalid_operation;
>> + }
>> + case GL_LOCATION_INDEX:
>> + if (res->Type != GL_PROGRAM_OUTPUT)
>> + goto invalid_operation;
>> + *val = RESOURCE_VAR(res)->data.index;
>> + return true;
>> +
>> + /* GL_ARB_tessellation_shader */
>> + case GL_IS_PER_PATCH:
>> + case GL_REFERENCED_BY_TESS_CONTROL_SHADER:
>> + case GL_REFERENCED_BY_TESS_EVALUATION_SHADER:
>> + /* GL_ARB_compute_shader */
>> + case GL_REFERENCED_BY_COMPUTE_SHADER:
>> + default:
>> + _mesa_error(ctx, GL_INVALID_ENUM,
>> + "%s(%s prop %s)", caller,
>> + _mesa_lookup_enum_by_nr(res->Type),
>> + _mesa_lookup_enum_by_nr(prop));
>> + return false;
>> + }
>> +
>> +invalid_operation:
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "%s(%s prop %s)", caller,
>> + _mesa_lookup_enum_by_nr(res->Type),
>> + _mesa_lookup_enum_by_nr(prop));
>> + return false;
>
> The classy thing to do is to undef things like VALIDATE_TYPE since
> they're super-specific to this function.
Sure, I'll add this.
>> +}
>> +
>> +extern void
>> +_mesa_get_program_resourceiv(struct gl_shader_program *shProg,
>> + GLenum interface, GLuint index, GLsizei propCount,
>> + const GLenum *props, GLsizei bufSize,
>> + GLsizei *length, GLint *params)
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> + GLint *val = (GLint *) params;
>> + const GLenum *prop = props;
>> + GLsizei amount = 0;
>> +
>> + struct gl_program_resource *res =
>> + _mesa_program_resource_find_index(shProg, interface, index);
>> +
>> + /* No such resource found. */
>> + if (!res) {
>> + _mesa_error(ctx, GL_INVALID_VALUE,
>> + "glGetProgramResourceiv(%s index %d)",
>> + _mesa_lookup_enum_by_nr(interface), index);
>> + return;
>> + }
>> +
>> + /* Write values until error occurs or bufSize reached. */
>> + for (int i = 0; i < bufSize; i++, val++, prop++) {
>> + if (_mesa_program_resource_prop(shProg, res, index, *prop, val,
>> + "glGetProgramResourceiv")) {
>
> this should be indented further in to make it clear that it's an
> argument to the function. I think emacs would (by default) indent it
> to the function's opening (, but other people will just indent it
> another 3 or 6 chars... not sure if there's a fixed standard.
Yep, this is wrong indentation, will fix.
>
>> + amount++;
>> + } else {
>> + /* Error happened. */
>> + return;
>> + }
>> + }
>> +
>> + /* If <length> is not NULL, the actual number of integer values
>> + * written to <params> will be written to <length>.
>> + */
>> + if (length)
>> + *length = amount;
>> +}
>> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
>> index 5046018..f1d038e 100644
>> --- a/src/mesa/main/shaderapi.h
>> +++ b/src/mesa/main/shaderapi.h
>> @@ -252,6 +252,18 @@ extern GLint
>> _mesa_program_resource_location_index(struct gl_shader_program *shProg,
>> GLenum interface, const char *name);
>>
>> +extern bool
>> +_mesa_program_resource_prop(struct gl_shader_program *shProg,
>> + struct gl_program_resource *res, GLuint index,
>> + const GLenum prop, GLint *val, const char *caller);
>> +
>> +extern void
>> +_mesa_get_program_resourceiv(struct gl_shader_program *shProg,
>> + GLenum interface, GLuint index,
>> + GLsizei propCount, const GLenum *props,
>> + GLsizei bufSize, GLsizei *length,
>> + GLint *params);
>> +
>> #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