[Mesa-dev] [PATCH v2] program_resource: subroutine active uniforms should return NumSubroutineUniforms
Tapani Pälli
tapani.palli at intel.com
Wed Aug 24 09:39:14 UTC 2016
On 08/24/2016 12:31 PM, Alejandro Piñeiro wrote:
> On 24/08/16 11:13, Tapani Pälli wrote:
>> Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
> Thanks for the review.
>
>> (one small coding-style suggestion below)
> I will not use this suggestion. Reason below.
>
>> On 08/23/2016 06:33 PM, Alejandro Piñeiro wrote:
>>> Before this commit, GetProgramInterfaceiv for pname ACTIVE_RESOURCES
>>> and all the <shader>_SUBROUTINE_UNIFORM programInterface were
>>> returning the count of resources on the shader program using that
>>> interface, instead of the num of uniform resources. This would get a
>>> wrong value (for example) if the shader has an array of subroutine
>>> uniforms.
>>>
>>> Note that this means that in order to get a proper value, the shader
>>> needs to be linked, something that is not explicitly mentioned on
>>> ARB_program_interface_query spec, but comes from the general
>>> definition of active uniform. If the program is not linked we
>>> return 0.
>>>
>>> v2: don't generate an error if the program is not linked, returning 0
>>> active uniforms instead, plus extra spec references (Tapani Palli)
>>>
>>> Fixes GL44-CTS.program_interface_query.subroutines-compute
>>> ---
>>> src/mesa/main/program_resource.c | 141
>>> ++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 118 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/src/mesa/main/program_resource.c
>>> b/src/mesa/main/program_resource.c
>>> index f2a9f00..6ddbdad 100644
>>> --- a/src/mesa/main/program_resource.c
>>> +++ b/src/mesa/main/program_resource.c
>>> @@ -66,6 +66,79 @@ supported_interface_enum(struct gl_context *ctx,
>>> GLenum iface)
>>> }
>>> }
>>> +static struct gl_shader_program *
>>> +lookup_linked_program(GLuint program,
>>> + const char *caller,
>>> + bool raise_link_error)
>>> +{
>>> + GET_CURRENT_CONTEXT(ctx);
>>> + struct gl_shader_program *prog =
>>> + _mesa_lookup_shader_program_err(ctx, program, caller);
>>> +
>>> + if (!prog)
>>> + return NULL;
>>> +
>>> + if (prog->LinkStatus == GL_FALSE) {
>>> + if (raise_link_error)
>> if (prog->LinkStatus == FALSE && raise_link_error)
> This is not exactly the same. Note that if the program exist prog is not
> NULL even if not linked, and we want this method to return NULL in that
> case too.
>
> With your proposed change, if the program is not linked, and
> raise_link_error is false, it will return at the end of the function,
> that just returns prog, that can be different to NULL.
Arg right, sorry got confused, raise_link_error is there indeed only for
the _mesa_error. This is fine then!
>
>>> + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not
>>> linked)",
>>> + caller);
>>> + return NULL;
>>> + }
>>> + return prog;
>>> +}
>>> +
>>> +static GLenum
>>> +stage_from_program_interface(GLenum programInterface)
>>> +{
>>> + switch(programInterface) {
>>> + case GL_VERTEX_SUBROUTINE_UNIFORM:
>>> + return MESA_SHADER_VERTEX;
>>> + case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
>>> + return MESA_SHADER_TESS_CTRL;
>>> + case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
>>> + return MESA_SHADER_TESS_EVAL;
>>> + case GL_GEOMETRY_SUBROUTINE_UNIFORM:
>>> + return MESA_SHADER_GEOMETRY;
>>> + case GL_FRAGMENT_SUBROUTINE_UNIFORM:
>>> + return MESA_SHADER_FRAGMENT;
>>> + case GL_COMPUTE_SUBROUTINE_UNIFORM:
>>> + return MESA_SHADER_COMPUTE;
>>> + default:
>>> + assert(!"unexpected programInterface value");
>>> + }
>>> +}
>>> +
>>> +static struct gl_linked_shader *
>>> +lookup_linked_shader(GLuint program,
>>> + GLenum programInterface,
>>> + const char *caller)
>>> +{
>>> + struct gl_shader_program *shLinkedProg =
>>> + lookup_linked_program(program, caller, false);
>>> + gl_shader_stage stage =
>>> stage_from_program_interface(programInterface);
>>> +
>>> + if (!shLinkedProg)
>>> + return NULL;
>>> +
>>> + return shLinkedProg->_LinkedShaders[stage];
>>> +}
>>> +
>>> +static bool
>>> +is_subroutine_uniform_program_interface(GLenum programInterface)
>>> +{
>>> + switch(programInterface) {
>>> + 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:
>>> + return true;
>>> + default:
>>> + return false;
>>> + }
>>> +}
>>> +
>>> void GLAPIENTRY
>>> _mesa_GetProgramInterfaceiv(GLuint program, GLenum programInterface,
>>> GLenum pname, GLint *params)
>>> @@ -101,9 +174,49 @@ _mesa_GetProgramInterfaceiv(GLuint program,
>>> GLenum programInterface,
>>> /* Validate pname against interface. */
>>> switch(pname) {
>>> case GL_ACTIVE_RESOURCES:
>>> - for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++)
>>> - if (shProg->ProgramResourceList[i].Type == programInterface)
>>> - (*params)++;
>>> + if (is_subroutine_uniform_program_interface(programInterface)) {
>>> + /* ARB_program_interface_query doesn't explicitly says that
>>> those
>>> + * uniforms would need a linked shader, or that should fail
>>> if it is
>>> + * not the case, but Section 7.6 (Uniform Variables) of the
>>> OpenGL
>>> + * 4.4 Core Profile says:
>>> + *
>>> + * "A uniform is considered an active uniform if the
>>> compiler and
>>> + * linker determine that the uniform will actually be
>>> accessed
>>> + * when the executable code is executed. In cases where the
>>> + * compiler and linker cannot make a conclusive
>>> determination,
>>> + * the uniform will be considered active."
>>> + *
>>> + * So in order to know the real number of active subroutine
>>> uniforms
>>> + * we would need a linked shader .
>>> + *
>>> + * At the same time, Section 7.3 (Program Objects) of the
>>> OpenGL 4.4
>>> + * Core Profile says:
>>> + *
>>> + * "The GL provides various commands allowing
>>> applications to
>>> + * enumerate and query properties of active variables
>>> and in-
>>> + * terface blocks for a specified program. If one of these
>>> + * commands is called with a program for which LinkProgram
>>> + * succeeded, the information recorded when the program was
>>> + * linked is returned. If one of these commands is
>>> called with a
>>> + * program for which LinkProgram failed, no error is
>>> generated
>>> + * unless otherwise noted."
>>> + * <skip>
>>> + * "If one of these commands is called with a program for
>>> which
>>> + * LinkProgram had never been called, no error is generated
>>> + * unless otherwise noted, and the program object is
>>> considered
>>> + * to have no active variables or interface blocks."
>>> + *
>>> + * So if the program is not linked we will return 0.
>>> + */
>>> + struct gl_linked_shader *sh =
>>> + lookup_linked_shader(program, programInterface,
>>> "glGetProgramInterfaceiv");
>>> +
>>> + *params = sh ? sh->NumSubroutineUniforms : 0;
>>> + } else {
>>> + for (i = 0, *params = 0; i < shProg->NumProgramResourceList;
>>> i++)
>>> + if (shProg->ProgramResourceList[i].Type == programInterface)
>>> + (*params)++;
>>> + }
>>> break;
>>> case GL_MAX_NAME_LENGTH:
>>> if (programInterface == GL_ATOMIC_COUNTER_BUFFER ||
>>> @@ -375,24 +488,6 @@ _mesa_GetProgramResourceiv(GLuint program, GLenum
>>> programInterface,
>>> propCount, props, bufSize, length,
>>> params);
>>> }
>>> -static struct gl_shader_program *
>>> -lookup_linked_program(GLuint program, const char *caller)
>>> -{
>>> - GET_CURRENT_CONTEXT(ctx);
>>> - 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)
>>> @@ -405,7 +500,7 @@ _mesa_GetProgramResourceLocation(GLuint program,
>>> GLenum programInterface,
>>> }
>>> struct gl_shader_program *shProg =
>>> - lookup_linked_program(program, "glGetProgramResourceLocation");
>>> + lookup_linked_program(program, "glGetProgramResourceLocation",
>>> true);
>>> if (!shProg || !name)
>>> return -1;
>>> @@ -461,7 +556,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint
>>> program, GLenum programInterface,
>>> }
>>> struct gl_shader_program *shProg =
>>> - lookup_linked_program(program,
>>> "glGetProgramResourceLocationIndex");
>>> + lookup_linked_program(program,
>>> "glGetProgramResourceLocationIndex", true);
>>> if (!shProg || !name)
>>> return -1;
>>
>>
More information about the mesa-dev
mailing list