[Mesa-dev] [PATCH 30/53] mesa: don't always set _NEW_PROGRAM when linking
Marek Olšák
maraeo at gmail.com
Wed Jan 4 00:16:57 UTC 2017
On Wed, Jan 4, 2017 at 12:56 AM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Wed, 2017-01-04 at 00:44 +0100, Marek Olšák wrote:
>> 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: */
>
> Right I noticed that st updates states during linking but don't you
> need to wait until after linking since the current program is meant to
> remain active should liking fail. A quick glance at the code shows
> places where linking can fail in the same place that state is being
> updated.
Not sure what you mean. st/mesa updates states only if the linked
program is current.
>
> Also the code in glUseProgram does this:
>
> /* Program is current, flush it */
> if (shTarget == ctx->_Shader) {
> FLUSH_VERTICES(ctx, _NEW_PROGRAM | _NEW_PROGRAM_CONSTANTS);
> }
Yeah, that's a bug. FLUSH_VERTICES should be called before
_mesa_reference_pipeline_object.
>
> Are you sure FLUSH_VERTICES() doesn't flush then update the current
> program, or wouldn't that code be wrong too?
FLUSH_VERTICES only submits draw calls queued in the vbo module and
then sets the state flags, but it doesn't update any states. The real
state updates will be done before the next draw call.
Marek
More information about the mesa-dev
mailing list