Re: [Intel-gfx] [PATCH] drm/i915: Extend LRC pinning to cover GPU context writeback

2015-12-03 Thread Daniel Vetter 
On Wed, Dec 02, 2015 at 05:02:07PM -0800, Yu Dai wrote:
> I tested this with GuC submission enabled and it fixes the issue I found
> during GPU reset.
> 
> Reviewed-by: Alex Dai 

Queued for -next, thanks for the patch.
-Daniel

> 
> On 12/01/2015 06:48 AM, Nick Hoath wrote:
> >Use the first retired request on a new context to unpin
> >the old context. This ensures that the hw context remains
> >bound until it has been written back to by the GPU.
> >Now that the context is pinned until later in the request/context
> >lifecycle, it no longer needs to be pinned from context_queue to
> >retire_requests.
> >This fixes an issue with GuC submission where the GPU might not
> >have finished writing back the context before it is unpinned. This
> >results in a GPU hang.
> >
> >v2: Moved the new pin to cover GuC submission (Alex Dai)
> > Moved the new unpin to request_retire to fix coverage leak
> >v3: Added switch to default context if freeing a still pinned
> > context just in case the hw was actually still using it
> >v4: Unwrapped context unpin to allow calling without a request
> >v5: Only create a switch to idle context if the ring doesn't
> > already have a request pending on it (Alex Dai)
> > Rename unsaved to dirty to avoid double negatives (Dave Gordon)
> > Changed _no_req postfix to __ prefix for consistency (Dave Gordon)
> > Split out per engine cleanup from context_free as it
> > was getting unwieldy
> > Corrected locking (Dave Gordon)
> >v6: Removed some bikeshedding (Mika Kuoppala)
> > Added explanation of the GuC hang that this fixes (Daniel Vetter)
> >v7: Removed extra per request pinning from ring reset code (Alex Dai)
> > Added forced ring unpin/clean in error case in context free (Alex Dai)
> >
> >Signed-off-by: Nick Hoath 
> >Issue: VIZ-4277
> >Cc: Daniel Vetter 
> >Cc: David Gordon 
> >Cc: Chris Wilson 
> >Cc: Alex Dai 
> >Cc: Mika Kuoppala 
> >---
> >  drivers/gpu/drm/i915/i915_drv.h  |   1 +
> >  drivers/gpu/drm/i915/i915_gem.c  |   7 +-
> >  drivers/gpu/drm/i915/intel_lrc.c | 136 
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.h |   1 +
> >  4 files changed, 118 insertions(+), 27 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> >b/drivers/gpu/drm/i915/i915_drv.h
> >index d5cf30b..e82717a 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -889,6 +889,7 @@ struct intel_context {
> > struct {
> > struct drm_i915_gem_object *state;
> > struct intel_ringbuffer *ringbuf;
> >+bool dirty;
> > int pin_count;
> > } engine[I915_NUM_RINGS];
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> >b/drivers/gpu/drm/i915/i915_gem.c
> >index e955499..69e9d96 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -1354,6 +1354,9 @@ static void i915_gem_request_retire(struct 
> >drm_i915_gem_request *request)
> >  {
> > trace_i915_gem_request_retire(request);
> >+if (i915.enable_execlists)
> >+intel_lr_context_complete_check(request);
> >+
> > /* We know the GPU must have read the request to have
> >  * sent us the seqno + interrupt, so use the position
> >  * of tail of the request to update the last known position
> >@@ -2765,10 +2768,6 @@ static void i915_gem_reset_ring_cleanup(struct 
> >drm_i915_private *dev_priv,
> > struct drm_i915_gem_request,
> > execlist_link);
> > list_del(_req->execlist_link);
> >-
> >-if (submit_req->ctx != ring->default_context)
> >-intel_lr_context_unpin(submit_req);
> >-
> > i915_gem_request_unreference(submit_req);
> > }
> > spin_unlock_irq(>execlist_lock);
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> >b/drivers/gpu/drm/i915/intel_lrc.c
> >index 06180dc..b4d9c8f 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -566,9 +566,6 @@ static int execlists_context_queue(struct 
> >drm_i915_gem_request *request)
> > struct drm_i915_gem_request *cursor;
> > int num_elements = 0;
> >-if (request->ctx != ring->default_context)
> >-intel_lr_context_pin(request);
> >-
> > i915_gem_request_reference(request);
> > spin_lock_irq(>execlist_lock);
> >@@ -732,6 +729,13 @@ intel_logical_ring_advance_and_submit(struct 
> >drm_i915_gem_request *request)
> > if (intel_ring_stopped(ring))
> > return;
> >+if (request->ctx != ring->default_context) {
> >+if (!request->ctx->engine[ring->id].dirty) {
> >+intel_lr_context_pin(request);
> >+

Re: [Intel-gfx] [PATCH] drm/i915: Extend LRC pinning to cover GPU context writeback

2015-12-02 Thread Yu Dai 
I tested this with GuC submission enabled and it fixes the issue I found 
during GPU reset.


Reviewed-by: Alex Dai 

On 12/01/2015 06:48 AM, Nick Hoath wrote:

Use the first retired request on a new context to unpin
the old context. This ensures that the hw context remains
bound until it has been written back to by the GPU.
Now that the context is pinned until later in the request/context
lifecycle, it no longer needs to be pinned from context_queue to
retire_requests.
This fixes an issue with GuC submission where the GPU might not
have finished writing back the context before it is unpinned. This
results in a GPU hang.

v2: Moved the new pin to cover GuC submission (Alex Dai)
 Moved the new unpin to request_retire to fix coverage leak
v3: Added switch to default context if freeing a still pinned
 context just in case the hw was actually still using it
v4: Unwrapped context unpin to allow calling without a request
v5: Only create a switch to idle context if the ring doesn't
 already have a request pending on it (Alex Dai)
 Rename unsaved to dirty to avoid double negatives (Dave Gordon)
 Changed _no_req postfix to __ prefix for consistency (Dave Gordon)
 Split out per engine cleanup from context_free as it
 was getting unwieldy
 Corrected locking (Dave Gordon)
v6: Removed some bikeshedding (Mika Kuoppala)
 Added explanation of the GuC hang that this fixes (Daniel Vetter)
v7: Removed extra per request pinning from ring reset code (Alex Dai)
 Added forced ring unpin/clean in error case in context free (Alex Dai)

Signed-off-by: Nick Hoath 
Issue: VIZ-4277
Cc: Daniel Vetter 
Cc: David Gordon 
Cc: Chris Wilson 
Cc: Alex Dai 
Cc: Mika Kuoppala 
---
  drivers/gpu/drm/i915/i915_drv.h  |   1 +
  drivers/gpu/drm/i915/i915_gem.c  |   7 +-
  drivers/gpu/drm/i915/intel_lrc.c | 136 ---
  drivers/gpu/drm/i915/intel_lrc.h |   1 +
  4 files changed, 118 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d5cf30b..e82717a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -889,6 +889,7 @@ struct intel_context {
struct {
struct drm_i915_gem_object *state;
struct intel_ringbuffer *ringbuf;
+   bool dirty;
int pin_count;
} engine[I915_NUM_RINGS];
  
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c

index e955499..69e9d96 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1354,6 +1354,9 @@ static void i915_gem_request_retire(struct 
drm_i915_gem_request *request)
  {
trace_i915_gem_request_retire(request);
  
+	if (i915.enable_execlists)

+   intel_lr_context_complete_check(request);
+
/* We know the GPU must have read the request to have
 * sent us the seqno + interrupt, so use the position
 * of tail of the request to update the last known position
@@ -2765,10 +2768,6 @@ static void i915_gem_reset_ring_cleanup(struct 
drm_i915_private *dev_priv,
struct drm_i915_gem_request,
execlist_link);
list_del(_req->execlist_link);
-
-   if (submit_req->ctx != ring->default_context)
-   intel_lr_context_unpin(submit_req);
-
i915_gem_request_unreference(submit_req);
}
spin_unlock_irq(>execlist_lock);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 06180dc..b4d9c8f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -566,9 +566,6 @@ static int execlists_context_queue(struct 
drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
  
-	if (request->ctx != ring->default_context)

-   intel_lr_context_pin(request);
-
i915_gem_request_reference(request);
  
  	spin_lock_irq(>execlist_lock);

@@ -732,6 +729,13 @@ intel_logical_ring_advance_and_submit(struct 
drm_i915_gem_request *request)
if (intel_ring_stopped(ring))
return;
  
+	if (request->ctx != ring->default_context) {

+   if (!request->ctx->engine[ring->id].dirty) {
+   intel_lr_context_pin(request);
+   request->ctx->engine[ring->id].dirty = true;
+   }
+   }
+
if (dev_priv->guc.execbuf_client)
i915_guc_submit(dev_priv->guc.execbuf_client, request);
else
@@ -958,12 +962,6 @@ void intel_execlists_retire_requests(struct 
intel_engine_cs *ring)
spin_unlock_irq(>execlist_lock);