[Mesa-dev] [PATCH] mesa: add KHR_no_error support for glUseProgram
Timothy Arceri
tarceri at itsqueeze.com
Tue May 9 01:17:07 UTC 2017
On 08/05/17 19:36, Nicolai Hähnle wrote:
> On 08.05.2017 02:25, Timothy Arceri wrote:
>> V3: use always_inline attribute (Suggested by Nicolai)
>>
>> Cc: Nicolai Hähnle <nhaehnle at gmail.com>
>> ---
>> src/mapi/glapi/gen/gl_API.xml | 2 +-
>> src/mesa/main/shaderapi.c | 75
>> +++++++++++++++++++++++++++++--------------
>> src/mesa/main/shaderapi.h | 2 ++
>> 3 files changed, 54 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/mapi/glapi/gen/gl_API.xml
>> b/src/mapi/glapi/gen/gl_API.xml
>> index a304ac0..50d60f5 100644
>> --- a/src/mapi/glapi/gen/gl_API.xml
>> +++ b/src/mapi/glapi/gen/gl_API.xml
>> @@ -5493,21 +5493,21 @@
>> </function>
>>
>> <function name="ShaderSource" es2="2.0" marshal="custom">
>> <param name="shader" type="GLuint"/>
>> <param name="count" type="GLsizei"/>
>> <param name="string" type="const GLchar * const *"/>
>> <param name="length" type="const GLint *"/>
>> <glx ignore="true"/>
>> </function>
>>
>> - <function name="UseProgram" es2="2.0">
>> + <function name="UseProgram" es2="2.0" no_error="true">
>> <param name="program" type="GLuint"/>
>> <glx ignore="true"/>
>> </function>
>>
>> <function name="Uniform1f" es2="2.0">
>> <param name="location" type="GLint"/>
>> <param name="v0" type="GLfloat"/>
>> <glx ignore="true"/>
>> </function>
>> <function name="Uniform2f" es2="2.0">
>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>> index eb75a3e..f91cc89 100644
>> --- a/src/mesa/main/shaderapi.c
>> +++ b/src/mesa/main/shaderapi.c
>> @@ -1841,20 +1841,70 @@ _mesa_ShaderSource(GLuint shaderObj, GLsizei
>> count,
>> free(source);
>> source = replacement;
>> }
>> #endif /* ENABLE_SHADER_CACHE */
>>
>> shader_source(sh, source);
>>
>> free(offsets);
>> }
>>
>> +/* The ARB_separate_shader_object spec says:
>> + *
>> + * "The executable code for an individual shader stage is taken from
>> + * the current program for that stage. If there is a current
>> program
>> + * object established by UseProgram, that program is considered
>> current
>> + * for all stages. Otherwise, if there is a bound program pipeline
>> + * object (section 2.14.PPO), the program bound to the appropriate
>> + * stage of the pipeline object is considered current."
>> + */
>> +static ALWAYS_INLINE void
>> +reference_pipeline_and_use_program(struct gl_context *ctx,
>> + struct gl_shader_program *shProg,
>> + bool no_error)
>> +{
>> + if (shProg) {
>> + /* Attach shader state to the binding point */
>> + _mesa_reference_pipeline_object(ctx, &ctx->_Shader, &ctx->Shader);
>> + /* Update the program */
>> + _mesa_use_shader_program(ctx, shProg);
>> + } else {
>> + /* Must be done first: detach the progam */
>> + _mesa_use_shader_program(ctx, shProg);
>> + /* Unattach shader_state binding point */
>> + _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
>> + ctx->Pipeline.Default);
>> + /* If a pipeline was bound, rebind it */
>> + if (ctx->Pipeline.Current) {
>> + if (no_error)
>> +
>> _mesa_BindProgramPipeline_no_error(ctx->Pipeline.Current->Name);
>> + else
>> + _mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
>> + }
>> + }
>> +}
>> +
>> + \
>> +
>> +void GLAPIENTRY
>> +_mesa_UseProgram_no_error(GLuint program)
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> + struct gl_shader_program *shProg = NULL;
>> +
>> + if (program) {
>> + shProg = _mesa_lookup_shader_program(ctx, program);
>> + }
>> +
>> + reference_pipeline_and_use_program(ctx, shProg, true);
>> +}
>> +
>
> Unless reference_pipeline_and_use_program has a separate use, I'd
> actually feel more comfortable if the *entire* body of _mesa_UseProgram
> was moved into the ALWAYS_INLINE function. This is better for
> maintainability because it would guarantee that any future change will
> automatically update both variants of the functions.
That would just result in some ugly code and would not actually reduce
the number of lines of code. The code takes different paths so nothing
would be updated automatically.
I'd really rather not do that, the no error path is just a single
look-up call, this is a pattern that is going to become very common as
more no_error support is added, I'd rather we be aware that we should
check the no_error variants in future when updating/reviewing api
changes rather than adding a bunch of ugly and hard to follow inline
functions everywhere.
For example this is what the body of the function will look like:
GET_CURRENT_CONTEXT(ctx);
struct gl_shader_program *shProg = NULL;
if (no_error) {
if (program) {
shProg = _mesa_lookup_shader_program(ctx, program);
} else {
if (_mesa_is_xfb_active_and_unpaused(ctx)) {
_mesa_error(ctx, GL_INVALID_OPERATION,
"glUseProgram(transform feedback active)");
return;
}
if (program) {
shProg = _mesa_lookup_shader_program_err(ctx, program,
"glUseProgram");
if (!shProg) {
return;
}
if (!shProg->data->LinkStatus) {
_mesa_error(ctx, GL_INVALID_OPERATION,
"glUseProgram(program %u not linked)", program);
return;
}
if (ctx->_Shader->Flags & GLSL_USE_PROG) {
print_shader_info(*shProg);
}
}
}
if (shProg) {
/* Attach shader state to the binding point */
_mesa_reference_pipeline_object(ctx, &ctx->_Shader, &ctx->Shader);
/* Update the program */
_mesa_use_shader_program(ctx, shProg);
} else {
/* Must be done first: detach the progam */
_mesa_use_shader_program(ctx, shProg);
/* Unattach shader_state binding point */
_mesa_reference_pipeline_object(ctx, &ctx->_Shader,
ctx->Pipeline.Default);
/* If a pipeline was bound, rebind it */
if (ctx->Pipeline.Current) {
if (no_error)
_mesa_BindProgramPipeline_no_error(ctx->Pipeline.Current->Name);
else
_mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
}
}
Thoughts?
>
> Cheers,
> Nicolai
>
>>
>> void GLAPIENTRY
>> _mesa_UseProgram(GLuint program)
>> {
>> GET_CURRENT_CONTEXT(ctx);
>> struct gl_shader_program *shProg = NULL;
>>
>> if (MESA_VERBOSE & VERBOSE_API)
>> _mesa_debug(ctx, "glUseProgram %u\n", program);
>>
>> @@ -1875,44 +1925,21 @@ _mesa_UseProgram(GLuint program)
>> return;
>> }
>>
>> #ifdef DEBUG
>> if (ctx->_Shader->Flags & GLSL_USE_PROG) {
>> print_shader_info(shProg);
>> }
>> #endif
>> }
>>
>> - /* The ARB_separate_shader_object spec says:
>> - *
>> - * "The executable code for an individual shader stage is
>> taken from
>> - * the current program for that stage. If there is a current
>> program
>> - * object established by UseProgram, that program is
>> considered current
>> - * for all stages. Otherwise, if there is a bound program
>> pipeline
>> - * object (section 2.14.PPO), the program bound to the
>> appropriate
>> - * stage of the pipeline object is considered current."
>> - */
>> - if (program) {
>> - /* Attach shader state to the binding point */
>> - _mesa_reference_pipeline_object(ctx, &ctx->_Shader, &ctx->Shader);
>> - /* Update the program */
>> - _mesa_use_shader_program(ctx, shProg);
>> - } else {
>> - /* Must be done first: detach the progam */
>> - _mesa_use_shader_program(ctx, shProg);
>> - /* Unattach shader_state binding point */
>> - _mesa_reference_pipeline_object(ctx, &ctx->_Shader,
>> ctx->Pipeline.Default);
>> - /* If a pipeline was bound, rebind it */
>> - if (ctx->Pipeline.Current) {
>> - _mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
>> - }
>> - }
>> + reference_pipeline_and_use_program(ctx, shProg, false);
>> }
>>
>>
>> void GLAPIENTRY
>> _mesa_ValidateProgram(GLuint program)
>> {
>> GET_CURRENT_CONTEXT(ctx);
>> validate_program(ctx, program);
>> }
>>
>> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
>> index 99b4fe8..0a28185 100644
>> --- a/src/mesa/main/shaderapi.h
>> +++ b/src/mesa/main/shaderapi.h
>> @@ -120,20 +120,22 @@ _mesa_IsProgram(GLuint name);
>>
>> extern GLboolean GLAPIENTRY
>> _mesa_IsShader(GLuint name);
>>
>> extern void GLAPIENTRY
>> _mesa_LinkProgram(GLuint programObj);
>>
>> extern void GLAPIENTRY
>> _mesa_ShaderSource(GLuint, GLsizei, const GLchar* const *, const
>> GLint *);
>>
>> +void GLAPIENTRY
>> +_mesa_UseProgram_no_error(GLuint);
>> extern void GLAPIENTRY
>> _mesa_UseProgram(GLuint);
>>
>> extern void GLAPIENTRY
>> _mesa_ValidateProgram(GLuint);
>>
>>
>> extern void GLAPIENTRY
>> _mesa_BindAttribLocation(GLuint program, GLuint, const GLchar *);
>>
>>
>
>
More information about the mesa-dev
mailing list