| Message ID | 1415021344-24875-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 03, 2014 at 01:29:04PM +0000, Dave Gordon wrote: > 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. What? Not unless someone had broken it... This is the code I expect to see here based on requests: static int ring_wait(struct intel_ringbuffer *ring, int n) { int ret; trace_intel_ringbuffer_wait(ring, n); do { struct i915_gem_request *rq; i915_gem_retire_requests__engine(ring->engine); if (ring->retired_head != -1) { ring->head = ring->retired_head; ring->retired_head = -1; ring->space = intel_ring_space(ring); if (ring->space >= n) return 0; } list_for_each_entry(rq, &ring->breadcrumbs, breadcrumb_link) if (__intel_ring_space(rq->tail, ring->tail, ring->size, I915_RING_RSVD) >= n) break; if (WARN_ON(&rq->breadcrumb_link == &ring->breadcrumbs)) return -EDEADLK; ret = i915_request_wait(rq); } while (ret == 0); return ret; } and that works for both execlists and regular... -Chris
On 03/11/14 20:59, Chris Wilson wrote: > On Mon, Nov 03, 2014 at 01:29:04PM +0000, Dave Gordon wrote: >> 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. > > What? Not unless someone had broken it... > > This is the code I expect to see here based on requests: > > static int ring_wait(struct intel_ringbuffer *ring, int n) > { > int ret; > > trace_intel_ringbuffer_wait(ring, n); > > do { > struct i915_gem_request *rq; > > i915_gem_retire_requests__engine(ring->engine); > if (ring->retired_head != -1) { > ring->head = ring->retired_head; > ring->retired_head = -1; > > ring->space = intel_ring_space(ring); > if (ring->space >= n) > return 0; > } > > list_for_each_entry(rq, &ring->breadcrumbs, breadcrumb_link) > if (__intel_ring_space(rq->tail, ring->tail, > ring->size, I915_RING_RSVD) >= n) > break; > > if (WARN_ON(&rq->breadcrumb_link == &ring->breadcrumbs)) > return -EDEADLK; > > ret = i915_request_wait(rq); > } while (ret == 0); > > return ret; > } I like this sample code better than the version currently in the driver, because it, like my patch, has only one place where the value is transferred from 'retired_head' to 'head', and it's clearly guarded by a test that 'retired_head' is not -1. But this isn't actually the code we have, which has four places where this copying is done (two for LRC and two for legacy), and one in each path is not so guarded. So *that's* what I'm fixing. It looks like this code also assumes one request queue per ringbuffer, rather than one per engine, which may be nicer but isn't what we currently have. > and that works for both execlists and regular... > -Chris My calculate-freespace and update-freespace functions are likewise LRC-agnostic, although the wait_for_request/wait_for_space functions that they're called from have (regrettably) been duplicated. Not my call, though. Finally, my new code is more resilient against misuse than the current version. At present, if a sequence of emit()s overruns the allocated length (possibly due to display code injecting extra instruction without setting up its own ring_begin/add_request pair, or simply a bug), then the ring space calculation can in some cases claim that there's suddenly a LOT of space (due to the way the modulo arithmetic is done) and confusion will follow. My version will return "no space" rather than "a lot of space" in this situation. BTW, I have some local patches which enforce strict checking of ring_begin/add_request pairing and generates warnings if there's a mismatch or an overrun or any other misuse. We've been using these to help identify and eliminate code that just stuffs instructions into the ring without notifying the scheduler. Interested? .Dave.
On Tue, Nov 04, 2014 at 02:17:07PM +0000, Dave Gordon wrote: > BTW, I have some local patches which enforce strict checking of > ring_begin/add_request pairing and generates warnings if there's a > mismatch or an overrun or any other misuse. We've been using these to > help identify and eliminate code that just stuffs instructions into the > ring without notifying the scheduler. Interested? Definitely (under the assumption that this helped tracked down bugs ofc). -Daniel
Reviewed the patch & it looks fine. Reviewed-by: "Akash Goel <akash.goels@gmail.com>" On Mon, Nov 3, 2014 at 6:59 PM, Dave Gordon <david.s.gordon@intel.com> wrote: > 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 <david.s.gordon@intel.com> > --- > 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 > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Nov 03, 2014 at 01:29:04PM +0000, Dave Gordon wrote: > 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 <david.s.gordon@intel.com> Is there an igt testcase which readily reproduces this? Or can we have one? -Daniel
On Monday 03 November 2014 06:59 PM, Dave Gordon wrote: > 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 <david.s.gordon@intel.com> > --- > 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); Should this be in another patch? Other than this other changes looks fine to me.\ Also, are you planning to add WARN_ON if there is a mismatch with ring_begin & add_request? > 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);
On 18/11/14 15:00, Deepak S wrote: > > On Monday 03 November 2014 06:59 PM, Dave Gordon wrote: >> 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 <david.s.gordon@intel.com> >> --- >> 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(-) >> [snip] >> 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); > > Should this be in another patch? > > Other than this other changes looks fine to me.\ > Also, are you planning to add WARN_ON if there is a mismatch with > ring_begin & add_request? Yes, that's another patch that I'll be sending out soon; it also checks for various other mistakes in ring management. Meanwhile I've decided to move the line above into that patch rather than this one; also, I've refactored this patch to break out the two sections that fix specific bugs in the LRC code, so I shall send out a new version shortly. .Dave.
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.
On Tue, Nov 18, 2014 at 9:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Nov 03, 2014 at 01:29:04PM +0000, Dave Gordon wrote: >> 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 <david.s.gordon@intel.com> > > Is there an igt testcase which readily reproduces this? Or can we have > one? Ping. Bugfixes really should have either a Testcase: line in the commit message indicating the igt used to reproduce/validate the fix or an explanation why testing with an igt case is not possible. -Daniel
Essentially the same as patch 3/3 from v2 of this set, but split into two patches; one to improve the robustness of the freespace calculation, and then one to update all the various places that calculate free space to call the new improved code. See individual commit messages for more detail.
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);
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 <david.s.gordon@intel.com> --- 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(-)