Re: [Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
On 28/02/2020 12:19, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-02-28 12:08:18)
On 27/02/2020 11:01, Chris Wilson wrote:
+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);
What is the purpose of this? Looks dodgy - like it can't really
guarantee much.
It should be fine if we do:
execbuf:context_close:
ce->gem_context = NULL;
add_to_timeline(); get(&ce->timeline->last_request);
if (!ce->gem_context)
return -ENOENT;
If add is before the get, we will wait on it.
If add is after the get, we will wait on the earlier request, and skip
this one -- it will not execute.
What guarantees we see the latest last_request here, or that execbuf
sees the NULL before we try the get? The code elsewhere isn't assuming
unstable ce->gem_context I think.. engines we can change, but I don't
remember we accounted for this.
+ if (!intel_context_pin_if_active(ce))
+ continue;
+
+ 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);
Raced with.. ? context_close? Can't be the fence yet, before it has been
committed.
Yes, there's still the set_engines vs context_close to worry about.
I'd just say "raced with context_close" then.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
Quoting Tvrtko Ursulin (2020-02-28 12:08:18)
>
> On 27/02/2020 11:01, Chris Wilson wrote:
> > +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);
>
> What is the purpose of this? Looks dodgy - like it can't really
> guarantee much.
It should be fine if we do:
execbuf:context_close:
ce->gem_context = NULL;
add_to_timeline(); get(&ce->timeline->last_request);
if (!ce->gem_context)
return -ENOENT;
If add is before the get, we will wait on it.
If add is after the get, we will wait on the earlier request, and skip
this one -- it will not execute.
> > + if (!intel_context_pin_if_active(ce))
> > + continue;
> > +
> > + 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);
>
> Raced with.. ? context_close? Can't be the fence yet, before it has been
> committed.
Yes, there's still the set_engines vs context_close to worry about.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
On 27/02/2020 11:01, Chris Wilson wrote:
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
Cc: Tvrtko Ursulin
---
Remember to initialise the mock_context as well.
---
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);
+ 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);
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) {
[Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
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
Cc: Tvrtko Ursulin
---
Remember to initialise the mock_context as well.
---
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);
+ 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);
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
[Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
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
Cc: Tvrtko Ursulin
---
Put the pin_if_active check back!
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 193 +++-
drivers/gpu/drm/i915/gem/i915_gem_context.h | 1 -
2 files changed, 102 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..8e2f8ab8ce6e 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);
+ 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);
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
[Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
Use the same engine_idle_release() routine for cleaning all old
ctx->engine[] state, closing any potential races with concurrent execbuf
submission.
Closes: https://gitlab.freedesktop.org/drm/intel/issues/1241
Signed-off-by: Chris Wilson
---
Reorder set-closed/engine_idle_release to avoid premature killing
Take a reference to prevent racing context free with engine cleanup
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 190 ++--
1 file changed, 100 insertions(+), 90 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 3e82739bdbc0..99206ec45876 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -243,7 +243,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);
@@ -270,8 +269,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;
@@ -305,7 +302,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)
@@ -492,30 +488,110 @@ 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);
+ 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);
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);
+ }
+ break;
+
+ case FENCE_FREE:
+ i915_gem_context_put(engines->ctx);
+ 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) {
+ int err = 0;
+
+ RCU_INIT_POINTER(ce->gem_context, NULL);
+
+ if (!ce->timeline) { /* XXX serialisation with execbuf? */
+ intel_context_set_banned(ce);
+ continue;
+ }
+
+ mutex_lock(&ce->timeline->mutex);
+ if (!list_empty(&ce->timeline->requests)) {
+ struct i915_request *rq;
+
+ rq = list_last_entry(&ce->timeline->requests,
+typeof(*rq),
+
[Intel-gfx] [PATCH] drm/i915/gem: Consolidate ctx->engines[] release
Use the same engine_idle_release() routine for cleaning all old
ctx->engine[] state, closing any potential races with concurrent execbuf
submission.
Closes: https://gitlab.freedesktop.org/drm/intel/issues/1241
Signed-off-by: Chris Wilson
---
Reorder set-closed/engine_idle_release to avoid premature killing
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 179 +++-
1 file changed, 96 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 3e82739bdbc0..b7d824a2b711 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -243,7 +243,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);
@@ -305,7 +304,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)
@@ -492,30 +490,107 @@ 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);
+ 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);
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);
+ }
+ 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;
+
+ GEM_BUG_ON(!engines);
+ i915_sw_fence_init(&engines->fence, engines_notify);
+ INIT_LIST_HEAD(&engines->link);
+
+ for_each_gem_engine(ce, engines, it) {
+ int err = 0;
+
+ RCU_INIT_POINTER(ce->gem_context, NULL);
+
+ if (!ce->timeline) { /* XXX serialisation with execbuf? */
+ intel_context_set_banned(ce);
+ continue;
+ }
+
+ mutex_lock(&ce->timeline->mutex);
+ if (!list_empty(&ce->timeline->requests)) {
+ struct i915_request *rq;
+
+ rq = list_last_entry(&ce->timeline->requests,
+typeof(*rq),
+link);
+
+ err = i915_sw_fence_await_dma_fence(&engines->fence,
+ &rq->fence, 0,
+ GFP_KERNEL);
+ }
+ mutex_unlock(&ce->timeline->mutex);
+ if (err < 0)
+ goto kill;
+ }
+
+ spin_lock_irq(&engines->ctx->stale.lock);
+
