-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 03/05/2014 12:23 PM, Eric Anholt wrote:
> Ian Romanick <i...@freedesktop.org> writes:
>
>> From: Gregory Hainaut <gregory.hain...@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.roman...@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-----
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
