Commit d7261b14 authored by Dave Airlie's avatar Dave Airlie

Merge tag 'drm-intel-fixes-2020-09-17' of...

Merge tag 'drm-intel-fixes-2020-09-17' of ssh://git.freedesktop.org/git/drm/drm-intel into drm-fixes

drm/i915 fixes for v5.9-rc6:
- Avoid exposing a partially constructed context
- Use RCU instead of mutex for context termination list iteration
- Avoid data race reported by KCSAN
- Filter wake_flags passed to default_wake_function
Signed-off-by: default avatarDave Airlie <airlied@redhat.com>
From: Jani Nikula <jani.nikula@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/87y2l8vlj3.fsf@intel.com
parents 4b1ededb 20612303
...@@ -439,29 +439,36 @@ static bool __cancel_engine(struct intel_engine_cs *engine) ...@@ -439,29 +439,36 @@ static bool __cancel_engine(struct intel_engine_cs *engine)
return __reset_engine(engine); return __reset_engine(engine);
} }
static struct intel_engine_cs *__active_engine(struct i915_request *rq) static bool
__active_engine(struct i915_request *rq, struct intel_engine_cs **active)
{ {
struct intel_engine_cs *engine, *locked; struct intel_engine_cs *engine, *locked;
bool ret = false;
/* /*
* Serialise with __i915_request_submit() so that it sees * Serialise with __i915_request_submit() so that it sees
* is-banned?, or we know the request is already inflight. * is-banned?, or we know the request is already inflight.
*
* Note that rq->engine is unstable, and so we double
* check that we have acquired the lock on the final engine.
*/ */
locked = READ_ONCE(rq->engine); locked = READ_ONCE(rq->engine);
spin_lock_irq(&locked->active.lock); spin_lock_irq(&locked->active.lock);
while (unlikely(locked != (engine = READ_ONCE(rq->engine)))) { while (unlikely(locked != (engine = READ_ONCE(rq->engine)))) {
spin_unlock(&locked->active.lock); spin_unlock(&locked->active.lock);
spin_lock(&engine->active.lock);
locked = engine; locked = engine;
spin_lock(&locked->active.lock);
} }
engine = NULL; if (!i915_request_completed(rq)) {
if (i915_request_is_active(rq) && rq->fence.error != -EIO) if (i915_request_is_active(rq) && rq->fence.error != -EIO)
engine = rq->engine; *active = locked;
ret = true;
}
spin_unlock_irq(&locked->active.lock); spin_unlock_irq(&locked->active.lock);
return engine; return ret;
} }
static struct intel_engine_cs *active_engine(struct intel_context *ce) static struct intel_engine_cs *active_engine(struct intel_context *ce)
...@@ -472,17 +479,16 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce) ...@@ -472,17 +479,16 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
if (!ce->timeline) if (!ce->timeline)
return NULL; return NULL;
mutex_lock(&ce->timeline->mutex); rcu_read_lock();
list_for_each_entry_reverse(rq, &ce->timeline->requests, link) { list_for_each_entry_rcu(rq, &ce->timeline->requests, link) {
if (i915_request_completed(rq)) if (i915_request_is_active(rq) && i915_request_completed(rq))
break; continue;
/* Check with the backend if the request is inflight */ /* Check with the backend if the request is inflight */
engine = __active_engine(rq); if (__active_engine(rq, &engine))
if (engine)
break; break;
} }
mutex_unlock(&ce->timeline->mutex); rcu_read_unlock();
return engine; return engine;
} }
...@@ -713,6 +719,7 @@ __create_context(struct drm_i915_private *i915) ...@@ -713,6 +719,7 @@ __create_context(struct drm_i915_private *i915)
ctx->i915 = i915; ctx->i915 = i915;
ctx->sched.priority = I915_USER_PRIORITY(I915_PRIORITY_NORMAL); ctx->sched.priority = I915_USER_PRIORITY(I915_PRIORITY_NORMAL);
mutex_init(&ctx->mutex); mutex_init(&ctx->mutex);
INIT_LIST_HEAD(&ctx->link);
spin_lock_init(&ctx->stale.lock); spin_lock_init(&ctx->stale.lock);
INIT_LIST_HEAD(&ctx->stale.engines); INIT_LIST_HEAD(&ctx->stale.engines);
...@@ -740,10 +747,6 @@ __create_context(struct drm_i915_private *i915) ...@@ -740,10 +747,6 @@ __create_context(struct drm_i915_private *i915)
for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++) for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES; ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
spin_lock(&i915->gem.contexts.lock);
list_add_tail(&ctx->link, &i915->gem.contexts.list);
spin_unlock(&i915->gem.contexts.lock);
return ctx; return ctx;
err_free: err_free:
...@@ -931,6 +934,7 @@ static int gem_context_register(struct i915_gem_context *ctx, ...@@ -931,6 +934,7 @@ static int gem_context_register(struct i915_gem_context *ctx,
struct drm_i915_file_private *fpriv, struct drm_i915_file_private *fpriv,
u32 *id) u32 *id)
{ {
struct drm_i915_private *i915 = ctx->i915;
struct i915_address_space *vm; struct i915_address_space *vm;
int ret; int ret;
...@@ -949,8 +953,16 @@ static int gem_context_register(struct i915_gem_context *ctx, ...@@ -949,8 +953,16 @@ static int gem_context_register(struct i915_gem_context *ctx,
/* And finally expose ourselves to userspace via the idr */ /* And finally expose ourselves to userspace via the idr */
ret = xa_alloc(&fpriv->context_xa, id, ctx, xa_limit_32b, GFP_KERNEL); ret = xa_alloc(&fpriv->context_xa, id, ctx, xa_limit_32b, GFP_KERNEL);
if (ret) if (ret)
put_pid(fetch_and_zero(&ctx->pid)); goto err_pid;
spin_lock(&i915->gem.contexts.lock);
list_add_tail(&ctx->link, &i915->gem.contexts.list);
spin_unlock(&i915->gem.contexts.lock);
return 0;
err_pid:
put_pid(fetch_and_zero(&ctx->pid));
return ret; return ret;
} }
......
...@@ -2060,6 +2060,14 @@ static inline void clear_ports(struct i915_request **ports, int count) ...@@ -2060,6 +2060,14 @@ static inline void clear_ports(struct i915_request **ports, int count)
memset_p((void **)ports, NULL, count); memset_p((void **)ports, NULL, count);
} }
static inline void
copy_ports(struct i915_request **dst, struct i915_request **src, int count)
{
/* A memcpy_p() would be very useful here! */
while (count--)
WRITE_ONCE(*dst++, *src++); /* avoid write tearing */
}
static void execlists_dequeue(struct intel_engine_cs *engine) static void execlists_dequeue(struct intel_engine_cs *engine)
{ {
struct intel_engine_execlists * const execlists = &engine->execlists; struct intel_engine_execlists * const execlists = &engine->execlists;
...@@ -2648,10 +2656,9 @@ static void process_csb(struct intel_engine_cs *engine) ...@@ -2648,10 +2656,9 @@ static void process_csb(struct intel_engine_cs *engine)
/* switch pending to inflight */ /* switch pending to inflight */
GEM_BUG_ON(!assert_pending_valid(execlists, "promote")); GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
memcpy(execlists->inflight, copy_ports(execlists->inflight,
execlists->pending, execlists->pending,
execlists_num_ports(execlists) * execlists_num_ports(execlists));
sizeof(*execlists->pending));
smp_wmb(); /* complete the seqlock */ smp_wmb(); /* complete the seqlock */
WRITE_ONCE(execlists->active, execlists->inflight); WRITE_ONCE(execlists->active, execlists->inflight);
......
...@@ -388,17 +388,38 @@ static bool __request_in_flight(const struct i915_request *signal) ...@@ -388,17 +388,38 @@ static bool __request_in_flight(const struct i915_request *signal)
* As we know that there are always preemption points between * As we know that there are always preemption points between
* requests, we know that only the currently executing request * requests, we know that only the currently executing request
* may be still active even though we have cleared the flag. * may be still active even though we have cleared the flag.
* However, we can't rely on our tracking of ELSP[0] to known * However, we can't rely on our tracking of ELSP[0] to know
* which request is currently active and so maybe stuck, as * which request is currently active and so maybe stuck, as
* the tracking maybe an event behind. Instead assume that * the tracking maybe an event behind. Instead assume that
* if the context is still inflight, then it is still active * if the context is still inflight, then it is still active
* even if the active flag has been cleared. * even if the active flag has been cleared.
*
* To further complicate matters, if there a pending promotion, the HW
* may either perform a context switch to the second inflight execlists,
* or it may switch to the pending set of execlists. In the case of the
* latter, it may send the ACK and we process the event copying the
* pending[] over top of inflight[], _overwriting_ our *active. Since
* this implies the HW is arbitrating and not struck in *active, we do
* not worry about complete accuracy, but we do require no read/write
* tearing of the pointer [the read of the pointer must be valid, even
* as the array is being overwritten, for which we require the writes
* to avoid tearing.]
*
* Note that the read of *execlists->active may race with the promotion
* of execlists->pending[] to execlists->inflight[], overwritting
* the value at *execlists->active. This is fine. The promotion implies
* that we received an ACK from the HW, and so the context is not
* stuck -- if we do not see ourselves in *active, the inflight status
* is valid. If instead we see ourselves being copied into *active,
* we are inflight and may signal the callback.
*/ */
if (!intel_context_inflight(signal->context)) if (!intel_context_inflight(signal->context))
return false; return false;
rcu_read_lock(); rcu_read_lock();
for (port = __engine_active(signal->engine); (rq = *port); port++) { for (port = __engine_active(signal->engine);
(rq = READ_ONCE(*port)); /* may race with promotion of pending[] */
port++) {
if (rq->context == signal->context) { if (rq->context == signal->context) {
inflight = i915_seqno_passed(rq->fence.seqno, inflight = i915_seqno_passed(rq->fence.seqno,
signal->fence.seqno); signal->fence.seqno);
......
...@@ -164,9 +164,13 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, ...@@ -164,9 +164,13 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
do { do {
list_for_each_entry_safe(pos, next, &x->head, entry) { list_for_each_entry_safe(pos, next, &x->head, entry) {
pos->func(pos, int wake_flags;
TASK_NORMAL, fence->error,
&extra); wake_flags = fence->error;
if (pos->func == autoremove_wake_function)
wake_flags = 0;
pos->func(pos, TASK_NORMAL, wake_flags, &extra);
} }
if (list_empty(&extra)) if (list_empty(&extra))
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment