[PATCH 1/1] drm/i915/active: Serialize access to barrier tasks lists
Andrzej Hajda
andrzej.hajda at intel.com
Fri Feb 3 15:52:41 UTC 2023
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 :),
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.
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