[PATCH 1/1] drm/i915/active: Serialize access to barrier tasks lists

Andi Shyti andi.shyti at linux.intel.com
Fri Feb 3 19:01:40 UTC 2023


On Fri, Feb 03, 2023 at 04:52:41PM +0100, Andrzej Hajda wrote:
> Hi,
> 
> On 03.02.2023 14:48, Janusz Krzysztofik wrote:
> > Barriers are now deleted from a barrier tasks list by temporarily removing
> > the list content, traversing that content with skip over the node to be
> > deleted, then adding the modified content back to the list.  Since that
> > complex operation is not serialized with other concurrent uses of the
> > list, functions that depend on the list being either empty or not empty
> > can take wrong decisions.
> > 
> > Protect access to the list pending that complex barrier delete operation
> > with a lock, and teach other users of the list to acquire the lock.  While
> > this solution may be considered suboptimal, it seems to be the the
> > smallest, least invasive fix.
> 
> The idea looks OK, just adding locking to lock-less lists looks somehow
> strange :),

yes :)

> on the other side even llist.h suggests locks in case of some scenarios, so
> maybe not so bad idea.
> 
> > 
> > References: https://gitlab.freedesktop.org/drm/intel/-/issues/6333
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_engine_cs.c           | 8 ++++++++
> >   drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c    | 6 +++++-
> >   drivers/gpu/drm/i915/gt/intel_engine_pm.c           | 6 +++++-
> >   drivers/gpu/drm/i915/gt/intel_engine_types.h        | 1 +
> >   drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c | 5 +++++
> >   drivers/gpu/drm/i915/i915_active.c                  | 7 +++++++
> >   6 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index d4e29da74612d..b6fb07c188d16 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -1148,6 +1148,7 @@ static int engine_setup_common(struct intel_engine_cs *engine)
> >   	int err;
> >   	init_llist_head(&engine->barrier_tasks);
> > +	spin_lock_init(&engine->barriers_lock);
> >   	err = init_status_page(engine);
> >   	if (err)
> > @@ -1404,6 +1405,8 @@ int intel_engines_init(struct intel_gt *gt)
> >    */
> >   void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> >   {
> > +	unsigned long flags;
> > +
> >   	GEM_BUG_ON(!list_empty(&engine->sched_engine->requests));
> >   	i915_sched_engine_put(engine->sched_engine);
> > @@ -1418,7 +1421,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> >   	if (engine->kernel_context)
> >   		intel_engine_destroy_pinned_context(engine->kernel_context);
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> >   	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> 
> Since checking of emptiness is quite frequent (5times?), you can add helper
> for it.

I don't think we need spinlocks around llist_empty() at all,
anyway.

Andi

> Regards
> Andrzej
> 
> >   	cleanup_status_page(engine);
> >   	intel_wa_list_free(&engine->ctx_wa_list);
> > @@ -2240,6 +2245,7 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> >   	struct i915_gpu_error * const error = &engine->i915->gpu_error;
> >   	struct i915_request *rq;
> >   	intel_wakeref_t wakeref;
> > +	unsigned long flags;
> >   	ktime_t dummy;
> >   	if (header) {
> > @@ -2254,8 +2260,10 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> >   		drm_printf(m, "*** WEDGED ***\n");
> >   	drm_printf(m, "\tAwake? %d\n", atomic_read(&engine->wakeref.count));
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> >   	drm_printf(m, "\tBarriers?: %s\n",
> >   		   str_yes_no(!llist_empty(&engine->barrier_tasks)));
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> >   	drm_printf(m, "\tLatency: %luus\n",
> >   		   ewma__engine_latency_read(&engine->latency));
> >   	if (intel_engine_supports_stats(engine))
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> > index 9a527e1f5be65..9c3c8c431f8d8 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> > @@ -378,9 +378,13 @@ int intel_engine_flush_barriers(struct intel_engine_cs *engine)
> >   	struct i915_sched_attr attr = { .priority = I915_PRIORITY_MIN };
> >   	struct intel_context *ce = engine->kernel_context;
> >   	struct i915_request *rq;
> > +	unsigned long flags;
> >   	int err;
> > -	if (llist_empty(&engine->barrier_tasks))
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> > +	err = !llist_empty(&engine->barrier_tasks);
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> > +	if (!err)
> >   		return 0;
> >   	if (!intel_engine_pm_get_if_awake(engine))
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > index e971b153fda97..708f8907f410b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > @@ -235,8 +235,12 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
> >   static void call_idle_barriers(struct intel_engine_cs *engine)
> >   {
> >   	struct llist_node *node, *next;
> > +	unsigned long flags;
> > -	llist_for_each_safe(node, next, llist_del_all(&engine->barrier_tasks)) {
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> > +	node = llist_del_all(&engine->barrier_tasks);
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> > +	llist_for_each_safe(node, next, node) {
> >   		struct dma_fence_cb *cb =
> >   			container_of((struct list_head *)node,
> >   				     typeof(*cb), node);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index 4fd54fb8810fb..ab9e0a6de70d4 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -396,6 +396,7 @@ struct intel_engine_cs {
> >   	struct intel_context *hung_ce;
> >   	struct llist_head barrier_tasks;
> > +	spinlock_t barriers_lock;
> >   	struct intel_context *kernel_context; /* pinned */
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> > index 273d440a53e3f..cbc03662fc693 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> > @@ -90,6 +90,7 @@ static void pulse_unlock_wait(struct pulse *p)
> >   static int __live_idle_pulse(struct intel_engine_cs *engine,
> >   			     int (*fn)(struct intel_engine_cs *cs))
> >   {
> > +	unsigned long flags;
> >   	struct pulse *p;
> >   	int err;
> > @@ -113,13 +114,17 @@ static int __live_idle_pulse(struct intel_engine_cs *engine,
> >   	i915_active_release(&p->active);
> >   	GEM_BUG_ON(i915_active_is_idle(&p->active));
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> >   	GEM_BUG_ON(llist_empty(&engine->barrier_tasks));
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> >   	err = fn(engine);
> >   	if (err)
> >   		goto out;
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> >   	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> >   	if (engine_sync_barrier(engine)) {
> >   		struct drm_printer m = drm_err_printer("pulse");
> > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> > index 7412abf166a8c..320840eea30ce 100644
> > --- a/drivers/gpu/drm/i915/i915_active.c
> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -370,6 +370,7 @@ static bool ____active_del_barrier(struct i915_active *ref,
> >   {
> >   	struct llist_node *head = NULL, *tail = NULL;
> >   	struct llist_node *pos, *next;
> > +	unsigned long flags;
> >   	GEM_BUG_ON(node->timeline != engine->kernel_context->timeline->fence_context);
> > @@ -388,6 +389,7 @@ static bool ____active_del_barrier(struct i915_active *ref,
> >   	 * we are actively using the barrier, we know that there will be
> >   	 * at least another opportunity when we idle.
> >   	 */
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> >   	llist_for_each_safe(pos, next, llist_del_all(&engine->barrier_tasks)) {
> >   		if (node == barrier_from_ll(pos)) {
> >   			node = NULL;
> > @@ -401,6 +403,7 @@ static bool ____active_del_barrier(struct i915_active *ref,
> >   	}
> >   	if (head)
> >   		llist_add_batch(head, tail, &engine->barrier_tasks);
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> >   	return !node;
> >   }
> > @@ -973,7 +976,9 @@ void i915_active_acquire_barrier(struct i915_active *ref)
> >   		spin_unlock_irqrestore(&ref->tree_lock, flags);
> >   		GEM_BUG_ON(!intel_engine_pm_is_awake(engine));
> > +		spin_lock_irqsave(&engine->barriers_lock, flags);
> >   		llist_add(barrier_to_ll(node), &engine->barrier_tasks);
> > +		spin_unlock_irqrestore(&engine->barriers_lock, flags);
> >   		intel_engine_pm_put_delay(engine, 2);
> >   	}
> >   }
> > @@ -993,7 +998,9 @@ void i915_request_add_active_barriers(struct i915_request *rq)
> >   	GEM_BUG_ON(intel_engine_is_virtual(engine));
> >   	GEM_BUG_ON(i915_request_timeline(rq) != engine->kernel_context->timeline);
> > +	spin_lock_irqsave(&engine->barriers_lock, flags);
> >   	node = llist_del_all(&engine->barrier_tasks);
> > +	spin_unlock_irqrestore(&engine->barriers_lock, flags);
> >   	if (!node)
> >   		return;
> >   	/*


More information about the Intel-gfx-trybot mailing list