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

[Nick Hoath]

Reviewed-by: Nick Hoath <nicholas.hoath at intel.com>

On 16/12/2015 18:36, Gordon, David S 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
> +		 */
>   		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);
>