* [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
@ 2019-09-19 11:19 Chris Wilson
2019-09-19 11:19 ` [PATCH v2 2/3] drm/i915: Lock signaler timeline while navigating Chris Wilson
` (6 more replies)
0 siblings, 7 replies; 12+ messages in thread
From: Chris Wilson @ 2019-09-19 11:19 UTC (permalink / raw)
To: intel-gfx
The request->timeline is only valid until the request is retired (i.e.
before it is completed). Upon retiring the request, the context may be
unpinned and freed, and along with it the timeline may be freed. We
therefore need to be very careful when chasing rq->timeline that the
pointer does not disappear beneath us. The vast majority of users are in
a protected context, either during request construction or retirement,
where the timeline->mutex is held and the timeline cannot disappear. It
is those few off the beaten path (where we access a second timeline) that
need extra scrutiny -- to be added in the next patch after first adding
the warnings about dangerous access.
One complication, where we cannot use the timeline->mutex itself, is
during request submission onto hardware (under spinlocks). Here, we want
to check on the timeline to finalize the breadcrumb, and so we need to
impose a second rule to ensure that the request->timeline is indeed
valid. As we are submitting the request, it's context and timeline must
be pinned, as it will be used by the hardware. Since it is pinned, we
know the request->timeline must still be valid, and we cannot submit the
idle barrier until after we release the engine->active.lock, ergo while
submitting and holding that spinlock, a second thread cannot release the
timeline.
v2: Don't be lazy inside selftests; hold the timeline->mutex for as long
as we need it, and tidy up acquiring the timeline with a bit of
refactoring (i915_active_add_request)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
.../gpu/drm/i915/gem/i915_gem_client_blt.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/gt/intel_context.c | 4 +-
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 57 ++++++++++++++++---
drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +-
drivers/gpu/drm/i915/gt/intel_engine_pool.h | 2 +-
drivers/gpu/drm/i915/gt/intel_lrc.c | 17 +++---
drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 27 +++++----
drivers/gpu/drm/i915/gt/intel_timeline.c | 6 +-
.../gpu/drm/i915/gt/intel_timeline_types.h | 1 +
drivers/gpu/drm/i915/gt/selftest_context.c | 18 ++++--
drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +-
drivers/gpu/drm/i915/i915_active.c | 2 +-
drivers/gpu/drm/i915/i915_active.h | 6 ++
drivers/gpu/drm/i915/i915_request.c | 28 +++++----
drivers/gpu/drm/i915/i915_request.h | 22 ++++++-
drivers/gpu/drm/i915/i915_vma.c | 6 +-
drivers/gpu/drm/i915/selftests/i915_active.c | 2 +-
drivers/gpu/drm/i915/selftests/igt_spinner.c | 2 +-
20 files changed, 147 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
index 29edfc343716..5efef9babadb 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -230,7 +230,7 @@ alloc_request(struct intel_overlay *overlay, void (*fn)(struct intel_overlay *))
if (IS_ERR(rq))
return rq;
- err = i915_active_ref(&overlay->last_flip, rq->timeline, rq);
+ err = i915_active_add_request(&overlay->last_flip, rq);
if (err) {
i915_request_add(rq);
return ERR_PTR(err);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
index f99920652751..7f61a8024133 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
@@ -211,7 +211,7 @@ static void clear_pages_worker(struct work_struct *work)
* keep track of the GPU activity within this vma/request, and
* propagate the signal from the request to w->dma.
*/
- err = i915_active_ref(&vma->active, rq->timeline, rq);
+ err = i915_active_add_request(&vma->active, rq);
if (err)
goto out_request;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index f1c0e5d958f3..4a34c4f62065 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -910,7 +910,7 @@ static int context_barrier_task(struct i915_gem_context *ctx,
if (emit)
err = emit(rq, data);
if (err == 0)
- err = i915_active_ref(&cb->base, rq->timeline, rq);
+ err = i915_active_add_request(&cb->base, rq);
i915_request_add(rq);
if (err)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index c0495811f493..26cb838c272c 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -298,7 +298,7 @@ int intel_context_prepare_remote_request(struct intel_context *ce,
/* Only suitable for use in remotely modifying this context */
GEM_BUG_ON(rq->hw_context == ce);
- if (rq->timeline != tl) { /* beware timeline sharing */
+ if (rcu_access_pointer(rq->timeline) != tl) { /* timeline sharing! */
err = mutex_lock_interruptible_nested(&tl->mutex,
SINGLE_DEPTH_NESTING);
if (err)
@@ -319,7 +319,7 @@ int intel_context_prepare_remote_request(struct intel_context *ce,
* words transfer the pinned ce object to tracked active request.
*/
GEM_BUG_ON(i915_active_is_idle(&ce->active));
- return i915_active_ref(&ce->active, rq->timeline, rq);
+ return i915_active_add_request(&ce->active, rq);
}
struct i915_request *intel_context_create_request(struct intel_context *ce)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 3c176b0f4b45..f451d5076bde 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -680,6 +680,8 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
engine->status_page.vma))
goto out_frame;
+ mutex_lock(&frame->timeline.mutex);
+
frame->ring.vaddr = frame->cs;
frame->ring.size = sizeof(frame->cs);
frame->ring.effective_size = frame->ring.size;
@@ -688,18 +690,22 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
frame->rq.i915 = engine->i915;
frame->rq.engine = engine;
frame->rq.ring = &frame->ring;
- frame->rq.timeline = &frame->timeline;
+ rcu_assign_pointer(frame->rq.timeline, &frame->timeline);
dw = intel_timeline_pin(&frame->timeline);
if (dw < 0)
goto out_timeline;
+ spin_lock_irq(&engine->active.lock);
dw = engine->emit_fini_breadcrumb(&frame->rq, frame->cs) - frame->cs;
+ spin_unlock_irq(&engine->active.lock);
+
GEM_BUG_ON(dw & 1); /* RING_TAIL must be qword aligned */
intel_timeline_unpin(&frame->timeline);
out_timeline:
+ mutex_unlock(&frame->timeline.mutex);
intel_timeline_fini(&frame->timeline);
out_frame:
kfree(frame);
@@ -1196,6 +1202,27 @@ static void hexdump(struct drm_printer *m, const void *buf, size_t len)
}
}
+static struct intel_timeline *get_timeline(struct i915_request *rq)
+{
+ struct intel_timeline *tl;
+
+ /*
+ * Even though we are holding the engine->active.lock here, there
+ * is no control over the submission queue per-se and we are
+ * inspecting the active state at a random point in time, with an
+ * unknown queue. Play safe and make sure the timeline remains valid.
+ * (Only being used for pretty printing, one extra kref shouldn't
+ * cause a camel stampede!)
+ */
+ rcu_read_lock();
+ tl = rcu_dereference(rq->timeline);
+ if (!kref_get_unless_zero(&tl->kref))
+ tl = NULL;
+ rcu_read_unlock();
+
+ return tl;
+}
+
static void intel_engine_print_registers(struct intel_engine_cs *engine,
struct drm_printer *m)
{
@@ -1290,27 +1317,37 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
int len;
len = snprintf(hdr, sizeof(hdr),
- "\t\tActive[%d: ",
+ "\t\tActive[%d]: ",
(int)(port - execlists->active));
- if (!i915_request_signaled(rq))
+ if (!i915_request_signaled(rq)) {
+ struct intel_timeline *tl = get_timeline(rq);
+
len += snprintf(hdr + len, sizeof(hdr) - len,
"ring:{start:%08x, hwsp:%08x, seqno:%08x}, ",
i915_ggtt_offset(rq->ring->vma),
- rq->timeline->hwsp_offset,
+ tl ? tl->hwsp_offset : 0,
hwsp_seqno(rq));
+
+ if (tl)
+ intel_timeline_put(tl);
+ }
snprintf(hdr + len, sizeof(hdr) - len, "rq: ");
print_request(m, rq, hdr);
}
for (port = execlists->pending; (rq = *port); port++) {
+ struct intel_timeline *tl = get_timeline(rq);
char hdr[80];
snprintf(hdr, sizeof(hdr),
"\t\tPending[%d] ring:{start:%08x, hwsp:%08x, seqno:%08x}, rq: ",
(int)(port - execlists->pending),
i915_ggtt_offset(rq->ring->vma),
- rq->timeline->hwsp_offset,
+ tl ? tl->hwsp_offset : 0,
hwsp_seqno(rq));
print_request(m, rq, hdr);
+
+ if (tl)
+ intel_timeline_put(tl);
}
spin_unlock_irqrestore(&engine->active.lock, flags);
} else if (INTEL_GEN(dev_priv) > 6) {
@@ -1388,6 +1425,8 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_lock_irqsave(&engine->active.lock, flags);
rq = intel_engine_find_active_request(engine);
if (rq) {
+ struct intel_timeline *tl = get_timeline(rq);
+
print_request(m, rq, "\t\tactive ");
drm_printf(m, "\t\tring->start: 0x%08x\n",
@@ -1400,8 +1439,12 @@ void intel_engine_dump(struct intel_engine_cs *engine,
rq->ring->emit);
drm_printf(m, "\t\tring->space: 0x%08x\n",
rq->ring->space);
- drm_printf(m, "\t\tring->hwsp: 0x%08x\n",
- rq->timeline->hwsp_offset);
+
+ if (tl) {
+ drm_printf(m, "\t\tring->hwsp: 0x%08x\n",
+ tl->hwsp_offset);
+ intel_timeline_put(tl);
+ }
print_request_ring(m, rq);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index 65b5ca74b394..ce61c83d68e9 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -103,7 +103,7 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
/* Context switch failed, hope for the best! Maybe reset? */
goto out_unlock;
- intel_timeline_enter(rq->timeline);
+ intel_timeline_enter(i915_request_timeline(rq));
/* Check again on the next retirement. */
engine->wakeref_serial = engine->serial + 1;
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pool.h b/drivers/gpu/drm/i915/gt/intel_engine_pool.h
index 7e2123b33594..1bd89cadc3b7 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pool.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pool.h
@@ -18,7 +18,7 @@ static inline int
intel_engine_pool_mark_active(struct intel_engine_pool_node *node,
struct i915_request *rq)
{
- return i915_active_ref(&node->active, rq->timeline, rq);
+ return i915_active_add_request(&node->active, rq);
}
static inline void
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index a99166a2d2eb..ac21587b75f6 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1825,7 +1825,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq)
{
u32 *cs;
- GEM_BUG_ON(!rq->timeline->has_initial_breadcrumb);
+ GEM_BUG_ON(!i915_request_timeline(rq)->has_initial_breadcrumb);
cs = intel_ring_begin(rq, 6);
if (IS_ERR(cs))
@@ -1841,7 +1841,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq)
*cs++ = MI_NOOP;
*cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
- *cs++ = rq->timeline->hwsp_offset;
+ *cs++ = i915_request_timeline(rq)->hwsp_offset;
*cs++ = 0;
*cs++ = rq->fence.seqno - 1;
@@ -2324,7 +2324,8 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
static struct i915_request *active_request(struct i915_request *rq)
{
- const struct list_head * const list = &rq->timeline->requests;
+ const struct list_head * const list =
+ &i915_request_active_timeline(rq)->requests;
const struct intel_context * const ce = rq->hw_context;
struct i915_request *active = NULL;
@@ -2855,7 +2856,7 @@ static u32 *gen8_emit_fini_breadcrumb(struct i915_request *request, u32 *cs)
{
cs = gen8_emit_ggtt_write(cs,
request->fence.seqno,
- request->timeline->hwsp_offset,
+ i915_request_active_timeline(request)->hwsp_offset,
0);
return gen8_emit_fini_breadcrumb_footer(request, cs);
@@ -2872,7 +2873,7 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
/* XXX flush+write+CS_STALL all in one upsets gem_concurrent_blt:kbl */
cs = gen8_emit_ggtt_write_rcs(cs,
request->fence.seqno,
- request->timeline->hwsp_offset,
+ i915_request_active_timeline(request)->hwsp_offset,
PIPE_CONTROL_FLUSH_ENABLE |
PIPE_CONTROL_CS_STALL);
@@ -2884,7 +2885,7 @@ gen11_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
{
cs = gen8_emit_ggtt_write_rcs(cs,
request->fence.seqno,
- request->timeline->hwsp_offset,
+ i915_request_active_timeline(request)->hwsp_offset,
PIPE_CONTROL_CS_STALL |
PIPE_CONTROL_TILE_CACHE_FLUSH |
PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH |
@@ -2948,7 +2949,7 @@ static u32 *gen12_emit_fini_breadcrumb(struct i915_request *request, u32 *cs)
{
cs = gen8_emit_ggtt_write(cs,
request->fence.seqno,
- request->timeline->hwsp_offset,
+ i915_request_active_timeline(request)->hwsp_offset,
0);
return gen12_emit_fini_breadcrumb_footer(request, cs);
@@ -2959,7 +2960,7 @@ gen12_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
{
cs = gen8_emit_ggtt_write_rcs(cs,
request->fence.seqno,
- request->timeline->hwsp_offset,
+ i915_request_active_timeline(request)->hwsp_offset,
PIPE_CONTROL_CS_STALL |
PIPE_CONTROL_TILE_CACHE_FLUSH |
PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH |
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index a25b84b12ef1..0747b8c9f768 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -322,7 +322,8 @@ static u32 *gen6_rcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
PIPE_CONTROL_DC_FLUSH_ENABLE |
PIPE_CONTROL_QW_WRITE |
PIPE_CONTROL_CS_STALL);
- *cs++ = rq->timeline->hwsp_offset | PIPE_CONTROL_GLOBAL_GTT;
+ *cs++ = i915_request_active_timeline(rq)->hwsp_offset |
+ PIPE_CONTROL_GLOBAL_GTT;
*cs++ = rq->fence.seqno;
*cs++ = MI_USER_INTERRUPT;
@@ -425,7 +426,7 @@ static u32 *gen7_rcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
PIPE_CONTROL_QW_WRITE |
PIPE_CONTROL_GLOBAL_GTT_IVB |
PIPE_CONTROL_CS_STALL);
- *cs++ = rq->timeline->hwsp_offset;
+ *cs++ = i915_request_active_timeline(rq)->hwsp_offset;
*cs++ = rq->fence.seqno;
*cs++ = MI_USER_INTERRUPT;
@@ -439,8 +440,8 @@ static u32 *gen7_rcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
static u32 *gen6_xcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
{
- GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
- GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
+ GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
*cs++ = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_DW_STORE_INDEX;
*cs++ = I915_GEM_HWS_SEQNO_ADDR | MI_FLUSH_DW_USE_GTT;
@@ -459,8 +460,8 @@ static u32 *gen7_xcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
{
int i;
- GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
- GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
+ GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
*cs++ = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_DW_STORE_INDEX;
*cs++ = I915_GEM_HWS_SEQNO_ADDR | MI_FLUSH_DW_USE_GTT;
@@ -938,8 +939,8 @@ static void i9xx_submit_request(struct i915_request *request)
static u32 *i9xx_emit_breadcrumb(struct i915_request *rq, u32 *cs)
{
- GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
- GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
+ GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
*cs++ = MI_FLUSH;
@@ -961,8 +962,8 @@ static u32 *gen5_emit_breadcrumb(struct i915_request *rq, u32 *cs)
{
int i;
- GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
- GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
+ GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
*cs++ = MI_FLUSH;
@@ -1815,7 +1816,7 @@ static int ring_request_alloc(struct i915_request *request)
int ret;
GEM_BUG_ON(!intel_context_is_pinned(request->hw_context));
- GEM_BUG_ON(request->timeline->has_initial_breadcrumb);
+ GEM_BUG_ON(i915_request_timeline(request)->has_initial_breadcrumb);
/*
* Flush enough space to reduce the likelihood of waiting after
@@ -1926,7 +1927,9 @@ u32 *intel_ring_begin(struct i915_request *rq, unsigned int num_dwords)
*/
GEM_BUG_ON(!rq->reserved_space);
- ret = wait_for_space(ring, rq->timeline, total_bytes);
+ ret = wait_for_space(ring,
+ i915_request_timeline(rq),
+ total_bytes);
if (unlikely(ret))
return ERR_PTR(ret);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
index 9cb01d9828f1..115a24d4a20a 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline.c
+++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
@@ -493,7 +493,7 @@ int intel_timeline_get_seqno(struct intel_timeline *tl,
static int cacheline_ref(struct intel_timeline_cacheline *cl,
struct i915_request *rq)
{
- return i915_active_ref(&cl->active, rq->timeline, rq);
+ return i915_active_add_request(&cl->active, rq);
}
int intel_timeline_read_hwsp(struct i915_request *from,
@@ -504,7 +504,7 @@ int intel_timeline_read_hwsp(struct i915_request *from,
struct intel_timeline *tl = from->timeline;
int err;
- GEM_BUG_ON(to->timeline == tl);
+ GEM_BUG_ON(rcu_access_pointer(to->timeline) == tl);
mutex_lock_nested(&tl->mutex, SINGLE_DEPTH_NESTING);
err = i915_request_completed(from);
@@ -541,7 +541,7 @@ void __intel_timeline_free(struct kref *kref)
container_of(kref, typeof(*timeline), kref);
intel_timeline_fini(timeline);
- kfree(timeline);
+ kfree_rcu(timeline, rcu);
}
static void timelines_fini(struct intel_gt *gt)
diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
index 2b1baf2fcc8e..c668c4c50e75 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
@@ -80,6 +80,7 @@ struct intel_timeline {
struct intel_gt *gt;
struct kref kref;
+ struct rcu_head rcu;
};
#endif /* __I915_TIMELINE_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
index 9d1ea26c7a2d..4ce1e25433d2 100644
--- a/drivers/gpu/drm/i915/gt/selftest_context.c
+++ b/drivers/gpu/drm/i915/gt/selftest_context.c
@@ -14,22 +14,28 @@
static int request_sync(struct i915_request *rq)
{
+ struct intel_timeline *tl = i915_request_timeline(rq);
long timeout;
int err = 0;
+ intel_timeline_get(tl);
i915_request_get(rq);
- i915_request_add(rq);
+ /* Opencode i915_request_add() so we can keep the timeline locked. */
+ __i915_request_commit(rq);
+ __i915_request_queue(rq, NULL);
+
timeout = i915_request_wait(rq, 0, HZ / 10);
- if (timeout < 0) {
+ if (timeout < 0)
err = timeout;
- } else {
- mutex_lock(&rq->timeline->mutex);
+ else
i915_request_retire_upto(rq);
- mutex_unlock(&rq->timeline->mutex);
- }
+
+ lockdep_unpin_lock(&tl->mutex, rq->cookie);
+ mutex_unlock(&tl->mutex);
i915_request_put(rq);
+ intel_timeline_put(tl);
return err;
}
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 26d05bd1bdc8..93a871bfd95d 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -1089,7 +1089,7 @@ static int live_suppress_wait_preempt(void *arg)
}
/* Disable NEWCLIENT promotion */
- __i915_active_request_set(&rq[i]->timeline->last_request,
+ __i915_active_request_set(&i915_request_timeline(rq[i])->last_request,
dummy);
i915_request_add(rq[i]);
}
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 6a447f1d0110..d5aac6ff803a 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -695,7 +695,7 @@ void i915_request_add_active_barriers(struct i915_request *rq)
struct llist_node *node, *next;
GEM_BUG_ON(intel_engine_is_virtual(engine));
- GEM_BUG_ON(rq->timeline != engine->kernel_context->timeline);
+ GEM_BUG_ON(i915_request_timeline(rq) != engine->kernel_context->timeline);
/*
* Attach the list of proto-fences to the in-flight request such
diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
index f95058f99057..949c6835335b 100644
--- a/drivers/gpu/drm/i915/i915_active.h
+++ b/drivers/gpu/drm/i915/i915_active.h
@@ -373,6 +373,12 @@ int i915_active_ref(struct i915_active *ref,
struct intel_timeline *tl,
struct i915_request *rq);
+static inline int
+i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
+{
+ return i915_active_ref(ref, i915_request_timeline(rq), rq);
+}
+
int i915_active_wait(struct i915_active *ref);
int i915_request_await_active(struct i915_request *rq,
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 606acde47fa0..fb6f21c41934 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -220,7 +220,6 @@ static bool i915_request_retire(struct i915_request *rq)
{
struct i915_active_request *active, *next;
- lockdep_assert_held(&rq->timeline->mutex);
if (!i915_request_completed(rq))
return false;
@@ -241,7 +240,8 @@ static bool i915_request_retire(struct i915_request *rq)
* Note this requires that we are always called in request
* completion order.
*/
- GEM_BUG_ON(!list_is_first(&rq->link, &rq->timeline->requests));
+ GEM_BUG_ON(!list_is_first(&rq->link,
+ &i915_request_timeline(rq)->requests));
rq->ring->head = rq->postfix;
/*
@@ -317,7 +317,7 @@ static bool i915_request_retire(struct i915_request *rq)
void i915_request_retire_upto(struct i915_request *rq)
{
- struct intel_timeline * const tl = rq->timeline;
+ struct intel_timeline * const tl = i915_request_timeline(rq);
struct i915_request *tmp;
GEM_TRACE("%s fence %llx:%lld, current %d\n",
@@ -325,7 +325,6 @@ void i915_request_retire_upto(struct i915_request *rq)
rq->fence.context, rq->fence.seqno,
hwsp_seqno(rq));
- lockdep_assert_held(&tl->mutex);
GEM_BUG_ON(!i915_request_completed(rq));
do {
@@ -661,9 +660,11 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
rq->gem_context = ce->gem_context;
rq->engine = ce->engine;
rq->ring = ce->ring;
- rq->timeline = tl;
+
+ rcu_assign_pointer(rq->timeline, tl);
rq->hwsp_seqno = tl->hwsp_seqno;
rq->hwsp_cacheline = tl->hwsp_cacheline;
+
rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
spin_lock_init(&rq->lock);
@@ -771,7 +772,8 @@ i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
return 0;
signal = list_prev_entry(signal, link);
- if (intel_timeline_sync_is_later(rq->timeline, &signal->fence))
+ if (intel_timeline_sync_is_later(i915_request_timeline(rq),
+ &signal->fence))
return 0;
return i915_sw_fence_await_dma_fence(&rq->submit,
@@ -947,7 +949,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
/* Squash repeated waits to the same timelines */
if (fence->context &&
- intel_timeline_sync_is_later(rq->timeline, fence))
+ intel_timeline_sync_is_later(i915_request_timeline(rq),
+ fence))
continue;
if (dma_fence_is_i915(fence))
@@ -961,7 +964,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
/* Record the latest fence used against each timeline */
if (fence->context)
- intel_timeline_sync_set(rq->timeline, fence);
+ intel_timeline_sync_set(i915_request_timeline(rq),
+ fence);
} while (--nchild);
return 0;
@@ -1103,7 +1107,7 @@ void i915_request_skip(struct i915_request *rq, int error)
static struct i915_request *
__i915_request_add_to_timeline(struct i915_request *rq)
{
- struct intel_timeline *timeline = rq->timeline;
+ struct intel_timeline *timeline = i915_request_timeline(rq);
struct i915_request *prev;
/*
@@ -1216,7 +1220,7 @@ void __i915_request_queue(struct i915_request *rq,
void i915_request_add(struct i915_request *rq)
{
struct i915_sched_attr attr = rq->gem_context->sched;
- struct intel_timeline * const tl = rq->timeline;
+ struct intel_timeline * const tl = i915_request_timeline(rq);
struct i915_request *prev;
lockdep_assert_held(&tl->mutex);
@@ -1271,7 +1275,9 @@ void i915_request_add(struct i915_request *rq)
* work on behalf of others -- but instead we should benefit from
* improved resource management. (Well, that's the theory at least.)
*/
- if (prev && i915_request_completed(prev) && prev->timeline == tl)
+ if (prev &&
+ i915_request_completed(prev) &&
+ rcu_access_pointer(prev->timeline) == tl)
i915_request_retire_upto(prev);
mutex_unlock(&tl->mutex);
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 8ac6e1226a56..b18f49528ded 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -113,7 +113,7 @@ struct i915_request {
struct intel_engine_cs *engine;
struct intel_context *hw_context;
struct intel_ring *ring;
- struct intel_timeline *timeline;
+ struct intel_timeline __rcu *timeline;
struct list_head signal_link;
/*
@@ -442,6 +442,26 @@ static inline bool i915_request_has_nopreempt(const struct i915_request *rq)
return unlikely(rq->flags & I915_REQUEST_NOPREEMPT);
}
+static inline struct intel_timeline *
+i915_request_timeline(struct i915_request *rq)
+{
+ /* Valid only while the request is being constructed (or retired). */
+ return rcu_dereference_protected(rq->timeline,
+ lockdep_is_held(&rcu_access_pointer(rq->timeline)->mutex));
+}
+
+static inline struct intel_timeline *
+i915_request_active_timeline(struct i915_request *rq)
+{
+ /*
+ * When in use during submission, we are protected by a guarantee that
+ * the context/timeline is pinned and must remain pinned until after
+ * this submission.
+ */
+ return rcu_dereference_protected(rq->timeline,
+ lockdep_is_held(&rq->engine->active.lock));
+}
+
bool i915_retire_requests(struct drm_i915_private *i915);
#endif /* I915_REQUEST_H */
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 411047d6a909..9d5b0f87c210 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -900,15 +900,13 @@ int i915_vma_move_to_active(struct i915_vma *vma,
* add the active reference first and queue for it to be dropped
* *last*.
*/
- err = i915_active_ref(&vma->active, rq->timeline, rq);
+ err = i915_active_add_request(&vma->active, rq);
if (unlikely(err))
return err;
if (flags & EXEC_OBJECT_WRITE) {
if (intel_frontbuffer_invalidate(obj->frontbuffer, ORIGIN_CS))
- i915_active_ref(&obj->frontbuffer->write,
- rq->timeline,
- rq);
+ i915_active_add_request(&obj->frontbuffer->write, rq);
dma_resv_add_excl_fence(vma->resv, &rq->fence);
obj->write_domain = I915_GEM_DOMAIN_RENDER;
diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
index 77d844ac8b71..afecfa081ff4 100644
--- a/drivers/gpu/drm/i915/selftests/i915_active.c
+++ b/drivers/gpu/drm/i915/selftests/i915_active.c
@@ -110,7 +110,7 @@ __live_active_setup(struct drm_i915_private *i915)
submit,
GFP_KERNEL);
if (err >= 0)
- err = i915_active_ref(&active->base, rq->timeline, rq);
+ err = i915_active_add_request(&active->base, rq);
i915_request_add(rq);
if (err) {
pr_err("Failed to track active ref!\n");
diff --git a/drivers/gpu/drm/i915/selftests/igt_spinner.c b/drivers/gpu/drm/i915/selftests/igt_spinner.c
index 11f04ad48e68..ee8450b871da 100644
--- a/drivers/gpu/drm/i915/selftests/igt_spinner.c
+++ b/drivers/gpu/drm/i915/selftests/igt_spinner.c
@@ -147,7 +147,7 @@ igt_spinner_create_request(struct igt_spinner *spin,
intel_gt_chipset_flush(engine->gt);
if (engine->emit_init_breadcrumb &&
- rq->timeline->has_initial_breadcrumb) {
+ i915_request_timeline(rq)->has_initial_breadcrumb) {
err = engine->emit_init_breadcrumb(rq);
if (err)
goto cancel_rq;
--
2.23.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] drm/i915: Lock signaler timeline while navigating
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
@ 2019-09-19 11:19 ` Chris Wilson
2019-09-19 11:19 ` [PATCH v2 3/3] drm/i915: Protect timeline->hwsp dereferencing Chris Wilson
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2019-09-19 11:19 UTC (permalink / raw)
To: intel-gfx
As we need to take a walk back along the signaler timeline to find the
fence before upon which we want to wait, we need to lock that timeline
to prevent it being modified as we walk. Similarly, we also need to
acquire a reference to the earlier fence while it still exists!
Though we lack the correct locking today, we are saved by the
overarching struct_mutex -- but that protection is being removed.
v2: Tvrtko made me realise I was being lax and using annotations to
ignore the AB-BA deadlock from the timeline overlap. As it would be
possible to construct a second request that was using a semaphore from the
same timeline as ourselves, we could quite easily end up in a situation
where we deadlocked in our mutex waits. Avoid that by using a trylock
and falling back to a normal dma-fence await if contended.
v3: Eek, the signal->timeline is volatile and must be carefully
dereferenced to ensure it is valid.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_request.c | 68 +++++++++++++++++++----------
1 file changed, 46 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index fb6f21c41934..9bd8538b1907 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -768,17 +768,43 @@ i915_request_create(struct intel_context *ce)
static int
i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
{
- if (list_is_first(&signal->link, &signal->timeline->requests))
- return 0;
+ struct intel_timeline *tl;
+ struct dma_fence *fence;
+ int err;
+
+ GEM_BUG_ON(i915_request_timeline(rq) ==
+ rcu_access_pointer(signal->timeline));
- signal = list_prev_entry(signal, link);
- if (intel_timeline_sync_is_later(i915_request_timeline(rq),
- &signal->fence))
+ rcu_read_lock();
+ tl = rcu_dereference(signal->timeline);
+ if (i915_request_started(signal) || !kref_get_unless_zero(&tl->kref))
+ tl = NULL;
+ rcu_read_unlock();
+ if (!tl) /* already started or maybe even completed */
return 0;
- return i915_sw_fence_await_dma_fence(&rq->submit,
- &signal->fence, 0,
- I915_FENCE_GFP);
+ fence = ERR_PTR(-EBUSY);
+ if (mutex_trylock(&tl->mutex)) {
+ fence = NULL;
+ if (!i915_request_started(signal) &&
+ !list_is_first(&signal->link, &tl->requests)) {
+ signal = list_prev_entry(signal, link);
+ fence = dma_fence_get(&signal->fence);
+ }
+ mutex_unlock(&tl->mutex);
+ }
+ intel_timeline_put(tl);
+ if (IS_ERR_OR_NULL(fence))
+ return PTR_ERR_OR_ZERO(fence);
+
+ err = 0;
+ if (intel_timeline_sync_is_later(i915_request_timeline(rq), fence))
+ err = i915_sw_fence_await_dma_fence(&rq->submit,
+ fence, 0,
+ I915_FENCE_GFP);
+ dma_fence_put(fence);
+
+ return err;
}
static intel_engine_mask_t
@@ -808,30 +834,23 @@ emit_semaphore_wait(struct i915_request *to,
u32 hwsp_offset;
int len;
u32 *cs;
- int err;
- GEM_BUG_ON(!from->timeline->has_initial_breadcrumb);
GEM_BUG_ON(INTEL_GEN(to->i915) < 8);
/* Just emit the first semaphore we see as request space is limited. */
if (already_busywaiting(to) & from->engine->mask)
- return i915_sw_fence_await_dma_fence(&to->submit,
- &from->fence, 0,
- I915_FENCE_GFP);
+ goto await_fence;
- err = i915_request_await_start(to, from);
- if (err < 0)
- return err;
+ if (i915_request_await_start(to, from) < 0)
+ goto await_fence;
/* Only submit our spinner after the signaler is running! */
- err = __i915_request_await_execution(to, from, NULL, gfp);
- if (err)
- return err;
+ if (__i915_request_await_execution(to, from, NULL, gfp))
+ goto await_fence;
/* We need to pin the signaler's HWSP until we are finished reading. */
- err = intel_timeline_read_hwsp(from, to, &hwsp_offset);
- if (err)
- return err;
+ if (intel_timeline_read_hwsp(from, to, &hwsp_offset))
+ goto await_fence;
len = 4;
if (has_token)
@@ -866,6 +885,11 @@ emit_semaphore_wait(struct i915_request *to,
to->sched.semaphores |= from->engine->mask;
to->sched.flags |= I915_SCHED_HAS_SEMAPHORE_CHAIN;
return 0;
+
+await_fence:
+ return i915_sw_fence_await_dma_fence(&to->submit,
+ &from->fence, 0,
+ I915_FENCE_GFP);
}
static int
--
2.23.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] drm/i915: Protect timeline->hwsp dereferencing
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
2019-09-19 11:19 ` [PATCH v2 2/3] drm/i915: Lock signaler timeline while navigating Chris Wilson
@ 2019-09-19 11:19 ` Chris Wilson
2019-09-19 12:22 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Patchwork
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2019-09-19 11:19 UTC (permalink / raw)
To: intel-gfx
As not only is the signal->timeline volatile, so will be acquiring the
timeline's HWSP. We must first carefully acquire the timeline from the
signaling request and then lock the timeline. With the removal of the
struct_mutex serialisation of request construction, we can have multiple
timelines active at once, and so we must avoid using the nested mutex
lock as it is quite possible for both timelines to be establishing
semaphores on the other and so deadlock.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gt/intel_timeline.c | 32 ++++++++++++++++++------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
index 115a24d4a20a..9d436e14ea8d 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline.c
+++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
@@ -500,17 +500,32 @@ int intel_timeline_read_hwsp(struct i915_request *from,
struct i915_request *to,
u32 *hwsp)
{
- struct intel_timeline_cacheline *cl = from->hwsp_cacheline;
- struct intel_timeline *tl = from->timeline;
+ struct intel_timeline *tl;
int err;
+ rcu_read_lock();
+ tl = rcu_dereference(from->timeline);
+ if (i915_request_completed(from) || !kref_get_unless_zero(&tl->kref))
+ tl = NULL;
+ rcu_read_unlock();
+ if (!tl) /* already completed */
+ return 1;
+
GEM_BUG_ON(rcu_access_pointer(to->timeline) == tl);
- mutex_lock_nested(&tl->mutex, SINGLE_DEPTH_NESTING);
- err = i915_request_completed(from);
- if (!err)
+ err = -EBUSY;
+ if (mutex_trylock(&tl->mutex)) {
+ struct intel_timeline_cacheline *cl = from->hwsp_cacheline;
+
+ if (i915_request_completed(from)) {
+ err = 1;
+ goto unlock;
+ }
+
err = cacheline_ref(cl, to);
- if (!err) {
+ if (err)
+ goto unlock;
+
if (likely(cl == tl->hwsp_cacheline)) {
*hwsp = tl->hwsp_offset;
} else { /* across a seqno wrap, recover the original offset */
@@ -518,8 +533,11 @@ int intel_timeline_read_hwsp(struct i915_request *from,
ptr_unmask_bits(cl->vaddr, CACHELINE_BITS) *
CACHELINE_BYTES;
}
+
+unlock:
+ mutex_unlock(&tl->mutex);
}
- mutex_unlock(&tl->mutex);
+ intel_timeline_put(tl);
return err;
}
--
2.23.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
2019-09-19 11:19 ` [PATCH v2 2/3] drm/i915: Lock signaler timeline while navigating Chris Wilson
2019-09-19 11:19 ` [PATCH v2 3/3] drm/i915: Protect timeline->hwsp dereferencing Chris Wilson
@ 2019-09-19 12:22 ` Patchwork
2019-09-19 12:23 ` ✗ Fi.CI.SPARSE: " Patchwork
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2019-09-19 12:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
URL : https://patchwork.freedesktop.org/series/66923/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
e13eda0cac6a drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
-:361: WARNING:LONG_LINE: line over 100 characters
#361: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:444:
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
-:372: WARNING:LONG_LINE: line over 100 characters
#372: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:464:
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
-:383: WARNING:LONG_LINE: line over 100 characters
#383: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:943:
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
-:394: WARNING:LONG_LINE: line over 100 characters
#394: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:966:
+ GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
-:509: WARNING:LONG_LINE: line over 100 characters
#509: FILE: drivers/gpu/drm/i915/gt/selftest_lrc.c:1092:
+ __i915_active_request_set(&i915_request_timeline(rq[i])->last_request,
total: 0 errors, 5 warnings, 0 checks, 573 lines checked
d80bab4e522c drm/i915: Lock signaler timeline while navigating
e3ec03a920da drm/i915: Protect timeline->hwsp dereferencing
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
` (2 preceding siblings ...)
2019-09-19 12:22 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Patchwork
@ 2019-09-19 12:23 ` Patchwork
2019-09-19 12:44 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2019-09-19 12:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
URL : https://patchwork.freedesktop.org/series/66923/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
+drivers/gpu/drm/i915/gt/intel_timeline.c:504:41: expected struct intel_timeline *tl
+drivers/gpu/drm/i915/gt/intel_timeline.c:504:41: got struct intel_timeline [noderef] <asn:4> *timeline
+drivers/gpu/drm/i915/gt/intel_timeline.c:504:41: warning: incorrect type in initializer (different address spaces)
+drivers/gpu/drm/i915/i915_request.c:771:49: expected struct list_head const *head
+drivers/gpu/drm/i915/i915_request.c:771:49: got struct list_head [noderef] <asn:4> *
+drivers/gpu/drm/i915/i915_request.c:771:49: warning: incorrect type in argument 2 (different address spaces)
+drivers/gpu/drm/i915/i915_request.c:813:9: warning: dereference of noderef expression
Commit: drm/i915: Lock signaler timeline while navigating
-O:drivers/gpu/drm/i915/i915_request.c:771:49: expected struct list_head const *head
-O:drivers/gpu/drm/i915/i915_request.c:771:49: got struct list_head [noderef] <asn:4> *
-O:drivers/gpu/drm/i915/i915_request.c:771:49: warning: incorrect type in argument 2 (different address spaces)
-O:drivers/gpu/drm/i915/i915_request.c:813:9: warning: dereference of noderef expression
+
Commit: drm/i915: Protect timeline->hwsp dereferencing
-O:drivers/gpu/drm/i915/gt/intel_timeline.c:504:41: expected struct intel_timeline *tl
-O:drivers/gpu/drm/i915/gt/intel_timeline.c:504:41: got struct intel_timeline [noderef] <asn:4> *timeline
-O:drivers/gpu/drm/i915/gt/intel_timeline.c:504:41: warning: incorrect type in initializer (different address spaces)
+
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
` (3 preceding siblings ...)
2019-09-19 12:23 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-09-19 12:44 ` Patchwork
2019-09-19 13:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
2019-09-19 20:09 ` ✓ Fi.CI.IGT: success for series starting with [v2,1/3] " Patchwork
6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2019-09-19 12:44 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
URL : https://patchwork.freedesktop.org/series/66923/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6919 -> Patchwork_14454
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/
Known issues
------------
Here are the changes found in Patchwork_14454 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@kms_addfb_basic@basic:
- fi-icl-u3: [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/fi-icl-u3/igt@kms_addfb_basic@basic.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/fi-icl-u3/igt@kms_addfb_basic@basic.html
#### Possible fixes ####
* igt@gem_close_race@basic-threads:
- fi-cml-u2: [INCOMPLETE][3] ([fdo#110566]) -> [PASS][4]
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/fi-cml-u2/igt@gem_close_race@basic-threads.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/fi-cml-u2/igt@gem_close_race@basic-threads.html
* igt@gem_mmap_gtt@basic-write-no-prefault:
- fi-icl-u3: [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6] +1 similar issue
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/fi-icl-u3/igt@gem_mmap_gtt@basic-write-no-prefault.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/fi-icl-u3/igt@gem_mmap_gtt@basic-write-no-prefault.html
* igt@i915_selftest@live_hangcheck:
- {fi-icl-guc}: [INCOMPLETE][7] ([fdo#107713] / [fdo#108569]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/fi-icl-guc/igt@i915_selftest@live_hangcheck.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/fi-icl-guc/igt@i915_selftest@live_hangcheck.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566
[fdo#111600]: https://bugs.freedesktop.org/show_bug.cgi?id=111600
[fdo#111739]: https://bugs.freedesktop.org/show_bug.cgi?id=111739
Participating hosts (53 -> 48)
------------------------------
Additional (1): fi-bsw-n3050
Missing (6): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_6919 -> Patchwork_14454
CI-20190529: 20190529
CI_DRM_6919: c36c0d42ee009514f713d0b51961c8e8fddfc641 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5192: 77c53210779c30cfb8a4ca2312675fe5be94f4d5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_14454: e3ec03a920da8604bfd0e12663cdcebf44490e01 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
e3ec03a920da drm/i915: Protect timeline->hwsp dereferencing
d80bab4e522c drm/i915: Lock signaler timeline while navigating
e13eda0cac6a drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
` (4 preceding siblings ...)
2019-09-19 12:44 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-09-19 13:02 ` Tvrtko Ursulin
2019-09-19 13:26 ` Chris Wilson
2019-09-19 20:09 ` ✓ Fi.CI.IGT: success for series starting with [v2,1/3] " Patchwork
6 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2019-09-19 13:02 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/09/2019 12:19, Chris Wilson wrote:
> The request->timeline is only valid until the request is retired (i.e.
> before it is completed). Upon retiring the request, the context may be
> unpinned and freed, and along with it the timeline may be freed. We
> therefore need to be very careful when chasing rq->timeline that the
> pointer does not disappear beneath us. The vast majority of users are in
> a protected context, either during request construction or retirement,
> where the timeline->mutex is held and the timeline cannot disappear. It
> is those few off the beaten path (where we access a second timeline) that
> need extra scrutiny -- to be added in the next patch after first adding
> the warnings about dangerous access.
>
> One complication, where we cannot use the timeline->mutex itself, is
> during request submission onto hardware (under spinlocks). Here, we want
> to check on the timeline to finalize the breadcrumb, and so we need to
> impose a second rule to ensure that the request->timeline is indeed
> valid. As we are submitting the request, it's context and timeline must
> be pinned, as it will be used by the hardware. Since it is pinned, we
> know the request->timeline must still be valid, and we cannot submit the
> idle barrier until after we release the engine->active.lock, ergo while
> submitting and holding that spinlock, a second thread cannot release the
> timeline.
>
> v2: Don't be lazy inside selftests; hold the timeline->mutex for as long
> as we need it, and tidy up acquiring the timeline with a bit of
> refactoring (i915_active_add_request)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
> .../gpu/drm/i915/gem/i915_gem_client_blt.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
> drivers/gpu/drm/i915/gt/intel_context.c | 4 +-
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 57 ++++++++++++++++---
> drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +-
> drivers/gpu/drm/i915/gt/intel_engine_pool.h | 2 +-
> drivers/gpu/drm/i915/gt/intel_lrc.c | 17 +++---
> drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 27 +++++----
> drivers/gpu/drm/i915/gt/intel_timeline.c | 6 +-
> .../gpu/drm/i915/gt/intel_timeline_types.h | 1 +
> drivers/gpu/drm/i915/gt/selftest_context.c | 18 ++++--
> drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +-
> drivers/gpu/drm/i915/i915_active.c | 2 +-
> drivers/gpu/drm/i915/i915_active.h | 6 ++
> drivers/gpu/drm/i915/i915_request.c | 28 +++++----
> drivers/gpu/drm/i915/i915_request.h | 22 ++++++-
> drivers/gpu/drm/i915/i915_vma.c | 6 +-
> drivers/gpu/drm/i915/selftests/i915_active.c | 2 +-
> drivers/gpu/drm/i915/selftests/igt_spinner.c | 2 +-
> 20 files changed, 147 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index 29edfc343716..5efef9babadb 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -230,7 +230,7 @@ alloc_request(struct intel_overlay *overlay, void (*fn)(struct intel_overlay *))
> if (IS_ERR(rq))
> return rq;
>
> - err = i915_active_ref(&overlay->last_flip, rq->timeline, rq);
> + err = i915_active_add_request(&overlay->last_flip, rq);
> if (err) {
> i915_request_add(rq);
> return ERR_PTR(err);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> index f99920652751..7f61a8024133 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> @@ -211,7 +211,7 @@ static void clear_pages_worker(struct work_struct *work)
> * keep track of the GPU activity within this vma/request, and
> * propagate the signal from the request to w->dma.
> */
> - err = i915_active_ref(&vma->active, rq->timeline, rq);
> + err = i915_active_add_request(&vma->active, rq);
> if (err)
> goto out_request;
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index f1c0e5d958f3..4a34c4f62065 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -910,7 +910,7 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> if (emit)
> err = emit(rq, data);
> if (err == 0)
> - err = i915_active_ref(&cb->base, rq->timeline, rq);
> + err = i915_active_add_request(&cb->base, rq);
>
> i915_request_add(rq);
> if (err)
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index c0495811f493..26cb838c272c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -298,7 +298,7 @@ int intel_context_prepare_remote_request(struct intel_context *ce,
> /* Only suitable for use in remotely modifying this context */
> GEM_BUG_ON(rq->hw_context == ce);
>
> - if (rq->timeline != tl) { /* beware timeline sharing */
> + if (rcu_access_pointer(rq->timeline) != tl) { /* timeline sharing! */
> err = mutex_lock_interruptible_nested(&tl->mutex,
> SINGLE_DEPTH_NESTING);
> if (err)
> @@ -319,7 +319,7 @@ int intel_context_prepare_remote_request(struct intel_context *ce,
> * words transfer the pinned ce object to tracked active request.
> */
> GEM_BUG_ON(i915_active_is_idle(&ce->active));
> - return i915_active_ref(&ce->active, rq->timeline, rq);
> + return i915_active_add_request(&ce->active, rq);
> }
>
> struct i915_request *intel_context_create_request(struct intel_context *ce)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 3c176b0f4b45..f451d5076bde 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -680,6 +680,8 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
> engine->status_page.vma))
> goto out_frame;
>
> + mutex_lock(&frame->timeline.mutex);
> +
> frame->ring.vaddr = frame->cs;
> frame->ring.size = sizeof(frame->cs);
> frame->ring.effective_size = frame->ring.size;
> @@ -688,18 +690,22 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
> frame->rq.i915 = engine->i915;
> frame->rq.engine = engine;
> frame->rq.ring = &frame->ring;
> - frame->rq.timeline = &frame->timeline;
> + rcu_assign_pointer(frame->rq.timeline, &frame->timeline);
>
> dw = intel_timeline_pin(&frame->timeline);
> if (dw < 0)
> goto out_timeline;
>
> + spin_lock_irq(&engine->active.lock);
> dw = engine->emit_fini_breadcrumb(&frame->rq, frame->cs) - frame->cs;
> + spin_unlock_irq(&engine->active.lock);
> +
> GEM_BUG_ON(dw & 1); /* RING_TAIL must be qword aligned */
>
> intel_timeline_unpin(&frame->timeline);
>
> out_timeline:
> + mutex_unlock(&frame->timeline.mutex);
> intel_timeline_fini(&frame->timeline);
> out_frame:
> kfree(frame);
> @@ -1196,6 +1202,27 @@ static void hexdump(struct drm_printer *m, const void *buf, size_t len)
> }
> }
>
> +static struct intel_timeline *get_timeline(struct i915_request *rq)
> +{
> + struct intel_timeline *tl;
> +
> + /*
> + * Even though we are holding the engine->active.lock here, there
> + * is no control over the submission queue per-se and we are
> + * inspecting the active state at a random point in time, with an
> + * unknown queue. Play safe and make sure the timeline remains valid.
> + * (Only being used for pretty printing, one extra kref shouldn't
> + * cause a camel stampede!)
> + */
> + rcu_read_lock();
> + tl = rcu_dereference(rq->timeline);
> + if (!kref_get_unless_zero(&tl->kref))
> + tl = NULL;
> + rcu_read_unlock();
How can it be NULL under the active lock? Isn't that the same assertion
from i915_timeline_get_active.
> +
> + return tl;
> +}
> +
> static void intel_engine_print_registers(struct intel_engine_cs *engine,
> struct drm_printer *m)
> {
> @@ -1290,27 +1317,37 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
> int len;
>
> len = snprintf(hdr, sizeof(hdr),
> - "\t\tActive[%d: ",
> + "\t\tActive[%d]: ",
> (int)(port - execlists->active));
> - if (!i915_request_signaled(rq))
> + if (!i915_request_signaled(rq)) {
> + struct intel_timeline *tl = get_timeline(rq);
> +
> len += snprintf(hdr + len, sizeof(hdr) - len,
> "ring:{start:%08x, hwsp:%08x, seqno:%08x}, ",
> i915_ggtt_offset(rq->ring->vma),
> - rq->timeline->hwsp_offset,
> + tl ? tl->hwsp_offset : 0,
> hwsp_seqno(rq));
> +
> + if (tl)
> + intel_timeline_put(tl);
> + }
> snprintf(hdr + len, sizeof(hdr) - len, "rq: ");
> print_request(m, rq, hdr);
> }
> for (port = execlists->pending; (rq = *port); port++) {
> + struct intel_timeline *tl = get_timeline(rq);
> char hdr[80];
>
> snprintf(hdr, sizeof(hdr),
> "\t\tPending[%d] ring:{start:%08x, hwsp:%08x, seqno:%08x}, rq: ",
> (int)(port - execlists->pending),
> i915_ggtt_offset(rq->ring->vma),
> - rq->timeline->hwsp_offset,
> + tl ? tl->hwsp_offset : 0,
> hwsp_seqno(rq));
> print_request(m, rq, hdr);
> +
> + if (tl)
> + intel_timeline_put(tl);
> }
> spin_unlock_irqrestore(&engine->active.lock, flags);
> } else if (INTEL_GEN(dev_priv) > 6) {
> @@ -1388,6 +1425,8 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> spin_lock_irqsave(&engine->active.lock, flags);
> rq = intel_engine_find_active_request(engine);
> if (rq) {
> + struct intel_timeline *tl = get_timeline(rq);
> +
> print_request(m, rq, "\t\tactive ");
>
> drm_printf(m, "\t\tring->start: 0x%08x\n",
> @@ -1400,8 +1439,12 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> rq->ring->emit);
> drm_printf(m, "\t\tring->space: 0x%08x\n",
> rq->ring->space);
> - drm_printf(m, "\t\tring->hwsp: 0x%08x\n",
> - rq->timeline->hwsp_offset);
> +
> + if (tl) {
> + drm_printf(m, "\t\tring->hwsp: 0x%08x\n",
> + tl->hwsp_offset);
> + intel_timeline_put(tl);
> + }
>
> print_request_ring(m, rq);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 65b5ca74b394..ce61c83d68e9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -103,7 +103,7 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine)
> /* Context switch failed, hope for the best! Maybe reset? */
> goto out_unlock;
>
> - intel_timeline_enter(rq->timeline);
> + intel_timeline_enter(i915_request_timeline(rq));
>
> /* Check again on the next retirement. */
> engine->wakeref_serial = engine->serial + 1;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pool.h b/drivers/gpu/drm/i915/gt/intel_engine_pool.h
> index 7e2123b33594..1bd89cadc3b7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pool.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pool.h
> @@ -18,7 +18,7 @@ static inline int
> intel_engine_pool_mark_active(struct intel_engine_pool_node *node,
> struct i915_request *rq)
> {
> - return i915_active_ref(&node->active, rq->timeline, rq);
> + return i915_active_add_request(&node->active, rq);
> }
>
> static inline void
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index a99166a2d2eb..ac21587b75f6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1825,7 +1825,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq)
> {
> u32 *cs;
>
> - GEM_BUG_ON(!rq->timeline->has_initial_breadcrumb);
> + GEM_BUG_ON(!i915_request_timeline(rq)->has_initial_breadcrumb);
>
> cs = intel_ring_begin(rq, 6);
> if (IS_ERR(cs))
> @@ -1841,7 +1841,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq)
> *cs++ = MI_NOOP;
>
> *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
> - *cs++ = rq->timeline->hwsp_offset;
> + *cs++ = i915_request_timeline(rq)->hwsp_offset;
> *cs++ = 0;
> *cs++ = rq->fence.seqno - 1;
>
> @@ -2324,7 +2324,8 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
>
> static struct i915_request *active_request(struct i915_request *rq)
> {
> - const struct list_head * const list = &rq->timeline->requests;
> + const struct list_head * const list =
> + &i915_request_active_timeline(rq)->requests;
> const struct intel_context * const ce = rq->hw_context;
> struct i915_request *active = NULL;
>
> @@ -2855,7 +2856,7 @@ static u32 *gen8_emit_fini_breadcrumb(struct i915_request *request, u32 *cs)
> {
> cs = gen8_emit_ggtt_write(cs,
> request->fence.seqno,
> - request->timeline->hwsp_offset,
> + i915_request_active_timeline(request)->hwsp_offset,
> 0);
>
> return gen8_emit_fini_breadcrumb_footer(request, cs);
> @@ -2872,7 +2873,7 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
> /* XXX flush+write+CS_STALL all in one upsets gem_concurrent_blt:kbl */
> cs = gen8_emit_ggtt_write_rcs(cs,
> request->fence.seqno,
> - request->timeline->hwsp_offset,
> + i915_request_active_timeline(request)->hwsp_offset,
> PIPE_CONTROL_FLUSH_ENABLE |
> PIPE_CONTROL_CS_STALL);
>
> @@ -2884,7 +2885,7 @@ gen11_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
> {
> cs = gen8_emit_ggtt_write_rcs(cs,
> request->fence.seqno,
> - request->timeline->hwsp_offset,
> + i915_request_active_timeline(request)->hwsp_offset,
> PIPE_CONTROL_CS_STALL |
> PIPE_CONTROL_TILE_CACHE_FLUSH |
> PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH |
> @@ -2948,7 +2949,7 @@ static u32 *gen12_emit_fini_breadcrumb(struct i915_request *request, u32 *cs)
> {
> cs = gen8_emit_ggtt_write(cs,
> request->fence.seqno,
> - request->timeline->hwsp_offset,
> + i915_request_active_timeline(request)->hwsp_offset,
> 0);
>
> return gen12_emit_fini_breadcrumb_footer(request, cs);
> @@ -2959,7 +2960,7 @@ gen12_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
> {
> cs = gen8_emit_ggtt_write_rcs(cs,
> request->fence.seqno,
> - request->timeline->hwsp_offset,
> + i915_request_active_timeline(request)->hwsp_offset,
> PIPE_CONTROL_CS_STALL |
> PIPE_CONTROL_TILE_CACHE_FLUSH |
> PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH |
> diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> index a25b84b12ef1..0747b8c9f768 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> @@ -322,7 +322,8 @@ static u32 *gen6_rcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> PIPE_CONTROL_DC_FLUSH_ENABLE |
> PIPE_CONTROL_QW_WRITE |
> PIPE_CONTROL_CS_STALL);
> - *cs++ = rq->timeline->hwsp_offset | PIPE_CONTROL_GLOBAL_GTT;
> + *cs++ = i915_request_active_timeline(rq)->hwsp_offset |
> + PIPE_CONTROL_GLOBAL_GTT;
> *cs++ = rq->fence.seqno;
>
> *cs++ = MI_USER_INTERRUPT;
> @@ -425,7 +426,7 @@ static u32 *gen7_rcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> PIPE_CONTROL_QW_WRITE |
> PIPE_CONTROL_GLOBAL_GTT_IVB |
> PIPE_CONTROL_CS_STALL);
> - *cs++ = rq->timeline->hwsp_offset;
> + *cs++ = i915_request_active_timeline(rq)->hwsp_offset;
> *cs++ = rq->fence.seqno;
>
> *cs++ = MI_USER_INTERRUPT;
> @@ -439,8 +440,8 @@ static u32 *gen7_rcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
>
> static u32 *gen6_xcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> {
> - GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
> - GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
> + GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
> + GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
>
> *cs++ = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_DW_STORE_INDEX;
> *cs++ = I915_GEM_HWS_SEQNO_ADDR | MI_FLUSH_DW_USE_GTT;
> @@ -459,8 +460,8 @@ static u32 *gen7_xcs_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> {
> int i;
>
> - GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
> - GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
> + GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
> + GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
>
> *cs++ = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_DW_STORE_INDEX;
> *cs++ = I915_GEM_HWS_SEQNO_ADDR | MI_FLUSH_DW_USE_GTT;
> @@ -938,8 +939,8 @@ static void i9xx_submit_request(struct i915_request *request)
>
> static u32 *i9xx_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> {
> - GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
> - GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
> + GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
> + GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
>
> *cs++ = MI_FLUSH;
>
> @@ -961,8 +962,8 @@ static u32 *gen5_emit_breadcrumb(struct i915_request *rq, u32 *cs)
> {
> int i;
>
> - GEM_BUG_ON(rq->timeline->hwsp_ggtt != rq->engine->status_page.vma);
> - GEM_BUG_ON(offset_in_page(rq->timeline->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
> + GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma);
> + GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR);
>
> *cs++ = MI_FLUSH;
>
> @@ -1815,7 +1816,7 @@ static int ring_request_alloc(struct i915_request *request)
> int ret;
>
> GEM_BUG_ON(!intel_context_is_pinned(request->hw_context));
> - GEM_BUG_ON(request->timeline->has_initial_breadcrumb);
> + GEM_BUG_ON(i915_request_timeline(request)->has_initial_breadcrumb);
>
> /*
> * Flush enough space to reduce the likelihood of waiting after
> @@ -1926,7 +1927,9 @@ u32 *intel_ring_begin(struct i915_request *rq, unsigned int num_dwords)
> */
> GEM_BUG_ON(!rq->reserved_space);
>
> - ret = wait_for_space(ring, rq->timeline, total_bytes);
> + ret = wait_for_space(ring,
> + i915_request_timeline(rq),
> + total_bytes);
> if (unlikely(ret))
> return ERR_PTR(ret);
> }
> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
> index 9cb01d9828f1..115a24d4a20a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
> @@ -493,7 +493,7 @@ int intel_timeline_get_seqno(struct intel_timeline *tl,
> static int cacheline_ref(struct intel_timeline_cacheline *cl,
> struct i915_request *rq)
> {
> - return i915_active_ref(&cl->active, rq->timeline, rq);
> + return i915_active_add_request(&cl->active, rq);
> }
>
> int intel_timeline_read_hwsp(struct i915_request *from,
> @@ -504,7 +504,7 @@ int intel_timeline_read_hwsp(struct i915_request *from,
> struct intel_timeline *tl = from->timeline;
> int err;
>
> - GEM_BUG_ON(to->timeline == tl);
> + GEM_BUG_ON(rcu_access_pointer(to->timeline) == tl);
>
> mutex_lock_nested(&tl->mutex, SINGLE_DEPTH_NESTING);
> err = i915_request_completed(from);
> @@ -541,7 +541,7 @@ void __intel_timeline_free(struct kref *kref)
> container_of(kref, typeof(*timeline), kref);
>
> intel_timeline_fini(timeline);
> - kfree(timeline);
> + kfree_rcu(timeline, rcu);
> }
>
> static void timelines_fini(struct intel_gt *gt)
> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> index 2b1baf2fcc8e..c668c4c50e75 100644
> --- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> @@ -80,6 +80,7 @@ struct intel_timeline {
> struct intel_gt *gt;
>
> struct kref kref;
> + struct rcu_head rcu;
> };
>
> #endif /* __I915_TIMELINE_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
> index 9d1ea26c7a2d..4ce1e25433d2 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> @@ -14,22 +14,28 @@
>
> static int request_sync(struct i915_request *rq)
> {
> + struct intel_timeline *tl = i915_request_timeline(rq);
> long timeout;
> int err = 0;
>
> + intel_timeline_get(tl);
> i915_request_get(rq);
>
> - i915_request_add(rq);
> + /* Opencode i915_request_add() so we can keep the timeline locked. */
> + __i915_request_commit(rq);
> + __i915_request_queue(rq, NULL);
Looking at i915_request_add here we also have tasklet kicking but I
guess for selftests it's not important.
> +
> timeout = i915_request_wait(rq, 0, HZ / 10);
> - if (timeout < 0) {
> + if (timeout < 0)
> err = timeout;
> - } else {
> - mutex_lock(&rq->timeline->mutex);
> + else
> i915_request_retire_upto(rq);
> - mutex_unlock(&rq->timeline->mutex);
> - }
> +
> + lockdep_unpin_lock(&tl->mutex, rq->cookie);
> + mutex_unlock(&tl->mutex);
>
> i915_request_put(rq);
> + intel_timeline_put(tl);
>
> return err;
> }
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 26d05bd1bdc8..93a871bfd95d 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -1089,7 +1089,7 @@ static int live_suppress_wait_preempt(void *arg)
> }
>
> /* Disable NEWCLIENT promotion */
> - __i915_active_request_set(&rq[i]->timeline->last_request,
> + __i915_active_request_set(&i915_request_timeline(rq[i])->last_request,
> dummy);
> i915_request_add(rq[i]);
> }
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 6a447f1d0110..d5aac6ff803a 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -695,7 +695,7 @@ void i915_request_add_active_barriers(struct i915_request *rq)
> struct llist_node *node, *next;
>
> GEM_BUG_ON(intel_engine_is_virtual(engine));
> - GEM_BUG_ON(rq->timeline != engine->kernel_context->timeline);
> + GEM_BUG_ON(i915_request_timeline(rq) != engine->kernel_context->timeline);
>
> /*
> * Attach the list of proto-fences to the in-flight request such
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index f95058f99057..949c6835335b 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -373,6 +373,12 @@ int i915_active_ref(struct i915_active *ref,
> struct intel_timeline *tl,
> struct i915_request *rq);
>
> +static inline int
> +i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
> +{
> + return i915_active_ref(ref, i915_request_timeline(rq), rq);
> +}
> +
> int i915_active_wait(struct i915_active *ref);
>
> int i915_request_await_active(struct i915_request *rq,
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 606acde47fa0..fb6f21c41934 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -220,7 +220,6 @@ static bool i915_request_retire(struct i915_request *rq)
> {
> struct i915_active_request *active, *next;
>
> - lockdep_assert_held(&rq->timeline->mutex);
> if (!i915_request_completed(rq))
> return false;
>
> @@ -241,7 +240,8 @@ static bool i915_request_retire(struct i915_request *rq)
> * Note this requires that we are always called in request
> * completion order.
> */
> - GEM_BUG_ON(!list_is_first(&rq->link, &rq->timeline->requests));
> + GEM_BUG_ON(!list_is_first(&rq->link,
> + &i915_request_timeline(rq)->requests));
> rq->ring->head = rq->postfix;
>
> /*
> @@ -317,7 +317,7 @@ static bool i915_request_retire(struct i915_request *rq)
>
> void i915_request_retire_upto(struct i915_request *rq)
> {
> - struct intel_timeline * const tl = rq->timeline;
> + struct intel_timeline * const tl = i915_request_timeline(rq);
> struct i915_request *tmp;
>
> GEM_TRACE("%s fence %llx:%lld, current %d\n",
> @@ -325,7 +325,6 @@ void i915_request_retire_upto(struct i915_request *rq)
> rq->fence.context, rq->fence.seqno,
> hwsp_seqno(rq));
>
> - lockdep_assert_held(&tl->mutex);
> GEM_BUG_ON(!i915_request_completed(rq));
>
> do {
> @@ -661,9 +660,11 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
> rq->gem_context = ce->gem_context;
> rq->engine = ce->engine;
> rq->ring = ce->ring;
> - rq->timeline = tl;
> +
> + rcu_assign_pointer(rq->timeline, tl);
> rq->hwsp_seqno = tl->hwsp_seqno;
> rq->hwsp_cacheline = tl->hwsp_cacheline;
> +
> rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>
> spin_lock_init(&rq->lock);
> @@ -771,7 +772,8 @@ i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
> return 0;
>
> signal = list_prev_entry(signal, link);
> - if (intel_timeline_sync_is_later(rq->timeline, &signal->fence))
> + if (intel_timeline_sync_is_later(i915_request_timeline(rq),
> + &signal->fence))
> return 0;
>
> return i915_sw_fence_await_dma_fence(&rq->submit,
> @@ -947,7 +949,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>
> /* Squash repeated waits to the same timelines */
> if (fence->context &&
> - intel_timeline_sync_is_later(rq->timeline, fence))
> + intel_timeline_sync_is_later(i915_request_timeline(rq),
> + fence))
> continue;
>
> if (dma_fence_is_i915(fence))
> @@ -961,7 +964,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>
> /* Record the latest fence used against each timeline */
> if (fence->context)
> - intel_timeline_sync_set(rq->timeline, fence);
> + intel_timeline_sync_set(i915_request_timeline(rq),
> + fence);
> } while (--nchild);
>
> return 0;
> @@ -1103,7 +1107,7 @@ void i915_request_skip(struct i915_request *rq, int error)
> static struct i915_request *
> __i915_request_add_to_timeline(struct i915_request *rq)
> {
> - struct intel_timeline *timeline = rq->timeline;
> + struct intel_timeline *timeline = i915_request_timeline(rq);
> struct i915_request *prev;
>
> /*
> @@ -1216,7 +1220,7 @@ void __i915_request_queue(struct i915_request *rq,
> void i915_request_add(struct i915_request *rq)
> {
> struct i915_sched_attr attr = rq->gem_context->sched;
> - struct intel_timeline * const tl = rq->timeline;
> + struct intel_timeline * const tl = i915_request_timeline(rq);
> struct i915_request *prev;
>
> lockdep_assert_held(&tl->mutex);
> @@ -1271,7 +1275,9 @@ void i915_request_add(struct i915_request *rq)
> * work on behalf of others -- but instead we should benefit from
> * improved resource management. (Well, that's the theory at least.)
> */
> - if (prev && i915_request_completed(prev) && prev->timeline == tl)
> + if (prev &&
> + i915_request_completed(prev) &&
> + rcu_access_pointer(prev->timeline) == tl)
> i915_request_retire_upto(prev);
>
> mutex_unlock(&tl->mutex);
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index 8ac6e1226a56..b18f49528ded 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -113,7 +113,7 @@ struct i915_request {
> struct intel_engine_cs *engine;
> struct intel_context *hw_context;
> struct intel_ring *ring;
> - struct intel_timeline *timeline;
> + struct intel_timeline __rcu *timeline;
> struct list_head signal_link;
>
> /*
> @@ -442,6 +442,26 @@ static inline bool i915_request_has_nopreempt(const struct i915_request *rq)
> return unlikely(rq->flags & I915_REQUEST_NOPREEMPT);
> }
>
> +static inline struct intel_timeline *
> +i915_request_timeline(struct i915_request *rq)
> +{
> + /* Valid only while the request is being constructed (or retired). */
> + return rcu_dereference_protected(rq->timeline,
> + lockdep_is_held(&rcu_access_pointer(rq->timeline)->mutex));
> +}
> +
> +static inline struct intel_timeline *
> +i915_request_active_timeline(struct i915_request *rq)
> +{
> + /*
> + * When in use during submission, we are protected by a guarantee that
> + * the context/timeline is pinned and must remain pinned until after
> + * this submission.
> + */
> + return rcu_dereference_protected(rq->timeline,
> + lockdep_is_held(&rq->engine->active.lock));
> +}
> +
> bool i915_retire_requests(struct drm_i915_private *i915);
>
> #endif /* I915_REQUEST_H */
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 411047d6a909..9d5b0f87c210 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -900,15 +900,13 @@ int i915_vma_move_to_active(struct i915_vma *vma,
> * add the active reference first and queue for it to be dropped
> * *last*.
> */
> - err = i915_active_ref(&vma->active, rq->timeline, rq);
> + err = i915_active_add_request(&vma->active, rq);
> if (unlikely(err))
> return err;
>
> if (flags & EXEC_OBJECT_WRITE) {
> if (intel_frontbuffer_invalidate(obj->frontbuffer, ORIGIN_CS))
> - i915_active_ref(&obj->frontbuffer->write,
> - rq->timeline,
> - rq);
> + i915_active_add_request(&obj->frontbuffer->write, rq);
>
> dma_resv_add_excl_fence(vma->resv, &rq->fence);
> obj->write_domain = I915_GEM_DOMAIN_RENDER;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
> index 77d844ac8b71..afecfa081ff4 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_active.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_active.c
> @@ -110,7 +110,7 @@ __live_active_setup(struct drm_i915_private *i915)
> submit,
> GFP_KERNEL);
> if (err >= 0)
> - err = i915_active_ref(&active->base, rq->timeline, rq);
> + err = i915_active_add_request(&active->base, rq);
> i915_request_add(rq);
> if (err) {
> pr_err("Failed to track active ref!\n");
> diff --git a/drivers/gpu/drm/i915/selftests/igt_spinner.c b/drivers/gpu/drm/i915/selftests/igt_spinner.c
> index 11f04ad48e68..ee8450b871da 100644
> --- a/drivers/gpu/drm/i915/selftests/igt_spinner.c
> +++ b/drivers/gpu/drm/i915/selftests/igt_spinner.c
> @@ -147,7 +147,7 @@ igt_spinner_create_request(struct igt_spinner *spin,
> intel_gt_chipset_flush(engine->gt);
>
> if (engine->emit_init_breadcrumb &&
> - rq->timeline->has_initial_breadcrumb) {
> + i915_request_timeline(rq)->has_initial_breadcrumb) {
> err = engine->emit_init_breadcrumb(rq);
> if (err)
> goto cancel_rq;
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 13:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
@ 2019-09-19 13:26 ` Chris Wilson
2019-09-19 17:11 ` Tvrtko Ursulin
0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2019-09-19 13:26 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-09-19 14:02:19)
>
> On 19/09/2019 12:19, Chris Wilson wrote:
> > +static struct intel_timeline *get_timeline(struct i915_request *rq)
> > +{
> > + struct intel_timeline *tl;
> > +
> > + /*
> > + * Even though we are holding the engine->active.lock here, there
> > + * is no control over the submission queue per-se and we are
> > + * inspecting the active state at a random point in time, with an
> > + * unknown queue. Play safe and make sure the timeline remains valid.
> > + * (Only being used for pretty printing, one extra kref shouldn't
> > + * cause a camel stampede!)
> > + */
> > + rcu_read_lock();
> > + tl = rcu_dereference(rq->timeline);
> > + if (!kref_get_unless_zero(&tl->kref))
> > + tl = NULL;
> > + rcu_read_unlock();
>
> How can it be NULL under the active lock? Isn't that the same assertion
> from i915_timeline_get_active.
Not NULL, but retired. The difference is that during submission we know
that this request's context/timeline must be currently pinned until
a subsequent request (containing the idle-barriers) is submitted. The
danger I worry about here is that subsequent idle request may be already
submitted and since the queued requests may *already* have been retired,
the timeline may be unpinned and indeed dropped it's last reference.
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
> > index 9d1ea26c7a2d..4ce1e25433d2 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> > @@ -14,22 +14,28 @@
> >
> > static int request_sync(struct i915_request *rq)
> > {
> > + struct intel_timeline *tl = i915_request_timeline(rq);
> > long timeout;
> > int err = 0;
> >
> > + intel_timeline_get(tl);
> > i915_request_get(rq);
> >
> > - i915_request_add(rq);
> > + /* Opencode i915_request_add() so we can keep the timeline locked. */
> > + __i915_request_commit(rq);
> > + __i915_request_queue(rq, NULL);
>
> Looking at i915_request_add here we also have tasklet kicking but I
> guess for selftests it's not important.
Yup, and immediately before a wait, that tasklet should be scheduled
naturally in the near future.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 13:26 ` Chris Wilson
@ 2019-09-19 17:11 ` Tvrtko Ursulin
2019-09-19 17:49 ` Chris Wilson
0 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2019-09-19 17:11 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/09/2019 14:26, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-19 14:02:19)
>>
>> On 19/09/2019 12:19, Chris Wilson wrote:
>>> +static struct intel_timeline *get_timeline(struct i915_request *rq)
>>> +{
>>> + struct intel_timeline *tl;
>>> +
>>> + /*
>>> + * Even though we are holding the engine->active.lock here, there
>>> + * is no control over the submission queue per-se and we are
>>> + * inspecting the active state at a random point in time, with an
>>> + * unknown queue. Play safe and make sure the timeline remains valid.
>>> + * (Only being used for pretty printing, one extra kref shouldn't
>>> + * cause a camel stampede!)
>>> + */
>>> + rcu_read_lock();
>>> + tl = rcu_dereference(rq->timeline);
>>> + if (!kref_get_unless_zero(&tl->kref))
>>> + tl = NULL;
>>> + rcu_read_unlock();
>>
>> How can it be NULL under the active lock? Isn't that the same assertion
>> from i915_timeline_get_active.
>
> Not NULL, but retired. The difference is that during submission we know
> that this request's context/timeline must be currently pinned until
> a subsequent request (containing the idle-barriers) is submitted. The
> danger I worry about here is that subsequent idle request may be already
> submitted and since the queued requests may *already* have been retired,
> the timeline may be unpinned and indeed dropped it's last reference.
But here it is under the engine->active.lock with interrupts disabled
and the requests are fetched from execlists ports. Timeline is not
guaranteed to be kept alive under these conditions? intel_context
reference will be held until process_csb schedules it out so I'd expect
timeline and hwsp to be there. But I could be lost in the new scheme of
things.
Regards,
Tvrtko
>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
>>> index 9d1ea26c7a2d..4ce1e25433d2 100644
>>> --- a/drivers/gpu/drm/i915/gt/selftest_context.c
>>> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
>>> @@ -14,22 +14,28 @@
>>>
>>> static int request_sync(struct i915_request *rq)
>>> {
>>> + struct intel_timeline *tl = i915_request_timeline(rq);
>>> long timeout;
>>> int err = 0;
>>>
>>> + intel_timeline_get(tl);
>>> i915_request_get(rq);
>>>
>>> - i915_request_add(rq);
>>> + /* Opencode i915_request_add() so we can keep the timeline locked. */
>>> + __i915_request_commit(rq);
>>> + __i915_request_queue(rq, NULL);
>>
>> Looking at i915_request_add here we also have tasklet kicking but I
>> guess for selftests it's not important.
>
> Yup, and immediately before a wait, that tasklet should be scheduled
> naturally in the near future.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 17:11 ` Tvrtko Ursulin
@ 2019-09-19 17:49 ` Chris Wilson
2019-09-20 8:47 ` Tvrtko Ursulin
0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2019-09-19 17:49 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-09-19 18:11:14)
>
> On 19/09/2019 14:26, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-09-19 14:02:19)
> >>
> >> On 19/09/2019 12:19, Chris Wilson wrote:
> >>> +static struct intel_timeline *get_timeline(struct i915_request *rq)
> >>> +{
> >>> + struct intel_timeline *tl;
> >>> +
> >>> + /*
> >>> + * Even though we are holding the engine->active.lock here, there
> >>> + * is no control over the submission queue per-se and we are
> >>> + * inspecting the active state at a random point in time, with an
> >>> + * unknown queue. Play safe and make sure the timeline remains valid.
> >>> + * (Only being used for pretty printing, one extra kref shouldn't
> >>> + * cause a camel stampede!)
> >>> + */
> >>> + rcu_read_lock();
> >>> + tl = rcu_dereference(rq->timeline);
> >>> + if (!kref_get_unless_zero(&tl->kref))
> >>> + tl = NULL;
> >>> + rcu_read_unlock();
> >>
> >> How can it be NULL under the active lock? Isn't that the same assertion
> >> from i915_timeline_get_active.
> >
> > Not NULL, but retired. The difference is that during submission we know
> > that this request's context/timeline must be currently pinned until
> > a subsequent request (containing the idle-barriers) is submitted. The
> > danger I worry about here is that subsequent idle request may be already
> > submitted and since the queued requests may *already* have been retired,
> > the timeline may be unpinned and indeed dropped it's last reference.
>
> But here it is under the engine->active.lock with interrupts disabled
> and the requests are fetched from execlists ports. Timeline is not
> guaranteed to be kept alive under these conditions? intel_context
> reference will be held until process_csb schedules it out so I'd expect
> timeline and hwsp to be there. But I could be lost in the new scheme of
> things.
I felt it was prudent to only rely on the active pin. You are right in
that we have a context reference if it is in active, and that context
holds a reference to the timeline. But... engine->active.lock is not
the lock that guards rq->timeline, so I feel uneasy on extending
i915_request_active_timeline() too far. Outside of the submission
pathway, inside a pretty printer, it feels safer (whatever changes may
come we don't have to worry about it) to not assume anything and just
use the failsafe rcu_dereference() + kref.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
` (5 preceding siblings ...)
2019-09-19 13:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
@ 2019-09-19 20:09 ` Patchwork
6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2019-09-19 20:09 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
URL : https://patchwork.freedesktop.org/series/66923/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6919_full -> Patchwork_14454_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_14454_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_schedule@preemptive-hang-bsd:
- shard-iclb: [PASS][1] -> [SKIP][2] ([fdo#111325]) +8 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb5/igt@gem_exec_schedule@preemptive-hang-bsd.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb2/igt@gem_exec_schedule@preemptive-hang-bsd.html
* igt@kms_cursor_crc@pipe-b-cursor-64x64-random:
- shard-apl: [PASS][3] -> [INCOMPLETE][4] ([fdo#103927]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-64x64-random.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-apl1/igt@kms_cursor_crc@pipe-b-cursor-64x64-random.html
* igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic:
- shard-skl: [PASS][5] -> [DMESG-WARN][6] ([fdo#105541])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-skl4/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-skl5/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html
* igt@kms_flip@flip-vs-expired-vblank:
- shard-skl: [PASS][7] -> [FAIL][8] ([fdo#105363])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-skl7/igt@kms_flip@flip-vs-expired-vblank.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-skl7/igt@kms_flip@flip-vs-expired-vblank.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt:
- shard-iclb: [PASS][9] -> [FAIL][10] ([fdo#103167]) +2 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: [PASS][11] -> [FAIL][12] ([fdo#108145] / [fdo#110403])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
* igt@kms_psr2_su@frontbuffer:
- shard-iclb: [PASS][13] -> [SKIP][14] ([fdo#109642] / [fdo#111068])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb6/igt@kms_psr2_su@frontbuffer.html
* igt@kms_psr@psr2_no_drrs:
- shard-iclb: [PASS][15] -> [SKIP][16] ([fdo#109441])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb2/igt@kms_psr@psr2_no_drrs.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb6/igt@kms_psr@psr2_no_drrs.html
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-apl: [PASS][17] -> [DMESG-WARN][18] ([fdo#108566]) +6 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-apl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-apl6/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
* igt@perf@blocking:
- shard-skl: [PASS][19] -> [FAIL][20] ([fdo#110728])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-skl1/igt@perf@blocking.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-skl6/igt@perf@blocking.html
* igt@prime_busy@hang-bsd2:
- shard-iclb: [PASS][21] -> [SKIP][22] ([fdo#109276]) +13 similar issues
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb1/igt@prime_busy@hang-bsd2.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb7/igt@prime_busy@hang-bsd2.html
#### Possible fixes ####
* igt@gem_exec_balancer@smoke:
- shard-iclb: [SKIP][23] ([fdo#110854]) -> [PASS][24]
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb6/igt@gem_exec_balancer@smoke.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb1/igt@gem_exec_balancer@smoke.html
* igt@gem_exec_schedule@preempt-other-chain-bsd:
- shard-iclb: [SKIP][25] ([fdo#111325]) -> [PASS][26] +6 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb6/igt@gem_exec_schedule@preempt-other-chain-bsd.html
* igt@gem_exec_schedule@preempt-queue-bsd1:
- shard-iclb: [SKIP][27] ([fdo#109276]) -> [PASS][28] +21 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb6/igt@gem_exec_schedule@preempt-queue-bsd1.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd1.html
* igt@gem_partial_pwrite_pread@write-display:
- shard-iclb: [INCOMPLETE][29] ([fdo#107713] / [fdo#109100]) -> [PASS][30]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb7/igt@gem_partial_pwrite_pread@write-display.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb7/igt@gem_partial_pwrite_pread@write-display.html
* igt@gem_pwrite@display:
- shard-apl: [INCOMPLETE][31] ([fdo#103927]) -> [PASS][32] +2 similar issues
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-apl6/igt@gem_pwrite@display.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-apl1/igt@gem_pwrite@display.html
* igt@i915_suspend@fence-restore-tiled2untiled:
- shard-apl: [DMESG-WARN][33] ([fdo#108566]) -> [PASS][34] +2 similar issues
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-apl2/igt@i915_suspend@fence-restore-tiled2untiled.html
* igt@kms_color@pipe-b-degamma:
- shard-skl: [FAIL][35] ([fdo#104782]) -> [PASS][36]
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-skl4/igt@kms_color@pipe-b-degamma.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-skl8/igt@kms_color@pipe-b-degamma.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move:
- shard-kbl: [FAIL][37] ([fdo#103167]) -> [PASS][38]
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-kbl3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-kbl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move.html
* igt@kms_frontbuffer_tracking@fbc-1p-rte:
- shard-iclb: [FAIL][39] ([fdo#103167] / [fdo#110378]) -> [PASS][40] +1 similar issue
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
- shard-iclb: [FAIL][41] ([fdo#103167]) -> [PASS][42] +7 similar issues
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
* igt@kms_psr@no_drrs:
- shard-iclb: [FAIL][43] ([fdo#108341]) -> [PASS][44]
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb1/igt@kms_psr@no_drrs.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb5/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_sprite_plane_move:
- shard-iclb: [SKIP][45] ([fdo#109441]) -> [PASS][46] +1 similar issue
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb5/igt@kms_psr@psr2_sprite_plane_move.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
* igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
- shard-skl: [INCOMPLETE][47] ([fdo#104108]) -> [PASS][48]
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-skl8/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-skl3/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
* igt@tools_test@tools_test:
- shard-apl: [SKIP][49] ([fdo#109271]) -> [PASS][50]
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-apl1/igt@tools_test@tools_test.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-apl7/igt@tools_test@tools_test.html
#### Warnings ####
* igt@gem_mocs_settings@mocs-isolation-bsd2:
- shard-iclb: [FAIL][51] ([fdo#111330]) -> [SKIP][52] ([fdo#109276])
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6919/shard-iclb1/igt@gem_mocs_settings@mocs-isolation-bsd2.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/shard-iclb7/igt@gem_mocs_settings@mocs-isolation-bsd2.html
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#110378]: https://bugs.freedesktop.org/show_bug.cgi?id=110378
[fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
[fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
[fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
[fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_6919 -> Patchwork_14454
CI-20190529: 20190529
CI_DRM_6919: c36c0d42ee009514f713d0b51961c8e8fddfc641 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5192: 77c53210779c30cfb8a4ca2312675fe5be94f4d5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_14454: e3ec03a920da8604bfd0e12663cdcebf44490e01 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14454/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer
2019-09-19 17:49 ` Chris Wilson
@ 2019-09-20 8:47 ` Tvrtko Ursulin
0 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2019-09-20 8:47 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/09/2019 18:49, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-19 18:11:14)
>>
>> On 19/09/2019 14:26, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-09-19 14:02:19)
>>>>
>>>> On 19/09/2019 12:19, Chris Wilson wrote:
>>>>> +static struct intel_timeline *get_timeline(struct i915_request *rq)
>>>>> +{
>>>>> + struct intel_timeline *tl;
>>>>> +
>>>>> + /*
>>>>> + * Even though we are holding the engine->active.lock here, there
>>>>> + * is no control over the submission queue per-se and we are
>>>>> + * inspecting the active state at a random point in time, with an
>>>>> + * unknown queue. Play safe and make sure the timeline remains valid.
>>>>> + * (Only being used for pretty printing, one extra kref shouldn't
>>>>> + * cause a camel stampede!)
>>>>> + */
>>>>> + rcu_read_lock();
>>>>> + tl = rcu_dereference(rq->timeline);
>>>>> + if (!kref_get_unless_zero(&tl->kref))
>>>>> + tl = NULL;
>>>>> + rcu_read_unlock();
>>>>
>>>> How can it be NULL under the active lock? Isn't that the same assertion
>>>> from i915_timeline_get_active.
>>>
>>> Not NULL, but retired. The difference is that during submission we know
>>> that this request's context/timeline must be currently pinned until
>>> a subsequent request (containing the idle-barriers) is submitted. The
>>> danger I worry about here is that subsequent idle request may be already
>>> submitted and since the queued requests may *already* have been retired,
>>> the timeline may be unpinned and indeed dropped it's last reference.
>>
>> But here it is under the engine->active.lock with interrupts disabled
>> and the requests are fetched from execlists ports. Timeline is not
>> guaranteed to be kept alive under these conditions? intel_context
>> reference will be held until process_csb schedules it out so I'd expect
>> timeline and hwsp to be there. But I could be lost in the new scheme of
>> things.
>
> I felt it was prudent to only rely on the active pin. You are right in
> that we have a context reference if it is in active, and that context
> holds a reference to the timeline. But... engine->active.lock is not
> the lock that guards rq->timeline, so I feel uneasy on extending
> i915_request_active_timeline() too far. Outside of the submission
> pathway, inside a pretty printer, it feels safer (whatever changes may
> come we don't have to worry about it) to not assume anything and just
> use the failsafe rcu_dereference() + kref.
Well okay, I can accept that in the overall situation.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-09-20 8:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 11:19 [PATCH v2 1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Chris Wilson
2019-09-19 11:19 ` [PATCH v2 2/3] drm/i915: Lock signaler timeline while navigating Chris Wilson
2019-09-19 11:19 ` [PATCH v2 3/3] drm/i915: Protect timeline->hwsp dereferencing Chris Wilson
2019-09-19 12:22 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915: Mark i915_request.timeline as a volatile, rcu pointer Patchwork
2019-09-19 12:23 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-09-19 12:44 ` ✓ Fi.CI.BAT: success " Patchwork
2019-09-19 13:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
2019-09-19 13:26 ` Chris Wilson
2019-09-19 17:11 ` Tvrtko Ursulin
2019-09-19 17:49 ` Chris Wilson
2019-09-20 8:47 ` Tvrtko Ursulin
2019-09-19 20:09 ` ✓ Fi.CI.IGT: success for series starting with [v2,1/3] " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.