[Mesa-dev] [PATCH] mesa: add KHR_no_error support for glUseProgram
Nicolai Hähnle
nhaehnle at gmail.com
Tue May 9 07:23:27 UTC 2017
On 09.05.2017 03:17, Timothy Arceri wrote:
> 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.
The example below really doesn't look that ugly to me, but I admit it's
a bit of personal taste. I do still prefer the inline variant, but I'm
not going to insist on it.
Cheers,
Nicolai
> 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 *);
>>>
>>>
>>
>>
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
More information about the mesa-dev
mailing list