On Wed, 2016-11-16 at 21:17 +0000, Emil Velikov wrote:
> On 11 November 2016 at 00:45, Timothy Arceri
> <timothy.arc...@collabora.com> wrote:
> >
> > This will allow us to directly store metadata we want to retain in
> > gl_program this metadata is currently stored in gl_linked_shader
> > and
> > will be lost if relinking fails even though the program will remain
> > in use and is still valid according to the spec.
> >
> > "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."
> >
> > This change will also help avoid the double handing that happens in
> > _mesa_copy_linked_program_data().
> > ---
> > src/compiler/glsl/linker.cpp | 15 +++++++++++++++
> > src/mesa/drivers/dri/i965/brw_link.cpp | 9 +--------
> > src/mesa/program/ir_to_mesa.cpp | 6 +-----
> > src/mesa/state_tracker/st_glsl_to_nir.cpp | 7 +------
> > src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 8 ++------
> > 5 files changed, 20 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/compiler/glsl/linker.cpp
> > b/src/compiler/glsl/linker.cpp
> > index 693a50b..f63c025 100644
> > --- a/src/compiler/glsl/linker.cpp
> > +++ b/src/compiler/glsl/linker.cpp
> > @@ -72,6 +72,7 @@
> > #include "ir.h"
> > #include "program.h"
> > #include "program/prog_instruction.h"
> > +#include "program/program.h"
> > #include "util/set.h"
> > #include "util/string_to_uint_map.h"
> > #include "linker.h"
> > @@ -2183,6 +2184,20 @@ link_intrastage_shaders(void *mem_ctx,
> > }
> >
> > gl_linked_shader *linked = ctx-
> > >Driver.NewShader(shader_list[0]->Stage);
> > +
> > + /* Create program and attach it to the linked shader */
> > + struct gl_program *gl_prog =
> > + ctx->Driver.NewProgram(ctx,
> > + _mesa_shader_stage_to_program(shader_
> > list[0]->Stage),
> > + prog->Name);
> > + if (!prog) {
> > + prog->LinkStatus = false;
> > + _mesa_delete_linked_shader(ctx, linked);
> > + return NULL;
> > + }
> > +
> > + _mesa_reference_program(ctx, &linked->Program, gl_prog);
> > +
> I'm not too sure referencing seems right in this patch.
> All the error paths seem to be missing the deref, is that intentional
> or a bug ? I'm leaning toward the latter.
It's intentional, _mesa_delete_linked_shader() will remove the
reference ... although we might just what to have gl_linked_shader take
ownership of gl_program here and not use _mesa_reference_program() at
all otherwise your right the ref count will still be at 1. I think all
link_shader paths currently have what looks like a hack where they
call _mesa_reference_program(ctx, &prog, NULL); at the end of linking I
think the correct way to do it is not use _mesa_reference_program() and
just assign the pointer directly taking ownership of it.
I think I'll make a fix that goes before this patch to tidy that up,
and update this patch also.
>
> >
> > linked->ir = new(linked) exec_list;
> > clone_ir_list(mem_ctx, linked->ir, main->ir);
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
> > b/src/mesa/drivers/dri/i965/brw_link.cpp
> > index 248d2f1..d2c8ed3 100644
> > --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> > @@ -221,14 +221,7 @@ brw_link_shader(struct gl_context *ctx, struct
> > gl_shader_program *shProg)
> > if (!shader)
> > continue;
> >
> > - struct gl_program *prog =
> > - ctx->Driver.NewProgram(ctx,
> > _mesa_shader_stage_to_program(stage),
> > - 0);
> > - if (!prog)
> > - return false;
> > -
> > - _mesa_reference_program(ctx, &shader->Program, prog);
> > -
> > + struct gl_program *prog = shader->Program;
> > prog->Parameters = _mesa_new_parameter_list();
> >
> > process_glsl_ir(brw, shProg, shader);
> > diff --git a/src/mesa/program/ir_to_mesa.cpp
> > b/src/mesa/program/ir_to_mesa.cpp
> > index e24fb50b..75234d7 100644
> > --- a/src/mesa/program/ir_to_mesa.cpp
> > +++ b/src/mesa/program/ir_to_mesa.cpp
> > @@ -2791,9 +2791,7 @@ get_mesa_program(struct gl_context *ctx,
> >
> > validate_ir_tree(shader->ir);
> >
> > - prog = ctx->Driver.NewProgram(ctx, target, shader_program-
> > >Name);
> > - if (!prog)
> > - return NULL;
> > + prog = shader->Program;
> > prog->Parameters = _mesa_new_parameter_list();
> > v.ctx = ctx;
> > v.prog = prog;
> > @@ -2927,8 +2925,6 @@ get_mesa_program(struct gl_context *ctx,
> > prog->info.fs.depth_layout = shader_program-
> > >FragDepthLayout;
> > }
> >
> > - _mesa_reference_program(ctx, &shader->Program, prog);
> > -
> > if ((ctx->_Shader->Flags & GLSL_NO_OPT) == 0) {
> > _mesa_optimize_program(ctx, prog, prog);
> > }
> > diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> > b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> > index 36531ad..cbc7e5a 100644
> > --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> > +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> > @@ -367,15 +367,10 @@ st_nir_get_mesa_program(struct gl_context
> > *ctx,
> > struct gl_linked_shader *shader)
> > {
> > struct gl_program *prog;
> > - GLenum target = _mesa_shader_stage_to_program(shader->Stage);
> >
> > validate_ir_tree(shader->ir);
> >
> > - prog = ctx->Driver.NewProgram(ctx, target, shader_program-
> > >Name);
> > - if (!prog)
> > - return NULL;
> > -
> > - _mesa_reference_program(ctx, &shader->Program, prog);
> > + prog = shader->Program;
> >
> > prog->Parameters = _mesa_new_parameter_list();
> >
> > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > index 7a4ee36..c430e1a 100644
> > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > @@ -6407,7 +6407,6 @@ get_mesa_program_tgsi(struct gl_context *ctx,
> > {
> > glsl_to_tgsi_visitor* v;
> > struct gl_program *prog;
> > - GLenum target = _mesa_shader_stage_to_program(shader->Stage);
> > struct gl_shader_compiler_options *options =
> > &ctx->Const.ShaderCompilerOptions[shader->Stage];
> > struct pipe_screen *pscreen = ctx->st->pipe->screen;
> > @@ -6415,11 +6414,7 @@ get_mesa_program_tgsi(struct gl_context
> > *ctx,
> >
> > validate_ir_tree(shader->ir);
> >
> > - prog = ctx->Driver.NewProgram(ctx, target, shader_program-
> > >Name);
> > - if (!prog)
> > - return NULL;
> > -
> > - _mesa_reference_program(ctx, &shader->Program, prog);
> > + prog = shader->Program;
> >
> > prog->Parameters = _mesa_new_parameter_list();
> > v = new glsl_to_tgsi_visitor();
> > @@ -6540,6 +6535,7 @@ get_mesa_program_tgsi(struct gl_context *ctx,
> > */
> > _mesa_associate_uniform_storage(ctx, shader_program, prog-
> > >Parameters);
> > if (!shader_program->LinkStatus) {
> > + _mesa_reference_program(ctx, &shader->Program, NULL);
> > free_glsl_to_tgsi_visitor(v);
> > return NULL;
> > }
> Mildly (if at all?) related comment: worth adding the above/similar
> hunk to st_glsl_to_nir.cpp:st_nir_get_mesa_program or there's
> something subtle happening there. In which case worth adding a
> comment
> ?
st_nir_get_mesa_program() never checks for a linking error (I'm not
100% sure why it is different in that respect)
> In either case that's something which can be addressed separately.
>
> With the extra deref(s) on error in link_intrastage_shaders (assuming
> I haven't lost my marbles), this and 10/70 are
> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>
> -Emil
> _______________________________________________
> 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
