[Mesa-dev] [PATCH] program_resource: subroutine active uniforms should return NumSubroutineUniforms
Tapani Pälli
tapani.palli at intel.com
Tue Aug 23 08:14:38 UTC 2016
On 08/23/2016 10:12 AM, Alejandro Piñeiro wrote:
> On 22/08/16 10:03, Tapani Pälli wrote:
>>
>> On 08/19/2016 09:31 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 number of active 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 the 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.
>>
>> I checked also and did not find evidence of linking requirement but I
>> found text that says we should be able to query properties without
>> errors for programs that either failed to link or are not linked at all:
>>
>> From 7.3 "Program Objects" in OpenGL ES 3.1 spec:
>>
>> First:
>>
>> "The GL provides various commands allowing applications to enumerate and
>> query properties of active variables and interface blocks for a
>> specified program."
>
> I see, thanks for the reference.
>
>> Then:
>>
>> "If one of these commands is called with a program for which LinkProgram
>> failed, no error is generated unless otherwise noted."
>
> Ok, so then I would need to soft the patch a little, doing the same but
> avoiding the error if it is not linked and ...
>
>> And also:
>>
>> "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."
>
> ... return 0 if it is not linked. FWIW, that would also means that when
> asking for the same the same info, GetProgramStageiv would fail, but
> GetProgramInterfaceiv would return 0. I guess that it is okish, as that
> info is only really correct if the program is linked.
IMO it looks like GetProgramStageiv is currently not correct from this
perspective as it does not follow these rules, it should not require
linking either as GL_ARB_shader_subroutine spec does not specify such
for the call.
> Thanks for the feedback.
>
>>
>>
>>> Fixes GL44-CTS.program_interface_query.subroutines-compute
>>> ---
>>>
>>> It is worth to mention that the implementation of glGetProgramStage
>>> also cames to the conclusion that the shader needs to be linked in
>>> order to provide the equivalent GL_ACTIVE_SUBROUTINES for each stage,
>>> and throws and INVALID_OPERATION if that is not the case, even if the
>>> spec of ARB_shader_subroutine or core GL 4.4 doesn't says so.
>>>
>>> See see shaderapi.c:_mesa_GetProgramStageiv for more info.
>>>
>>> It is worth to note that although this commit doesn't cause any
>>> regression on piglit it causes a CTS regression:
>>>
>>> Regresses GL44-CTS.program_interface_query.empty-shaders
>>>
>>> And the reason is that this tests tries to get ACTIVE_RESOURCES for a
>>> <shader>_SUBROUTINE_UNIFORM with a program not linked. Taking into
>>> account all what I said, I think that that test is wrong. At least one
>>> of the tests is wrong, as it is not possible to change mesa to fix one
>>> without breaking the other.
>>>
>>> Final: I have just sent a new piglit tests to the list, in order to
>>> ensure that the equivalent queries from ARB_shader_subroutine and
>>> ARB_program_interface_query returns the same values:
>>>
>>> https://lists.freedesktop.org/archives/piglit/2016-August/020488.html
>>>
>>>
>>> src/mesa/main/program_resource.c | 108
>>> +++++++++++++++++++++++++++++++--------
>>> 1 file changed, 87 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/src/mesa/main/program_resource.c
>>> b/src/mesa/main/program_resource.c
>>> index f2a9f00..a7088cd 100644
>>> --- a/src/mesa/main/program_resource.c
>>> +++ b/src/mesa/main/program_resource.c
>>> @@ -66,6 +66,61 @@ supported_interface_enum(struct gl_context *ctx,
>>> GLenum iface)
>>> }
>>> }
>>>
>>> +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;
>>> +}
>>> +
>>> +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;
>>> + }
>>> +}
>>> +
>>> +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");
>>> + }
>>> +}
>>> +
>>> void GLAPIENTRY
>>> _mesa_GetProgramInterfaceiv(GLuint program, GLenum programInterface,
>>> GLenum pname, GLint *params)
>>> @@ -101,9 +156,38 @@ _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 active subroutine uniforms we
>>> need the
>>> + * linked shader.
>>> + */
>>> + struct gl_shader_program *shLinkedProg =
>>> + lookup_linked_program(program, "glGetProgramInterfaceiv");
>>> + if (!shLinkedProg)
>>> + return;
>>> + gl_shader_stage stage =
>>> stage_from_program_interface(programInterface);
>>> + struct gl_linked_shader *sh =
>>> shLinkedProg->_LinkedShaders[stage];
>>> + if (!sh) {
>>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>>> "glGetProgramInterfaceiv");
>>> + return;
>>> + }
>>> +
>>> + *params = sh->NumSubroutineUniforms;
>>> + } 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 +459,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)
>>>
>>
>
More information about the mesa-dev
mailing list