Nouveau - Aug 2009 - [PATCH 1/4] drm/nouveau: refactor nouveau_dma_wait()

A cleanup of nouveau_dma_wait(): extract a sub-function and eliminate
two variables to improve readability.

No functional changes.

Signed-off-by: Pekka Paalanen <pq at iki.fi>
---
 drivers/gpu/drm/nouveau/nouveau_dma.c |   72 ++++++++++++++++++---------------
 1 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c
b/drivers/gpu/drm/nouveau/nouveau_dma.c
index b1f3a71..b787651 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
@@ -115,14 +115,43 @@ READ_GET(struct nouveau_channel *chan, uint32_t *get)
 	return true;
 }
 
+static int
+dma_wait_ring_wrap(struct nouveau_channel *chan, int size, uint32_t get,
+		   int *timeout)
+{
+	/* Emit jump to the start of the ring buffer. */
+	OUT_RING(chan, 0x20000000 | chan->pushbuf_base);
+
+	if (get <= NOUVEAU_DMA_SKIPS) {
+		/* corner case - will be idle */
+		if (chan->dma.put <= NOUVEAU_DMA_SKIPS)
+			WRITE_PUT(NOUVEAU_DMA_SKIPS + 1);
+
+		for (; *timeout; (*timeout)--) {
+			if (READ_GET(chan, &get) && get > NOUVEAU_DMA_SKIPS)
+				break;
+
+			DRM_UDELAY(1);
+		}
+
+		if (*timeout == 0)
+			return -EBUSY;
+	}
+
+	WRITE_PUT(NOUVEAU_DMA_SKIPS);
+	chan->dma.cur  = NOUVEAU_DMA_SKIPS;
+	chan->dma.put  = NOUVEAU_DMA_SKIPS;
+	chan->dma.free = get - (NOUVEAU_DMA_SKIPS + 1);
+	return 0;
+}
+
 int
 nouveau_dma_wait(struct nouveau_channel *chan, int size)
 {
-	const int us_timeout = 100000;
+	int us_timeout = 100000;
 	uint32_t get;
-	int ret = -EBUSY, i;
 
-	for (i = 0; i < us_timeout; i++) {
+	for (; us_timeout; us_timeout--) {
 		if (!READ_GET(chan, &get)) {
 			DRM_UDELAY(1);
 			continue;
@@ -131,41 +160,18 @@ nouveau_dma_wait(struct nouveau_channel *chan, int size)
 		if (chan->dma.put >= get) {
 			chan->dma.free = chan->dma.max - chan->dma.cur;
 
-			if (chan->dma.free < size) {
-				OUT_RING(chan, 0x20000000|chan->pushbuf_base);
-				if (get <= NOUVEAU_DMA_SKIPS) {
-					/*corner case - will be idle*/
-					if (chan->dma.put <= NOUVEAU_DMA_SKIPS)
-						WRITE_PUT(NOUVEAU_DMA_SKIPS + 1);
-
-					for (; i < us_timeout; i++) {
-						if (READ_GET(chan, &get) &&
-						    get > NOUVEAU_DMA_SKIPS)
-							break;
-
-						DRM_UDELAY(1);
-					}
-
-					if (i >= us_timeout)
-						break;
-				}
-
-				WRITE_PUT(NOUVEAU_DMA_SKIPS);
-				chan->dma.cur  -				chan->dma.put  = NOUVEAU_DMA_SKIPS;
-				chan->dma.free = get - (NOUVEAU_DMA_SKIPS + 1);
-			}
+			if (chan->dma.free < size)
+				if (dma_wait_ring_wrap(chan, size, get,
+								&us_timeout))
+					return -EBUSY;
 		} else {
 			chan->dma.free = get - chan->dma.cur - 1;
 		}
 
-		if (chan->dma.free >= size) {
-			ret = 0;
-			break;
-		}
+		if (chan->dma.free >= size)
+			return 0;
 
 		DRM_UDELAY(1);
 	}
-
-	return ret;
+	return -EBUSY;
 }
-- 
1.6.3.3

Make WRITE_PUT() an inline function, and pass chan as an argument.
Update chan->dma.put in WRITE_PUT() instead of (almost) everywhere by
hand afterwards.

Signed-off-by: Pekka Paalanen <pq at iki.fi>
---
 drivers/gpu/drm/nouveau/nouveau_dma.c |    5 ++---
 drivers/gpu/drm/nouveau/nouveau_dma.h |   16 +++++++++-------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c
b/drivers/gpu/drm/nouveau/nouveau_dma.c
index b787651..28e9c11 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
@@ -125,7 +125,7 @@ dma_wait_ring_wrap(struct nouveau_channel *chan, int size,
uint32_t get,
 	if (get <= NOUVEAU_DMA_SKIPS) {
 		/* corner case - will be idle */
 		if (chan->dma.put <= NOUVEAU_DMA_SKIPS)
-			WRITE_PUT(NOUVEAU_DMA_SKIPS + 1);
+			WRITE_PUT(chan, NOUVEAU_DMA_SKIPS + 1);
 
 		for (; *timeout; (*timeout)--) {
 			if (READ_GET(chan, &get) && get > NOUVEAU_DMA_SKIPS)
@@ -138,9 +138,8 @@ dma_wait_ring_wrap(struct nouveau_channel *chan, int size,
uint32_t get,
 			return -EBUSY;
 	}
 
-	WRITE_PUT(NOUVEAU_DMA_SKIPS);
+	WRITE_PUT(chan, NOUVEAU_DMA_SKIPS);
 	chan->dma.cur  = NOUVEAU_DMA_SKIPS;
-	chan->dma.put  = NOUVEAU_DMA_SKIPS;
 	chan->dma.free = get - (NOUVEAU_DMA_SKIPS + 1);
 	return 0;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.h
b/drivers/gpu/drm/nouveau/nouveau_dma.h
index cdaa37d..74db74e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dma.h
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.h
@@ -118,11 +118,14 @@ BEGIN_RING(struct nouveau_channel *chan, int subc, int
mthd, int size)
 	OUT_RING(chan, (subc << 13) | (size << 18) | mthd);
 }
 
-#define WRITE_PUT(val) do {                                                   
\
-	DRM_MEMORYBARRIER();                                                   \
-	nouveau_bo_rd32(chan->pushbuf_bo, 0);                                  \
-	nvchan_wr32(chan->user_put, ((val) << 2) + chan->pushbuf_base);   
\
-} while (0)
+static inline void
+WRITE_PUT(struct nouveau_channel *chan, uint32_t index)
+{
+	DRM_MEMORYBARRIER();
+	nouveau_bo_rd32(chan->pushbuf_bo, 0);
+	nvchan_wr32(chan->user_put, index * 4 + chan->pushbuf_base);
+	chan->dma.put = index;
+}
 
 static inline void
 FIRE_RING(struct nouveau_channel *chan)
@@ -135,8 +138,7 @@ FIRE_RING(struct nouveau_channel *chan)
 		return;
 	chan->accel_done = true;
 
-	WRITE_PUT(chan->dma.cur);
-	chan->dma.put = chan->dma.cur;
+	WRITE_PUT(chan, chan->dma.cur);
 }
 
 static inline void
-- 
1.6.3.3

A PCI write may be cached somewhere in the a PCI bridge/bus for some
time. Doing a PCI read will force all pending writes to be finished.

Add a dummy read to force the PUT register write to arrive to hardware
at FIRE_RING time.

Add comments for the barrier and flush in WRITE_PUT().

Signed-off-by: Pekka Paalanen <pq at iki.fi>
---
 drivers/gpu/drm/nouveau/nouveau_dma.h |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.h
b/drivers/gpu/drm/nouveau/nouveau_dma.h
index 74db74e..aada9be 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dma.h
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.h
@@ -121,8 +121,13 @@ BEGIN_RING(struct nouveau_channel *chan, int subc, int
mthd, int size)
 static inline void
 WRITE_PUT(struct nouveau_channel *chan, uint32_t index)
 {
+	/* no CPU instruction reordering over this point */
 	DRM_MEMORYBARRIER();
-	nouveau_bo_rd32(chan->pushbuf_bo, 0);
+
+	/* flush pushbuf writes, if it happens to be in VRAM */
+	(void)nouveau_bo_rd32(chan->pushbuf_bo, 0);
+
+	/* fire */
 	nvchan_wr32(chan->user_put, index * 4 + chan->pushbuf_base);
 	chan->dma.put = index;
 }
@@ -139,6 +144,9 @@ FIRE_RING(struct nouveau_channel *chan)
 	chan->accel_done = true;
 
 	WRITE_PUT(chan, chan->dma.cur);
+
+	/* flush the fire PCI write to the card */
+	(void)nvchan_rd32(chan->user_get);
 }
 
 static inline void
-- 
1.6.3.3

In the wrap around case, it was resetting to SKIPS + 1, which would
leave one dword after the actual SKIPs untouched. It is a bit odd that
the undefined dword has not caused real trouble, or perhaps it is
written a "safe" value by accident.

Rename dma_wait_ring_wrap() to ring_wrap() and clean it up. Make
the dma.free computation just like in nouveau_dma_free().

Signed-off-by: Pekka Paalanen <pq at iki.fi>
---
 drivers/gpu/drm/nouveau/nouveau_dma.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c
b/drivers/gpu/drm/nouveau/nouveau_dma.c
index 28e9c11..5ede79c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
@@ -115,33 +115,31 @@ READ_GET(struct nouveau_channel *chan, uint32_t *get)
 	return true;
 }
 
-static int
-dma_wait_ring_wrap(struct nouveau_channel *chan, int size, uint32_t get,
-		   int *timeout)
+static void
+ring_wrap(struct nouveau_channel *chan, int size, uint32_t get, int *timeout)
 {
 	/* Emit jump to the start of the ring buffer. */
 	OUT_RING(chan, 0x20000000 | chan->pushbuf_base);
 
-	if (get <= NOUVEAU_DMA_SKIPS) {
+	if (get < NOUVEAU_DMA_SKIPS) {
 		/* corner case - will be idle */
-		if (chan->dma.put <= NOUVEAU_DMA_SKIPS)
-			WRITE_PUT(chan, NOUVEAU_DMA_SKIPS + 1);
+		if (chan->dma.put < NOUVEAU_DMA_SKIPS)
+			WRITE_PUT(chan, NOUVEAU_DMA_SKIPS);
 
 		for (; *timeout; (*timeout)--) {
-			if (READ_GET(chan, &get) && get > NOUVEAU_DMA_SKIPS)
+			if (READ_GET(chan, &get) && get >= NOUVEAU_DMA_SKIPS)
 				break;
 
 			DRM_UDELAY(1);
 		}
 
 		if (*timeout == 0)
-			return -EBUSY;
+			return;
 	}
 
 	WRITE_PUT(chan, NOUVEAU_DMA_SKIPS);
 	chan->dma.cur  = NOUVEAU_DMA_SKIPS;
-	chan->dma.free = get - (NOUVEAU_DMA_SKIPS + 1);
-	return 0;
+	chan->dma.free = get - chan->dma.cur - 1;
 }
 
 int
@@ -160,9 +158,7 @@ nouveau_dma_wait(struct nouveau_channel *chan, int size)
 			chan->dma.free = chan->dma.max - chan->dma.cur;
 
 			if (chan->dma.free < size)
-				if (dma_wait_ring_wrap(chan, size, get,
-								&us_timeout))
-					return -EBUSY;
+				ring_wrap(chan, size, get, &us_timeout);
 		} else {
 			chan->dma.free = get - chan->dma.cur - 1;
 		}
-- 
1.6.3.3

On Fri, Aug 21, 2009 at 6:58 AM, Pekka Paalanen<pq at iki.fi>
wrote:
> On Thu, 20 Aug 2009 20:27:49 +0200
> Maarten Maathuis <madman2003 at gmail.com> wrote:
>
>> I think the -1 on chan->dma.free is unnecessary.
>
> Maybe it is for the wrap-around jump command, the OUT_RING?
> I don't any other way of reserving memory for writing that.
The initial pushbuf size is deducted by 2, because nv50 needs it for
some reason.
Originally it was 1, to accommodate the jump.
>
> --
> Pekka Paalanen
> http://www.iki.fi/pq/
>