[Mesa-stable] [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs
Timothy Arceri
timothy.arceri at collabora.com
Thu Oct 27 00:08:41 UTC 2016
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 entry
> > > > > =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_shader
> > > > > _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.
>
>
> >
> >
> >
> > >
> > >
> > >
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > This patch fixes regression
> > > > > > > caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
> > > > > > >
> > > > > > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97
> > > > > > > 71
> > > > > > > 5
> > > > > > > Cc: "12.0 13.0" <mesa-stable at lists.freedesktop.org>
> > > > > > > ---
> > > > > > > src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++++++++++--
> > > > > > > -----
> > > > > > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
> > > > > > > b/src/mesa/drivers/dri/i965/brw_link.cpp
> > > > > > > index 5ea9773..ffb66a9 100644
> > > > > > > --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> > > > > > > +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> > > > > > > @@ -290,14 +290,17 @@ brw_link_shader(struct gl_context
> > > > > > > *ctx,
> > > > > > > struct
> > > > > > > gl_shader_program *shProg)
> > > > > > >
> > > > > > > build_program_resource_list(ctx, shProg);
> > > > > > >
> > > > > > > - for (stage = 0; stage < ARRAY_SIZE(shProg-
> > > > > > > >
> > > > > > > > _LinkedShaders);
> > > > > > > stage++) {
> > > > > > > - struct gl_linked_shader *shader = shProg-
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > _LinkedShaders[stage];
> > > > > > > - if (!shader)
> > > > > > > - continue;
> > > > > > > + /* We can't free IR for SSO programs since those may
> > > > > > > need
> > > > > > > relinking. */
> > > > > > > + if (!shProg->SeparateShader) {
> > > > > > > + for (stage = 0; stage < ARRAY_SIZE(shProg-
> > > > > > > >
> > > > > > > >
> > > > > > > > _LinkedShaders);
> > > > > > > stage++) {
> > > > > > > + struct gl_linked_shader *shader = shProg-
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > _LinkedShaders[stage];
> > > > > > > + if (!shader)
> > > > > > > + continue;
> > > > > > >
> > > > > > > - /* The GLSL IR won't be needed anymore. */
> > > > > > > - ralloc_free(shader->ir);
> > > > > > > - shader->ir = NULL;
> > > > > > > + /* The GLSL IR won't be needed anymore. */
> > > > > > > + ralloc_free(shader->ir);
> > > > > > > + shader->ir = NULL;
> > > > > > > + }
> > > > > > > }
> > > > > > >
> > > > > > > return true;
> > > > > _______________________________________________
> > > > > 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
> > _______________________________________________
> > 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