[Mesa-dev] [PATCH 6/7] nir: add shader reference counting

[Rob Clark]

On Thu, Nov 5, 2015 at 7:24 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Saturday, October 24, 2015 01:07:59 PM Rob Clark wrote:
>> From: Rob Clark <robclark at 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?
>>
>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>
> I'm pretty uncomfortable with mixing ralloc and reference counting.
>
> A ralloc context is essentially a linked list of child allocations.
> Freeing an item alters the parent context's linked list.  As such,
> this is inherently not threadsafe at all.  (It works great if you
> keep memory contexts within a single thread...)

I agree, refcnt'ing should be mutually exclusive with a shared parent memctx.

Although currently nir_shader_create() is always called w/ a null
memctx, so I'm going with the theory that refcnt'ing is more useful
than parenting the shader under something else ;-)

But I'm all for debug asserts to make sure anyone using a memctx
parent isn't using refcnting and visa versa..

I guess even for debug builds we could add a do-not-reparent bit in
ralloc_header, perhaps?  (Although I guess that would need some magic
in nir_sweep()..)

> I'm afraid that if a NIR shader is used in multiple threads, with
> reference counting...and allocated out of a parent context...then
> we could corrupt the parent memory context's list, which would be
> an awful bug to track down.

Yeah, lets try to disallow that.  However the cases where we want
refcnt'ing we definitely won't be having a parent memctx..

> If reference counting is useful, then perhaps the best solution is
> to alter nir_shader_create() to *not* take a mem_ctx parameter, and
> simply do:
>
>    nir_shader *shader = ralloc(NULL, nir_shader);
>
> Then, we're guaranteed that nir_shaders won't have a parent memory
> context, avoiding the potential threading fiasco.  We can still use
> ralloc safely within a shader, but manage whole shaders entirely with
> reference counting...

I'm all for this approach :-)

BR,
-R

> --Ken