[Mesa-stable] [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs
Timothy Arceri
timothy.arceri at collabora.com
Thu Oct 27 01:37:55 UTC 2016
On Wed, 2016-10-26 at 20:19 -0400, Ilia Mirkin wrote:
> On Wed, Oct 26, 2016 at 8:08 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> >
> > On Wed, 2016-10-26 at 22:51 +1100, Timothy Arceri wrote:
> > >
> > > On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
> > > >
> > > >
> > > > Hi;
> > > >
> > > > On 10/26/2016 11:27 AM, Tapani Pälli wrote:
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On 10/26/2016 11:21 AM, Timothy Arceri wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > SSO shader programs can be later modified by
> > > > > > > > > attaching/detaching
> > > > > > > > > shaders and relinked, this requires IR.
> > > > > > > >
> > > > > > > > Doesn't relinking recreate the IR? We can relink
> > > > > > > > exiting
> > > > > > > > shaders
> > > > > > > > into
> > > > > > > > new programs. The IR is cloned from gl_shader (the
> > > > > > > > compiled
> > > > > > > > IR)
> > > > > > > > before
> > > > > > > > this happens.
> > > > > > > >
> > > > > > > > Where exactly are things falling over for SSO?
> > > > > > >
> > > > > > > I went this way as I haven't seen this happening
> > > > > > > elsewhere
> > > > > > > but
> > > > > > > when
> > > > > > > relinking program created by glCreateShaderProgram. TBH
> > > > > > > I'm
> > > > > > > not
> > > > > > > sure
> > > > > > > if
> > > > > > > this could happen with regular programs, I would assume
> > > > > > > we
> > > > > > > had
> > > > > > > already
> > > > > > > bugs if it would be so (?)
> > > > > > >
> > > > > > > In practice things go bad in brw_link_shader where
> > > > > > > process_glsl_ir()
> > > > > > > gets called, like this:
> > > > > > >
> > > > > > > --- 8< ---
> > > > > > > #0 do_expression_flattening (instructions=instructions at e
> > > > > > > ntry
> > > > > > > =0
> > > > > > > x0,
> > > > > > > predicate=predicate at entry=0x7ffff0f808a0
> > > > > > > <mat_op_to_vec_predicate(ir_instruction*)>) at
> > > > > > > glsl/ir_expression_flattening.cpp:60
> > > > > > > #1 0x00007ffff0f816b9 in do_mat_op_to_vec
> > > > > > > (instructions=0x0)
> > > > > > > at
> > > > > > > glsl/lower_mat_op_to_vec.cpp:96
> > > > > > > #2 0x00007ffff10385bd in process_glsl_ir
> > > > > > > (shader_prog=0x9b74d8,
> > > > > > > shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> > > > > > > #3 brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> > > > > > > brw_link.cpp:234
> > > > > > > #4 0x00007ffff0ec1f31 in _mesa_glsl_link_shader
> > > > > > > (ctx=0x7d2478,
> > > > > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> > > > > > > #5 0x00007ffff0ddc99b in _mesa_link_program
> > > > > > > (ctx=0x7d2478,
> > > > > > > shProg=0x9b74d8) at main/shaderapi.c:1098
> > > > > > > #6 0x00007ffff7ac8d15 in stub_glLinkProgram (program=2)
> > > > > > > at
> > > > > > > /home/tpalli/source/fdo/piglit/tests/util/piglit-
> > > > > > > dispatch-
> > > > > > > gen.c:33005
> > > > > > > #7 0x00000000004019ab in
> > > > > > > relink_program_created_by_glCreateShaderProgram () at
> > > > > > > /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_sh
> > > > > > > ader
> > > > > > > _o
> > > > > > > bjects
> > > > > > > /api-errors.c:78
> > > > > >
> > > > > > There is a comment above that line in the test.
> > > > > >
> > > > > > /* Issue #14 of the GL_ARB_separate_shader_objects spec
> > > > > > says:
> > > > > > *
> > > > > > * "14. Should glLinkProgram work to re-link a shader
> > > > > > created
> > > > > > with
> > > > > > * glCreateShaderProgram?
> > > > > > *
> > > > > > * RESOLVED: NO because the shader created by
> > > > > > * glCreateShaderProgram is detached and deleted
> > > > > > as
> > > > > > part
> > > > > > of
> > > > > > * the glCreateShaderProgram sequence. This means
> > > > > > if
> > > > > > you
> > > > > > * call glLinkProgram on a program returned from
> > > > > > * glCreateShaderProgram, you'll find the re-link
> > > > > > fails
> > > > > > * because no shader object is attached.
> > > > > > *
> > > > > > * An application is free to attach one or more
> > > > > > new
> > > > > > shader
> > > > > > * objects to the program and then relink would
> > > > > > work.
> > > > > > *
> > > > > > * This is fine because re-linking isn't
> > > > > > necessary/expected."
> > > > > > */
> > > > > >
> > > > > > Which means we shouldn't able to relink this shader. We
> > > > > > shouldn't
> > > > > > be
> > > > > > able to get as far along as we do when we get the error
> > > > > > message.
> > > > >
> > > > > Ah right, so this should be detected somewhere within
> > > > > shaderapi,
> > > > > will
> > > > > take a look there. The reason I suspected we end here with
> > > > > SSO
> > > > > was
> > > > > that
> > > > > this fails just on the old gen models so something definitely
> > > > > goes
> > > > > different ways with those. But I'll look at shaderapi first.
> > > >
> > > > Now I realized that I could bail in linker with following:
> > > > if (shProg->SeparateShader && shProg->NumShaders == 0)
> > > >
> > > > BUT following commit states that having NumShaders == 0 is OK
> > > > for
> > > > compatibility profile:
> > > >
> > > > 76cfb472077dc83c892b4cddf79333341deaa7b5
> > > >
> > > > Timothy, do you think I could still bailout with the condition
> > > > above?
> > > > I
> > > > really do wonder the case mentioned in commit where someone
> > > > links
> > > > a
> > > > shader without any stages, does this really make sense (what is
> > > > the
> > > > use
> > > > case) or is this allowed just because spec does not happen to
> > > > prohibit this?
> > >
> > > ok, I see. The test says it *should* relink when in compat
> > > profile.
> >
> > Actually we still return early it just shouldn't throw an error.
> >
> > >
> > > In
> > > which case it should fall back to fixed-function. I'm not
> > > entirely
> > > sure
> > > what path things take from here but it seems something is not
> > > working
> > > correctly when creating the fallback shader.
> >
> > I've taken a closer look. We don't do anything about creating
> > fallback
> > shaders in GLSL IR so we shouldn't be hitting the code where the
> > error
> > happens.
> >
> > I believe the problem is in glDetachShader() we remove the
> > gl_shader
> > but we leave gl_linked_shader in place.
>
> AFAIK that's a valid sequence - glAttachShader(); glLinkProgram();
> glDetachShader(); glDeleteShader(); - the program should still be
> usable.
Agreed but as far as I can tell we shouldn't even need gl_linked_shader
after glLinkProgram.
We should probably just free it after linking. Everything we want
*should* have been copied to gl_program, however in practice it seems
there are some things we still get from gl_linked_shader
like NumImages, but with the changes I landed recently we could just
be getting this from the new shader_info struct.
>
> -ilia
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-stable
mailing list