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.arc...@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.arc...@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.arc...@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.arc...@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.arc...@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.
Also the code in glUseProgram does this:
/* Program is current, flush it */
if (shTarget == ctx->_Shader) {
FLUSH_VERTICES(ctx, _NEW_PROGRAM | _NEW_PROGRAM_CONSTANTS);
}
Are you sure FLUSH_VERTICES() doesn't flush then update the current
program, or wouldn't that code be wrong too?
> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
