On 15/04/16 13:12, Chris Wilson wrote:
On Fri, Apr 15, 2016 at 12:54:34PM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
In GuC mode submitting requests is only putting them into the
GuC queue with the actual submission to hardware following at
some future point. This makes the per engine last context
tracking insufficient for closing the premature context
unpin race.
Instead we need to make requests track and pin the previous
context on the same engine, and only unpin them when they
themselves are retired. This will ensure contexts remain pinned
in all cases until the are fully saved by the GPU.
v2: Only track contexts.
v3: Limit to GuC for now.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Alex Dai <yu.dai@xxxxxxxxx>
Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
Cc: Nick Hoath <nicholas.hoath@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 4 +++-
drivers/gpu/drm/i915/i915_gem.c | 5 +++++
drivers/gpu/drm/i915/intel_lrc.c | 15 +++++++++++++++
3 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9ed1b305beb..80f2d959ed7b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2290,6 +2290,9 @@ struct drm_i915_gem_request {
struct intel_context *ctx;
struct intel_ringbuffer *ringbuf;
+ /** Context preceding this one on the same engine. Can be NULL. */
+ struct intel_context *prev_ctx;
+
/** Batch buffer related to this request if any (used for
error state dump only) */
struct drm_i915_gem_object *batch_obj;
@@ -2325,7 +2328,6 @@ struct drm_i915_gem_request {
/** Execlists no. of times this request has been sent to the ELSP */
int elsp_submitted;
-
};
struct drm_i915_gem_request * __must_check
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e9db56c7a31f..18e747132ce5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1413,6 +1413,11 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
list_del_init(&request->list);
i915_gem_request_remove_from_client(request);
+ if (request->prev_ctx) {
+ intel_lr_context_unpin(request->prev_ctx, request->engine);
+ request->prev_ctx = NULL;
+ }
+
i915_gem_request_unreference(request);
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2f49dfae5560..73be9eac8ecc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -791,6 +791,21 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
if (intel_engine_stopped(engine))
return 0;
+ /*
+ * Pin the previous context on this engine to ensure it is not
+ * prematurely unpinned in GuC mode.
+ * Previous context will be unpinned when this request is retired,
+ * ensuring the GPU has switched out from the previous context and
+ * into a new context at that point.
+ */
+ if (dev_priv->guc.execbuf_client && engine->last_context) {
+ intel_lr_context_pin(engine->last_context, engine);
+ request->prev_ctx = engine->last_context;
+ }
Nak.
We just want,
request->pinned_ctx = engine->last_context;
engine->last_context = request->ctx;
as that works for everyone (and replaces the current last_context dance)
Not sure if you are against the implementation or the idea.
It is deliberately limited to GuC for now, until the lazy ringbuffer
unpin happens for all platforms.
Then - engine needs to hold the pin on the last context in case there
are no subsequent requests to keep it pinned.
In your snippet where do you pin it before assigning to any object?
I could optimize my implementation by not doing the pin-unpin in the
case when the context changes between current and last, but other than
that it looks correct to me.
And I don't think pinned_ctx is a good name. Previous context is
descriptive and self documenting so wins easily IMHO.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
