[Mesa-dev] [PATCH 05/21] meta: Fix saving the program pipeline state

[Ian Romanick]

On 04/29/2014 08:43 PM, Chia-I Wu wrote:
> On Wed, Apr 30, 2014 at 8:52 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> This code was broken in some odd ways before.  Too much state was being
>> saved, it was being restored in the wrong order, and in the wrong way.
>> The biggest problem was that the pipeline object was restored before
>> restoring the programs attached to the default pipeline.
>>
>> Fixes a regression in the glean texgen test.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>> Reviewed-by: Eric Anholt <eric at anholt.net>
>> ---
>>  src/mesa/drivers/common/meta.c | 34 ++++++++++++++++++++--------------
>>  src/mesa/drivers/common/meta.h |  1 -
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
>> index ac27abb..92bc185 100644
>> --- a/src/mesa/drivers/common/meta.c
>> +++ b/src/mesa/drivers/common/meta.c
>> @@ -577,11 +577,15 @@ _mesa_meta_begin(struct gl_context *ctx, GLbitfield state)
>>        }
>>
>>        if (ctx->Extensions.ARB_separate_shader_objects) {
>> -         /* Warning it must be done before _mesa_UseProgram call */
>> -         _mesa_reference_pipeline_object(ctx, &save->_Shader, ctx->_Shader);
>> -         _mesa_reference_pipeline_object(ctx, &save->Pipeline,
>> -                                         ctx->Pipeline.Current);
>> -         _mesa_BindProgramPipeline(0);
>> +         /* Warning: This must be done before saving the current programs.
>> +          * Otherwise the programs attached to the pipeline will be saved
>> +          * instead of the programs attached to the default pipeline.
>> +          */
>> +         if (ctx->Pipeline.Current != ctx->Pipeline.Default) {
>> +            _mesa_reference_pipeline_object(ctx, &save->Pipeline,
>> +                                            ctx->Pipeline.Current);
>> +            _mesa_BindProgramPipeline(0);
>> +         }
>>        }
>>
>>        for (i = 0; i < MESA_SHADER_STAGES; i++) {
>> @@ -929,14 +933,6 @@ _mesa_meta_end(struct gl_context *ctx)
>>                            save->ATIFragmentShaderEnabled);
>>        }
>>
>> -      /* Warning it must be done before _mesa_use_shader_program call */
>> -      if (ctx->Extensions.ARB_separate_shader_objects) {
>> -         _mesa_reference_pipeline_object(ctx, &ctx->_Shader, save->_Shader);
>> -         _mesa_reference_pipeline_object(ctx, &ctx->Pipeline.Current,
>> -                                         save->Pipeline);
>> -         _mesa_reference_pipeline_object(ctx, &save->Pipeline, NULL);
>> -      }
>> -
>>        if (ctx->Extensions.ARB_vertex_shader) {
>>          _mesa_use_shader_program(ctx, GL_VERTEX_SHADER,
>>                                    save->Shader[MESA_SHADER_VERTEX],
>> @@ -956,10 +952,20 @@ _mesa_meta_end(struct gl_context *ctx)
>>        _mesa_reference_shader_program(ctx, &ctx->_Shader->ActiveProgram,
>>                                      save->ActiveShader);
>>
>> +      /* Warning: This must be done after _mesa_use_shader_program call.
>> +       * Otherwise the programs will be restored to the pipeline object
>> +       * instead of to the default pipeline.
>> +       */
>> +      if (save->Pipeline) {
>> +         assert(ctx->Extensions.ARB_separate_shader_objects);
>> +         _mesa_bind_pipeline(ctx, save->Pipeline);
> This issue does not appear to be fixed
> 
>   http://lists.freedesktop.org/archives/mesa-dev/2014-April/057999.html
> 
> The attached change to piglit triggers it.

What should the result have been with that patch?  I tried that with my
current sso6 branch, and
arb_separate_shader_objects-rendezvous_by_location passes with or
without this patch.

>> +
>> +         _mesa_reference_pipeline_object(ctx, &save->Pipeline, NULL);
>> +      }
>> +
>>        for (i = 0; i < MESA_SHADER_STAGES; i++)
>>           _mesa_reference_shader_program(ctx, &save->Shader[i], NULL);
>>        _mesa_reference_shader_program(ctx, &save->ActiveShader, NULL);
>> -      _mesa_reference_pipeline_object(ctx, &save->_Shader, NULL);
>>     }
>>
>>     if (state & MESA_META_STENCIL_TEST) {
>> diff --git a/src/mesa/drivers/common/meta.h b/src/mesa/drivers/common/meta.h
>> index fde4f9a..0a34792 100644
>> --- a/src/mesa/drivers/common/meta.h
>> +++ b/src/mesa/drivers/common/meta.h
>> @@ -121,7 +121,6 @@ struct save_state
>>     GLboolean ATIFragmentShaderEnabled;
>>     struct gl_shader_program *Shader[MESA_SHADER_STAGES];
>>     struct gl_shader_program *ActiveShader;
>> -   struct gl_pipeline_object   *_Shader;
>>     struct gl_pipeline_object   *Pipeline;
>>
>>     /** MESA_META_STENCIL_TEST */
>> --
>> 1.8.1.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev