drm/i915: Bug fixes to ring 'head' updating

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.

Quoted 10 lines · Dave Gordon

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.

Quoted 5 lines

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

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?

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 <[email protected]>

---
  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(-)

drivers/gpu/drm/i915/i915_dma.c +2 -3

index 9a73533..1646416 100644
@@ -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;

drivers/gpu/drm/i915/intel_lrc.c +17 -19

index cd74e5c..11a9047 100644
@@ -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

drivers/gpu/drm/i915/intel_ringbuffer.c +16 -5

index a8f72e8..1150862 100644
@@ -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)

drivers/gpu/drm/i915/intel_ringbuffer.c +11 -19

  	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;
  }
 

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -0

index 96479c8..2a1e484 100644

Quoted 26 lines · Dave Gordon

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 <[email protected]>

---
  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(-)

Quoted 36 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +16 -5

index a8f72e8..1150862 100644
@@ -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)

drivers/gpu/drm/i915/intel_ringbuffer.c bool intel_ring_stopped(struct intel_engine_cs *ring) · line 87

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -1

@@ -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?

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_dma.c +2 -3

index 9a73533..1646416 100644
@@ -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;

drivers/gpu/drm/i915/intel_lrc.c +12 -8

index cd74e5c..11a9047 100644
@@ -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;

drivers/gpu/drm/i915/intel_lrc.c +1 -4

@@ -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,

drivers/gpu/drm/i915/intel_lrc.c +3 -6

@@ -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;

}

drivers/gpu/drm/i915/intel_lrc.c +1 -1

@@ -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

drivers/gpu/drm/i915/intel_ringbuffer.c +19 -8

index a8f72e8..1150862 100644
@@ -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));

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -8

@@ -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,

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -7

@@ -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) {

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -1

@@ -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;

}

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -1

@@ -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;
 }
 

drivers/gpu/drm/i915/intel_ringbuffer.h +1 -0

index 96479c8..2a1e484 100644
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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 <[email protected]>

Quoted 16 lines · Dave Gordon

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.

Quoted 2 lines

Change-Id Id7ce9096ed100a2882c68a54206f30b6c87e92fa

Signed-off-by Dave Gordon <[email protected]>

Is there an igt testcase which readily reproduces this? Or can we have one?

The request queue is per-engine, and may therefore contain requests from several different contexts/ringbuffers. In determining which request to wait for, this function should only consider requests from the ringbuffer that it's checking for space, and ignore any that it finds that belong to other contexts.

Signed-off-by Dave Gordon <[email protected]>

---
  drivers/gpu/drm/i915/intel_lrc.c |   10 ++++++++++
  1 file changed, 10 insertions(+)

drivers/gpu/drm/i915/intel_lrc.c +3 -0

index 2a1a719..1003b3a 100644
@@ -835,6 +835,16 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
  	}
 
  	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

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.

Signed-off-by Dave Gordon <[email protected]>

---
  drivers/gpu/drm/i915/intel_lrc.c |    1 -
  1 file changed, 1 deletion(-)

drivers/gpu/drm/i915/intel_lrc.c +0 -1

index 1003b3a..ad31373 100644
@@ -891,7 +891,6 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
  	end = jiffies + 60 * HZ;
 
  	do {
-		ringbuf->head = I915_READ_HEAD(ring);
  		ringbuf->space = intel_ring_space(ringbuf);
  		if (ringbuf->space >= bytes) {
  			ret = 0;

Looks fine to me

There are numerous places in the code where the driver's idea of how much space is left in a ring is updated using the driver's latest notions of the positions of 'head' and 'tail' for the ring. Among them are some that update one or both of these values before (re)doing the calculation. In particular, there are four different places in the code where 'last_retired_head' is copied to 'head' and then set to -1; and two of these do not have a guard to check that it has actually been updated since last time it was consumed, leaving the possibility that the dummy -1 can be transferred from 'last_retired_head' to 'head', causing the space calculation to produce 'impossible' results (previously seen on Android/VLV).

This code therefore consolidates all the calculation and updating of these values, such that there is only one place where the ring space is updated, and it ALWAYS uses (and consumes) 'last_retired_head' if (and ONLY if) it has been updated since the last call.

Signed-off-by Dave Gordon <[email protected]>

---
  drivers/gpu/drm/i915/i915_dma.c         |    5 ++-
  drivers/gpu/drm/i915/intel_lrc.c        |   25 +++++----------
  drivers/gpu/drm/i915/intel_ringbuffer.c |   51 ++++++++++++++++---------------
  drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
  4 files changed, 37 insertions(+), 45 deletions(-)

drivers/gpu/drm/i915/i915_dma.c +2 -3

index 5dc37f0..4a98399 100644
@@ -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;

drivers/gpu/drm/i915/intel_lrc.c +7 -18

index ad31373..c9a5227 100644
@@ -825,14 +825,8 @@ 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) {
  		/*

@@ -860,11 +854,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,

@@ -890,12 +881,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->space = intel_ring_space(ringbuf);
-		if (ringbuf->space >= bytes) {
-			ret = 0;
+		if (intel_ring_space(ringbuf) >= bytes)
  			break;
-		}
 
  		msleep(1);
 

@@ -936,7 +925,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;
  }

@@ -1777,8 +1766,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

drivers/gpu/drm/i915/intel_ringbuffer.c +27 -24

index ae09258..a08ae65 100644
@@ -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)

@@ -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));

@@ -1880,14 +1891,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,

@@ -1905,10 +1910,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;
  }
 

@@ -1934,14 +1936,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) {

@@ -1989,7 +1991,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;
  }

@@ -2061,6 +2063,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,

@@ -2077,7 +2080,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;
  }
 

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -0

index aab2e2f..24a5723 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index ae09258..a08ae65 100644

Quoted 5 lines · Dave Gordon

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index ae09258..a08ae65 100644

drivers/gpu/drm/i915/intel_ringbuffer.c intel_ring_initialized(struct intel_engine_cs *ring) · line 58

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -3

@@ -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;

Changing the free space computation doesn't seem required, but resulting in Chris&me just discussing it on irc to convince ourselves it's not broken accidentally now. Can you please respin you patch with this change dropped?

In generally it's good practice to review the diff after committing a patch and hunt for any unecessary changes. Imo even when the endresult looks a bit ulgy (e.g. crazy ordering of static functions which requires tons of predeclarations) it's better if the resulting diff looks cleaner. And if things get out of hand we can always do a pure cleanup patch. -Daniel

Quoted 22 lines · Daniel Vetter

Quoted 5 lines · Dave Gordon

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index ae09258..a08ae65 100644

drivers/gpu/drm/i915/intel_ringbuffer.c intel_ring_initialized(struct intel_engine_cs *ring) · line 58

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -3

@@ -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;

Changing the free space computation doesn't seem required, but resulting in Chris&me just discussing it on irc to convince ourselves it's not broken accidentally now. Can you please respin you patch with this change dropped?

Quoted 6 lines

In generally it's good practice to review the diff after committing a patch and hunt for any unecessary changes. Imo even when the endresult looks a bit ulgy (e.g. crazy ordering of static functions which requires tons of predeclarations) it's better if the resulting diff looks cleaner. And if things get out of hand we can always do a pure cleanup patch. -Daniel

This isn't an accidental change; it's to improve resilience in the case that head and/or tail end up with values they shouldn't have.

Here's a C program to model the two different calculations in a tiny ring:

#include <stdio.h>

#define FREE 4
#define SIZE 8

main()

{
        int h, t, s1, s2;

        printf("%s\t%s\t%s\t%s\n", "Head", "Tail", "OSpace", "NSpace");
        for (h = 0; h <= SIZE; ++h)
        {
                for (t = 0; t <= SIZE; ++t)
                {
                        s1 = h - (t + FREE);
                        if (s1 < 0)
                                s1 += SIZE;

                        s2 = h - t;
                        if (s2 <= 0)
                                s2 += SIZE;
                        s2 -= FREE;

                        printf("%2d\t%2d\t%2d\t%2d\n", h, t, s1, s2);
                }
                printf("\n");
        }
}

OSpace (s1) uses the old code, whereas NSpace (s2) is my new method. They agree for all well-behaved cases, but look at this snippet:

Head Tail OSpace NSpace
6 0 2 2
6 1 1 1
6 2 0 0
6 3 7 -1
6 4 6 -2
6 5 5 -3
6 6 4 4
6 7 3 3
6 8 2 2

Both the old and new code give the right answers if HEAD and TAIL have legitimate values. But if TAIL should somehow advance further than it should, and run into the reserved area, the old code might tell you that there's now LOTS of space available, whereas the new code returns "less than zero" space available.

For example, the old calculation tells us that if head=6 and tail=4 then there are 6 slots available -- which just can't be right, as by definition the answer should never be more than (SIZE-RSVD). I'd much rather get the answer "-2" for this case, as it's then obvious that this really shouldn't happen.

In particular, this new code would mean that the commonly-used test (available >= required) would immediately reject further writes into the ring after an overrun, giving some chance that we can recover from or at least diagnose the original problem; whereas allowing more writes would likely both confuse the h/w and destroy the evidence.

It's also easier to understand, IMHO (if experienced programmers such as you & Chris had to discuss the old code to be confident that it was correct, that already suggests that it's not as clear as it could be).

The used space in a ring is given by the cyclic distance from the consumer to the producer; conversely, the available space in a ring is the cyclic distance from the producer to the consumer, MINUS the amount reserved. The new code expresses that directly, without having to figure out the meaning of ADDING the reserved amount to the tail before subtracting it from head. In ASCII pix:

+++++++++++++++++++
+ free + 0
+ free + 1
+ reserved + 2
+ reserved + 3

(consumer) HEAD-> +      used       +  4
                  +      used       +  5
                  +      used       +  6
                  +      used       +  7
                  +      used       +  8
                  +      used       +  9
(producer) TAIL-> +      free       + 10
                  +      free       + 11
                  +++++++++++++++++++

The sketch above shows the situation with size=12, reserved=2, TAIL=10 and HEAD=4. Slots 4 to 9 are used (TAIL-HEAD = 10-4 = 6 slots). The available space is (HEAD-TAIL (+SIZE) - RSVD = 4-10+12-2 = 4 slots).

+++++++++++++++++++
+ used + 0
+ used + 1

(producer) TAIL-> +      free       +  2
                  +      free       +  3
                  +    reserved     +  4
                  +    reserved     +  5
(consumer) HEAD-> +      used       +  6
                  +      used       +  7
                  +      used       +  8
                  +      used       +  9
                  +      used       + 10
                  +      used       + 11
                  +++++++++++++++++++

After TAIL has wrapped, we might have this situation with TAIL=2 and HEAD=6. Used space is (TAIL-HEAD (+SIZE) = 2-6+12 = 8 slots), and available space is (HEAD-TAIL - RSVD) = 6-2-2 = 2 slots. Straightforward and easy to understand :)

So, I'd definitely prefer to keep this new code. We not only want to do the calculation in only one place, but we want to do it in the best possible way, with the minimum chance of propagating errors and confusing both h/w and developers if someone does introduce a ring overrun or off-by-one error in some ring-stuffing code elsewhere. (BTW,

Quoted 30 lines · Dave Gordon

Quoted 28 lines · Daniel Vetter

Quoted 5 lines · Dave Gordon

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index ae09258..a08ae65 100644

drivers/gpu/drm/i915/intel_ringbuffer.c intel_ring_initialized(struct intel_engine_cs *ring) · line 58

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -3

@@ -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;

Changing the free space computation doesn't seem required, but resulting in Chris&me just discussing it on irc to convince ourselves it's not broken accidentally now. Can you please respin you patch with this change dropped?

In generally it's good practice to review the diff after committing a patch and hunt for any unecessary changes. Imo even when the endresult looks a bit ulgy (e.g. crazy ordering of static functions which requires tons of predeclarations) it's better if the resulting diff looks cleaner. And if things get out of hand we can always do a pure cleanup patch. -Daniel

Quoted 2 lines

This isn't an accidental change; it's to improve resilience in the case that head and/or tail end up with values they shouldn't have.

There are numerous places in the code where the driver's idea of how much space is left in a ring is updated using the driver's latest notions of the positions of 'head' and 'tail' for the ring. Among them are some that update one or both of these values before (re)doing the calculation. In particular, there are four different places in the code where 'last_retired_head' is copied to 'head' and then set to -1; and two of these do not have a guard to check that it has actually been updated since last time it was consumed, leaving the possibility that the dummy -1 can be transferred from 'last_retired_head' to 'head', causing the space calculation to produce 'impossible' results (previously seen on Android/VLV).

This code therefore consolidates all the calculation and updating of these values, such that there is only one place where the ring space is updated, and it ALWAYS uses (and consumes) 'last_retired_head' if (and ONLY if) it has been updated since the last call.

Signed-off-by Dave Gordon <[email protected]>