[PATCH 1/3] drm/i915: Make LRC (un)pinning work on context and engine

[PATCH 1/3] drm/i915: Make LRC (un)pinning work on context and engine

[Tvrtko Ursulin]

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

[PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context

[Tvrtko Ursulin]

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

[PATCH 3/3] drm/i915: Fix premature LRC unpin in GuC mode

[Tvrtko Ursulin]

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.

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>
---
I cannot test this with GuC but it passes BAT with execlists
and some real world smoke tests.
---
 drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
 drivers/gpu/drm/i915/intel_lrc.c        | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index c25083c78ba7..0b419e165836 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
 
-		if (ring->last_context)
+		if (ring->last_context && i915.enable_execlists)
+			intel_lr_context_unpin(ring->last_context, ring);
+		else if (ring->last_context)
 			i915_gem_context_unreference(ring->last_context);
 
 		ring->default_context = NULL;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5c3f57fed916..b8a7e126d6d2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -918,6 +918,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 	struct intel_engine_cs  *ring = params->ring;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ringbuffer *ringbuf = params->ctx->engine[ring->id].ringbuf;
+	struct intel_context    *ctx = params->request->ctx;
 	u64 exec_start;
 	int instp_mode;
 	u32 instp_mask;
@@ -982,6 +983,12 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
 
 	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
 
+	if (ring->last_context && ring->last_context != ctx) {
+		intel_lr_context_unpin(ring->last_context, ring);
+		intel_lr_context_pin(ctx, ring);
+		ring->last_context = ctx;
+	}
+
 	i915_gem_execbuffer_move_to_active(vmas, params->request);
 	i915_gem_execbuffer_retire_commands(params);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Make LRC pinning own a reference to the context

[Chris Wilson]

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

Re: [PATCH 3/3] drm/i915: Fix premature LRC unpin in GuC mode

[Chris Wilson]

On Wed, Jan 20, 2016 at 01:40:57PM +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.
> 
> 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>
> ---
> I cannot test this with GuC but it passes BAT with execlists
> and some real world smoke tests.
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
>  drivers/gpu/drm/i915/intel_lrc.c        | 7 +++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index c25083c78ba7..0b419e165836 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	for (i = 0; i < I915_NUM_RINGS; i++) {
>  		struct intel_engine_cs *ring = &dev_priv->ring[i];
>  
> -		if (ring->last_context)
> +		if (ring->last_context && i915.enable_execlists)
> +			intel_lr_context_unpin(ring->last_context, ring);
> +		else if (ring->last_context)
>  			i915_gem_context_unreference(ring->last_context);
>  
>  		ring->default_context = NULL;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5c3f57fed916..b8a7e126d6d2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -918,6 +918,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  	struct intel_engine_cs  *ring = params->ring;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ringbuffer *ringbuf = params->ctx->engine[ring->id].ringbuf;
> +	struct intel_context    *ctx = params->request->ctx;
>  	u64 exec_start;
>  	int instp_mode;
>  	u32 instp_mask;
> @@ -982,6 +983,12 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  
>  	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>  
> +	if (ring->last_context && ring->last_context != ctx) {
> +		intel_lr_context_unpin(ring->last_context, ring);
> +		intel_lr_context_pin(ctx, ring);
> +		ring->last_context = ctx;
> +	}

I think this is the wrong location and should be part of submitting the
context inside the engine (because intel_execlists_submission should not
as it is entirely duplicating the common GEM batch submision code and
the unique part is engine->add_request()).

Note that it should be:

if (engine->last_context != request->ctx) {
	if (engine->last_context)
		intel_lr_context_unpin(engine->last_context, engine);
	engine->last_context = request->ctx;
	intel_lr_context_pin(engine->last_context, engine);
}
-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

Re: [PATCH 1/3] drm/i915: Make LRC (un)pinning work on context and engine

[Chris Wilson]

On Wed, Jan 20, 2016 at 01:40:55PM +0000, Tvrtko Ursulin wrote:
> 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>
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

Re: [PATCH 3/3] drm/i915: Fix premature LRC unpin in GuC mode

[Tvrtko Ursulin]

On 20/01/16 13:55, Chris Wilson wrote:
> On Wed, Jan 20, 2016 at 01:40:57PM +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.
>>
>> 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>
>> ---
>> I cannot test this with GuC but it passes BAT with execlists
>> and some real world smoke tests.
>> ---
>>   drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
>>   drivers/gpu/drm/i915/intel_lrc.c        | 7 +++++++
>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>> index c25083c78ba7..0b419e165836 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
>>   	for (i = 0; i < I915_NUM_RINGS; i++) {
>>   		struct intel_engine_cs *ring = &dev_priv->ring[i];
>>
>> -		if (ring->last_context)
>> +		if (ring->last_context && i915.enable_execlists)
>> +			intel_lr_context_unpin(ring->last_context, ring);
>> +		else if (ring->last_context)
>>   			i915_gem_context_unreference(ring->last_context);
>>
>>   		ring->default_context = NULL;
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 5c3f57fed916..b8a7e126d6d2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -918,6 +918,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>>   	struct intel_engine_cs  *ring = params->ring;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct intel_ringbuffer *ringbuf = params->ctx->engine[ring->id].ringbuf;
>> +	struct intel_context    *ctx = params->request->ctx;
>>   	u64 exec_start;
>>   	int instp_mode;
>>   	u32 instp_mask;
>> @@ -982,6 +983,12 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>>
>>   	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>>
>> +	if (ring->last_context && ring->last_context != ctx) {
>> +		intel_lr_context_unpin(ring->last_context, ring);
>> +		intel_lr_context_pin(ctx, ring);
>> +		ring->last_context = ctx;
>> +	}
>
> I think this is the wrong location and should be part of submitting the
> context inside the engine (because intel_execlists_submission should not
> as it is entirely duplicating the common GEM batch submision code and
> the unique part is engine->add_request()).

So into engine->emit_request you are saying? That works just as well 
AFAICS, just making sure I understood correctly.

> Note that it should be:
>
> if (engine->last_context != request->ctx) {
> 	if (engine->last_context)
> 		intel_lr_context_unpin(engine->last_context, engine);
> 	engine->last_context = request->ctx;
> 	intel_lr_context_pin(engine->last_context, engine);
> }

Ooops!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Fix premature LRC unpin in GuC mode

[Chris Wilson]

On Wed, Jan 20, 2016 at 02:06:43PM +0000, Tvrtko Ursulin wrote:
> 
> On 20/01/16 13:55, Chris Wilson wrote:
> >On Wed, Jan 20, 2016 at 01:40:57PM +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.
> >>
> >>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>
> >>---
> >>I cannot test this with GuC but it passes BAT with execlists
> >>and some real world smoke tests.
> >>---
> >>  drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
> >>  drivers/gpu/drm/i915/intel_lrc.c        | 7 +++++++
> >>  2 files changed, 10 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >>index c25083c78ba7..0b419e165836 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_context.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >>@@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
> >>  	for (i = 0; i < I915_NUM_RINGS; i++) {
> >>  		struct intel_engine_cs *ring = &dev_priv->ring[i];
> >>
> >>-		if (ring->last_context)
> >>+		if (ring->last_context && i915.enable_execlists)
> >>+			intel_lr_context_unpin(ring->last_context, ring);
> >>+		else if (ring->last_context)
> >>  			i915_gem_context_unreference(ring->last_context);
> >>
> >>  		ring->default_context = NULL;
> >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>index 5c3f57fed916..b8a7e126d6d2 100644
> >>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>@@ -918,6 +918,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
> >>  	struct intel_engine_cs  *ring = params->ring;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_ringbuffer *ringbuf = params->ctx->engine[ring->id].ringbuf;
> >>+	struct intel_context    *ctx = params->request->ctx;
> >>  	u64 exec_start;
> >>  	int instp_mode;
> >>  	u32 instp_mask;
> >>@@ -982,6 +983,12 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
> >>
> >>  	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
> >>
> >>+	if (ring->last_context && ring->last_context != ctx) {
> >>+		intel_lr_context_unpin(ring->last_context, ring);
> >>+		intel_lr_context_pin(ctx, ring);
> >>+		ring->last_context = ctx;
> >>+	}
> >
> >I think this is the wrong location and should be part of submitting the
> >context inside the engine (because intel_execlists_submission should not
> >as it is entirely duplicating the common GEM batch submision code and
> >the unique part is engine->add_request()).
> 
> So into engine->emit_request you are saying? That works just as well
> AFAICS, just making sure I understood correctly.

Oh, yeah you haven't got the unify add_request/emit_request patch...

So gen8_emit_request(), but considering the other patches in flight,
intel_logical_ring_advance_and_submit() will be the ultimate location.
-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

Re: [PATCH 3/3] drm/i915: Fix premature LRC unpin in GuC mode

[Nick Hoath]

On 20/01/2016 14:06, Tvrtko Ursulin wrote:
>
> On 20/01/16 13:55, Chris Wilson wrote:
>> On Wed, Jan 20, 2016 at 01:40:57PM +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.
>>>
>>> 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>
>>> ---
>>> I cannot test this with GuC but it passes BAT with execlists
>>> and some real world smoke tests.
>>> ---
>>>    drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
>>>    drivers/gpu/drm/i915/intel_lrc.c        | 7 +++++++
>>>    2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>> index c25083c78ba7..0b419e165836 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>> @@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
>>>    	for (i = 0; i < I915_NUM_RINGS; i++) {
>>>    		struct intel_engine_cs *ring = &dev_priv->ring[i];
>>>
>>> -		if (ring->last_context)
>>> +		if (ring->last_context && i915.enable_execlists)
>>> +			intel_lr_context_unpin(ring->last_context, ring);
>>> +		else if (ring->last_context)
>>>    			i915_gem_context_unreference(ring->last_context);
>>>
>>>    		ring->default_context = NULL;
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 5c3f57fed916..b8a7e126d6d2 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -918,6 +918,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>>>    	struct intel_engine_cs  *ring = params->ring;
>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
>>>    	struct intel_ringbuffer *ringbuf = params->ctx->engine[ring->id].ringbuf;
>>> +	struct intel_context    *ctx = params->request->ctx;
>>>    	u64 exec_start;
>>>    	int instp_mode;
>>>    	u32 instp_mask;
>>> @@ -982,6 +983,12 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>>>
>>>    	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>>>
>>> +	if (ring->last_context && ring->last_context != ctx) {
>>> +		intel_lr_context_unpin(ring->last_context, ring);
>>> +		intel_lr_context_pin(ctx, ring);
>>> +		ring->last_context = ctx;
>>> +	}
>>
>> I think this is the wrong location and should be part of submitting the
>> context inside the engine (because intel_execlists_submission should not
>> as it is entirely duplicating the common GEM batch submision code and
>> the unique part is engine->add_request()).
>
> So into engine->emit_request you are saying? That works just as well
> AFAICS, just making sure I understood correctly.

I think it should go in to intel_logical_ring_advance_and_submit. The 
extra pinning is being put in place to cover GPU usage of the pin. It 
should probably therefore go in to the last common place between 
execlists & GUC, as close to hardware submission as possible.

>
>> Note that it should be:
>>
>> if (engine->last_context != request->ctx) {
>> 	if (engine->last_context)
>> 		intel_lr_context_unpin(engine->last_context, engine);
>> 	engine->last_context = request->ctx;
>> 	intel_lr_context_pin(engine->last_context, engine);
>> }
>
> Ooops!
>
> Regards,
>
> Tvrtko
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Make LRC (un)pinning work on context and engine

[Patchwork]

== Summary ==

Built on e9545b0b60b73d8be3d41048af5b8f2c1e2fc4c1 drm-intel-nightly: 2016y-01m-20d-13h-55m-37s UTC integration manifest

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                dmesg-warn -> PASS       (skl-i5k-2)

bdw-nuci7        total:143  pass:134  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:143  pass:135  dwarn:1   dfail:1   fail:0   skip:6  
bsw-nuc-2        total:143  pass:117  dwarn:2   dfail:0   fail:0   skip:24 
byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:143  pass:104  dwarn:1   dfail:0   fail:0   skip:38 
ivb-t430s        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:129  dwarn:0   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:129  dwarn:0   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1232/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Fix premature LRC unpin in GuC mode

[Tvrtko Ursulin]

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.

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>
---
I cannot test this with GuC but it passes BAT with execlists
and some real world smoke tests.
---
 drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
 drivers/gpu/drm/i915/intel_lrc.c        | 9 ++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index c25083c78ba7..0b419e165836 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
 
-		if (ring->last_context)
+		if (ring->last_context && i915.enable_execlists)
+			intel_lr_context_unpin(ring->last_context, ring);
+		else if (ring->last_context)
 			i915_gem_context_unreference(ring->last_context);
 
 		ring->default_context = NULL;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e7c273acac1e..72ed176e091d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1120,7 +1120,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;
@@ -1859,6 +1859,13 @@ 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);
+		intel_lr_context_pin(request->ctx, ring);
+		ring->last_context = request->ctx;
+	}
+
 	/*
 	 * 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

Re: [PATCH v2] drm/i915: Fix premature LRC unpin in GuC mode

[Chris Wilson]

On Wed, Jan 20, 2016 at 02:50:47PM +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.
> 
> 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>
> ---
> I cannot test this with GuC but it passes BAT with execlists
> and some real world smoke tests.
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 4 +++-
>  drivers/gpu/drm/i915/intel_lrc.c        | 9 ++++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index c25083c78ba7..0b419e165836 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	for (i = 0; i < I915_NUM_RINGS; i++) {
>  		struct intel_engine_cs *ring = &dev_priv->ring[i];
>  

This is the nasty part where the GPU still has access to the backing
pages as we release them. A hard to hit module-unload vs active GPU for
sure, but it is something that we can prevent.

The context-fini vs engine-fini ordering is also apparently tied to the
use of intel_gpu_reset() here (as it clobbers the GPU without dropping
the GEM state, causing a foul up when tearing down the engine).

If we had actually called i915_reset (and so i915_gem_reset) instead, we
should expect last_context to be NULL... Oops, Tvrtko you need to
inspect i915_gem_context_reset() as well for reseting last_context back
to NULL after the GPU reset.

As for here, I would just pencil in a plan to replace this chunk
entirely with a call to i915_reset(), or a slightly trimmed down
version and tidy up the active GEM cleanup in the process.
-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