[Intel-gfx] [PATCH 1/4] drm/i915: teardown default context in reverse, update comments

[Chris Wilson]

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 at intel.com>
> > ---
> >  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