On Mon, Dec 21, 2015 at 11:48:35AM +0100, Daniel Vetter wrote:
> On Wed, Dec 16, 2015 at 06:36:48PM +0000, Dave Gordon wrote:
> > We set up engines in forwards order, so some things (notably the
> > default context) are "owned" by engine 0 (the render engine, aka "RCS").
> > For symmetry and to make sure such shared objects don't disappear too
> > early, we should generally run teardown loops in the reverse order,
> > so that engine 0 is processed last.
> >
> > This patch changes i915_gem_context_fini() to do that, and clarifies the
> > comments in i915_gem_context_{init,fini}() about the refcounting of the
> > default {struct intel_)context: the refcount is just ONE, no matter how
> > many rings exist or are active, and this refcount is nominally ascribed
> > to the render ring (RCS), which is set up first and now torn down last.
> >
> > Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 900ffd0..e143ea5 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev)
> > for (i = 0; i < I915_NUM_RINGS; i++) {
> > struct intel_engine_cs *ring = &dev_priv->ring[i];
> >
> > - /* NB: RCS will hold a ref for all rings */
> > + /*
> > + * Although each engine has a pointer to the global default
> > + * context, they don't contribute to the refcount on the
> > + * context. We consider that RCS (which is set up first and
> > + * torn down last) holds this reference on behalf of all the
> > + * other engines
> > + */
>
> Instead of piles of comments, can't we just reference-count this pointer
> properly? Pointers to reference-counted objects which don't hold a full
> reference are just fraught with peril, and doing that should imo only be
> done when there's really clear performance data justifying the
> atomic_inc/dec overhead. Init/teardown code isn't such a place.
>
> This misdesign goes back to the original execlist merge, which expanded
> the default context from RCS to all engines.
>
> Or do I miss something and we can't do this?
We can. It's actually even easier to just do dev_priv->kernel_context.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
