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 +0000
>
> 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 = -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;
> + ring->last_retired_head = -1;
>
> + ring->space = ring_space(ring);
> return 0;
> }
>
> --
> 2.0.0.rc0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
