From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 01/22] drm/i915/gem: Consolidate ctx->engines[] release
Date: Mon, 02 Mar 2020 20:29:09 +0200 [thread overview]
Message-ID: <87d09u7gqy.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <20200302085812.4172450-1-chris@chris-wilson.co.uk>
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Use the same engine_idle_release() routine for cleaning all old
> ctx->engine[] state, closing any potential races with concurrent execbuf
> submission.
>
> v2ish: Use the ce->pin_count to close the execbuf gap.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/1241
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 193 +++++++++---------
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 1 -
> .../gpu/drm/i915/gem/selftests/mock_context.c | 3 +
> 3 files changed, 105 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index e525ead073f7..cb6b6be48978 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -242,7 +242,6 @@ static void __free_engines(struct i915_gem_engines *e, unsigned int count)
> if (!e->engines[count])
> continue;
>
> - RCU_INIT_POINTER(e->engines[count]->gem_context, NULL);
> intel_context_put(e->engines[count]);
> }
> kfree(e);
> @@ -255,7 +254,11 @@ static void free_engines(struct i915_gem_engines *e)
>
> static void free_engines_rcu(struct rcu_head *rcu)
> {
> - free_engines(container_of(rcu, struct i915_gem_engines, rcu));
> + struct i915_gem_engines *engines =
> + container_of(rcu, struct i915_gem_engines, rcu);
> +
> + i915_sw_fence_fini(&engines->fence);
> + free_engines(engines);
> }
>
> static struct i915_gem_engines *default_engines(struct i915_gem_context *ctx)
> @@ -269,8 +272,6 @@ static struct i915_gem_engines *default_engines(struct i915_gem_context *ctx)
> if (!e)
> return ERR_PTR(-ENOMEM);
>
> - e->ctx = ctx;
> -
> for_each_engine(engine, gt, id) {
> struct intel_context *ce;
>
> @@ -304,7 +305,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
> list_del(&ctx->link);
> spin_unlock(&ctx->i915->gem.contexts.lock);
>
> - free_engines(rcu_access_pointer(ctx->engines));
> mutex_destroy(&ctx->engines_mutex);
>
> if (ctx->timeline)
> @@ -491,30 +491,104 @@ static void kill_engines(struct i915_gem_engines *engines)
> static void kill_stale_engines(struct i915_gem_context *ctx)
> {
> struct i915_gem_engines *pos, *next;
> - unsigned long flags;
>
> - spin_lock_irqsave(&ctx->stale.lock, flags);
> + spin_lock_irq(&ctx->stale.lock);
Reader ponders why it was with flags on the first place.
> + GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
> list_for_each_entry_safe(pos, next, &ctx->stale.engines, link) {
> - if (!i915_sw_fence_await(&pos->fence))
> + if (!i915_sw_fence_await(&pos->fence)) {
> + list_del_init(&pos->link);
Is this for making the bed for kill path list_empty?
> continue;
> + }
>
> - spin_unlock_irqrestore(&ctx->stale.lock, flags);
> + spin_unlock_irq(&ctx->stale.lock);
>
> kill_engines(pos);
>
> - spin_lock_irqsave(&ctx->stale.lock, flags);
> + spin_lock_irq(&ctx->stale.lock);
> + GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
> list_safe_reset_next(pos, next, link);
> list_del_init(&pos->link); /* decouple from FENCE_COMPLETE */
>
> i915_sw_fence_complete(&pos->fence);
> }
> - spin_unlock_irqrestore(&ctx->stale.lock, flags);
> + spin_unlock_irq(&ctx->stale.lock);
> }
>
> static void kill_context(struct i915_gem_context *ctx)
> {
> kill_stale_engines(ctx);
> - kill_engines(__context_engines_static(ctx));
> +}
> +
> +static int engines_notify(struct i915_sw_fence *fence,
> + enum i915_sw_fence_notify state)
> +{
> + struct i915_gem_engines *engines =
> + container_of(fence, typeof(*engines), fence);
> +
> + switch (state) {
> + case FENCE_COMPLETE:
> + if (!list_empty(&engines->link)) {
> + struct i915_gem_context *ctx = engines->ctx;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ctx->stale.lock, flags);
> + list_del(&engines->link);
> + spin_unlock_irqrestore(&ctx->stale.lock, flags);
> + }
> + i915_gem_context_put(engines->ctx);
> + break;
> +
> + case FENCE_FREE:
> + init_rcu_head(&engines->rcu);
> + call_rcu(&engines->rcu, free_engines_rcu);
> + break;
> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> +static void engines_idle_release(struct i915_gem_context *ctx,
> + struct i915_gem_engines *engines)
> +{
> + struct i915_gem_engines_iter it;
> + struct intel_context *ce;
> +
> + i915_sw_fence_init(&engines->fence, engines_notify);
> + INIT_LIST_HEAD(&engines->link);
> +
> + engines->ctx = i915_gem_context_get(ctx);
> +
> + for_each_gem_engine(ce, engines, it) {
> + struct dma_fence *fence;
> + int err = 0;
> +
> + /* serialises with execbuf */
> + RCU_INIT_POINTER(ce->gem_context, NULL);
> + if (!intel_context_pin_if_active(ce))
> + continue;
Ok so this should ensure the lifetime past the execbufs.
> +
> + fence = i915_active_fence_get(&ce->timeline->last_request);
> + if (fence) {
> + err = i915_sw_fence_await_dma_fence(&engines->fence,
> + fence, 0,
> + GFP_KERNEL);
> + dma_fence_put(fence);
> + }
> + intel_context_unpin(ce);
> + if (err < 0)
> + goto kill;
> + }
> +
> + spin_lock_irq(&ctx->stale.lock);
> + if (!i915_gem_context_is_closed(ctx))
> + list_add_tail(&engines->link, &ctx->stale.engines);
> + spin_unlock_irq(&ctx->stale.lock);
> +
> +kill:
> + if (list_empty(&engines->link)) /* raced, already closed */
> + kill_engines(engines);
> +
> + i915_sw_fence_commit(&engines->fence);
> }
>
> static void set_closed_name(struct i915_gem_context *ctx)
> @@ -538,11 +612,16 @@ static void context_close(struct i915_gem_context *ctx)
> {
> struct i915_address_space *vm;
>
> + /* Flush any concurrent set_engines() */
> + mutex_lock(&ctx->engines_mutex);
> + engines_idle_release(ctx, rcu_replace_pointer(ctx->engines, NULL, 1));
> i915_gem_context_set_closed(ctx);
> - set_closed_name(ctx);
> + mutex_unlock(&ctx->engines_mutex);
>
> mutex_lock(&ctx->mutex);
>
> + set_closed_name(ctx);
Looks more natural spot for this.
-Mika
> +
> vm = i915_gem_context_vm(ctx);
> if (vm)
> i915_vm_close(vm);
> @@ -1626,77 +1705,6 @@ static const i915_user_extension_fn set_engines__extensions[] = {
> [I915_CONTEXT_ENGINES_EXT_BOND] = set_engines__bond,
> };
>
> -static int engines_notify(struct i915_sw_fence *fence,
> - enum i915_sw_fence_notify state)
> -{
> - struct i915_gem_engines *engines =
> - container_of(fence, typeof(*engines), fence);
> -
> - switch (state) {
> - case FENCE_COMPLETE:
> - if (!list_empty(&engines->link)) {
> - struct i915_gem_context *ctx = engines->ctx;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&ctx->stale.lock, flags);
> - list_del(&engines->link);
> - spin_unlock_irqrestore(&ctx->stale.lock, flags);
> - }
> - break;
> -
> - case FENCE_FREE:
> - init_rcu_head(&engines->rcu);
> - call_rcu(&engines->rcu, free_engines_rcu);
> - break;
> - }
> -
> - return NOTIFY_DONE;
> -}
> -
> -static void engines_idle_release(struct i915_gem_engines *engines)
> -{
> - struct i915_gem_engines_iter it;
> - struct intel_context *ce;
> - unsigned long flags;
> -
> - GEM_BUG_ON(!engines);
> - i915_sw_fence_init(&engines->fence, engines_notify);
> -
> - INIT_LIST_HEAD(&engines->link);
> - spin_lock_irqsave(&engines->ctx->stale.lock, flags);
> - if (!i915_gem_context_is_closed(engines->ctx))
> - list_add(&engines->link, &engines->ctx->stale.engines);
> - spin_unlock_irqrestore(&engines->ctx->stale.lock, flags);
> - if (list_empty(&engines->link)) /* raced, already closed */
> - goto kill;
> -
> - for_each_gem_engine(ce, engines, it) {
> - struct dma_fence *fence;
> - int err;
> -
> - if (!ce->timeline)
> - continue;
> -
> - fence = i915_active_fence_get(&ce->timeline->last_request);
> - if (!fence)
> - continue;
> -
> - err = i915_sw_fence_await_dma_fence(&engines->fence,
> - fence, 0,
> - GFP_KERNEL);
> -
> - dma_fence_put(fence);
> - if (err < 0)
> - goto kill;
> - }
> - goto out;
> -
> -kill:
> - kill_engines(engines);
> -out:
> - i915_sw_fence_commit(&engines->fence);
> -}
> -
> static int
> set_engines(struct i915_gem_context *ctx,
> const struct drm_i915_gem_context_param *args)
> @@ -1739,8 +1747,6 @@ set_engines(struct i915_gem_context *ctx,
> if (!set.engines)
> return -ENOMEM;
>
> - set.engines->ctx = ctx;
> -
> for (n = 0; n < num_engines; n++) {
> struct i915_engine_class_instance ci;
> struct intel_engine_cs *engine;
> @@ -1793,6 +1799,11 @@ set_engines(struct i915_gem_context *ctx,
>
> replace:
> mutex_lock(&ctx->engines_mutex);
> + if (i915_gem_context_is_closed(ctx)) {
> + mutex_unlock(&ctx->engines_mutex);
> + free_engines(set.engines);
> + return -ENOENT;
> + }
> if (args->size)
> i915_gem_context_set_user_engines(ctx);
> else
> @@ -1801,7 +1812,7 @@ set_engines(struct i915_gem_context *ctx,
> mutex_unlock(&ctx->engines_mutex);
>
> /* Keep track of old engine sets for kill_context() */
> - engines_idle_release(set.engines);
> + engines_idle_release(ctx, set.engines);
>
> return 0;
> }
> @@ -2077,8 +2088,6 @@ static int clone_engines(struct i915_gem_context *dst,
> if (!clone)
> goto err_unlock;
>
> - clone->ctx = dst;
> -
> for (n = 0; n < e->num_engines; n++) {
> struct intel_engine_cs *engine;
>
> @@ -2121,8 +2130,7 @@ static int clone_engines(struct i915_gem_context *dst,
> i915_gem_context_unlock_engines(src);
>
> /* Serialised by constructor */
> - free_engines(__context_engines_static(dst));
> - RCU_INIT_POINTER(dst->engines, clone);
> + engines_idle_release(dst, rcu_replace_pointer(dst->engines, clone, 1));
> if (user_engines)
> i915_gem_context_set_user_engines(dst);
> else
> @@ -2553,6 +2561,9 @@ i915_gem_engines_iter_next(struct i915_gem_engines_iter *it)
> const struct i915_gem_engines *e = it->engines;
> struct intel_context *ctx;
>
> + if (unlikely(!e))
> + return NULL;
> +
> do {
> if (it->idx >= e->num_engines)
> return NULL;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index 3ae61a355d87..57b7ae2893e1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> @@ -207,7 +207,6 @@ static inline void
> i915_gem_engines_iter_init(struct i915_gem_engines_iter *it,
> struct i915_gem_engines *engines)
> {
> - GEM_BUG_ON(!engines);
> it->engines = engines;
> it->idx = 0;
> }
> diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> index b12ea1daa29d..e7e3c620f542 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> @@ -23,6 +23,9 @@ mock_context(struct drm_i915_private *i915,
> INIT_LIST_HEAD(&ctx->link);
> ctx->i915 = i915;
>
> + spin_lock_init(&ctx->stale.lock);
> + INIT_LIST_HEAD(&ctx->stale.engines);
> +
> i915_gem_context_set_persistence(ctx);
>
> mutex_init(&ctx->engines_mutex);
> --
> 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-03-02 18:30 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 8:57 [Intel-gfx] [PATCH 01/22] drm/i915/gem: Consolidate ctx->engines[] release Chris Wilson
2020-03-02 8:57 ` [Intel-gfx] [PATCH 02/22] drm/i915/gt: Prevent allocation on a banned context Chris Wilson
2020-03-02 8:57 ` [Intel-gfx] [PATCH 03/22] drm/i915/gem: Check that the context wasn't closed during setup Chris Wilson
2020-03-02 8:57 ` [Intel-gfx] [PATCH 04/22] drm/i915: Drop inspection of execbuf flags during evict Chris Wilson
2020-03-02 8:57 ` [Intel-gfx] [PATCH 05/22] drm/i915/gem: Extract transient execbuf flags from i915_vma Chris Wilson
2020-03-02 8:57 ` [Intel-gfx] [PATCH 06/22] drm/i915/gem: Only call eb_lookup_vma once during execbuf ioctl Chris Wilson
2020-03-02 9:39 ` Maarten Lankhorst
2020-03-02 8:57 ` [Intel-gfx] [PATCH 07/22] drm/i915/perf: Reintroduce wait on OA configuration completion Chris Wilson
2020-03-02 10:29 ` Lionel Landwerlin
2020-03-02 8:57 ` [Intel-gfx] [PATCH 08/22] drm/i915: Wrap i915_active in a simple kreffed struct Chris Wilson
2020-03-02 16:18 ` Mika Kuoppala
2020-03-02 16:23 ` Chris Wilson
2020-03-02 16:34 ` Mika Kuoppala
2020-03-02 16:42 ` Chris Wilson
2020-03-02 8:57 ` [Intel-gfx] [PATCH 09/22] drm/i915: Extend i915_request_await_active to use all timelines Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 10/22] drm/i915/perf: Schedule oa_config after modifying the contexts Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 11/22] drm/i915/selftests: Add request throughput measurement to perf Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 12/22] drm/i915/execlists: Check the sentinel is alone in the ELSP Chris Wilson
2020-03-02 14:04 ` Mika Kuoppala
2020-03-02 8:58 ` [Intel-gfx] [PATCH 13/22] drm/i915/execlists: Reduce preempt-to-busy roundtrip delay Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 14/22] dma-buf: Prettify typecasts for dma-fence-chain Chris Wilson
2020-03-02 16:11 ` Mika Kuoppala
2020-03-02 8:58 ` [Intel-gfx] [PATCH 15/22] dma-buf: Report signaled links inside dma-fence-chain Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 16/22] dma-buf: Exercise dma-fence-chain under selftests Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 17/22] dma-buf: Proxy fence, an unsignaled fence placeholder Chris Wilson
2020-03-03 16:24 ` kbuild test robot
2020-03-02 8:58 ` [Intel-gfx] [PATCH 18/22] drm/syncobj: Allow use of dma-fence-proxy Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 19/22] drm/i915/gem: Teach execbuf how to wait on future syncobj Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 20/22] drm/i915/gem: Allow combining submit-fences with syncobj Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 21/22] drm/i915/gt: Declare when we enabled timeslicing Chris Wilson
2020-03-02 8:58 ` [Intel-gfx] [PATCH 22/22] drm/i915/gt: Yield the timeslice if caught waiting on a user semaphore Chris Wilson
2020-03-02 13:58 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/22] drm/i915/gem: Consolidate ctx->engines[] release Patchwork
2020-03-02 14:18 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2020-03-02 14:24 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2020-03-02 18:29 ` Mika Kuoppala [this message]
2020-03-02 21:43 ` [Intel-gfx] [PATCH 01/22] " Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87d09u7gqy.fsf@gaia.fi.intel.com \
--to=mika.kuoppala@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox