On 06/02/2020 16:32, Chris Wilson wrote:
Virtual engines are fleeting. They carry a reference count and may be freed
when their last request is retired. This makes them unsuitable for the
task of housing engine->retire.work so assert that it is not used.
Tvrtko tracked down an instance where we did indeed violate this rule.
In virtual_submit_request, we flush a completed request directly with
__i915_request_submit and this causes us to queue that request on the
veng's breadcrumb list and signal it. Leading us down a path where we
should not attach the retire.
v2: Always select a physical engine before submitting, and so avoid
using the veng as a signaler.
Reported-by: Tvrtko Ursulin <[email protected]>
Fixes: dc93c9b69315 ("drm/i915/gt: Schedule request retirement when signaler
idles")
Signed-off-by: Chris Wilson <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
---
drivers/gpu/drm/i915/gt/intel_engine.h | 1 +
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
drivers/gpu/drm/i915/gt/intel_lrc.c | 21 ++++++++++++++++++---
drivers/gpu/drm/i915/i915_request.c | 2 ++
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index b36ec1fddc3d..5b21ca5478c2 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -217,6 +217,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs
*engine);
static inline void
intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
{
+ GEM_BUG_ON(!engine->breadcrumbs.irq_work.func);
irq_work_queue(&engine->breadcrumbs.irq_work);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 7ef1d37970f6..8a5054f21bf8 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -99,6 +99,9 @@ static bool add_retire(struct intel_engine_cs *engine,
void intel_engine_add_retire(struct intel_engine_cs *engine,
struct intel_timeline *tl)
{
+ /* We don't deal well with the engine disappearing beneath us */
+ GEM_BUG_ON(intel_engine_is_virtual(engine));
+
if (add_retire(engine, tl))
schedule_work(&engine->retire_work);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
b/drivers/gpu/drm/i915/gt/intel_lrc.c
index c196fb90c59f..639b5be56026 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -4883,6 +4883,22 @@ static void virtual_submission_tasklet(unsigned long
data)
local_irq_enable();
}
+static void __ve_request_submit(const struct virtual_engine *ve,
+ struct i915_request *rq)
+{
+ struct intel_engine_cs *engine = ve->siblings[0]; /* totally random! */
We don't preserve the execution engine in ce->inflight? No.. Will random
engine have any effect? Will proper waiters get signaled?
+
+ /*
+ * Select a real engine to act as our permanent storage
+ * and signaler for the stale request, and prevent
+ * this virtual engine from leaking into the execution state.
+ */
+ spin_lock(&engine->active.lock);
Nesting phys lock under veng lock will be okay?
Regards,
Tvrtko
+ rq->engine = engine;
+ __i915_request_submit(rq);
+ spin_unlock(&engine->active.lock);
+}
+
static void virtual_submit_request(struct i915_request *rq)
{
struct virtual_engine *ve = to_virtual_engine(rq->engine);
@@ -4900,12 +4916,12 @@ static void virtual_submit_request(struct i915_request
*rq)
old = ve->request;
if (old) { /* background completion event from preempt-to-busy */
GEM_BUG_ON(!i915_request_completed(old));
- __i915_request_submit(old);
+ __ve_request_submit(ve, old);
i915_request_put(old);
}
if (i915_request_completed(rq)) {
- __i915_request_submit(rq);
+ __ve_request_submit(ve, rq);
ve->base.execlists.queue_priority_hint = INT_MIN;
ve->request = NULL;
@@ -5004,7 +5020,6 @@ intel_execlists_create_virtual(struct intel_engine_cs
**siblings,
snprintf(ve->base.name, sizeof(ve->base.name), "virtual");
intel_engine_init_active(&ve->base, ENGINE_VIRTUAL);
- intel_engine_init_breadcrumbs(&ve->base);
intel_engine_init_execlists(&ve->base);
ve->base.cops = &virtual_context_ops;
diff --git a/drivers/gpu/drm/i915/i915_request.c
b/drivers/gpu/drm/i915/i915_request.c
index 0ecc2cf64216..2c45d4b93e2c 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -358,6 +358,8 @@ bool __i915_request_submit(struct i915_request *request)
GEM_BUG_ON(!irqs_disabled());
lockdep_assert_held(&engine->active.lock);
+ GEM_BUG_ON(intel_engine_is_virtual(engine));
+
/*
* With the advent of preempt-to-busy, we frequently encounter
* requests that we have unsubmitted from HW, but left running
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
