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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/05/2014 12:23 PM, Eric Anholt wrote:
> 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.

This is trying to handle a sequence like:

    glBindProgramPipeline(pipe);
    ...
    glUseProgram(prog);
    ...
    glUseProgram(0);
    // pipe is now active again.

The _mesa_use_program call and _mesa_reference_pipeline_object call in
the else-statement reset things to the "nobody active" state, and the
_mesa_BindProgramPipeline reactivates the previously active pipeline.

There are some bits like this that could get cleaned up a bit.  I
think after all of ARB_sso lands, I want to rip out EXT_sso.  The EXT
version has some very different semantics, and some of the ugliness is
to simplify supporting those.  Once that is gone, we can get to a much
cleaner state, I think.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iEYEARECAAYFAlMgsGAACgkQX1gOwKyEAw86hgCcCE1XGfEPaBFPaSIgyDEqzM4C
DJkAn1bSKq5uzoVBs9Xmtmutvi7VEB1x
=PyOS
-----END PGP SIGNATURE-----