On 7/12/2021 14:36, Matthew Brost wrote:
On Mon, Jul 12, 2021 at 08:05:30PM +0000, Matthew Brost wrote:
On Mon, Jul 12, 2021 at 11:23:14AM -0700, John Harrison wrote:
On 6/24/2021 00:04, Matthew Brost wrote:
Hold a reference to the intel_context over life of an i915_request.
Without this an i915_request can exist after the context has been
destroyed (e.g. request retired, context closed, but user space holds a
reference to the request from an out fence). In the case of GuC
submission + virtual engine, the engine that the request references is
also destroyed which can trigger bad pointer dref in fence ops (e.g.
Maybe quickly explain a why this is different for GuC submission vs
execlist? Presumably it is about only decomposing virtual engines to
physical ones in execlist mode?
Yes, it because in execlists we always end up pointing to a physical
engine in the end while in GuC mode we can be pointing to dynamically
allocated virtual engine. I can update the comment.
i915_fence_get_driver_name). We could likely change
i915_fence_get_driver_name to avoid touching the engine but let's just
be safe and hold the intel_context reference.
Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_request.c | 54 ++++++++++++-----------------
1 file changed, 22 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index de9deb95b8b1..dec5a35c9aa2 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -126,39 +126,17 @@ static void i915_fence_release(struct dma_fence *fence)
i915_sw_fence_fini(&rq->semaphore);
/*
- * Keep one request on each engine for reserved use under mempressure
- *
- * We do not hold a reference to the engine here and so have to be
- * very careful in what rq->engine we poke. The virtual engine is
- * referenced via the rq->context and we released that ref during
- * i915_request_retire(), ergo we must not dereference a virtual
- * engine here. Not that we would want to, as the only consumer of
- * the reserved engine->request_pool is the power management parking,
- * which must-not-fail, and that is only run on the physical engines.
- *
- * Since the request must have been executed to be have completed,
- * we know that it will have been processed by the HW and will
- * not be unsubmitted again, so rq->engine and rq->execution_mask
- * at this point is stable. rq->execution_mask will be a single
- * bit if the last and _only_ engine it could execution on was a
- * physical engine, if it's multiple bits then it started on and
- * could still be on a virtual engine. Thus if the mask is not a
- * power-of-two we assume that rq->engine may still be a virtual
- * engine and so a dangling invalid pointer that we cannot dereference
- *
- * For example, consider the flow of a bonded request through a virtual
- * engine. The request is created with a wide engine mask (all engines
- * that we might execute on). On processing the bond, the request mask
- * is reduced to one or more engines. If the request is subsequently
- * bound to a single engine, it will then be constrained to only
- * execute on that engine and never returned to the virtual engine
- * after timeslicing away, see __unwind_incomplete_requests(). Thus we
- * know that if the rq->execution_mask is a single bit, rq->engine
- * can be a physical engine with the exact corresponding mask.
+ * Keep one request on each engine for reserved use under mempressure,
+ * do not use with virtual engines as this really is only needed for
+ * kernel contexts.
*/
- if (is_power_of_2(rq->execution_mask) &&
- !cmpxchg(&rq->engine->request_pool, NULL, rq))
+ if (!intel_engine_is_virtual(rq->engine) &&
+ !cmpxchg(&rq->engine->request_pool, NULL, rq)) {
+ intel_context_put(rq->context);
return;
+ }
+
+ intel_context_put(rq->context);
The put is actually unconditional? So it could be moved before the if?
Yep, I think so.
Wait nope. We reference rq->engine which could a virtual engine and the
intel_context_put could free that engine. So we need to do the put after
we reference it.
Matt
Doh! That's a pretty good reason.
Okay, with a tweaked description to explain about virtual engines being
different on GuC vs execlist...
Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>
Matt
John.
kmem_cache_free(global.slab_requests, rq);
}
@@ -977,7 +955,18 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
}
}
- rq->context = ce;
+ /*
+ * Hold a reference to the intel_context over life of an i915_request.
+ * Without this an i915_request can exist after the context has been
+ * destroyed (e.g. request retired, context closed, but user space holds
+ * a reference to the request from an out fence). In the case of GuC
+ * submission + virtual engine, the engine that the request references
+ * is also destroyed which can trigger bad pointer dref in fence ops
+ * (e.g. i915_fence_get_driver_name). We could likely change these
+ * functions to avoid touching the engine but let's just be safe and
+ * hold the intel_context reference.
+ */
+ rq->context = intel_context_get(ce);
rq->engine = ce->engine;
rq->ring = ce->ring;
rq->execution_mask = ce->engine->mask;
@@ -1054,6 +1043,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
GEM_BUG_ON(!list_empty(&rq->sched.waiters_list));
err_free:
+ intel_context_put(ce);
kmem_cache_free(global.slab_requests, rq);
err_unreserve:
intel_context_unpin(ce);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
