[Mesa-dev] [PATCH 14/70] mesa: create new gl_shader_program_data struct

[Timothy Arceri]

On Fri, 2016-11-18 at 20:35 +0000, Emil Velikov wrote:
> On 11 November 2016 at 00:45, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > 
> > This will be used to share data between gl_program and
> > gl_shader_program
> > allowing for greater code simplification as we can remove a number
> > of
> > awkward uses of gl_shader_program.
> > ---
> >  src/mesa/main/mtypes.h    | 25 +++++++++++++++++++++++++
> >  src/mesa/main/shaderobj.c | 41
> > +++++++++++++++++++++++++++++++++++++++++
> >  src/mesa/main/shaderobj.h |  5 +++++
> >  3 files changed, 71 insertions(+)
> > 
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 600b1da..9500ec9 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -2625,6 +2625,31 @@ struct gl_program_resource
> >  };
> > 
> >  /**
> > + * A data structure to be shared by gl_shader_program and
> > gl_program.
> > + */
> > +struct gl_shader_program_data
> > +{
> > +   GLint RefCount;  /**< Reference count */
> > +
> > +   unsigned NumUniformStorage;
> > +   unsigned NumHiddenUniforms;
> > +   struct gl_uniform_storage *UniformStorage;
> > +
> > +   unsigned NumUniformBlocks;
> > +   struct gl_uniform_block *UniformBlocks;
> > +
> > +   unsigned NumShaderStorageBlocks;
> > +   struct gl_uniform_block *ShaderStorageBlocks;
> > +
> > +   struct gl_active_atomic_buffer *AtomicBuffers;
> > +   unsigned NumAtomicBuffers;
> > +
> > +   GLboolean LinkStatus;   /**< GL_LINK_STATUS */
> > +   GLboolean Validated;
> > +   GLchar *InfoLog;
> > +};
> > +
> > +/**
> >   * A GLSL program object.
> >   * Basically a linked collection of vertex and fragment shaders.
> >   */
> > diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> > index 8fd574e..a753a1b 100644
> > --- a/src/mesa/main/shaderobj.c
> > +++ b/src/mesa/main/shaderobj.c
> > @@ -41,6 +41,7 @@
> >  #include "program/prog_parameter.h"
> >  #include "util/ralloc.h"
> >  #include "util/string_to_uint_map.h"
> > +#include "util/u_atomic.h"
> > 
> >  /*****************************************************************
> > *****/
> >  /*** Shader object
> > functions                                        ***/
> > @@ -208,6 +209,35 @@ _mesa_lookup_shader_err(struct gl_context
> > *ctx, GLuint name, const char *caller)
> >  /*****************************************************************
> > *****/
> > 
> > 
> > +void
> > +_mesa_reference_shader_program_data(struct gl_context *ctx,
> > +                                    struct gl_shader_program_data
> > **ptr,
> > +                                    struct gl_shader_program_data
> > *data)
> > +{
> > +   if (*ptr == data)
> > +      return;
> > +
> > +   if (*ptr) {
> > +      struct gl_shader_program_data *oldData = *ptr;
> > +
> > +      assert(oldData->RefCount > 0);
> > +
> > +      if (p_atomic_dec_zero(&oldData->RefCount)) {
> Yay for atomics and good bye locking ;-)
> 
> > 
> > +         assert(ctx);
> > +         ralloc_free(oldData);
> > +      }
> > +
> > +      *ptr = NULL;
> > +   }
> > +
> > +   assert(!*ptr);
> Dull moment, when can this trigger ? We seems to have this in a fair
> few places in mesa, yet nothing obvious comes up.

Yeah I think I copied this from the other functions it looks safe to
remove, probably left over from a previous change.

> 
> > 
> > +   if (data) {
> > +      p_atomic_inc(&data->RefCount);
> > +   }
> > +
> Please drop the extra parenthesis.
> 
> > 
> > +   *ptr = data;
> > +}
> > +
> >  /**
> >   * Set ptr to point to shProg.
> >   * If ptr is pointing to another object, decrement its refcount
> > (and delete
> > @@ -249,6 +279,17 @@ _mesa_reference_shader_program_(struct
> > gl_context *ctx,
> >     }
> >  }
> > 
> > +static struct gl_shader_program_data *
> > +create_shader_program_data()
> > +{
> > +   struct gl_shader_program_data *data;
> > +   data = rzalloc(NULL, struct gl_shader_program_data);
> Worth passing in a ctx, (gl_shader_program *) as opposed to using
> NULL ?

No. The ref counting is designed to clean this up because by the end of
this series it will be possible for the user to have freed the program
(which will free gl_shader_program) but the program can still be active
if a new program hasn't been pushed into the pipeline which means
gl_program still needs access to this data.

> 
> > 
> > +   if (data) {
> > +      data->RefCount = 1;
> > +   }
> Drop the parenthesis ?

I don't really mind. I can drop them however I don't think this is a
rule, it's even been suggested at some point we always use them to
avoid silly bugs being introduced.

> 
> -Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev