[Mesa-dev] [PATCH 03/12] mesa/sso: replace Shader binding point with _Shader

[Eric Anholt]

Ian Romanick <idr at freedesktop.org> writes:

> From: Gregory Hainaut <gregory.hainaut at gmail.com>
>
> 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...
>
> V2: formating improvement
>
> V3 (idr):
> * Build fix.  The original patch added calls to _mesa_use_shader_program with
>   4 parameters, but the fourth parameter isn't added to that function until a
>   much later patch.  Just drop that parameter for now.
> * Trivial reformatting.
> * Updated comment of gl_context::_Shader
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/mesa/main/mtypes.h      | 15 ++++++++
>  src/mesa/main/pipelineobj.c |  8 ++++
>  src/mesa/main/shaderapi.c   | 91 +++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 111 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index d05649c..8a03afd 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2824,6 +2824,9 @@ struct gl_pipeline_shader_state
>     /** Currently bound pipeline object. See _mesa_BindProgramPipeline() */
>     struct gl_pipeline_object *Current;
>  
> +   /* Default Object to ensure that _Shader is never NULL */
> +   struct gl_pipeline_object *Default;
> +
>     /** Pipeline objects */
>     struct _mesa_HashTable *Objects;
>  };
> @@ -4131,6 +4134,18 @@ struct gl_context
>  
>     struct gl_pipeline_shader_state Pipeline; /**< GLSL pipeline shader object state */
>     struct gl_pipeline_object Shader; /**< GLSL shader object state */
> +
> +   /**
> +    * Current active shader pipeline state
> +    *
> +    * Almost all internal users want ::_Shader instead of ::Shader.  The
> +    * exceptions are bits of legacy GLSL API that do not know about separate
> +    * shader objects.
> +    *
> +    * Points to ::Shader, ::Pipeline.Current, or ::Pipeline.Default.
> +    */
> +   struct gl_pipeline_object *_Shader;
> +
>     struct gl_shader_compiler_options ShaderCompilerOptions[MESA_SHADER_STAGES];
>  
>     struct gl_query_state Query;  /**< occlusion, timer queries */
> diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> index 27012df..849c781 100644
> --- a/src/mesa/main/pipelineobj.c
> +++ b/src/mesa/main/pipelineobj.c
> @@ -94,6 +94,10 @@ _mesa_init_pipeline(struct gl_context *ctx)
>     ctx->Pipeline.Objects = _mesa_NewHashTable();
>  
>     ctx->Pipeline.Current = NULL;
> +
> +   /* Install a default Pipeline */
> +   ctx->Pipeline.Default = _mesa_new_pipeline_object(ctx, 0);
> +   _mesa_reference_pipeline_object(ctx, &ctx->_Shader, ctx->Pipeline.Default);
>  }
>  
>  
> @@ -117,6 +121,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 5060cbb..71b2ef5 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -44,6 +44,7 @@
>  #include "main/hash.h"
>  #include "main/hash_table.h"
>  #include "main/mtypes.h"
> +#include "main/pipelineobj.h"
>  #include "main/shaderapi.h"
>  #include "main/shaderobj.h"
>  #include "main/transformfeedback.h"
> @@ -144,6 +145,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);
>     mtx_destroy(&ctx->Shader.Mutex);
>  }
> @@ -1541,7 +1544,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 ...
> +    */

This is a spec citation, and it would be nice to see it formatted like
one in the 3 places it's quoted in the file.

> +   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.Current) {
> +         _mesa_BindProgramPipeline(ctx->Pipeline.Current->Name);
> +      }
> +   }
>  }

Calling _mesa_BindProgramPipeline when returning to the old pipeline
object, but not when first disabling the old pipeline object seems
sketchy to me.  But I don't think there's anything in the
BindProgramPipeline call you are actually need to execute here, in
either case.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140305/7523f81f/attachment.pgp>