[Intel-gfx] [PATCH 17/50] drm/i915: Switch back to an array of logical per-engine HW contexts

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Fri Apr 12 13:31:31 UTC 2019


On 12/04/2019 09:53, Chris Wilson wrote:
> We switched to a tree of per-engine HW context to accommodate the
> introduction of virtual engines. However, we plan to also support
> multiple instances of the same engine within the GEM context, defeating
> our use of the engine as a key to looking up the HW context. Just
> allocate a logical per-engine instance and always use an index into the
> ctx->engines[]. Later on, this ctx->engines[] may be replaced by a user
> specified map.
> 
> v2: Add for_each_gem_engine() helper to iterator within the engines lock
> v3: intel_context_create_request() helper
> v4: s/unsigned long/unsigned int/ 4 billion engines is quite enough.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_context.c       | 112 +++-------------
>   drivers/gpu/drm/i915/gt/intel_context.h       |  27 +---
>   drivers/gpu/drm/i915/gt/intel_context_types.h |   2 -
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   2 +-
>   drivers/gpu/drm/i915/gt/mock_engine.c         |   3 +-
>   drivers/gpu/drm/i915/gvt/scheduler.c          |   2 +-
>   drivers/gpu/drm/i915/i915_gem.c               |  24 ++--
>   drivers/gpu/drm/i915/i915_gem_context.c       | 124 ++++++++++++++++--
>   drivers/gpu/drm/i915/i915_gem_context.h       |  51 +++++++
>   drivers/gpu/drm/i915/i915_gem_context_types.h |  33 ++++-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c    |  70 +++++-----
>   drivers/gpu/drm/i915/i915_perf.c              |  76 ++++++-----
>   drivers/gpu/drm/i915/i915_request.c           |  15 +--
>   drivers/gpu/drm/i915/intel_guc_submission.c   |  23 ++--
>   .../gpu/drm/i915/selftests/i915_gem_context.c |   2 +-
>   drivers/gpu/drm/i915/selftests/mock_context.c |  14 +-
>   16 files changed, 338 insertions(+), 242 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 15ac99c5dd4a..5e506e648454 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -17,7 +17,7 @@ static struct i915_global_context {
>   	struct kmem_cache *slab_ce;
>   } global;
>   
> -struct intel_context *intel_context_alloc(void)
> +static struct intel_context *intel_context_alloc(void)
>   {
>   	return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
>   }
> @@ -28,104 +28,17 @@ void intel_context_free(struct intel_context *ce)
>   }
>   
>   struct intel_context *
> -intel_context_lookup(struct i915_gem_context *ctx,
> +intel_context_create(struct i915_gem_context *ctx,
>   		     struct intel_engine_cs *engine)
>   {
> -	struct intel_context *ce = NULL;
> -	struct rb_node *p;
> -
> -	spin_lock(&ctx->hw_contexts_lock);
> -	p = ctx->hw_contexts.rb_node;
> -	while (p) {
> -		struct intel_context *this =
> -			rb_entry(p, struct intel_context, node);
> -
> -		if (this->engine == engine) {
> -			GEM_BUG_ON(this->gem_context != ctx);
> -			ce = this;
> -			break;
> -		}
> -
> -		if (this->engine < engine)
> -			p = p->rb_right;
> -		else
> -			p = p->rb_left;
> -	}
> -	spin_unlock(&ctx->hw_contexts_lock);
> -
> -	return ce;
> -}
> -
> -struct intel_context *
> -__intel_context_insert(struct i915_gem_context *ctx,
> -		       struct intel_engine_cs *engine,
> -		       struct intel_context *ce)
> -{
> -	struct rb_node **p, *parent;
> -	int err = 0;
> -
> -	spin_lock(&ctx->hw_contexts_lock);
> -
> -	parent = NULL;
> -	p = &ctx->hw_contexts.rb_node;
> -	while (*p) {
> -		struct intel_context *this;
> -
> -		parent = *p;
> -		this = rb_entry(parent, struct intel_context, node);
> -
> -		if (this->engine == engine) {
> -			err = -EEXIST;
> -			ce = this;
> -			break;
> -		}
> -
> -		if (this->engine < engine)
> -			p = &parent->rb_right;
> -		else
> -			p = &parent->rb_left;
> -	}
> -	if (!err) {
> -		rb_link_node(&ce->node, parent, p);
> -		rb_insert_color(&ce->node, &ctx->hw_contexts);
> -	}
> -
> -	spin_unlock(&ctx->hw_contexts_lock);
> -
> -	return ce;
> -}
> -
> -void __intel_context_remove(struct intel_context *ce)
> -{
> -	struct i915_gem_context *ctx = ce->gem_context;
> -
> -	spin_lock(&ctx->hw_contexts_lock);
> -	rb_erase(&ce->node, &ctx->hw_contexts);
> -	spin_unlock(&ctx->hw_contexts_lock);
> -}
> -
> -struct intel_context *
> -intel_context_instance(struct i915_gem_context *ctx,
> -		       struct intel_engine_cs *engine)
> -{
> -	struct intel_context *ce, *pos;
> -
> -	ce = intel_context_lookup(ctx, engine);
> -	if (likely(ce))
> -		return intel_context_get(ce);
> +	struct intel_context *ce;
>   
>   	ce = intel_context_alloc();
>   	if (!ce)
>   		return ERR_PTR(-ENOMEM);
>   
>   	intel_context_init(ce, ctx, engine);
> -
> -	pos = __intel_context_insert(ctx, engine, ce);
> -	if (unlikely(pos != ce)) /* Beaten! Use their HW context instead */
> -		intel_context_free(ce);
> -
> -	GEM_BUG_ON(intel_context_lookup(ctx, engine) != pos);
> -	return intel_context_get(pos);
> +	return ce;
>   }
>   
>   int __intel_context_do_pin(struct intel_context *ce)
> @@ -204,6 +117,8 @@ intel_context_init(struct intel_context *ce,
>   		   struct i915_gem_context *ctx,
>   		   struct intel_engine_cs *engine)
>   {
> +	GEM_BUG_ON(!engine->cops);
> +
>   	kref_init(&ce->ref);
>   
>   	ce->gem_context = ctx;
> @@ -254,3 +169,18 @@ void intel_context_exit_engine(struct intel_context *ce)
>   {
>   	intel_engine_pm_put(ce->engine);
>   }
> +
> +struct i915_request *intel_context_create_request(struct intel_context *ce)
> +{
> +	struct i915_request *rq;
> +	int err;
> +
> +	err = intel_context_pin(ce);
> +	if (unlikely(err))
> +		return ERR_PTR(err);
> +
> +	rq = i915_request_create(ce);
> +	intel_context_unpin(ce);
> +
> +	return rq;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index b746add6b71d..63392c88cd98 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -12,24 +12,16 @@
>   #include "intel_context_types.h"
>   #include "intel_engine_types.h"
>   
> -struct intel_context *intel_context_alloc(void);
> -void intel_context_free(struct intel_context *ce);
> -
>   void intel_context_init(struct intel_context *ce,
>   			struct i915_gem_context *ctx,
>   			struct intel_engine_cs *engine);
>   
> -/**
> - * intel_context_lookup - Find the matching HW context for this (ctx, engine)
> - * @ctx - the parent GEM context
> - * @engine - the target HW engine
> - *
> - * May return NULL if the HW context hasn't been instantiated (i.e. unused).
> - */
>   struct intel_context *
> -intel_context_lookup(struct i915_gem_context *ctx,
> +intel_context_create(struct i915_gem_context *ctx,
>   		     struct intel_engine_cs *engine);
>   
> +void intel_context_free(struct intel_context *ce);
> +
>   /**
>    * intel_context_lock_pinned - Stablises the 'pinned' status of the HW context
>    * @ce - the context
> @@ -71,17 +63,6 @@ static inline void intel_context_unlock_pinned(struct intel_context *ce)
>   	mutex_unlock(&ce->pin_mutex);
>   }
>   
> -struct intel_context *
> -__intel_context_insert(struct i915_gem_context *ctx,
> -		       struct intel_engine_cs *engine,
> -		       struct intel_context *ce);
> -void
> -__intel_context_remove(struct intel_context *ce);
> -
> -struct intel_context *
> -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)
> @@ -144,4 +125,6 @@ static inline void intel_context_timeline_unlock(struct intel_context *ce)
>   	mutex_unlock(&ce->ring->timeline->mutex);
>   }
>   
> +struct i915_request *intel_context_create_request(struct intel_context *ce);
> +
>   #endif /* __INTEL_CONTEXT_H__ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index f02d27734e3b..3579c2708321 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -10,7 +10,6 @@
>   #include <linux/kref.h>
>   #include <linux/list.h>
>   #include <linux/mutex.h>
> -#include <linux/rbtree.h>
>   #include <linux/types.h>
>   
>   #include "i915_active_types.h"
> @@ -61,7 +60,6 @@ struct intel_context {
>   	struct i915_active_request active_tracker;
>   
>   	const struct intel_context_ops *ops;
> -	struct rb_node node;
>   
>   	/** sseu: Control eu/slice partitioning */
>   	struct intel_sseu sseu;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 6cb90137b2fa..2a1c438bf0ad 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -716,7 +716,7 @@ static int pin_context(struct i915_gem_context *ctx,
>   	struct intel_context *ce;
>   	int err;
>   
> -	ce = intel_context_instance(ctx, engine);
> +	ce = i915_gem_context_get_engine(ctx, engine->id);
>   	if (IS_ERR(ce))
>   		return PTR_ERR(ce);
>   
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index 85cdbfe1d989..2941916b37bf 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -23,6 +23,7 @@
>    */
>   
>   #include "i915_drv.h"
> +#include "i915_gem_context.h"
>   #include "intel_context.h"
>   #include "intel_engine_pm.h"
>   
> @@ -286,7 +287,7 @@ int mock_engine_init(struct intel_engine_cs *engine)
>   	i915_timeline_set_subclass(&engine->timeline, TIMELINE_ENGINE);
>   
>   	engine->kernel_context =
> -		intel_context_instance(i915->kernel_context, engine);
> +		i915_gem_context_get_engine(i915->kernel_context, engine->id);
>   	if (IS_ERR(engine->kernel_context))
>   		goto err_timeline;
>   
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 606fc2713240..8b6574e1b495 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -1188,7 +1188,7 @@ 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_instance(ctx, engine);
> +		ce = i915_gem_context_get_engine(ctx, i);
>   		if (IS_ERR(ce)) {
>   			ret = PTR_ERR(ce);
>   			goto out_shadow_ctx;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 50266e87c225..8a0278618279 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4312,8 +4312,9 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>   
>   static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   {
> -	struct i915_gem_context *ctx;
>   	struct intel_engine_cs *engine;
> +	struct i915_gem_context *ctx;
> +	struct i915_gem_engines *e;
>   	enum intel_engine_id id;
>   	int err = 0;
>   
> @@ -4330,18 +4331,21 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	if (IS_ERR(ctx))
>   		return PTR_ERR(ctx);
>   
> +	e = i915_gem_context_engine_list_lock(ctx);
> +
>   	for_each_engine(engine, i915, id) {
> +		struct intel_context *ce = e->engines[id];
>   		struct i915_request *rq;
>   
> -		rq = i915_request_alloc(engine, ctx);
> +		rq = intel_context_create_request(ce);
>   		if (IS_ERR(rq)) {
>   			err = PTR_ERR(rq);
> -			goto out_ctx;
> +			goto err_active;

Argh onion with jumps!

>   		}
>   
>   		err = 0;
> -		if (engine->init_context)
> -			err = engine->init_context(rq);
> +		if (rq->engine->init_context)
> +			err = rq->engine->init_context(rq);
>   
>   		i915_request_add(rq);
>   		if (err)
> @@ -4355,15 +4359,10 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	}
>   
>   	for_each_engine(engine, i915, id) {
> -		struct intel_context *ce;
> -		struct i915_vma *state;
> +		struct intel_context *ce = e->engines[id];
> +		struct i915_vma *state = ce->state;
>   		void *vaddr;
>   
> -		ce = intel_context_lookup(ctx, engine);
> -		if (!ce)
> -			continue;
> -
> -		state = ce->state;
>   		if (!state)
>   			continue;
>   
> @@ -4419,6 +4418,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   	}
>   
>   out_ctx:
> +	i915_gem_context_engine_list_unlock(ctx);
>   	i915_gem_context_set_closed(ctx);
>   	i915_gem_context_put(ctx);
>   	return err;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 1e1770047cc2..99c89dc7533e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -150,7 +150,7 @@ lookup_user_engine(struct i915_gem_context *ctx, u16 class, u16 instance)
>   	if (!engine)
>   		return ERR_PTR(-EINVAL);
>   
> -	return intel_context_instance(ctx, engine);
> +	return i915_gem_context_get_engine(ctx, engine->id);
>   }
>   
>   static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
> @@ -242,10 +242,51 @@ static void release_hw_id(struct i915_gem_context *ctx)
>   	mutex_unlock(&i915->contexts.mutex);
>   }
>   
> -static void i915_gem_context_free(struct i915_gem_context *ctx)
> +static void __free_engines(struct i915_gem_engines *e, unsigned int count)
> +{
> +	while (count--) {
> +		if (!e->engines[count])
> +			continue;
> +
> +		intel_context_put(e->engines[count]);
> +	}
> +	kfree(e);
> +}
> +
> +static void free_engines(struct i915_gem_engines *e)
> +{
> +	__free_engines(e, e->num_engines);
> +}
> +
> +static struct i915_gem_engines *default_engines(struct i915_gem_context *ctx)
>   {
> -	struct intel_context *it, *n;
> +	struct intel_engine_cs *engine;
> +	struct i915_gem_engines *e;
> +	enum intel_engine_id id;
>   
> +	e = kzalloc(struct_size(e, engines, I915_NUM_ENGINES), GFP_KERNEL);
> +	if (!e)
> +		return ERR_PTR(-ENOMEM);
> +
> +	e->i915 = ctx->i915;
> +	for_each_engine(engine, ctx->i915, id) {
> +		struct intel_context *ce;
> +
> +		ce = intel_context_create(ctx, engine);
> +		if (IS_ERR(ce)) {
> +			__free_engines(e, id);
> +			return ERR_CAST(ce);
> +		}
> +
> +		e->engines[id] = ce;
> +	}
> +	e->num_engines = id;
> +
> +	return e;
> +}
> +
> +static void i915_gem_context_free(struct i915_gem_context *ctx)
> +{
>   	lockdep_assert_held(&ctx->i915->drm.struct_mutex);
>   	GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
>   	GEM_BUG_ON(!list_empty(&ctx->active_engines));
> @@ -253,8 +294,8 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
>   	release_hw_id(ctx);
>   	i915_ppgtt_put(ctx->ppgtt);
>   
> -	rbtree_postorder_for_each_entry_safe(it, n, &ctx->hw_contexts, node)
> -		intel_context_put(it);
> +	free_engines(rcu_access_pointer(ctx->engines));
> +	mutex_destroy(&ctx->engines_mutex);
>   
>   	if (ctx->timeline)
>   		i915_timeline_put(ctx->timeline);
> @@ -363,6 +404,8 @@ static struct i915_gem_context *
>   __create_context(struct drm_i915_private *dev_priv)
>   {
>   	struct i915_gem_context *ctx;
> +	struct i915_gem_engines *e;
> +	int err;
>   	int i;
>   
>   	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> @@ -376,8 +419,13 @@ __create_context(struct drm_i915_private *dev_priv)
>   	INIT_LIST_HEAD(&ctx->active_engines);
>   	mutex_init(&ctx->mutex);
>   
> -	ctx->hw_contexts = RB_ROOT;
> -	spin_lock_init(&ctx->hw_contexts_lock);
> +	mutex_init(&ctx->engines_mutex);
> +	e = default_engines(ctx);
> +	if (IS_ERR(e)) {
> +		err = PTR_ERR(e);
> +		goto err_free;
> +	}
> +	RCU_INIT_POINTER(ctx->engines, e);
>   
>   	INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
>   	INIT_LIST_HEAD(&ctx->handles_list);
> @@ -399,6 +447,10 @@ __create_context(struct drm_i915_private *dev_priv)
>   		ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
>   
>   	return ctx;
> +
> +err_free:
> +	kfree(ctx);
> +	return ERR_PTR(err);
>   }
>   
>   static struct i915_hw_ppgtt *
> @@ -862,7 +914,7 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>   {
>   	struct drm_i915_private *i915 = ctx->i915;
>   	struct context_barrier_task *cb;
> -	struct intel_context *ce, *next;
> +	struct i915_gem_engines_iter it;
>   	int err = 0;
>   
>   	lockdep_assert_held(&i915->drm.struct_mutex);
> @@ -875,22 +927,27 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>   	i915_active_init(i915, &cb->base, cb_retire);
>   	i915_active_acquire(&cb->base);
>   
> -	rbtree_postorder_for_each_entry_safe(ce, next, &ctx->hw_contexts, node) {
> -		struct intel_engine_cs *engine = ce->engine;
> +	for_each_gem_engine(it, ctx) {
> +		struct intel_context *ce = it.context;
>   		struct i915_request *rq;
>   
> -		if (!(engine->mask & engines))
> +		if (!(ce->engine->mask & engines))
> +			continue;
> +
> +		if (!intel_context_is_pinned(ce))
>   			continue;
>   
>   		if (I915_SELFTEST_ONLY(context_barrier_inject_fault &
> -				       engine->mask)) {
> +				       ce->engine->mask)) {
>   			err = -ENXIO;
> +			i915_gem_engines_iter_fini(&it);

This would be our only iterator which needs manual cleanup. But only on 
abort, not normal termination.

Would it be cleaner if for_each_gem_engine took engines which the caller 
would previously have to obtain via i915_gem_context_engine_list_lock? 
And the caller would also be obviously responsible for unlocking.


>   			break;
>   		}
>   
> -		rq = i915_request_alloc(engine, ctx);
> +		rq = intel_context_create_request(ce);
>   		if (IS_ERR(rq)) {
>   			err = PTR_ERR(rq);
> +			i915_gem_engines_iter_fini(&it);
>   			break;
>   		}
>   
> @@ -901,8 +958,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>   			err = i915_active_ref(&cb->base, rq->fence.context, rq);
>   
>   		i915_request_add(rq);
> -		if (err)
> +		if (err) {
> +			i915_gem_engines_iter_fini(&it);
>   			break;
> +		}
>   	}
>   
>   	cb->task = err ? NULL : task; /* caller needs to unwind instead */
> @@ -1739,6 +1798,43 @@ int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
>   	return err;
>   }
>   
> +/* GEM context-engines iterator: for_each_gem_engine() */
> +bool i915_gem_engines_iter_next(struct i915_gem_engines_iter *it)
> +{
> +	if (!it->engines)
> +		goto end;

When can engines be null? Should it be GEM_BUG_ON if onyl when used 
outside the for_each_gem_engine?

> +
> +	do {
> +		if (it->idx >= it->engines->num_engines)
> +			goto end;
> +
> +		it->context = it->engines->engines[it->idx++];
> +	} while (!it->context);
> +
> +	return true;
> +
> +end:
> +	i915_gem_engines_iter_fini(it);
> +	return false;
> +}
> +
> +void i915_gem_engines_iter_init(struct i915_gem_engines_iter *it,
> +				struct i915_gem_context *ctx)
> +{
> +	it->gem_context = ctx;
> +	it->engines = i915_gem_context_engine_list_lock(ctx);
> +	it->idx = 0;

Clear it->context just in case?

> +}
> +
> +void i915_gem_engines_iter_fini(struct i915_gem_engines_iter *it)
> +{
> +	struct i915_gem_context *ctx;
> +
> +	ctx = fetch_and_zero(&it->gem_context);

This is iter_fini can be called multiple times? When does that come to 
play? Oh... when i915_gem_engines_iter_next went past the array and the 
loop decides to bail out.

If iterator took engines as parameter this could also go.

Regards,

Tvrtko


> +	if (ctx)
> +		i915_gem_context_engine_list_unlock(ctx);
> +}
> +
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/mock_context.c"
>   #include "selftests/i915_gem_context.c"
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 5a8e080499fb..25238f750286 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -176,6 +176,57 @@ static inline void i915_gem_context_put(struct i915_gem_context *ctx)
>   	kref_put(&ctx->ref, i915_gem_context_release);
>   }
>   
> +static inline struct i915_gem_engines *
> +i915_gem_context_engine_list(struct i915_gem_context *ctx)
> +{
> +	return rcu_dereference_protected(ctx->engines,
> +					 lockdep_is_held(&ctx->engines_mutex));
> +}
> +
> +static inline struct i915_gem_engines *
> +i915_gem_context_engine_list_lock(struct i915_gem_context *ctx)
> +	__acquires(&ctx->engines_mutex)
> +{
> +	mutex_lock(&ctx->engines_mutex);
> +	return i915_gem_context_engine_list(ctx);
> +}
> +
> +static inline void
> +i915_gem_context_engine_list_unlock(struct i915_gem_context *ctx)
> +	__releases(&ctx->engines_mutex)
> +{
> +	mutex_unlock(&ctx->engines_mutex);
> +}
> +
> +static inline struct intel_context *
> +i915_gem_context_lookup_engine(struct i915_gem_context *ctx, unsigned int idx)
> +{
> +	return i915_gem_context_engine_list(ctx)->engines[idx];
> +}
> +
> +static inline struct intel_context *
> +i915_gem_context_get_engine(struct i915_gem_context *ctx, unsigned int idx)
> +{
> +	struct intel_context *ce = ERR_PTR(-EINVAL);
> +
> +	rcu_read_lock(); {
> +		struct i915_gem_engines *e = rcu_dereference(ctx->engines);
> +		if (likely(idx < e->num_engines && e->engines[idx]))
> +			ce = intel_context_get(e->engines[idx]);
> +	} rcu_read_unlock();
> +
> +	return ce;
> +}
> +
> +#define for_each_gem_engine(it, ctx) \
> +	for (i915_gem_engines_iter_init(&(it), (ctx)); \
> +	     i915_gem_engines_iter_next(&(it));)
> +
> +void i915_gem_engines_iter_init(struct i915_gem_engines_iter *it,
> +				struct i915_gem_context *ctx);
> +bool i915_gem_engines_iter_next(struct i915_gem_engines_iter *it);
> +void i915_gem_engines_iter_fini(struct i915_gem_engines_iter *it);
> +
>   struct i915_lut_handle *i915_lut_handle_alloc(void);
>   void i915_lut_handle_free(struct i915_lut_handle *lut);
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_context_types.h b/drivers/gpu/drm/i915/i915_gem_context_types.h
> index d282a6ab3b9f..5d146b7b84ec 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context_types.h
> @@ -29,6 +29,20 @@ struct i915_hw_ppgtt;
>   struct i915_timeline;
>   struct intel_ring;
>   
> +struct i915_gem_engines {
> +	struct rcu_work rcu;
> +	struct drm_i915_private *i915;
> +	unsigned int num_engines;
> +	struct intel_context *engines[];
> +};
> +
> +struct i915_gem_engines_iter {
> +	struct intel_context *context;
> +	struct i915_gem_context *gem_context;
> +	struct i915_gem_engines *engines;
> +	unsigned int idx;
> +};
> +
>   /**
>    * struct i915_gem_context - client state
>    *
> @@ -42,6 +56,21 @@ struct i915_gem_context {
>   	/** file_priv: owning file descriptor */
>   	struct drm_i915_file_private *file_priv;
>   
> +	/**
> +	 * @engines: User defined engines for this context
> +	 *
> +	 * NULL means to use legacy definitions (including random meaning of
> +	 * I915_EXEC_BSD with I915_EXEC_BSD_SELECTOR overrides).
> +	 *
> +	 * If defined, execbuf uses the I915_EXEC_MASK as an index into
> +	 * array, and various uAPI other the ability to lookup up an
> +	 * index from this array to select an engine operate on.
> +	 *
> +	 * User defined by I915_CONTEXT_PARAM_ENGINE.
> +	 */
> +	struct i915_gem_engines __rcu *engines;
> +	struct mutex engines_mutex; /* guards writes to engines */
> +
>   	struct i915_timeline *timeline;
>   
>   	/**
> @@ -134,10 +163,6 @@ struct i915_gem_context {
>   
>   	struct i915_sched_attr sched;
>   
> -	/** hw_contexts: per-engine logical HW state */
> -	struct rb_root hw_contexts;
> -	spinlock_t hw_contexts_lock;
> -
>   	/** ring_size: size for allocating the per-engine ring buffer */
>   	u32 ring_size;
>   	/** desc_template: invariant fields for the HW context descriptor */
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 166a33c0d3ed..679f7c1561ba 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2076,9 +2076,7 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>   	return file_priv->bsd_engine;
>   }
>   
> -#define I915_USER_RINGS (4)
> -
> -static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> +static const enum intel_engine_id user_ring_map[] = {
>   	[I915_EXEC_DEFAULT]	= RCS0,
>   	[I915_EXEC_RENDER]	= RCS0,
>   	[I915_EXEC_BLT]		= BCS0,
> @@ -2086,10 +2084,8 @@ static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
>   	[I915_EXEC_VEBOX]	= VECS0
>   };
>   
> -static int eb_pin_context(struct i915_execbuffer *eb,
> -			  struct intel_engine_cs *engine)
> +static int eb_pin_context(struct i915_execbuffer *eb, struct intel_context *ce)
>   {
> -	struct intel_context *ce;
>   	int err;
>   
>   	/*
> @@ -2100,21 +2096,16 @@ 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.
>   	 */
>   	err = intel_context_pin(ce);
> -	intel_context_put(ce);
>   	if (err)
>   		return err;
>   
> -	eb->engine = engine;
> +	eb->engine = ce->engine;
>   	eb->context = ce;
>   	return 0;
>   }
> @@ -2124,25 +2115,19 @@ static void eb_unpin_context(struct i915_execbuffer *eb)
>   	intel_context_unpin(eb->context);
>   }
>   
> -static int
> -eb_select_engine(struct i915_execbuffer *eb,
> -		 struct drm_file *file,
> -		 struct drm_i915_gem_execbuffer2 *args)
> +static unsigned int
> +eb_select_legacy_ring(struct i915_execbuffer *eb,
> +		      struct drm_file *file,
> +		      struct drm_i915_gem_execbuffer2 *args)
>   {
>   	struct drm_i915_private *i915 = eb->i915;
>   	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
> -	struct intel_engine_cs *engine;
> -
> -	if (user_ring_id > I915_USER_RINGS) {
> -		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
> -		return -EINVAL;
> -	}
>   
> -	if ((user_ring_id != I915_EXEC_BSD) &&
> -	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
> +	if (user_ring_id != I915_EXEC_BSD &&
> +	    (args->flags & I915_EXEC_BSD_MASK)) {
>   		DRM_DEBUG("execbuf with non bsd ring but with invalid "
>   			  "bsd dispatch flags: %d\n", (int)(args->flags));
> -		return -EINVAL;
> +		return -1;
>   	}
>   
>   	if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(i915, VCS1)) {
> @@ -2157,20 +2142,39 @@ eb_select_engine(struct i915_execbuffer *eb,
>   		} else {
>   			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
>   				  bsd_idx);
> -			return -EINVAL;
> +			return -1;
>   		}
>   
> -		engine = i915->engine[_VCS(bsd_idx)];
> -	} else {
> -		engine = i915->engine[user_ring_map[user_ring_id]];
> +		return _VCS(bsd_idx);
>   	}
>   
> -	if (!engine) {
> -		DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
> -		return -EINVAL;
> +	if (user_ring_id >= ARRAY_SIZE(user_ring_map)) {
> +		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
> +		return -1;
>   	}
>   
> -	return eb_pin_context(eb, engine);
> +	return user_ring_map[user_ring_id];
> +}
> +
> +static int
> +eb_select_engine(struct i915_execbuffer *eb,
> +		 struct drm_file *file,
> +		 struct drm_i915_gem_execbuffer2 *args)
> +{
> +	struct intel_context *ce;
> +	unsigned int idx;
> +	int err;
> +
> +	idx = eb_select_legacy_ring(eb, file, args);
> +
> +	ce = i915_gem_context_get_engine(eb->gem_context, idx);
> +	if (IS_ERR(ce))
> +		return PTR_ERR(ce);
> +
> +	err = eb_pin_context(eb, ce);
> +	intel_context_put(ce);
> +
> +	return err;
>   }
>   
>   static void
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index afaeabe5e531..6cec24f1ade6 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1203,35 +1203,36 @@ static int i915_oa_read(struct i915_perf_stream *stream,
>   static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
>   					    struct i915_gem_context *ctx)
>   {
> -	struct intel_engine_cs *engine = i915->engine[RCS0];
> -	struct intel_context *ce;
> +	struct i915_gem_engines_iter it;
>   	int err;
>   
> -	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);
> +	if (err)
>   		return ERR_PTR(err);
> +
> +	for_each_gem_engine(it, ctx) {
> +		struct intel_context *ce = it.context;
> +
> +		if (ce->engine->class != RENDER_CLASS)
> +			continue;
> +
> +		/*
> +		 * As the ID is the gtt offset of the context's vma we
> +		 * pin the vma to ensure the ID remains fixed.
> +		 */
> +		err = intel_context_pin(ce);
> +		if (err == 0) {
> +			i915->perf.oa.pinned_ctx = ce;
> +			i915_gem_engines_iter_fini(&it);
> +			break;
> +		}
>   	}
>   
> -	/*
> -	 * As the ID is the gtt offset of the context's vma we
> -	 * pin the vma to ensure the ID remains fixed.
> -	 *
> -	 * NB: implied RCS engine...
> -	 */
> -	err = intel_context_pin(ce);
>   	mutex_unlock(&i915->drm.struct_mutex);
> -	intel_context_put(ce);
>   	if (err)
>   		return ERR_PTR(err);
>   
> -	i915->perf.oa.pinned_ctx = ce;
> -
> -	return ce;
> +	return i915->perf.oa.pinned_ctx;
>   }
>   
>   /**
> @@ -1717,7 +1718,6 @@ gen8_update_reg_state_unlocked(struct intel_context *ce,
>   static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>   				       const struct i915_oa_config *oa_config)
>   {
> -	struct intel_engine_cs *engine = dev_priv->engine[RCS0];
>   	unsigned int map_type = i915_coherent_map_type(dev_priv);
>   	struct i915_gem_context *ctx;
>   	struct i915_request *rq;
> @@ -1746,30 +1746,40 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>   
>   	/* Update all contexts now that we've stalled the submission. */
>   	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> -		struct intel_context *ce = intel_context_lookup(ctx, engine);
> -		u32 *regs;
> +		struct i915_gem_engines_iter it;
>   
> -		/* OA settings will be set upon first use */
> -		if (!ce || !ce->state)
> -			continue;
> +		for_each_gem_engine(it, ctx) {
> +			struct intel_context *ce = it.context;
> +			u32 *regs;
> +
> +			if (ce->engine->class != RENDER_CLASS)
> +				continue;
>   
> -		regs = i915_gem_object_pin_map(ce->state->obj, map_type);
> -		if (IS_ERR(regs))
> -			return PTR_ERR(regs);
> +			/* OA settings will be set upon first use */
> +			if (!ce->state)
> +				continue;
>   
> -		ce->state->obj->mm.dirty = true;
> -		regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
> +			regs = i915_gem_object_pin_map(ce->state->obj,
> +						       map_type);
> +			if (IS_ERR(regs)) {
> +				i915_gem_engines_iter_fini(&it);
> +				return PTR_ERR(regs);
> +			}
>   
> -		gen8_update_reg_state_unlocked(ce, regs, oa_config);
> +			ce->state->obj->mm.dirty = true;
> +			regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
>   
> -		i915_gem_object_unpin_map(ce->state->obj);
> +			gen8_update_reg_state_unlocked(ce, regs, oa_config);
> +
> +			i915_gem_object_unpin_map(ce->state->obj);
> +		}
>   	}
>   
>   	/*
>   	 * Apply the configuration by doing one context restore of the edited
>   	 * context image.
>   	 */
> -	rq = i915_request_create(engine->kernel_context);
> +	rq = i915_request_create(dev_priv->engine[RCS0]->kernel_context);
>   	if (IS_ERR(rq))
>   		return PTR_ERR(rq);
>   
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index eaf8851dd3bc..65a949b3659a 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -785,7 +785,6 @@ 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
> @@ -799,21 +798,13 @@ 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_instance(ctx, engine);
> +	ce = i915_gem_context_get_engine(ctx, engine->id);
>   	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:
> +	rq = intel_context_create_request(ce);
>   	intel_context_put(ce);
> +
>   	return rq;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 1b6d6403ee92..9dbb036fd7f1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -364,11 +364,9 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
>   static void guc_stage_desc_init(struct intel_guc_client *client)
>   {
>   	struct intel_guc *guc = client->guc;
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct intel_engine_cs *engine;
>   	struct i915_gem_context *ctx = client->owner;
> +	struct i915_gem_engines_iter it;
>   	struct guc_stage_desc *desc;
> -	unsigned int tmp;
>   	u32 gfx_addr;
>   
>   	desc = __get_stage_desc(client);
> @@ -382,10 +380,12 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
>   	desc->priority = client->priority;
>   	desc->db_id = client->doorbell_id;
>   
> -	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
> -		struct intel_context *ce = intel_context_lookup(ctx, engine);
> -		u32 guc_engine_id = engine->guc_id;
> -		struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
> +	for_each_gem_engine(it, ctx) {
> +		struct intel_context *ce = it.context;
> +		struct guc_execlist_context *lrc;
> +
> +		if (!(ce->engine->mask & client->engines))
> +			continue;
>   
>   		/* TODO: We have a design issue to be solved here. Only when we
>   		 * receive the first batch, we know which engine is used by the
> @@ -394,8 +394,8 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
>   		 * for now who owns a GuC client. But for future owner of GuC
>   		 * client, need to make sure lrc is pinned prior to enter here.
>   		 */
> -		if (!ce || !ce->state)
> -			break;	/* XXX: continue? */
> +		if (!ce->state)
> +			continue;
>   
>   		/*
>   		 * XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
> @@ -404,6 +404,7 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
>   		 * Instead, the GuC uses the LRCA of the user mode context (see
>   		 * guc_add_request below).
>   		 */
> +		lrc = &desc->lrc[ce->engine->guc_id];
>   		lrc->context_desc = lower_32_bits(ce->lrc_desc);
>   
>   		/* The state page is after PPHWSP */
> @@ -414,14 +415,14 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
>   		 * here. In proxy submission, it wants the stage id
>   		 */
>   		lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
> -				(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
> +				(ce->engine->guc_id << GUC_ELC_ENGINE_OFFSET);
>   
>   		lrc->ring_begin = intel_guc_ggtt_offset(guc, ce->ring->vma);
>   		lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
>   		lrc->ring_next_free_location = lrc->ring_begin;
>   		lrc->ring_current_tail_pointer_value = 0;
>   
> -		desc->engines_used |= (1 << guc_engine_id);
> +		desc->engines_used |= BIT(ce->engine->guc_id);
>   	}
>   
>   	DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 214d1fd2f4dc..7fd224a4ca4c 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -1094,7 +1094,7 @@ __igt_ctx_sseu(struct drm_i915_private *i915,
>   
>   	wakeref = intel_runtime_pm_get(i915);
>   
> -	ce = intel_context_instance(ctx, i915->engine[RCS0]);
> +	ce = i915_gem_context_get_engine(ctx, RCS0);
>   	if (IS_ERR(ce)) {
>   		ret = PTR_ERR(ce);
>   		goto out_rpm;
> diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
> index 0426093bf1d9..71c750693585 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_context.c
> @@ -30,6 +30,7 @@ mock_context(struct drm_i915_private *i915,
>   	     const char *name)
>   {
>   	struct i915_gem_context *ctx;
> +	struct i915_gem_engines *e;
>   	int ret;
>   
>   	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> @@ -40,8 +41,11 @@ mock_context(struct drm_i915_private *i915,
>   	INIT_LIST_HEAD(&ctx->link);
>   	ctx->i915 = i915;
>   
> -	ctx->hw_contexts = RB_ROOT;
> -	spin_lock_init(&ctx->hw_contexts_lock);
> +	mutex_init(&ctx->engines_mutex);
> +	e = default_engines(ctx);
> +	if (IS_ERR(e))
> +		goto err_free;
> +	RCU_INIT_POINTER(ctx->engines, e);
>   
>   	INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
>   	INIT_LIST_HEAD(&ctx->handles_list);
> @@ -51,7 +55,7 @@ mock_context(struct drm_i915_private *i915,
>   
>   	ret = i915_gem_context_pin_hw_id(ctx);
>   	if (ret < 0)
> -		goto err_handles;
> +		goto err_engines;
>   
>   	if (name) {
>   		struct i915_hw_ppgtt *ppgtt;
> @@ -69,7 +73,9 @@ mock_context(struct drm_i915_private *i915,
>   
>   	return ctx;
>   
> -err_handles:
> +err_engines:
> +	free_engines(rcu_access_pointer(ctx->engines));
> +err_free:
>   	kfree(ctx);
>   	return NULL;
>   
> 


More information about the Intel-gfx mailing list