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

Tue Jan 3 23:44:06 UTC 2017

On Wed, Jan 4, 2017 at 12:36 AM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Tue, 2017-01-03 at 23:55 +0100, Marek Olšák wrote:
>> On Tue, Jan 3, 2017 at 11:14 PM, Timothy Arceri
>> <timothy.arceri at collabora.com> wrote:
>> > On Tue, 2017-01-03 at 22:41 +0100, Marek Olšák wrote:
>> > > On Tue, Jan 3, 2017 at 10:14 PM, Timothy Arceri
>> > > <timothy.arceri at collabora.com> wrote:
>> > > > On Tue, 2017-01-03 at 22:07 +0100, Marek Olšák wrote:
>> > > > > 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.
>> > > >
>> > > > But the state doesn't change just from linking does it? The new
>> > > > programs won't be made current until glUseProgram() is called.
>> > >
>> > > The GL state changes if the program is current and re-linked.
>> > > (not
>> > > sure if all of the internal Mesa state changes too)
>> >
>> > Right but this patch only changes things so that FLUSH_VERTICES is
>> > not
>> > called when the program is *not* current.
>>
>> That idea is OK. The problem is the patch also moves FLUSH_VERTICES
>> after the _mesa_glsl_link_shader call, which breaks the behavior if
>> the program *is* current.
>>
>> Consider this:
>>
>> glUseProgram();
>> glBegin();
>> glVertex();
>> glEnd();
>> // the vbo module doesn't create a draw call in glEnd; instead, it
>> waits
>> // for another glBegin call and the driver doesn't see any draw calls
>> yet.
>> glDetachShader();
>> glAttachShader();
>> glLinkProgram() {
>>   _mesa_glsl_link_shader(); // this replaces the current program
>>   FLUSH_VERTICES(); // this finally creates the draw call from
>> Begin/End,
>>   // but with the new program
>> }
>>
>>
>> The correct sequence is:
>>
>> glLinkProgram() {
>>   FLUSH_VERTICES(); // create the draw call from Begin/End
>>  _mesa_glsl_link_shader(); // replace the current program
>> }
>
> ok I've taken a closer look at things and see what you getting at. My
> reason for moving it later is to prepare for patch 34 [1].
>
> I guess I could do:
>
>    if (shProg->data->LinkStatus && programs_in_use) {
>       FLUSH_VERTICES(ctx, 0);
>       FLUSH_VERTICES(ctx, _NEW_PROGRAM);
>    }

Still not correct. This is correct:

/* It's important to flush the vbo module before modifying any states: */
FLUSH_VERTICES(ctx, 0);
/* Now you can link the shaders: */
_mesa_glsl_link_shader(ctx, shProg);

/* Now you can set _NEW_PROGRAM conditionally and don't have to use
FLUSH_VERTICES, because you've flushed it already. */
if (/* something */)
   ctx->NewState |= _NEW_PROGRAM;

FLUSH_VERTICES(ctx, 0) is actually commonly used if you don't wanna
set any flag.

Marek