On Sat, 06 Apr 2013 08:52:35 -0600 Brian Paul <bri...@vmware.com> wrote:
> One more comment for now below. I may not get to review the rest for > a few days. > > -Brian Thanks very much for the review. I redid properly my patch and rebase my work on a more recent mesa version. I want to do a piglit non regression test first and double check any typo. Then I will probabaly post the V2 friday except if you prefer to finish the first review first. FYI, I choose to keep the shorter "Current" so we got the basic "Pipeline.Current" Cheers, Gregory > > On 04/05/2013 03:27 PM, gregory wrote: > > To avoid NULL pointer check a default pipeline object is installed in > > _Shader when no > > program is current > > > > The spec say that UseProgram/UseShaderProgramEXT/ActiveProgramEXT got an > > higher > > priority over the pipeline object. When default program is uninstall, the > > pipeline is > > used if any was bound. > > > > Note: A careful rename need to be done now... > > --- > > src/mesa/main/mtypes.h | 4 ++ > > src/mesa/main/pipelineobj.c | 8 ++++ > > src/mesa/main/shaderapi.c | 91 > > +++++++++++++++++++++++++++++++++++++++++-- > > 3 files changed, 100 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > > index 2445574..dc54f3d 100644 > > --- a/src/mesa/main/mtypes.h > > +++ b/src/mesa/main/mtypes.h > > @@ -2433,6 +2433,9 @@ struct gl_pipeline_shader_state > > /** Currently bound pipeline object. See _mesa_BindProgramPipeline() */ > > struct gl_shader_state *PipelineObj; > > > > + /* Default Object to ensure that _Shader is never NULL */ > > + struct gl_shader_state *Default; > > + > > /** Pipeline objects */ > > struct _mesa_HashTable *Objects; > > }; > > @@ -3541,6 +3544,7 @@ struct gl_context > > > > struct gl_pipeline_shader_state Pipeline; /**< GLSL pipeline shader > > object state */ > > struct gl_shader_state Shader; /**< GLSL shader object state */ > > + struct gl_shader_state *_Shader; /**< Points to ::Shader or > > ::Pipeline.PipelineObj or ::Pipeline.Default */ > > If gl_shader_state gets renamed to gl_pipeline_object then this field > would probably be better name CurrentPipeline, or similar. > > > > > struct gl_shader_compiler_options > > ShaderCompilerOptions[MESA_SHADER_TYPES]; > > > > struct gl_query_state Query; /**< occlusion, timer queries */ > > diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c > > index 7a56c67..c805cdf 100644 > > --- a/src/mesa/main/pipelineobj.c > > +++ b/src/mesa/main/pipelineobj.c > > @@ -97,6 +97,10 @@ _mesa_init_pipeline(struct gl_context *ctx) > > ctx->Pipeline.Objects = _mesa_NewHashTable(); > > > > ctx->Pipeline.PipelineObj = NULL; > > + > > + /* Install a default Pipeline */ > > + ctx->Pipeline.Default = _mesa_new_pipeline_object(ctx, 0); > > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader, > > ctx->Pipeline.Default); > > } > > > > > > @@ -120,6 +124,10 @@ _mesa_free_pipeline_data(struct gl_context *ctx) > > { > > _mesa_HashDeleteAll(ctx->Pipeline.Objects, delete_pipelineobj_cb, ctx); > > _mesa_DeleteHashTable(ctx->Pipeline.Objects); > > + > > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader, NULL); > > + _mesa_delete_pipeline_object(ctx, ctx->Pipeline.Default); > > + > > } > > > > /** > > diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c > > index a86a429..1092287 100644 > > --- a/src/mesa/main/shaderapi.c > > +++ b/src/mesa/main/shaderapi.c > > @@ -43,6 +43,7 @@ > > #include "main/hash.h" > > #include "main/mfeatures.h" > > #include "main/mtypes.h" > > +#include "main/pipelineobj.h" > > #include "main/shaderapi.h" > > #include "main/shaderobj.h" > > #include "main/transformfeedback.h" > > @@ -138,6 +139,8 @@ _mesa_free_shader_state(struct gl_context *ctx) > > _mesa_reference_shader_program(ctx,&ctx->Shader.ActiveProgram, NULL); > > > > /* Extended for ARB_separate_shader_objects */ > > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader, NULL); > > + > > assert(ctx->Shader.RefCount == 1); > > _glthread_DESTROY_MUTEX(ctx->Shader.Mutex); > > } > > @@ -1453,7 +1456,29 @@ _mesa_UseProgram(GLhandleARB program) > > shProg = NULL; > > } > > > > - _mesa_use_program(ctx, shProg); > > + /* > > + * The executable code for an individual shader stage is taken from the > > + * current program for that stage. If there is a current program object > > + * for any shader stage or for uniform updates established by > > UseProgram, > > + * UseShaderProgramEXT, or ActiveProgramEXT, the current program for > > that > > + * stage (if any) is considered current. Otherwise, if there is a bound > > + * program pipeline object ... > > + */ > > + if (program) { > > + /* Attach shader state to the binding point */ > > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader,&ctx->Shader); > > + /* Update the program */ > > + _mesa_use_program(ctx, shProg); > > + } else { > > + /* Must be done first: detach the progam */ > > + _mesa_use_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.PipelineObj) { > > + _mesa_BindProgramPipeline (ctx->Pipeline.PipelineObj->Name); > > + } > > + } > > } > > > > > > @@ -1769,7 +1794,37 @@ _mesa_UseShaderProgramEXT(GLenum type, GLuint > > program) > > } > > } > > > > - _mesa_use_shader_program(ctx, type, shProg); > > + /* > > + * The executable code for an individual shader stage is taken from the > > + * current program for that stage. If there is a current program object > > + * for any shader stage or for uniform updates established by > > UseProgram, > > + * UseShaderProgramEXT, or ActiveProgramEXT, the current program for > > that > > + * stage (if any) is considered current. Otherwise, if there is a bound > > + * program pipeline object ... > > + */ > > + 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, type, shProg,&ctx->Shader); > > + } else { > > + /* Must be done first: detach the progam */ > > + _mesa_use_shader_program(ctx, type, shProg,&ctx->Shader); > > + > > + /* Nothing remains current */ > > + if ( ctx->Shader.CurrentVertexProgram == NULL&& > > + ctx->Shader.CurrentGeometryProgram == NULL&& > > + ctx->Shader.CurrentFragmentProgram == NULL&& > > + ctx->Shader.ActiveProgram == NULL) { > > + > > + /* 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.PipelineObj) { > > + _mesa_BindProgramPipeline (ctx->Pipeline.PipelineObj->Name); > > + } > > + } > > + } > > } > > > > > > @@ -1784,7 +1839,37 @@ _mesa_ActiveProgramEXT(GLuint program) > > ? _mesa_lookup_shader_program_err(ctx, program, > > "glActiveProgramEXT") > > : NULL; > > > > - _mesa_active_program(ctx, shProg, "glActiveProgramEXT"); > > + /* > > + * The executable code for an individual shader stage is taken from the > > + * current program for that stage. If there is a current program object > > + * for any shader stage or for uniform updates established by > > UseProgram, > > + * UseShaderProgramEXT, or ActiveProgramEXT, the current program for > > that > > + * stage (if any) is considered current. Otherwise, if there is a bound > > + * program pipeline object ... > > + */ > > + if(shProg != NULL) { > > + /* Attach shader state to the binding point */ > > + _mesa_reference_pipeline_object(ctx,&ctx->_Shader,&ctx->Shader); > > + _mesa_active_program(ctx, shProg, "glActiveProgramEXT"); > > + } else { > > + /* Must be done first: unset the current active progam */ > > + _mesa_active_program(ctx, shProg, "glActiveProgramEXT"); > > + > > + /* Nothing remains current */ > > + if ( ctx->Shader.CurrentVertexProgram == NULL&& > > + ctx->Shader.CurrentGeometryProgram == NULL&& > > + ctx->Shader.CurrentFragmentProgram == NULL&& > > + ctx->Shader.ActiveProgram == NULL) { > > + > > + /* 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.PipelineObj) { > > + _mesa_BindProgramPipeline (ctx->Pipeline.PipelineObj->Name); > > + } > > + } > > + } > > + > > return; > > } > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev