[Intel-gfx] [PATCH] drm/i915: Bug fixes to ring 'head' updating
Deepak S
deepak.s at intel.com
Tue Nov 18 16:00:46 CET 2014
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 at 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);
More information about the Intel-gfx
mailing list