drm/nouveau: fix a race condition in nouveau_dma_wait()

Can be triggered easily on certain cards (NV46 and NV50 of mine) by
running "dmesg", the DRM's channel will lockup.

Signed-off-by: Ben Skeggs <bskeggs@redhat.com>

diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c b/drivers/gpu/drm/nouveau/nouveau_dma.c
index 7afbe8b..50d9e67 100644 (file)
--- a/drivers/gpu/drm/nouveau/nouveau_dma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
@@ -126,47 +126,52 @@ OUT_RINGp(struct nouveau_channel *chan, const void *data, unsigned nr_dwords)
        chan->dma.cur += nr_dwords;
 }
 
-static inline bool
-READ_GET(struct nouveau_channel *chan, uint32_t *get)
+/* Fetch and adjust GPU GET pointer
+ *
+ * Returns:
+ *  value >= 0, the adjusted GET pointer
+ *  -EINVAL if GET pointer currently outside main push buffer
+ *  -EBUSY if timeout exceeded
+ */
+static inline int
+READ_GET(struct nouveau_channel *chan, uint32_t *prev_get, uint32_t *timeout)
 {
        uint32_t val;
 
        val = nvchan_rd32(chan, chan->user_get);
-       if (val < chan->pushbuf_base ||
-           val > chan->pushbuf_base + (chan->dma.max << 2)) {
-               /* meaningless to dma_wait() except to know whether the
-                * GPU has stalled or not
-                */
-               *get = val;
-               return false;
+
+       /* reset counter as long as GET is still advancing, this is
+        * to avoid misdetecting a GPU lockup if the GPU happens to
+        * just be processing an operation that takes a long time
+        */
+       if (val != *prev_get) {
+               *prev_get = val;
+               *timeout = 0;
+       }
+
+       if ((++*timeout & 0xff) == 0) {
+               DRM_UDELAY(1);
+               if (*timeout > 100000)
+                       return -EBUSY;
        }
 
-       *get = (val - chan->pushbuf_base) >> 2;
-       return true;
+       if (val < chan->pushbuf_base ||
+           val > chan->pushbuf_base + (chan->dma.max << 2))
+               return -EINVAL;
+
+       return (val - chan->pushbuf_base) >> 2;
 }
 
 int
 nouveau_dma_wait(struct nouveau_channel *chan, int size)
 {
-       uint32_t get, prev_get = 0, cnt = 0;
-       bool get_valid;
+       uint32_t prev_get = 0, cnt = 0;
+       int get;
 
        while (chan->dma.free < size) {
-               /* reset counter as long as GET is still advancing, this is
-                * to avoid misdetecting a GPU lockup if the GPU happens to
-                * just be processing an operation that takes a long time
-                */
-               get_valid = READ_GET(chan, &get);
-               if (get != prev_get) {
-                       prev_get = get;
-                       cnt = 0;
-               }
-
-               if ((++cnt & 0xff) == 0) {
-                       DRM_UDELAY(1);
-                       if (cnt > 100000)
-                               return -EBUSY;
-               }
+               get = READ_GET(chan, &prev_get, &cnt);
+               if (unlikely(get == -EBUSY))
+                       return -EBUSY;
 
                /* loop until we have a usable GET pointer.  the value
                 * we read from the GPU may be outside the main ring if
@@ -177,7 +182,7 @@ nouveau_dma_wait(struct nouveau_channel *chan, int size)
                 * from the SKIPS area, so the code below doesn't have to deal
                 * with some fun corner cases.
                 */
-               if (!get_valid || get < NOUVEAU_DMA_SKIPS)
+               if (unlikely(get == -EINVAL) || get < NOUVEAU_DMA_SKIPS)
                        continue;
 
                if (get <= chan->dma.cur) {
@@ -203,6 +208,19 @@ nouveau_dma_wait(struct nouveau_channel *chan, int size)
                         * after processing the currently pending commands.
                         */
                        OUT_RING(chan, chan->pushbuf_base | 0x20000000);
+
+                       /* wait for GET to depart from the skips area.
+                        * prevents writing GET==PUT and causing a race
+                        * condition that causes us to think the GPU is
+                        * idle when it's not.
+                        */
+                       do {
+                               get = READ_GET(chan, &prev_get, &cnt);
+                               if (unlikely(get == -EBUSY))
+                                       return -EBUSY;
+                               if (unlikely(get == -EINVAL))
+                                       continue;
+                       } while (get <= NOUVEAU_DMA_SKIPS);
                        WRITE_PUT(NOUVEAU_DMA_SKIPS);
 
                        /* we're now submitting commands at the start of