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.
> 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
?
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
