On Thu, Nov 5, 2015 at 1:13 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Sat, Oct 24, 2015 at 10:07 AM, Rob Clark <robdcl...@gmail.com> wrote:
>> From: Rob Clark <robcl...@freedesktop.org>
>>
>> For gallium, at least, we'll need this to manage shader's lifetimes,
>> since in some cases both the driver and the state tracker will need
>> to hold on to a reference for variant managing.
>>
>> Use nir_shader_mutable() before doing any IR opt/lowering/etc, to
>> ensure you are not modifying a copy someone else is also holding a
>> reference to. In this way, unnecessary nir_shader_clone()s are
>> avoided whenever possible.
>>
>> TODO: Any places missed to s/ralloc_free()/nir_shader_unref()/ outside
>> of freedreno/ir3?
>
> I talked with Ken about this a few days ago, and it doesn't seem like
> a good idea. Adding reference counting to NIR means mixing two very
> different memory management models. NIR already uses the ralloc model
> where a shader is tied to a context. Allowing both ralloc and
> refcount models is a bad idea because freeing a context could cause
> the shader to go out-of-scope early from the refcount perspective and
> refcounting means you don't know what thread the shader is deleted in
> which is really bad for ralloc. The only way we could do this is if
> we only used ralloc internally to NIR and disallowed parenting a
> nir_shader to anything. I don't really think we want this, at least
> not for the i965 driver.
>
> The best option here would probably be to make a little flyweight
> wrapper object for TGSI to use that acts as the ralloc context for a
> NIR shader and is, itself, reference-counted. You still have to
> provide similar guarantees (said object can never have a parent), but
> it would keep those guarantees obvious.
Hmm, maybe a better idea still (and simpler) would be to just disallow
nir_shader_ref() if parent memctx is not null?
This way we can keep the centralized checks for refcnt>1 in the
nir-pass runner function/macro/whatever..
BR,
-R
> Sorry :-/
> --Jason
>
>> Signed-off-by: Rob Clark <robcl...@freedesktop.org>
>> ---
>> src/gallium/drivers/vc4/vc4_program.c | 2 +-
>> src/glsl/nir/nir.c | 2 ++
>> src/glsl/nir/nir.h | 43
>> +++++++++++++++++++++++++++++++++++
>> src/mesa/program/program.c | 3 ++-
>> 4 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/vc4/vc4_program.c
>> b/src/gallium/drivers/vc4/vc4_program.c
>> index 5d7564b..9a055df 100644
>> --- a/src/gallium/drivers/vc4/vc4_program.c
>> +++ b/src/gallium/drivers/vc4/vc4_program.c
>> @@ -1723,7 +1723,7 @@ vc4_shader_ntq(struct vc4_context *vc4, enum qstage
>> stage,
>> c->num_uniforms);
>> }
>>
>> - ralloc_free(c->s);
>> + nir_shader_unref(c->s);
>>
>> return c;
>> }
>> diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c
>> index 2defa36..53a11f5 100644
>> --- a/src/glsl/nir/nir.c
>> +++ b/src/glsl/nir/nir.c
>> @@ -36,6 +36,8 @@ nir_shader_create(void *mem_ctx,
>> {
>> nir_shader *shader = ralloc(mem_ctx, nir_shader);
>>
>> + p_atomic_set(&shader->refcount, 1);
>> +
>> exec_list_make_empty(&shader->uniforms);
>> exec_list_make_empty(&shader->inputs);
>> exec_list_make_empty(&shader->outputs);
>> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
>> index 926747c..3ab720b 100644
>> --- a/src/glsl/nir/nir.h
>> +++ b/src/glsl/nir/nir.h
>> @@ -34,6 +34,7 @@
>> #include "util/ralloc.h"
>> #include "util/set.h"
>> #include "util/bitset.h"
>> +#include "util/u_atomic.h"
>> #include "nir_types.h"
>> #include "shader_enums.h"
>> #include <stdio.h>
>> @@ -1546,6 +1547,8 @@ typedef struct nir_shader_info {
>> } nir_shader_info;
>>
>> typedef struct nir_shader {
>> + int refcount;
>> +
>> /** list of uniforms (nir_variable) */
>> struct exec_list uniforms;
>>
>> @@ -1894,6 +1897,46 @@ void nir_print_instr(const nir_instr *instr, FILE
>> *fp);
>>
>> nir_shader * nir_shader_clone(void *mem_ctx, const nir_shader *s);
>>
>> +static inline void
>> +nir_shader_ref(nir_shader *shader)
>> +{
>> + p_atomic_inc(&shader->refcount);
>> +}
>> +
>> +static inline void
>> +nir_shader_unref(nir_shader *shader)
>> +{
>> + if (p_atomic_dec_zero(&shader->refcount)) {
>> + ralloc_free(shader);
>> + }
>> +}
>> +
>> +/* A shader with only a single reference is mutable: */
>> +static inline bool
>> +nir_shader_is_mutable(nir_shader *shader)
>> +{
>> + return p_atomic_read(&shader->refcount) == 1;
>> +}
>> +
>> +/* Convert a shader reference into a mutable shader reference. Ie. if
>> + * there is only a single reference to the shader, then return that,
>> + * otherwise clone and drop reference to existing shader.
>> + *
>> + * TODO maybe an assert that shader->refcnt == 1 in the various opt/
>> + * lowering passes? If only there was a good central place to put it.
>> + */
>> +static inline nir_shader *
>> +nir_shader_mutable(nir_shader *shader)
>> +{
>> + if (nir_shader_is_mutable(shader)) {
>> + return shader;
>> + } else {
>> + nir_shader *ns = nir_shader_clone(ralloc_parent(shader), shader);
>> + nir_shader_unref(shader);
>> + return ns;
>> + }
>> +}
>> +
>> #ifdef DEBUG
>> void nir_validate_shader(nir_shader *shader);
>> #else
>> diff --git a/src/mesa/program/program.c b/src/mesa/program/program.c
>> index 0e78e6a..c2da66e 100644
>> --- a/src/mesa/program/program.c
>> +++ b/src/mesa/program/program.c
>> @@ -38,6 +38,7 @@
>> #include "prog_parameter.h"
>> #include "prog_instruction.h"
>> #include "util/ralloc.h"
>> +#include "nir.h"
>>
>>
>> /**
>> @@ -273,7 +274,7 @@ _mesa_delete_program(struct gl_context *ctx, struct
>> gl_program *prog)
>> }
>>
>> if (prog->nir) {
>> - ralloc_free(prog->nir);
>> + nir_shader_unref(prog->nir);
>> }
>>
>> mtx_destroy(&prog->Mutex);
>> --
>> 2.5.0
>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
