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

[Ian Romanick]

On 04/30/2014 06:59 PM, Chia-I Wu wrote:
> On Thu, May 1, 2014 at 12:11 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> 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.
> The patch should not affect the result, but I got this error after applying it
> 
> $ ./bin/arb_separate_shader_object-rendezvous_by_location -auto
> Probe color at (0,0)
>   Expected: 0.000000 1.000000 0.000000 1.000000
>   Observed: 1.000000 1.000000 1.000000 1.000000
> PIGLIT: {'result': 'fail' }
> 
> I am also on sso6 (except that I merged it to master).

I wasn't hitting because I wasn't hitting meta for the clear path.  With
meta blocked out, I get the same failure.  I should have a fix out
momentarilly... just one last piglit run...