* [PATCH v2 1/3] drm/i915: Make LRC (un)pinning work on context and engine
@ 2016-01-21 10:10 Tvrtko Ursulin
2016-01-21 10:10 ` [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context Tvrtko Ursulin
2016-01-21 10:10 ` [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode Tvrtko Ursulin
0 siblings, 2 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-01-21 10:10 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Previously intel_lr_context_(un)pin were operating on requests
which is in conflict with their names.
If we make them take a context and an engine, it makes the names
make more sense and it also makes future fixes possible.
v2: Rebase for default_context/kernel_context change.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 49 ++++++++++++++++++++--------------------
drivers/gpu/drm/i915/intel_lrc.h | 3 ++-
3 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6a3e4ee7f7e2..864fe2382875 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2680,7 +2680,7 @@ void i915_gem_request_free(struct kref *req_ref)
if (ctx) {
if (i915.enable_execlists && ctx != req->i915->kernel_context)
- intel_lr_context_unpin(req);
+ intel_lr_context_unpin(ctx, req->ring);
i915_gem_context_unreference(ctx);
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 134379dc4dd9..c1376fed30a5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -225,7 +225,8 @@ enum {
#define GEN8_CTX_ID_SHIFT 32
#define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x17
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
+static int intel_lr_context_pin(struct intel_context *ctx,
+ struct intel_engine_cs *engine);
static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
struct drm_i915_gem_object *default_ctx_obj);
@@ -599,7 +600,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
int num_elements = 0;
if (request->ctx != request->i915->kernel_context)
- intel_lr_context_pin(request);
+ intel_lr_context_pin(request->ctx, ring);
i915_gem_request_reference(request);
@@ -704,7 +705,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
}
if (request->ctx != request->i915->kernel_context)
- ret = intel_lr_context_pin(request);
+ ret = intel_lr_context_pin(request->ctx, request->ring);
return ret;
}
@@ -1004,7 +1005,8 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
ctx->engine[ring->id].state;
if (ctx_obj && (ctx != req->i915->kernel_context))
- intel_lr_context_unpin(req);
+ intel_lr_context_unpin(ctx, ring);
+
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
}
@@ -1048,8 +1050,8 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
return 0;
}
-static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
- struct intel_context *ctx)
+static int intel_lr_context_do_pin(struct intel_context *ctx,
+ struct intel_engine_cs *ring)
{
struct drm_device *dev = ring->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1092,41 +1094,40 @@ unpin_ctx_obj:
return ret;
}
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
+static int intel_lr_context_pin(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
int ret = 0;
- struct intel_engine_cs *ring = rq->ring;
- if (rq->ctx->engine[ring->id].pin_count++ == 0) {
- ret = intel_lr_context_do_pin(ring, rq->ctx);
+ if (ctx->engine[engine->id].pin_count++ == 0) {
+ ret = intel_lr_context_do_pin(ctx, engine);
if (ret)
goto reset_pin_count;
}
return ret;
reset_pin_count:
- rq->ctx->engine[ring->id].pin_count = 0;
+ ctx->engine[engine->id].pin_count = 0;
return ret;
}
-void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
+void intel_lr_context_unpin(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
- struct intel_engine_cs *ring = rq->ring;
- struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
- struct intel_ringbuffer *ringbuf = rq->ringbuf;
+ struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
- WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+ WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
- if (!ctx_obj)
+ if (WARN_ON_ONCE(!ctx_obj))
return;
- if (--rq->ctx->engine[ring->id].pin_count == 0) {
- kunmap(kmap_to_page(rq->ctx->engine[ring->id].lrc_reg_state));
- intel_unpin_ringbuffer_obj(ringbuf);
+ if (--ctx->engine[engine->id].pin_count == 0) {
+ kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
+ intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
i915_gem_object_ggtt_unpin(ctx_obj);
- rq->ctx->engine[ring->id].lrc_vma = NULL;
- rq->ctx->engine[ring->id].lrc_desc = 0;
- rq->ctx->engine[ring->id].lrc_reg_state = NULL;
+ ctx->engine[engine->id].lrc_vma = NULL;
+ ctx->engine[engine->id].lrc_desc = 0;
+ ctx->engine[engine->id].lrc_reg_state = NULL;
}
}
@@ -2030,7 +2031,7 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
goto error;
/* As this is the default context, always pin it */
- ret = intel_lr_context_do_pin(ring, dctx);
+ ret = intel_lr_context_do_pin(dctx, ring);
if (ret) {
DRM_ERROR(
"Failed to pin and map ringbuffer %s: %d\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 49af638f6213..e6cda3e225d0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -101,7 +101,8 @@ void intel_lr_context_free(struct intel_context *ctx);
uint32_t intel_lr_context_size(struct intel_engine_cs *ring);
int intel_lr_context_deferred_alloc(struct intel_context *ctx,
struct intel_engine_cs *ring);
-void intel_lr_context_unpin(struct drm_i915_gem_request *req);
+void intel_lr_context_unpin(struct intel_context *ctx,
+ struct intel_engine_cs *engine);
void intel_lr_context_reset(struct drm_device *dev,
struct intel_context *ctx);
uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context
2016-01-21 10:10 [PATCH v2 1/3] drm/i915: Make LRC (un)pinning work on context and engine Tvrtko Ursulin
@ 2016-01-21 10:10 ` Tvrtko Ursulin
2016-01-21 10:10 ` [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode Tvrtko Ursulin
1 sibling, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-01-21 10:10 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Will simplify the following fix and sounds logical.
v2: Add some whitespace to separate logic better. (Chris Wilson)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c1376fed30a5..b35788ee2f83 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1103,6 +1103,8 @@ static int intel_lr_context_pin(struct intel_context *ctx,
ret = intel_lr_context_do_pin(ctx, engine);
if (ret)
goto reset_pin_count;
+
+ i915_gem_context_reference(ctx);
}
return ret;
@@ -1128,6 +1130,8 @@ void intel_lr_context_unpin(struct intel_context *ctx,
ctx->engine[engine->id].lrc_vma = NULL;
ctx->engine[engine->id].lrc_desc = 0;
ctx->engine[engine->id].lrc_reg_state = NULL;
+
+ i915_gem_context_unreference(ctx);
}
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode
2016-01-21 10:10 [PATCH v2 1/3] drm/i915: Make LRC (un)pinning work on context and engine Tvrtko Ursulin
2016-01-21 10:10 ` [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context Tvrtko Ursulin
@ 2016-01-21 10:10 ` Tvrtko Ursulin
2016-01-21 11:02 ` Chris Wilson
1 sibling, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-01-21 10:10 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
In GuC mode LRC pinning lifetime depends exclusively on the
request liftime. Since that is terminated by the seqno update
that opens up a race condition between GPU finishing writing
out the context image and the driver unpinning the LRC.
To extend the LRC lifetime we will employ a similar approach
to what legacy ringbuffer submission does.
We will start tracking the last submitted context per engine
and keep it pinned until it is replaced by another one.
Note that the driver unload path is a bit fragile and could
benefit greatly from efforts to unify the legacy and exec
list submission code paths.
At the moment i915_gem_context_fini has special casing for the
two which are potentialy not needed, and also depends on
i915_gem_cleanup_ringbuffer running before itself.
v2:
* Move pinning into engine->emit_request and actually fix
the reference/unreference logic. (Chris Wilson)
* ring->dev can be NULL on driver unload so use a different
route towards it.
v3:
* Rebase.
* Handle the reset path. (Chris Wilson)
* Exclude default context from the pinning - it is impossible
to get it right before default context special casing in
general is eliminated.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Issue: VIZ-4277
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
drivers/gpu/drm/i915/i915_gem_context.c | 15 +++++++++++----
drivers/gpu/drm/i915/intel_lrc.c | 13 ++++++++++++-
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6a4f64b03db6..47870b463bbd 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -341,10 +341,14 @@ void i915_gem_context_reset(struct drm_device *dev)
struct intel_context *lctx = ring->last_context;
if (lctx) {
- if (lctx->legacy_hw_ctx.rcs_state && i == RCS)
- i915_gem_object_ggtt_unpin(lctx->legacy_hw_ctx.rcs_state);
+ if (!i915.enable_execlists) {
+ if (lctx->legacy_hw_ctx.rcs_state && i == RCS)
+ i915_gem_object_ggtt_unpin(lctx->legacy_hw_ctx.rcs_state);
- i915_gem_context_unreference(lctx);
+ i915_gem_context_unreference(lctx);
+ } else {
+ intel_lr_context_unpin(lctx, ring);
+ }
ring->last_context = NULL;
}
}
@@ -432,7 +436,10 @@ void i915_gem_context_fini(struct drm_device *dev)
struct intel_engine_cs *ring = &dev_priv->ring[i];
if (ring->last_context) {
- i915_gem_context_unreference(ring->last_context);
+ if (i915.enable_execlists)
+ intel_lr_context_unpin(ring->last_context, ring);
+ else
+ i915_gem_context_unreference(ring->last_context);
ring->last_context = NULL;
}
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b35788ee2f83..8619415f60c7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1118,7 +1118,7 @@ void intel_lr_context_unpin(struct intel_context *ctx,
{
struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
- WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+ WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
if (WARN_ON_ONCE(!ctx_obj))
return;
@@ -1857,6 +1857,17 @@ static int gen8_emit_request(struct drm_i915_gem_request *request)
u32 cmd;
int ret;
+ if (ring->last_context != request->ctx) {
+ if (ring->last_context)
+ intel_lr_context_unpin(ring->last_context, ring);
+ if (request->ctx != request->i915->kernel_context) {
+ intel_lr_context_pin(request->ctx, ring);
+ ring->last_context = request->ctx;
+ } else {
+ ring->last_context = NULL;
+ }
+ }
+
/*
* Reserve space for 2 NOOPs at the end of each request to be
* used as a workaround for not being allowed to do lite
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode
2016-01-21 10:10 ` [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode Tvrtko Ursulin
@ 2016-01-21 11:02 ` Chris Wilson
2016-01-21 11:28 ` Chris Wilson
2016-01-22 16:33 ` Tvrtko Ursulin
0 siblings, 2 replies; 9+ messages in thread
From: Chris Wilson @ 2016-01-21 11:02 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Thu, Jan 21, 2016 at 10:10:42AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> In GuC mode LRC pinning lifetime depends exclusively on the
> request liftime. Since that is terminated by the seqno update
> that opens up a race condition between GPU finishing writing
> out the context image and the driver unpinning the LRC.
>
> To extend the LRC lifetime we will employ a similar approach
> to what legacy ringbuffer submission does.
>
> We will start tracking the last submitted context per engine
> and keep it pinned until it is replaced by another one.
>
> Note that the driver unload path is a bit fragile and could
> benefit greatly from efforts to unify the legacy and exec
> list submission code paths.
>
> At the moment i915_gem_context_fini has special casing for the
> two which are potentialy not needed, and also depends on
> i915_gem_cleanup_ringbuffer running before itself.
>
> v2:
> * Move pinning into engine->emit_request and actually fix
> the reference/unreference logic. (Chris Wilson)
>
> * ring->dev can be NULL on driver unload so use a different
> route towards it.
>
> v3:
> * Rebase.
> * Handle the reset path. (Chris Wilson)
> * Exclude default context from the pinning - it is impossible
> to get it right before default context special casing in
> general is eliminated.
Since you have the refactoring in place, eliminating the special case is
trivial. So patch 2.5?
Looks like http://patchwork.freedesktop.org/patch/69972/ but you already
have half of that patch in place.
Also if we take a step to add patch 2.1:
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index c25083c78ba7..d43c886fd95a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -321,6 +321,14 @@ err_destroy:
return ERR_PTR(ret);
}
+static void i915_gem_context_unpin(struct intel_context *ctx,
+ struct intel_engine_cs *rcs)
+{
+ if (rcs->id == RCS && ctx->legacy_hw_ctx.rcs_state)
+ i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
+ i915_gem_context_unreference(ctx);
+}
+
void i915_gem_context_reset(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -338,13 +346,9 @@ void i915_gem_context_reset(struct drm_device *dev)
for (i = 0; i < I915_NUM_RINGS; i++) {
struct intel_engine_cs *ring = &dev_priv->ring[i];
- struct intel_context *lctx = ring->last_context;
- if (lctx) {
- if (lctx->legacy_hw_ctx.rcs_state && i == RCS)
- i915_gem_object_ggtt_unpin(lctx->legacy_hw_ctx.rcs_state);
-
- i915_gem_context_unreference(lctx);
+ if (ring->last_context) {
+ i915_gem_context_unpin(ring->last_context, ring);
ring->last_context = NULL;
}
@@ -424,25 +428,18 @@ void i915_gem_context_fini(struct drm_device *dev)
* to offset the do_switch part, so that i915_gem_context_unreference()
* can then free the base object correctly. */
WARN_ON(!dev_priv->ring[RCS].last_context);
- if (dev_priv->ring[RCS].last_context == dctx) {
- /* Fake switch to NULL context */
- WARN_ON(dctx->legacy_hw_ctx.rcs_state->active);
- i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
- i915_gem_context_unreference(dctx);
- dev_priv->ring[RCS].last_context = NULL;
- }
-
i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
}
for (i = 0; i < I915_NUM_RINGS; i++) {
struct intel_engine_cs *ring = &dev_priv->ring[i];
- if (ring->last_context)
- i915_gem_context_unreference(ring->last_context);
+ if (ring->last_context) {
+ i915_gem_context_unpin(ring->last_context, ring);
+ ring->last_context = NULL;
+ }
ring->default_context = NULL;
- ring->last_context = NULL;
}
i915_gem_context_unreference(dctx);
Then we just end up with
if (i915.enable_execlists)
intel_lr_context_unpin(ring->last_context, ring);
else
i915_gem_context_unpin(ring->last_context, ring);
which is much less messy in this patch and one step closer to unifying
engine->pin_context()/engine->unpin_context() vfuns.
The only tricky part there is having to disentangle the legacy paths,
but doing so should allow us to see clearly that the sequence is just
intel_gpu_reset();
for_each_engine() unpin(engine->last_context);
unpin(dev_priv->default_context);
After a few more passes, we should be able to call the reset and cleanup
functions higher up (as part of the stopping the GPU upon suspend
procedure before tearing down the sw structs) at which point we only
need the unpin(default_context) here.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode
2016-01-21 11:02 ` Chris Wilson
@ 2016-01-21 11:28 ` Chris Wilson
2016-01-22 16:33 ` Tvrtko Ursulin
1 sibling, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2016-01-21 11:28 UTC (permalink / raw)
To: Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin, Nick Hoath
On Thu, Jan 21, 2016 at 11:02:50AM +0000, Chris Wilson wrote:
> intel_gpu_reset();
> for_each_engine() unpin(engine->last_context);
> unpin(dev_priv->default_context);
>
> After a few more passes, we should be able to call the reset and cleanup
> functions higher up (as part of the stopping the GPU upon suspend
> procedure before tearing down the sw structs) at which point we only
> need the unpin(default_context) here.
unref(dev_priv->default_context) !
whether or not the individual engines have a pin on it will be left to
themselves to decide.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode
2016-01-21 11:02 ` Chris Wilson
2016-01-21 11:28 ` Chris Wilson
@ 2016-01-22 16:33 ` Tvrtko Ursulin
2016-01-22 16:44 ` Chris Wilson
1 sibling, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-01-22 16:33 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx, Tvrtko Ursulin, Nick Hoath
On 21/01/16 11:02, Chris Wilson wrote:
> On Thu, Jan 21, 2016 at 10:10:42AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In GuC mode LRC pinning lifetime depends exclusively on the
>> request liftime. Since that is terminated by the seqno update
>> that opens up a race condition between GPU finishing writing
>> out the context image and the driver unpinning the LRC.
>>
>> To extend the LRC lifetime we will employ a similar approach
>> to what legacy ringbuffer submission does.
>>
>> We will start tracking the last submitted context per engine
>> and keep it pinned until it is replaced by another one.
>>
>> Note that the driver unload path is a bit fragile and could
>> benefit greatly from efforts to unify the legacy and exec
>> list submission code paths.
>>
>> At the moment i915_gem_context_fini has special casing for the
>> two which are potentialy not needed, and also depends on
>> i915_gem_cleanup_ringbuffer running before itself.
>>
>> v2:
>> * Move pinning into engine->emit_request and actually fix
>> the reference/unreference logic. (Chris Wilson)
>>
>> * ring->dev can be NULL on driver unload so use a different
>> route towards it.
>>
>> v3:
>> * Rebase.
>> * Handle the reset path. (Chris Wilson)
>> * Exclude default context from the pinning - it is impossible
>> to get it right before default context special casing in
>> general is eliminated.
>
> Since you have the refactoring in place, eliminating the special case is
> trivial. So patch 2.5?
>
> Looks like http://patchwork.freedesktop.org/patch/69972/ but you already
> have half of that patch in place.
>
> Also if we take a step to add patch 2.1:
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index c25083c78ba7..d43c886fd95a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -321,6 +321,14 @@ err_destroy:
> return ERR_PTR(ret);
> }
>
> +static void i915_gem_context_unpin(struct intel_context *ctx,
> + struct intel_engine_cs *rcs)
> +{
> + if (rcs->id == RCS && ctx->legacy_hw_ctx.rcs_state)
> + i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> + i915_gem_context_unreference(ctx);
> +}
> +
> void i915_gem_context_reset(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -338,13 +346,9 @@ void i915_gem_context_reset(struct drm_device *dev)
>
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct intel_engine_cs *ring = &dev_priv->ring[i];
> - struct intel_context *lctx = ring->last_context;
>
> - if (lctx) {
> - if (lctx->legacy_hw_ctx.rcs_state && i == RCS)
> - i915_gem_object_ggtt_unpin(lctx->legacy_hw_ctx.rcs_state);
> -
> - i915_gem_context_unreference(lctx);
> + if (ring->last_context) {
> + i915_gem_context_unpin(ring->last_context, ring);
> ring->last_context = NULL;
> }
>
> @@ -424,25 +428,18 @@ void i915_gem_context_fini(struct drm_device *dev)
> * to offset the do_switch part, so that i915_gem_context_unreference()
> * can then free the base object correctly. */
> WARN_ON(!dev_priv->ring[RCS].last_context);
> - if (dev_priv->ring[RCS].last_context == dctx) {
> - /* Fake switch to NULL context */
> - WARN_ON(dctx->legacy_hw_ctx.rcs_state->active);
> - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> - i915_gem_context_unreference(dctx);
> - dev_priv->ring[RCS].last_context = NULL;
> - }
> -
> i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> }
>
> for (i = 0; i < I915_NUM_RINGS; i++) {
> struct intel_engine_cs *ring = &dev_priv->ring[i];
>
> - if (ring->last_context)
> - i915_gem_context_unreference(ring->last_context);
> + if (ring->last_context) {
> + i915_gem_context_unpin(ring->last_context, ring);
> + ring->last_context = NULL;
> + }
>
> ring->default_context = NULL;
> - ring->last_context = NULL;
> }
>
> i915_gem_context_unreference(dctx);
>
> Then we just end up with
>
> if (i915.enable_execlists)
> intel_lr_context_unpin(ring->last_context, ring);
> else
> i915_gem_context_unpin(ring->last_context, ring);
>
> which is much less messy in this patch and one step closer to unifying
> engine->pin_context()/engine->unpin_context() vfuns.
>
> The only tricky part there is having to disentangle the legacy paths,
> but doing so should allow us to see clearly that the sequence is just
>
> intel_gpu_reset();
> for_each_engine() unpin(engine->last_context);
> unpin(dev_priv->default_context);
>
> After a few more passes, we should be able to call the reset and cleanup
> functions higher up (as part of the stopping the GPU upon suspend
> procedure before tearing down the sw structs) at which point we only
> need the unpin(default_context) here.
I was confused by the current code which does the reset after unpinning:
...
i915_gem_reset(dev);
simulated = dev_priv->gpu_error.stop_rings != 0;
ret = intel_gpu_reset(dev);
...
What is right then?
Sounds bad to be unpinning with the GPU in unknown state. But obviously
it has been like this for who knows how long. So I have no idea. :(
Asking since GuC is at the moment struggling with reset, but then again
that might be something completely else...
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode
2016-01-22 16:33 ` Tvrtko Ursulin
@ 2016-01-22 16:44 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2016-01-22 16:44 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Fri, Jan 22, 2016 at 04:33:18PM +0000, Tvrtko Ursulin wrote:
> I was confused by the current code which does the reset after unpinning:
>
> ...
> i915_gem_reset(dev);
>
> simulated = dev_priv->gpu_error.stop_rings != 0;
>
> ret = intel_gpu_reset(dev);
> ...
>
>
> What is right then?
Yeah, it would be better to do the sw reset after the hw reset so that
the GPU is a known state when we tear down everything.
> Sounds bad to be unpinning with the GPU in unknown state. But
> obviously it has been like this for who knows how long. So I have no
> idea. :(
i915_reset() is only called on a hung GPU, and I expect we simply
haven't stressed the system enough with an active-vs-hung pair of
engines to be able to see stray writes and whatnot.
The downside is that if we jiggle the i915_gem_reset() we have to ask
the awkward question of what to do with state when reset fails? We
should keep it around because the hw state is unknown - but that can be
a significant amount of memory trapped.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] drm/i915: Make LRC (un)pinning work on context and engine
@ 2016-01-20 13:40 Tvrtko Ursulin
2016-01-20 13:40 ` [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context Tvrtko Ursulin
0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-01-20 13:40 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Previously intel_lr_context_(un)pin were operating on requests
which is in conflict with their names.
If we make them take a context and an engine, it makes the names
make more sense and it also makes future fixes possible.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 48 ++++++++++++++++++++--------------------
drivers/gpu/drm/i915/intel_lrc.h | 3 ++-
3 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6b0102da859c..a752b00d4ff3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2681,7 +2681,7 @@ void i915_gem_request_free(struct kref *req_ref)
if (ctx) {
if (i915.enable_execlists) {
if (ctx != req->ring->default_context)
- intel_lr_context_unpin(req);
+ intel_lr_context_unpin(ctx, req->ring);
}
i915_gem_context_unreference(ctx);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index faaf49077fea..48ca51a36948 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -225,7 +225,8 @@ enum {
#define GEN8_CTX_ID_SHIFT 32
#define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x17
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
+static int intel_lr_context_pin(struct intel_context *ctx,
+ struct intel_engine_cs *engine);
static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
struct drm_i915_gem_object *default_ctx_obj);
@@ -599,7 +600,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
int num_elements = 0;
if (request->ctx != ring->default_context)
- intel_lr_context_pin(request);
+ intel_lr_context_pin(request->ctx, ring);
i915_gem_request_reference(request);
@@ -691,7 +692,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
request->ringbuf = request->ctx->engine[request->ring->id].ringbuf;
if (request->ctx != request->ring->default_context) {
- ret = intel_lr_context_pin(request);
+ ret = intel_lr_context_pin(request->ctx, request->ring);
if (ret)
return ret;
}
@@ -1007,7 +1008,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
ctx->engine[ring->id].state;
if (ctx_obj && (ctx != ring->default_context))
- intel_lr_context_unpin(req);
+ intel_lr_context_unpin(ctx, ring);
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
}
@@ -1051,8 +1052,8 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
return 0;
}
-static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
- struct intel_context *ctx)
+static int intel_lr_context_do_pin(struct intel_context *ctx,
+ struct intel_engine_cs *ring)
{
struct drm_device *dev = ring->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1095,41 +1096,40 @@ unpin_ctx_obj:
return ret;
}
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
+static int intel_lr_context_pin(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
int ret = 0;
- struct intel_engine_cs *ring = rq->ring;
- if (rq->ctx->engine[ring->id].pin_count++ == 0) {
- ret = intel_lr_context_do_pin(ring, rq->ctx);
+ if (ctx->engine[engine->id].pin_count++ == 0) {
+ ret = intel_lr_context_do_pin(ctx, engine);
if (ret)
goto reset_pin_count;
}
return ret;
reset_pin_count:
- rq->ctx->engine[ring->id].pin_count = 0;
+ ctx->engine[engine->id].pin_count = 0;
return ret;
}
-void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
+void intel_lr_context_unpin(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
- struct intel_engine_cs *ring = rq->ring;
- struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
- struct intel_ringbuffer *ringbuf = rq->ringbuf;
+ struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
- WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+ WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
- if (!ctx_obj)
+ if (WARN_ON_ONCE(!ctx_obj))
return;
- if (--rq->ctx->engine[ring->id].pin_count == 0) {
- kunmap(kmap_to_page(rq->ctx->engine[ring->id].lrc_reg_state));
- intel_unpin_ringbuffer_obj(ringbuf);
+ if (--ctx->engine[engine->id].pin_count == 0) {
+ kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
+ intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
i915_gem_object_ggtt_unpin(ctx_obj);
- rq->ctx->engine[ring->id].lrc_vma = NULL;
- rq->ctx->engine[ring->id].lrc_desc = 0;
- rq->ctx->engine[ring->id].lrc_reg_state = NULL;
+ ctx->engine[engine->id].lrc_vma = NULL;
+ ctx->engine[engine->id].lrc_desc = 0;
+ ctx->engine[engine->id].lrc_reg_state = NULL;
}
}
@@ -2032,7 +2032,7 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
goto error;
/* As this is the default context, always pin it */
- ret = intel_lr_context_do_pin(ring, ring->default_context);
+ ret = intel_lr_context_do_pin(ring->default_context, ring);
if (ret) {
DRM_ERROR(
"Failed to pin and map ringbuffer %s: %d\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 49af638f6213..e6cda3e225d0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -101,7 +101,8 @@ void intel_lr_context_free(struct intel_context *ctx);
uint32_t intel_lr_context_size(struct intel_engine_cs *ring);
int intel_lr_context_deferred_alloc(struct intel_context *ctx,
struct intel_engine_cs *ring);
-void intel_lr_context_unpin(struct drm_i915_gem_request *req);
+void intel_lr_context_unpin(struct intel_context *ctx,
+ struct intel_engine_cs *engine);
void intel_lr_context_reset(struct drm_device *dev,
struct intel_context *ctx);
uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context
2016-01-20 13:40 [PATCH 1/3] drm/i915: Make LRC (un)pinning work on context and engine Tvrtko Ursulin
@ 2016-01-20 13:40 ` Tvrtko Ursulin
2016-01-20 13:50 ` Chris Wilson
0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2016-01-20 13:40 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Will simplify the following fix and sounds logical.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 48ca51a36948..5c3f57fed916 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1105,6 +1105,7 @@ static int intel_lr_context_pin(struct intel_context *ctx,
ret = intel_lr_context_do_pin(ctx, engine);
if (ret)
goto reset_pin_count;
+ i915_gem_context_reference(ctx);
}
return ret;
@@ -1130,6 +1131,7 @@ void intel_lr_context_unpin(struct intel_context *ctx,
ctx->engine[engine->id].lrc_vma = NULL;
ctx->engine[engine->id].lrc_desc = 0;
ctx->engine[engine->id].lrc_reg_state = NULL;
+ i915_gem_context_unreference(ctx);
}
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context
2016-01-20 13:40 ` [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context Tvrtko Ursulin
@ 2016-01-20 13:50 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2016-01-20 13:50 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Wed, Jan 20, 2016 at 01:40:56PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Will simplify the following fix and sounds logical.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Nick Hoath <nicholas.hoath@intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 48ca51a36948..5c3f57fed916 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1105,6 +1105,7 @@ static int intel_lr_context_pin(struct intel_context *ctx,
> ret = intel_lr_context_do_pin(ctx, engine);
> if (ret)
> goto reset_pin_count;
> + i915_gem_context_reference(ctx);
> }
> return ret;
>
> @@ -1130,6 +1131,7 @@ void intel_lr_context_unpin(struct intel_context *ctx,
> ctx->engine[engine->id].lrc_vma = NULL;
> ctx->engine[engine->id].lrc_desc = 0;
> ctx->engine[engine->id].lrc_reg_state = NULL;
> + i915_gem_context_unreference(ctx);
I would appreciate some whitespace, especially here, since this is
important to be aware of -- we may need to consider that this is the
last reference and so have it stand out.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-01-22 16:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-21 10:10 [PATCH v2 1/3] drm/i915: Make LRC (un)pinning work on context and engine Tvrtko Ursulin
2016-01-21 10:10 ` [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context Tvrtko Ursulin
2016-01-21 10:10 ` [PATCH v3 3/3] drm/i915: Fix premature LRC unpin in GuC mode Tvrtko Ursulin
2016-01-21 11:02 ` Chris Wilson
2016-01-21 11:28 ` Chris Wilson
2016-01-22 16:33 ` Tvrtko Ursulin
2016-01-22 16:44 ` Chris Wilson
-- strict thread matches above, loose matches on Subject: below --
2016-01-20 13:40 [PATCH 1/3] drm/i915: Make LRC (un)pinning work on context and engine Tvrtko Ursulin
2016-01-20 13:40 ` [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context Tvrtko Ursulin
2016-01-20 13:50 ` Chris Wilson
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.