[Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs
Timothy Arceri
timothy.arceri at collabora.com
Wed Oct 26 11:51:34 UTC 2016
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. 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.
>
>
> >
> >
> >
> > >
> > > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > 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=9771
> > > > > > 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
More information about the mesa-dev
mailing list