[Mesa-dev] [PATCH 30/53] mesa: don't always set _NEW_PROGRAM when linking

[Marek Olšák]

On Tue, Jan 3, 2017 at 9:57 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Tue, 2017-01-03 at 20:47 +0100, Marek Olšák wrote:
>> On Tue, Jan 3, 2017 at 3:43 AM, Timothy Arceri
>> <timothy.arceri at collabora.com> wrote:
>> > We only need to set it when linking was successful and the program
>> > being linked is currently active.
>> >
>> > The programs_in_use mask is just used as a flag for now but in
>> > a following patch we will use it to update the CurrentProgram
>> > array.
>> > ---
>> >  src/mesa/main/shaderapi.c | 22 +++++++++++++++++++++-
>> >  1 file changed, 21 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>> > index 022afab..6c03dcb 100644
>> > --- a/src/mesa/main/shaderapi.c
>> > +++ b/src/mesa/main/shaderapi.c
>> > @@ -1083,10 +1083,30 @@ _mesa_link_program(struct gl_context *ctx,
>> > struct gl_shader_program *shProg)
>> >        return;
>> >     }
>> >
>> > -   FLUSH_VERTICES(ctx, _NEW_PROGRAM);
>> > +   unsigned programs_in_use = 0;
>> > +   if (ctx->_Shader)
>> > +      for (unsigned stage = 0; stage < MESA_SHADER_STAGES;
>> > stage++) {
>> > +         if (ctx->_Shader->CurrentProgram[stage] == shProg) {
>> > +            programs_in_use |= 1 << stage;
>> > +         }
>> > +   }
>> >
>> >     _mesa_glsl_link_shader(ctx, shProg);
>> >
>> > +   /* From section 7.3 (Program Objects) of the OpenGL 4.5 spec:
>> > +    *
>> > +    *    "If LinkProgram or ProgramBinary successfully re-links a
>> > program
>> > +    *     object that is active for any shader stage, then the
>> > newly generated
>> > +    *     executable code will be installed as part of the current
>> > rendering
>> > +    *     state for all shader stages where the program is active.
>> > +    *     Additionally, the newly generated executable code is
>> > made part of
>> > +    *     the state of any program pipeline for all stages where
>> > the program
>> > +    *     is attached."
>> > +    */
>> > +   if (shProg->data->LinkStatus && programs_in_use) {
>> > +      FLUSH_VERTICES(ctx, _NEW_PROGRAM);
>> > +   }
>>
>> This doesn't seem correct. If the context has unflushed vertices,
>> calling FLUSH_VERTICES after linking will use the new linked program
>> instead of the previous program for which the vertices were
>> submitted.
>
> Your probably right but this doesn't make anything worse than it
> already is right?
>
> Before this change we where always calling FLUSH_VERTICES(ctx,
> _NEW_PROGRAM);

The useless flagging is a different issue.

FLUSH_VERTICES should be called before a state change, because it
flushes draw calls that the driver hasn't even seen yet. After the
driver processes those draw calls, _NEW_PROGRAM is set. That's how it
works.

If you instead change a state and then call FLUSH_VERTICES, the
previous unflushed draw calls will use the new state, which is
incorrect.

Marek