<div dir="ltr"><div style="font-family:arial,sans-serif;font-size:13.3333339691162px">Reviewed the patch & it looks fine.</div><div style="font-family:arial,sans-serif;font-size:13.3333339691162px">Reviewed-by: "Akash Goel <<a href="mailto:akash.goels@gmail.com" target="_blank">akash.goels@gmail.com</a>>"</div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Nov 3, 2014 at 6:59 PM, Dave Gordon <<a href="mailto:david.s.gordon@intel.com" target="_blank">david.s.gordon@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Fixes to both the LRC and the legacy ringbuffer code to correctly calculate and update the available space in a ring. The logical ring code was updating the software ring 'head' value by reading the hardware 'HEAD' register. In LRC mode, this is not valid as the hardware is not necessarily executing the same context that is being processed by the software. Thus reading the h/w HEAD could put an unrelated (undefined, effectively random) value into the s/w 'head' -- A Bad Thing for the free space calculations. In addition, the old code could update a ringbuffer's 'head' value from the 'last_retired_head' even when the latter hadn't been recently updated and therefore had a value of -1; this would also confuse the freespace calculations. Now, we consume 'last_retired_head' in just one place, ensuring that this confusion does not arise. Change-Id: Id7ce9096ed100a2882c68a54206f30b6c87e92fa Signed-off-by: Dave Gordon <<a href="mailto:david.s.gordon@intel.com">david.s.gordon@intel.com</a>> --- drivers/gpu/drm/i915/i915_dma.c | 5 ++- drivers/gpu/drm/i915/intel_lrc.c | 36 ++++++++++----------- drivers/gpu/drm/i915/intel_ringbuffer.c | 53 ++++++++++++++++--------------- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 4 files changed, 48 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 9a73533..1646416 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -154,11 +154,10 @@ void i915_kernel_lost_context(struct drm_device *dev) if (drm_core_check_feature(dev, DRIVER_MODESET)) return; + ringbuf->last_retired_head = -1; ringbuf->head = I915_READ_HEAD(ring) & HEAD_ADDR; ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR; - ringbuf->space = ringbuf->head - (ringbuf->tail + I915_RING_FREE_SPACE); - if (ringbuf->space < 0) - ringbuf->space += ringbuf->size; + intel_ring_update_space(ringbuf); if (!dev->primary->master) return; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index cd74e5c..11a9047 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -827,16 +827,20 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, u32 seqno = 0; int ret; - if (ringbuf->last_retired_head != -1) { - ringbuf->head = ringbuf->last_retired_head; - ringbuf->last_retired_head = -1; - - ringbuf->space = intel_ring_space(ringbuf); - if (ringbuf->space >= bytes) - return 0; - } + if (intel_ring_space(ringbuf) >= bytes) + return 0; list_for_each_entry(request, &ring->request_list, list) { + /* + * The request queue is per-engine, so can contain requests + * from multiple ringbuffers. Here, we must ignore any that + * aren't from the ringbuffer we're considering. + */ + struct intel_context *ctx = request->ctx; + if (ctx->engine[ring->id].ringbuf != ringbuf) + continue; + + /* Would completion of this request free enough space? */ if (__intel_ring_space(request->tail, ringbuf->tail, ringbuf->size) >= bytes) { seqno = request->seqno; @@ -852,11 +856,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, return ret; i915_gem_retire_requests_ring(ring); - ringbuf->head = ringbuf->last_retired_head; - ringbuf->last_retired_head = -1; - ringbuf->space = intel_ring_space(ringbuf); - return 0; + return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC; } static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf, @@ -882,13 +883,10 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf, * case by choosing an insanely large timeout. */ end = jiffies + 60 * HZ; + ret = 0; do { - ringbuf->head = I915_READ_HEAD(ring); - ringbuf->space = intel_ring_space(ringbuf); - if (ringbuf->space >= bytes) { - ret = 0; + if (intel_ring_space(ringbuf) >= bytes) break; - } msleep(1); @@ -929,7 +927,7 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf) iowrite32(MI_NOOP, virt++); ringbuf->tail = 0; - ringbuf->space = intel_ring_space(ringbuf); + intel_ring_update_space(ringbuf); return 0; } @@ -1708,8 +1706,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, ringbuf->effective_size = ringbuf->size; ringbuf->head = 0; ringbuf->tail = 0; - ringbuf->space = ringbuf->size; ringbuf->last_retired_head = -1; + intel_ring_update_space(ringbuf); /* TODO: For now we put this in the mappable region so that we can reuse * the existing ringbuffer code which ioremaps it. When we start diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index a8f72e8..1150862 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -52,16 +52,27 @@ intel_ring_initialized(struct intel_engine_cs *ring) int __intel_ring_space(int head, int tail, int size) { - int space = head - (tail + I915_RING_FREE_SPACE); - if (space < 0) + int space = head - tail; + if (space <= 0) space += size; - return space; + return space - I915_RING_FREE_SPACE; +} + +void intel_ring_update_space(struct intel_ringbuffer *ringbuf) +{ + if (ringbuf->last_retired_head != -1) { + ringbuf->head = ringbuf->last_retired_head; + ringbuf->last_retired_head = -1; + } + + ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR, + ringbuf->tail, ringbuf->size); } int intel_ring_space(struct intel_ringbuffer *ringbuf) { - return __intel_ring_space(ringbuf->head & HEAD_ADDR, - ringbuf->tail, ringbuf->size); + intel_ring_update_space(ringbuf); + return ringbuf->space; } bool intel_ring_stopped(struct intel_engine_cs *ring) @@ -73,7 +84,7 @@ bool intel_ring_stopped(struct intel_engine_cs *ring) void __intel_ring_advance(struct intel_engine_cs *ring) { struct intel_ringbuffer *ringbuf = ring->buffer; - ringbuf->tail &= ringbuf->size - 1; + intel_ring_advance(ring); if (intel_ring_stopped(ring)) return; ring->write_tail(ring, ringbuf->tail); @@ -592,10 +603,10 @@ static int init_ring_common(struct intel_engine_cs *ring) if (!drm_core_check_feature(ring->dev, DRIVER_MODESET)) i915_kernel_lost_context(ring->dev); else { + ringbuf->last_retired_head = -1; ringbuf->head = I915_READ_HEAD(ring); ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR; - ringbuf->space = intel_ring_space(ringbuf); - ringbuf->last_retired_head = -1; + intel_ring_update_space(ringbuf); } memset(&ring->hangcheck, 0, sizeof(ring->hangcheck)); @@ -1876,14 +1887,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) u32 seqno = 0; int ret; - if (ringbuf->last_retired_head != -1) { - ringbuf->head = ringbuf->last_retired_head; - ringbuf->last_retired_head = -1; - - ringbuf->space = intel_ring_space(ringbuf); - if (ringbuf->space >= n) - return 0; - } + if (intel_ring_space(ringbuf) >= n) + return 0; list_for_each_entry(request, &ring->request_list, list) { if (__intel_ring_space(request->tail, ringbuf->tail, @@ -1901,10 +1906,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) return ret; i915_gem_retire_requests_ring(ring); - ringbuf->head = ringbuf->last_retired_head; - ringbuf->last_retired_head = -1; - ringbuf->space = intel_ring_space(ringbuf); return 0; } @@ -1930,14 +1932,14 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n) * case by choosing an insanely large timeout. */ end = jiffies + 60 * HZ; + ret = 0; trace_i915_ring_wait_begin(ring); do { + if (intel_ring_space(ringbuf) >= n) + break; ringbuf->head = I915_READ_HEAD(ring); - ringbuf->space = intel_ring_space(ringbuf); - if (ringbuf->space >= n) { - ret = 0; + if (intel_ring_space(ringbuf) >= n) break; - } if (!drm_core_check_feature(dev, DRIVER_MODESET) && dev->primary->master) { @@ -1985,7 +1987,7 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring) iowrite32(MI_NOOP, virt++); ringbuf->tail = 0; - ringbuf->space = intel_ring_space(ringbuf); + intel_ring_update_space(ringbuf); return 0; } @@ -2057,6 +2059,7 @@ int intel_ring_begin(struct intel_engine_cs *ring, int num_dwords) { struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct intel_ringbuffer *ringbuf = ring->buffer; int ret; ret = i915_gem_check_wedge(&dev_priv->gpu_error, @@ -2073,7 +2076,7 @@ int intel_ring_begin(struct intel_engine_cs *ring, if (ret) return ret; - ring->buffer->space -= num_dwords * sizeof(uint32_t); + ringbuf->space -= num_dwords * sizeof(uint32_t); return 0; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 96479c8..2a1e484 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -403,6 +403,7 @@ static inline void intel_ring_advance(struct intel_engine_cs *ring) ringbuf->tail &= ringbuf->size - 1; } int __intel_ring_space(int head, int tail, int size); +void intel_ring_update_space(struct intel_ringbuffer *ringbuf); int intel_ring_space(struct intel_ringbuffer *ringbuf); bool intel_ring_stopped(struct intel_engine_cs *ring); void __intel_ring_advance(struct intel_engine_cs *ring); -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div> </div></div>