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

[Rob Clark]

On Thu, Nov 5, 2015 at 5:58 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Thu, Nov 5, 2015 at 10:53 AM, Rob Clark <robdclark at gmail.com> wrote:
>> On Thu, Nov 5, 2015 at 1:13 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>> On Sat, Oct 24, 2015 at 10:07 AM, Rob Clark <robdclark at gmail.com> 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?
>>>
>>> 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?
>
> Yeah, we could do that.  However, we have no way of doing a similar
> check to say you can't reparent it if refcount > 1.

well, we could always add a nir_shader_reparent()..

current state, this all seems a bit hypothetical since
nir_shader_create() is always called w/ a NULL parent memctx..

>> This way we can keep the centralized checks for refcnt>1 in the
>> nir-pass runner function/macro/whatever..
>
> Meh.  Call the "get me a unique shader" function in your backend
> before you do any transformations on it..  There's no reason to call
> it multiple times.  If the refcount goes from 1 to > 1 during your
> optimization loop, you're sunk anyway.

I do actually do this now, but there are places where mesa st will do
lowering passes, etc.  I do really like the
kill-all-birds-with-one-stone approach of putting the check in the
nir-pass runner.

BR,
-R