[PATCH] drm/i915: Bug fixes to ring 'head' updating

[PATCH] drm/i915: Bug fixes to ring 'head' updating

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

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Dave Gordon]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 11620 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 14660 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Deepak S]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Dave Gordon]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 0/3] drm/i915: Bug fixes to ring 'head' updating

[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.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] drm/i915: Check for matching ringbuffer in logical_ring_wait_request()

[Dave Gordon]

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 <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2a1a719..1003b3a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -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
+		 * 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;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/3] drm/i915: Don't read 'HEAD' MMIO register in LRC mode

[Dave Gordon]

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 <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 1003b3a..ad31373 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -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;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Dave Gordon]

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 <david.s.gordon@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5dc37f0..4a98399 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 ad31373..c9a5227 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -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
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ae09258..a08ae65 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)
@@ -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;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index aab2e2f..24a5723 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -404,6 +404,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

Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Daniel Vetter]

On Tue, Nov 18, 2014 at 08:07:22PM +0000, Dave Gordon wrote:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ae09258..a08ae65 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;

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Dave Gordon]

On 24/11/14 10:04, Daniel Vetter wrote:
> On Tue, Nov 18, 2014 at 08:07:22PM +0000, Dave Gordon wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index ae09258..a08ae65 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;
> 
> 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

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,

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Check for matching ringbuffer in logical_ring_wait_request()

[Deepak S]

On Wednesday 19 November 2014 01:37 AM, Dave Gordon wrote:
> 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 <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c |   10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2a1a719..1003b3a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -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
> +		 * 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;

Looks fine to me
Reviewed-by: Deepak S <deepak.s@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Don't read 'HEAD' MMIO register in LRC mode

[Deepak S]

On Wednesday 19 November 2014 01:37 AM, Dave Gordon wrote:
> 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 <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c |    1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 1003b3a..ad31373 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -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

Reviewed-by: Deepak S<deepak.s@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Deepak S]

On Wednesday 19 November 2014 01:37 AM, Dave Gordon wrote:
> 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 <david.s.gordon@intel.com>
> ---
>   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(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5dc37f0..4a98399 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 ad31373..c9a5227 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -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
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ae09258..a08ae65 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)
> @@ -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;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index aab2e2f..24a5723 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -404,6 +404,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);

Looks fine to me
Reviewed-by: Deepak S<deepak.s@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Daniel Vetter]

On Mon, Nov 24, 2014 at 02:32:25PM +0000, Dave Gordon wrote:
> On 24/11/14 10:04, Daniel Vetter wrote:
> > On Tue, Nov 18, 2014 at 08:07:22PM +0000, Dave Gordon wrote:
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index ae09258..a08ae65 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;
> > 
> > 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
> 
> 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,

Given the extensive explanation you've delivered here this _definitely_
should be in a separate patch. Rule-of-thumb: If you have multipled pieces
in your commit message/explanation and tie them together in the
explanation with an "and" you should consider splitting the patch along
the "and"s. Except when it's all really trivial stuff (e.g. going ocd over
docs or so). For the commit message please just reuse the above mail, it's
a great explanation!

And could we perhaps WARN_ON if the negative space thing happens, which
should make such an occurance a lot easier to understand?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Chris Wilson]

On Tue, Nov 25, 2014 at 12:41:16PM +0100, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 02:32:25PM +0000, Dave Gordon wrote:
> > On 24/11/14 10:04, Daniel Vetter wrote:
> > > On Tue, Nov 18, 2014 at 08:07:22PM +0000, Dave Gordon wrote:
> > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> index ae09258..a08ae65 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;
> > > 
> > > 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
> > 
> > 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.

Looks like a fun story. How about tackling a real issue like we should
prevent updating TAIL whilst in the same cacheline as HEAD?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 0/2] Updates to ring freespace calculations

[Dave Gordon]

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.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/2] drm/i915: Make ring freespace calculation more robust

[Dave Gordon]

The used space in a ring is given by the cyclic distance from the
consumer (HEAD) to the producer (TAIL), i.e. ((tail-head) MOD size);
conversely, the available space in a ring is the cyclic distance
from the producer to the consumer, MINUS the amount reserved for a
"gap" that is supposed to guarantee that the producer never catches
up with or overruns the consumer. Note that some GEN h/w requires
that TAIL never approach to within one cacheline of HEAD, so the gap
is usually set to twice the cacheline size to ensure this.

While the existing code gives the correct answer for correct inputs,
if the producer HAS overrun into the reserved space, the result can
be a value larger than the maximum valid value (size-reserved). We
can improve this by reorganising the calculation, so that in the
event of overrun the result will be negative rather than over-large.

This means that the commonly-used test (available >= required)
will then 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 of what went wrong.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1d01b51..a5cfaae 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -52,10 +52,10 @@ 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;
 }
 
 int intel_ring_space(struct intel_ringbuffer *ringbuf)
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/2] drm/i915: Consolidate ring freespace calculations

[Dave Gordon]

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 <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |   25 ++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   42 ++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
 3 files changed, 30 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 03b5c04..3d67906 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -920,14 +920,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) {
 		/*
@@ -955,11 +949,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,
@@ -985,12 +976,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);
 
@@ -1031,7 +1020,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;
 }
@@ -1881,8 +1870,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);
 
 	if (ringbuf->obj == NULL) {
 		ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a5cfaae..19f64da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -58,10 +58,21 @@ int __intel_ring_space(int head, int tail, int size)
 	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)
@@ -589,10 +600,10 @@ static int init_ring_common(struct intel_engine_cs *ring)
 		goto out;
 	}
 
+	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));
 
@@ -1891,14 +1902,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,
@@ -1916,10 +1921,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;
 }
 
@@ -1945,14 +1947,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;
-		}
 
 		msleep(1);
 
@@ -1993,7 +1995,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;
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index fe426cf..1897f21 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -408,6 +408,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

Re: [PATCH v3 2/2] drm/i915: Consolidate ring freespace

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-9              365/366              357/366
SNB                                  450/450              450/450
IVB                                  498/498              498/498
BYT                                  289/289              289/289
HSW                                  564/564              564/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_nonexisting-fb      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_pipe_crc_basic_bad-nb-words-3      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_render_direct-render      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_busy-flip-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_flip-vs-dpms-interruptible      PASS(2, M26)      NSPT(1, M26)
*ILK  igt_kms_flip_flip-vs-panning      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      NSPT(1, M26)PASS(1, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(1, M26)      PASS(1, M26)
 ILK  igt_kms_flip_wf_vblank-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(1, M26)      DMESG_WARN(1, M26)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drm/i915: Consolidate ring freespace calculations

[Daniel Vetter]

On Thu, Nov 27, 2014 at 11:22:49AM +0000, Dave Gordon wrote:
> 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 <david.s.gordon@intel.com>

Both applied, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx