[Mesa-stable] [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs
Timothy Arceri
timothy.arceri at collabora.com
Thu Oct 27 10:03:12 UTC 2016
On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
> 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
> > > > > > > > @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.
I've started work on this:
https://patchwork.freedesktop.org/series/14471/
But I'm not sure I have is in me to do another big refactor right now.
Maybe we could put this out there as a beginner/easy task for someone
to do.
IMO what we should do is turn gl_linked_shader into a temporary
structure that holds stuff we need during linking (IR, packed_varyings
list, etc) and release it after linking. The result would be much more
consistent code, currently we seem to just randomly grab/store things
from/to gl_linked_shader or gl_program without much thought.
The spec states:
"If a program object that is active for any shader stage is re-linked
unsuccessfully, the link status will be set to FALSE, but any existing
executables and associated state will remain part of the current
rendering state until a subsequent call to UseProgram,
UseProgramStages, or BindProgramPipeline removes them from use."
And since we destroy the previous gl_linked_shader when relinking we
shouldn't be storing anything related to state in here anyway, since we
will end up a broken state if linking fails.
Freeing gl_linked_program will help us avoid this mistake in future.
We would also probably want to move the pointer out to gl_program out
of gl_linked_shader and instead have an array like _LinkedShader in
gl_shader_program for gl_program.
>
> >
> >
> > -ilia
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> 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