On Mon, May 05, 2014 at 01:07:33AM -0700, Chris Wilson wrote: > During the review of > > commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42 > Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > 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 <chris@xxxxxxxxxxxxxxxxxx> > 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.volkin@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
