[Mesa-dev] [PATCH 11/70] glsl: create gl_program at the start of linking rather than the end

[Emil Velikov]

On 16 November 2016 at 21:47, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Wed, 2016-11-16 at 21:17 +0000, Emil Velikov wrote:
>> On 11 November 2016 at 00:45, Timothy Arceri
>> <timothy.arceri at 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.
>
After a long and careful look I believe you're spot on. The new
patches v2 11a and v2 11 are
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

> st_nir_get_mesa_program() never checks for a linking error (I'm not
> 100% sure why it is different in that respect)
>
Ack. We can unravel this at some other time.

-Emil