Quoting Chris Wilson (2020-02-06 16:32:43)
> 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 <tvrtko.ursulin@xxxxxxxxx>
> Fixes: dc93c9b69315 ("drm/i915/gt: Schedule request retirement when signaler idles")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
> 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! */
> +
> + /*
> + * 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);
> + rq->engine = engine;
> + __i915_request_submit(rq);
> + spin_unlock(&engine->active.lock);
This won't do either as it inverts the ve/phys locking order... And wait
for it...
We call ve->submit_request() underneath the phys->active.lock when
unsubmitting.
Bleurgh. Let's take the path in v1 for a bit while I see if this can be
unravelled.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
