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

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/i915: teardown default context in reverse, update comments
Date: Mon, 21 Dec 2015 11:48:35 +0100	[thread overview]
Message-ID: <20151221104835.GJ30437@phenom.ffwll.local> (raw)
In-Reply-To: <1450291011-31486-2-git-send-email-david.s.gordon@intel.com>

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@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?

Thanks, Daniel

>  		ring->default_context = ctx;
>  	}
>  
> @@ -431,14 +437,21 @@ void i915_gem_context_fini(struct drm_device *dev)
>  		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
>  	}
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for (i = I915_NUM_RINGS; --i >= 0;) {
>  		struct intel_engine_cs *ring = &dev_priv->ring[i];
>  
> -		if (ring->last_context)
> +		if (ring->last_context) {
>  			i915_gem_context_unreference(ring->last_context);
> +			ring->last_context = NULL;
> +		}
>  
> +		/*
> +		 * These default_context pointers don't contribute to the
> +		 * refcount on the context. We consider that RCS holds its
> +		 * reference on behalf of all the other engines, so there's
> +		 * just a single unreference() call below.
> +		 */
>  		ring->default_context = NULL;
> -		ring->last_context = NULL;
>  	}
>  
>  	i915_gem_context_unreference(dctx);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx