Re: [PATCH 16/29] drm/i915: Export intel_context_instance()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 08/04/2019 10:17, Chris Wilson wrote:
> We want to pass in a intel_context into intel_context_pin() and that
> requires us to first be able to lookup the intel_context!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_context.c    | 37 +++++++++++-----------
>   drivers/gpu/drm/i915/gt/intel_context.h    | 19 +++++++----
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c  |  8 ++++-
>   drivers/gpu/drm/i915/gt/mock_engine.c      |  8 ++++-
>   drivers/gpu/drm/i915/gvt/scheduler.c       |  7 +++-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++--
>   drivers/gpu/drm/i915/i915_perf.c           | 21 ++++++++----
>   drivers/gpu/drm/i915/i915_request.c        | 11 ++++++-
>   8 files changed, 83 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 6ae6a3f58364..a1267739e369 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -104,7 +104,7 @@ void __intel_context_remove(struct intel_context *ce)
>   	spin_unlock(&ctx->hw_contexts_lock);
>   }
>   
> -static struct intel_context *
> +struct intel_context *
>   intel_context_instance(struct i915_gem_context *ctx,
>   		       struct intel_engine_cs *engine)
>   {
> @@ -112,7 +112,7 @@ intel_context_instance(struct i915_gem_context *ctx,
>   
>   	ce = intel_context_lookup(ctx, engine);
>   	if (likely(ce))
> -		return ce;
> +		return intel_context_get(ce);
>   
>   	ce = intel_context_alloc();
>   	if (!ce)
> @@ -125,7 +125,7 @@ intel_context_instance(struct i915_gem_context *ctx,
>   		intel_context_free(ce);
>   
>   	GEM_BUG_ON(intel_context_lookup(ctx, engine) != pos);
> -	return pos;
> +	return intel_context_get(pos);
>   }
>   
>   struct intel_context *
> @@ -139,30 +139,30 @@ intel_context_pin_lock(struct i915_gem_context *ctx,
>   	if (IS_ERR(ce))
>   		return ce;
>   
> -	if (mutex_lock_interruptible(&ce->pin_mutex))
> +	if (mutex_lock_interruptible(&ce->pin_mutex)) {
> +		intel_context_put(ce);
>   		return ERR_PTR(-EINTR);
> +	}
>   
>   	return ce;
>   }
>   
> -struct intel_context *
> -intel_context_pin(struct i915_gem_context *ctx,
> -		  struct intel_engine_cs *engine)
> +void intel_context_pin_unlock(struct intel_context *ce)
> +	__releases(ce->pin_mutex)
>   {
> -	struct intel_context *ce;
> -	int err;
> -
> -	ce = intel_context_instance(ctx, engine);
> -	if (IS_ERR(ce))
> -		return ce;
> +	mutex_unlock(&ce->pin_mutex);
> +	intel_context_put(ce);
> +}
>   
> -	if (likely(atomic_inc_not_zero(&ce->pin_count)))
> -		return ce;
> +int __intel_context_do_pin(struct intel_context *ce)
> +{
> +	int err;
>   
>   	if (mutex_lock_interruptible(&ce->pin_mutex))
> -		return ERR_PTR(-EINTR);
> +		return -EINTR;
>   
>   	if (likely(!atomic_read(&ce->pin_count))) {
> +		struct i915_gem_context *ctx = ce->gem_context;
>   		intel_wakeref_t wakeref;
>   
>   		err = 0;
> @@ -172,7 +172,6 @@ intel_context_pin(struct i915_gem_context *ctx,
>   			goto err;
>   
>   		i915_gem_context_get(ctx);
> -		GEM_BUG_ON(ce->gem_context != ctx);
>   
>   		mutex_lock(&ctx->mutex);
>   		list_add(&ce->active_link, &ctx->active_engines);
> @@ -186,11 +185,11 @@ intel_context_pin(struct i915_gem_context *ctx,
>   	GEM_BUG_ON(!intel_context_is_pinned(ce)); /* no overflow! */
>   
>   	mutex_unlock(&ce->pin_mutex);
> -	return ce;
> +	return 0;
>   
>   err:
>   	mutex_unlock(&ce->pin_mutex);
> -	return ERR_PTR(err);
> +	return err;
>   }
>   
>   void intel_context_unpin(struct intel_context *ce)
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 9aeef88176b9..da342e9a8c2e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -49,11 +49,7 @@ intel_context_is_pinned(struct intel_context *ce)
>   	return atomic_read(&ce->pin_count);
>   }
>   
> -static inline void intel_context_pin_unlock(struct intel_context *ce)
> -__releases(ce->pin_mutex)
> -{
> -	mutex_unlock(&ce->pin_mutex);
> -}

Could leave this as static inline since the only addition is kref_put so 
compiler could decide what to do? Don't mind either way.

> +void intel_context_pin_unlock(struct intel_context *ce);
>   
>   struct intel_context *
>   __intel_context_insert(struct i915_gem_context *ctx,
> @@ -63,7 +59,18 @@ void
>   __intel_context_remove(struct intel_context *ce);
>   
>   struct intel_context *
> -intel_context_pin(struct i915_gem_context *ctx, struct intel_engine_cs *engine);
> +intel_context_instance(struct i915_gem_context *ctx,
> +		       struct intel_engine_cs *engine);
> +
> +int __intel_context_do_pin(struct intel_context *ce);
> +
> +static inline int intel_context_pin(struct intel_context *ce)
> +{
> +	if (likely(atomic_inc_not_zero(&ce->pin_count)))
> +		return 0;
> +
> +	return __intel_context_do_pin(ce);
> +}
>   
>   static inline void __intel_context_pin(struct intel_context *ce)
>   {
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index d378b276e476..f6828c0276eb 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -697,11 +697,17 @@ static int pin_context(struct i915_gem_context *ctx,
>   		       struct intel_context **out)
>   {
>   	struct intel_context *ce;
> +	int err;
>   
> -	ce = intel_context_pin(ctx, engine);
> +	ce = intel_context_instance(ctx, engine);
>   	if (IS_ERR(ce))
>   		return PTR_ERR(ce);
>   
> +	err = intel_context_pin(ce);
> +	intel_context_put(ce);
> +	if (err)
> +		return err;
> +
>   	*out = ce;
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index d9f68c89dff4..a79d9909d171 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -239,6 +239,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>   				    int id)
>   {
>   	struct mock_engine *engine;
> +	int err;
>   
>   	GEM_BUG_ON(id >= I915_NUM_ENGINES);
>   
> @@ -278,10 +279,15 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>   	INIT_LIST_HEAD(&engine->hw_queue);
>   
>   	engine->base.kernel_context =
> -		intel_context_pin(i915->kernel_context, &engine->base);
> +		intel_context_instance(i915->kernel_context, &engine->base);
>   	if (IS_ERR(engine->base.kernel_context))
>   		goto err_breadcrumbs;
>   
> +	err = intel_context_pin(engine->base.kernel_context);
> +	intel_context_put(engine->base.kernel_context);
> +	if (err)
> +		goto err_breadcrumbs;
> +
>   	return &engine->base;
>   
>   err_breadcrumbs:
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 40d9f549a0cd..606fc2713240 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -1188,12 +1188,17 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
>   		INIT_LIST_HEAD(&s->workload_q_head[i]);
>   		s->shadow[i] = ERR_PTR(-EINVAL);
>   
> -		ce = intel_context_pin(ctx, engine);
> +		ce = intel_context_instance(ctx, engine);
>   		if (IS_ERR(ce)) {
>   			ret = PTR_ERR(ce);
>   			goto out_shadow_ctx;
>   		}
>   
> +		ret = intel_context_pin(ce);
> +		intel_context_put(ce);
> +		if (ret)
> +			goto out_shadow_ctx;
> +
>   		s->shadow[i] = ce;
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 35732c2ae17f..c700cbc2f594 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2101,14 +2101,19 @@ static int eb_pin_context(struct i915_execbuffer *eb,
>   	if (err)
>   		return err;
>   
> +	ce = intel_context_instance(eb->gem_context, engine);
> +	if (IS_ERR(ce))
> +		return PTR_ERR(ce);
> +
>   	/*
>   	 * Pinning the contexts may generate requests in order to acquire
>   	 * GGTT space, so do this first before we reserve a seqno for
>   	 * ourselves.
>   	 */
> -	ce = intel_context_pin(eb->gem_context, engine);
> -	if (IS_ERR(ce))
> -		return PTR_ERR(ce);
> +	err = intel_context_pin(ce);
> +	intel_context_put(ce);
> +	if (err)
> +		return err;
>   
>   	eb->engine = engine;
>   	eb->context = ce;
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 328a740e72cb..afaeabe5e531 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1205,11 +1205,17 @@ static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
>   {
>   	struct intel_engine_cs *engine = i915->engine[RCS0];
>   	struct intel_context *ce;
> -	int ret;
> +	int err;
>   
> -	ret = i915_mutex_lock_interruptible(&i915->drm);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	ce = intel_context_instance(ctx, engine);
> +	if (IS_ERR(ce))
> +		return ce;
> +
> +	err = i915_mutex_lock_interruptible(&i915->drm);
> +	if (err) {
> +		intel_context_put(ce);
> +		return ERR_PTR(err);
> +	}
>   
>   	/*
>   	 * As the ID is the gtt offset of the context's vma we
> @@ -1217,10 +1223,11 @@ static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
>   	 *
>   	 * NB: implied RCS engine...
>   	 */
> -	ce = intel_context_pin(ctx, engine);
> +	err = intel_context_pin(ce);
>   	mutex_unlock(&i915->drm.struct_mutex);
> -	if (IS_ERR(ce))
> -		return ce;
> +	intel_context_put(ce);
> +	if (err)
> +		return ERR_PTR(err);
>   
>   	i915->perf.oa.pinned_ctx = ce;
>   
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 7932d1209247..8abd891d9287 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -753,6 +753,7 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>   	struct drm_i915_private *i915 = engine->i915;
>   	struct intel_context *ce;
>   	struct i915_request *rq;
> +	int err;
>   
>   	/*
>   	 * Preempt contexts are reserved for exclusive use to inject a
> @@ -766,13 +767,21 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>   	 * GGTT space, so do this first before we reserve a seqno for
>   	 * ourselves.
>   	 */
> -	ce = intel_context_pin(ctx, engine);
> +	ce = intel_context_instance(ctx, engine);
>   	if (IS_ERR(ce))
>   		return ERR_CAST(ce);
>   
> +	err = intel_context_pin(ce);
> +	if (err) {
> +		rq = ERR_PTR(err);
> +		goto err_put;
> +	}
> +
>   	rq = i915_request_create(ce);
>   	intel_context_unpin(ce);
>   
> +err_put:
> +	intel_context_put(ce);
>   	return rq;
>   }
>   
> 

The pattern of instance-pin-put is repeated so many times it begs to be 
promoted to something like intel_context_pin_instance.

Side note, it is a bit confusing between reference count and pin count. 
I won't try to make sense of it all now.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx