Re: [Intel-gfx] [PATCH 2/2] drm/i915: Flush request queue when waiting for ring space

2014-05-07 Thread Volkin, Bradley D 
On Mon, May 05, 2014 at 01:07:33AM -0700, Chris Wilson wrote:
 During the review of
 
 commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42
 Author: Chris Wilson ch...@chris-wilson.co.uk
 Date:   Mon Jan 27 22:43:07 2014 +
 
 drm/i915: Prevent recursion by retiring requests when the ring is full
 
 Ville raised the point that our interaction with request-tail was
 likely to foul up other uses elsewhere (such as hang check comparing
 ACTHD against requests).
 
 However, we also need to restore the implicit retire requests that certain
 test cases depend upon (e.g. igt/gem_exec_lut_handle), this raises the
 spectre that the ppgtt will randomly call i915_gpu_idle() and recurse
 back into intel_ring_begin().
 
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78023

There's one minor cleanup suggested below.

Overall I think the code is better after the patch. I don't really like 
reintroducing the potential recursion, but I suppose that's a separate
issue. With the cleanup...

Reviewed-by: Brad Volkin bradley.d.vol...@intel.com

 ---
  drivers/gpu/drm/i915/i915_drv.h |  1 +
  drivers/gpu/drm/i915/i915_gem.c |  3 +--
  drivers/gpu/drm/i915/intel_ringbuffer.c | 36 
 -
  3 files changed, 15 insertions(+), 25 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
 index 5f4f631aecef..69a4e161ff37 100644
 --- a/drivers/gpu/drm/i915/i915_drv.h
 +++ b/drivers/gpu/drm/i915/i915_drv.h
 @@ -2155,6 +2155,7 @@ struct drm_i915_gem_request *
  i915_gem_find_active_request(struct intel_ring_buffer *ring);
  
  bool i915_gem_retire_requests(struct drm_device *dev);
 +void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
  int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
 bool interruptible);
  static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
 diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
 index dae51c3701f3..2f2a668c01ac 100644
 --- a/drivers/gpu/drm/i915/i915_gem.c
 +++ b/drivers/gpu/drm/i915/i915_gem.c
 @@ -64,7 +64,6 @@ static unsigned long i915_gem_inactive_scan(struct shrinker 
 *shrinker,
  static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long 
 target);
  static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
  static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
 -static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
  
  static bool cpu_cache_is_coherent(struct drm_device *dev,
 enum i915_cache_level level)
 @@ -2448,7 +2447,7 @@ void i915_gem_reset(struct drm_device *dev)
  /**
   * This function clears the request list as sequence numbers are passed.
   */
 -static void
 +void
  i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
  {
   uint32_t seqno;
 diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
 b/drivers/gpu/drm/i915/intel_ringbuffer.c
 index d6b7b884adf9..9dce15cbce73 100644
 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
 +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
 @@ -40,14 +40,19 @@
   */
  #define CACHELINE_BYTES 64
  
 -static inline int ring_space(struct intel_ring_buffer *ring)
 +static inline int __ring_space(int head, int tail, int size)
  {
 - int space = (ring-head  HEAD_ADDR) - (ring-tail + 
 I915_RING_FREE_SPACE);
 + int space = head - (tail + I915_RING_FREE_SPACE);
   if (space  0)
 - space += ring-size;
 + space += size;
   return space;
  }
  
 +static inline int ring_space(struct intel_ring_buffer *ring)
 +{
 + return __ring_space(ring-head  HEAD_ADDR, ring-tail, ring-size);
 +}
 +
  static bool intel_ring_stopped(struct intel_ring_buffer *ring)
  {
   struct drm_i915_private *dev_priv = ring-dev-dev_private;
 @@ -1495,26 +1500,11 @@ static int intel_ring_wait_request(struct 
 intel_ring_buffer *ring, int n)
   }
  
   list_for_each_entry(request, ring-request_list, list) {
 - int space;
 -
 - if (request-tail == -1)
 - continue;
 -
 - space = request-tail - (ring-tail + I915_RING_FREE_SPACE);
 - if (space  0)
 - space += ring-size;
 - if (space = n) {
 + if (__ring_space(request-tail, ring-tail, ring-size) = n) {
   seqno = request-seqno;
   tail = request-tail;

We don't actually use the local 'tail' anymore.

   break;
   }
 -
 - /* Consume this request in case we need more space than
 -  * is available and so need to prevent a race between
 -  * updating last_retired_head and direct reads of
 -  * I915_RING_HEAD. It also provides a nice sanity check.
 -  */
 - request-tail

[Intel-gfx] [PATCH 2/2] drm/i915: Flush request queue when waiting for ring space

2014-05-05 Thread Chris Wilson 
During the review of

commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42
Author: Chris Wilson ch...@chris-wilson.co.uk
Date:   Mon Jan 27 22:43:07 2014 +

drm/i915: Prevent recursion by retiring requests when the ring is full

Ville raised the point that our interaction with request-tail was
likely to foul up other uses elsewhere (such as hang check comparing
ACTHD against requests).

However, we also need to restore the implicit retire requests that certain
test cases depend upon (e.g. igt/gem_exec_lut_handle), this raises the
spectre that the ppgtt will randomly call i915_gpu_idle() and recurse
back into intel_ring_begin().

Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78023
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c |  3 +--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 36 -
 3 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5f4f631aecef..69a4e161ff37 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2155,6 +2155,7 @@ struct drm_i915_gem_request *
 i915_gem_find_active_request(struct intel_ring_buffer *ring);
 
 bool i915_gem_retire_requests(struct drm_device *dev);
+void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
 int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
  bool interruptible);
 static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dae51c3701f3..2f2a668c01ac 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -64,7 +64,6 @@ static unsigned long i915_gem_inactive_scan(struct shrinker 
*shrinker,
 static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long 
target);
 static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
 static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
-static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
 
 static bool cpu_cache_is_coherent(struct drm_device *dev,
  enum i915_cache_level level)
@@ -2448,7 +2447,7 @@ void i915_gem_reset(struct drm_device *dev)
 /**
  * This function clears the request list as sequence numbers are passed.
  */
-static void
+void
 i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
 {
uint32_t seqno;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d6b7b884adf9..9dce15cbce73 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -40,14 +40,19 @@
  */
 #define CACHELINE_BYTES 64
 
-static inline int ring_space(struct intel_ring_buffer *ring)
+static inline int __ring_space(int head, int tail, int size)
 {
-   int space = (ring-head  HEAD_ADDR) - (ring-tail + 
I915_RING_FREE_SPACE);
+   int space = head - (tail + I915_RING_FREE_SPACE);
if (space  0)
-   space += ring-size;
+   space += size;
return space;
 }
 
+static inline int ring_space(struct intel_ring_buffer *ring)
+{
+   return __ring_space(ring-head  HEAD_ADDR, ring-tail, ring-size);
+}
+
 static bool intel_ring_stopped(struct intel_ring_buffer *ring)
 {
struct drm_i915_private *dev_priv = ring-dev-dev_private;
@@ -1495,26 +1500,11 @@ static int intel_ring_wait_request(struct 
intel_ring_buffer *ring, int n)
}
 
list_for_each_entry(request, ring-request_list, list) {
-   int space;
-
-   if (request-tail == -1)
-   continue;
-
-   space = request-tail - (ring-tail + I915_RING_FREE_SPACE);
-   if (space  0)
-   space += ring-size;
-   if (space = n) {
+   if (__ring_space(request-tail, ring-tail, ring-size) = n) {
seqno = request-seqno;
tail = request-tail;
break;
}
-
-   /* Consume this request in case we need more space than
-* is available and so need to prevent a race between
-* updating last_retired_head and direct reads of
-* I915_RING_HEAD. It also provides a nice sanity check.
-*/
-   request-tail = -1;
}
 
if (seqno == 0)
@@ -1524,11 +1514,11 @@ static int intel_ring_wait_request(struct 
intel_ring_buffer *ring, int n)
if (ret)
return ret;
 
-   ring-head = tail;
-   ring-space = ring_space(ring);
-   if (WARN_ON(ring-space  n))
-   return -ENOSPC;
+   i915_gem_retire_requests_ring(ring);
+   ring-head = ring-last_retired_head;
+

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Flush request queue when waiting for ring space

2014-05-05 Thread Ben Widawsky 
On Mon, May 05, 2014 at 09:07:33AM +0100, Chris Wilson wrote:
 During the review of
 
 commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42
 Author: Chris Wilson ch...@chris-wilson.co.uk
 Date:   Mon Jan 27 22:43:07 2014 +
 
 drm/i915: Prevent recursion by retiring requests when the ring is full
 
 Ville raised the point that our interaction with request-tail was
 likely to foul up other uses elsewhere (such as hang check comparing
 ACTHD against requests).
 
 However, we also need to restore the implicit retire requests that certain
 test cases depend upon (e.g. igt/gem_exec_lut_handle), this raises the
 spectre that the ppgtt will randomly call i915_gpu_idle() and recurse
 back into intel_ring_begin().

Forgive my ignorance. Why is i915_gpu_idle() randomly being called for
PPGTT? I don't see anything PPGTT specific here.

[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Flush request queue when waiting for ring space

2014-05-05 Thread Chris Wilson 
On Mon, May 05, 2014 at 11:49:12AM -0700, Ben Widawsky wrote:
 On Mon, May 05, 2014 at 09:07:33AM +0100, Chris Wilson wrote:
  During the review of
  
  commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42
  Author: Chris Wilson ch...@chris-wilson.co.uk
  Date:   Mon Jan 27 22:43:07 2014 +
  
  drm/i915: Prevent recursion by retiring requests when the ring is full
  
  Ville raised the point that our interaction with request-tail was
  likely to foul up other uses elsewhere (such as hang check comparing
  ACTHD against requests).
  
  However, we also need to restore the implicit retire requests that certain
  test cases depend upon (e.g. igt/gem_exec_lut_handle), this raises the
  spectre that the ppgtt will randomly call i915_gpu_idle() and recurse
  back into intel_ring_begin().
 
 Forgive my ignorance. Why is i915_gpu_idle() randomly being called for
 PPGTT? I don't see anything PPGTT specific here.

commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42
Author: Chris Wilson ch...@chris-wilson.co.uk
Date:   Mon Jan 27 22:43:07 2014 +

drm/i915: Prevent recursion by retiring requests when the ring is full

As the VM do not track activity of objects and instead use a large
hammer to forcibly idle and evict all of their associated objects when
one is released, it is possible for that to cause a recursion when we
need to wait for free space on a ring and call retire requests.
(intel_ring_begin - intel_ring_wait_request -
i915_gem_retire_requests_ring - i915_gem_context_free -
i915_gem_evict_vm - i915_gpu_idle - intel_ring_begin etc)

In order to remove the requirement for calling retire-requests from
intel_ring_wait_request, we have to inline a couple of steps from
retiring requests, notably we have to record the position of the request
we wait for and use that to update the available ring space.
-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