On Thu, Nov 5, 2015 at 7:24 PM, Kenneth Graunke <kenn...@whitecape.org> wrote: > On Saturday, October 24, 2015 01:07:59 PM Rob Clark 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? >> >> Signed-off-by: Rob Clark <robcl...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
