drm/radeon: fix a bug with the ring syncing code

Rings need to lock in order, otherwise
the ring subsystem can deadlock.

v2: fix error handling and number of locked doublewords.
v3: stop creating unneeded semaphores.

Signed-off-by: Christian König <deathsimple@vodafone.de>
Reviewed-by: Jerome Glisse <jglisse@redhat.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 794182a..f7372c4 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -460,6 +460,10 @@ void radeon_semaphore_emit_signal(struct radeon_device *rdev, int ring,
                                  struct radeon_semaphore *semaphore);
 void radeon_semaphore_emit_wait(struct radeon_device *rdev, int ring,
                                struct radeon_semaphore *semaphore);
+int radeon_semaphore_sync_rings(struct radeon_device *rdev,
+                               struct radeon_semaphore *semaphore,
+                               bool sync_to[RADEON_NUM_RINGS],
+                               int dst_ring);
 void radeon_semaphore_free(struct radeon_device *rdev,
                           struct radeon_semaphore *semaphore);
 

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
index e7b0b5d..24fb001 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -118,6 +118,7 @@ static int radeon_cs_get_ring(struct radeon_cs_parser *p, u32 ring, s32 priority
 static int radeon_cs_sync_rings(struct radeon_cs_parser *p)
 {
        bool sync_to_ring[RADEON_NUM_RINGS] = { };
+       bool need_sync = false;
        int i, r;
 
        for (i = 0; i < p->nrelocs; i++) {
@@ -126,36 +127,24 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)
 
                if (!(p->relocs[i].flags & RADEON_RELOC_DONT_SYNC)) {
                        struct radeon_fence *fence = p->relocs[i].robj->tbo.sync_obj;
-                       if (!radeon_fence_signaled(fence)) {
+                       if (fence->ring != p->ring && !radeon_fence_signaled(fence)) {
                                sync_to_ring[fence->ring] = true;
+                               need_sync = true;
                        }
                }
        }
 
-       for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-               /* no need to sync to our own or unused rings */
-               if (i == p->ring || !sync_to_ring[i] || !p->rdev->ring[i].ready)
-                       continue;
-
-               if (!p->ib->fence->semaphore) {
-                       r = radeon_semaphore_create(p->rdev, &p->ib->fence->semaphore);
-                       if (r)
-                               return r;
-               }
-
-               r = radeon_ring_lock(p->rdev, &p->rdev->ring[i], 3);
-               if (r)
-                       return r;
-               radeon_semaphore_emit_signal(p->rdev, i, p->ib->fence->semaphore);
-               radeon_ring_unlock_commit(p->rdev, &p->rdev->ring[i]);
+       if (!need_sync) {
+               return 0;
+       }
 
-               r = radeon_ring_lock(p->rdev, &p->rdev->ring[p->ring], 3);
-               if (r)
-                       return r;
-               radeon_semaphore_emit_wait(p->rdev, p->ring, p->ib->fence->semaphore);
-               radeon_ring_unlock_commit(p->rdev, &p->rdev->ring[p->ring]);
+       r = radeon_semaphore_create(p->rdev, &p->ib->fence->semaphore);
+       if (r) {
+               return r;
        }
-       return 0;
+
+       return radeon_semaphore_sync_rings(p->rdev, p->ib->fence->semaphore,
+                                          sync_to_ring, p->ring);
 }
 
 int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data)

diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c
index 61dd4e3..930a08a 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_semaphore.c
+++ b/drivers/gpu/drm/radeon/radeon_semaphore.c
@@ -149,6 +149,62 @@ void radeon_semaphore_emit_wait(struct radeon_device *rdev, int ring,
        radeon_semaphore_ring_emit(rdev, ring, &rdev->ring[ring], semaphore, true);
 }
 
+int radeon_semaphore_sync_rings(struct radeon_device *rdev,
+                               struct radeon_semaphore *semaphore,
+                               bool sync_to[RADEON_NUM_RINGS],
+                               int dst_ring)
+{
+       int i, r;
+
+       for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+               unsigned num_ops = i == dst_ring ? RADEON_NUM_RINGS : 1;
+
+               /* don't lock unused rings */
+               if (!sync_to[i] && i != dst_ring)
+                       continue;
+
+               /* prevent GPU deadlocks */
+               if (!rdev->ring[i].ready) {
+                       dev_err(rdev->dev, "Trying to sync to a disabled ring!");
+                       r = -EINVAL;
+                       goto error;
+               }
+
+                r = radeon_ring_lock(rdev, &rdev->ring[i], num_ops * 8);
+                if (r)
+                       goto error;
+       }
+
+       for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+               /* no need to sync to our own or unused rings */
+               if (!sync_to[i] || i == dst_ring)
+                        continue;
+
+               radeon_semaphore_emit_signal(rdev, i, semaphore);
+               radeon_semaphore_emit_wait(rdev, dst_ring, semaphore);
+       }
+
+       for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+
+               /* don't unlock unused rings */
+               if (!sync_to[i] && i != dst_ring)
+                       continue;
+
+               radeon_ring_unlock_commit(rdev, &rdev->ring[i]);
+       }
+
+       return 0;
+
+error:
+       /* unlock all locks taken so far */
+       for (--i; i >= 0; --i) {
+               if (sync_to[i] || i == dst_ring) {
+                       radeon_ring_unlock_undo(rdev, &rdev->ring[i]);
+               }
+       }
+       return r;
+}
+
 void radeon_semaphore_free(struct radeon_device *rdev,
                           struct radeon_semaphore *semaphore)
 {

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index f493c64..5e3d54d 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -222,8 +222,8 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
 {
        struct radeon_device *rdev;
        uint64_t old_start, new_start;
-       struct radeon_fence *fence;
-       int r, i;
+       struct radeon_fence *fence, *old_fence;
+       int r;
 
        rdev = radeon_get_rdev(bo->bdev);
        r = radeon_fence_create(rdev, &fence, radeon_copy_ring_index(rdev));
@@ -242,6 +242,7 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
                break;
        default:
                DRM_ERROR("Unknown placement %d\n", old_mem->mem_type);
+               radeon_fence_unref(&fence);
                return -EINVAL;
        }
        switch (new_mem->mem_type) {
@@ -253,42 +254,35 @@ static int radeon_move_blit(struct ttm_buffer_object *bo,
                break;
        default:
                DRM_ERROR("Unknown placement %d\n", old_mem->mem_type);
+               radeon_fence_unref(&fence);
                return -EINVAL;
        }
        if (!rdev->ring[radeon_copy_ring_index(rdev)].ready) {
                DRM_ERROR("Trying to move memory with ring turned off.\n");
+               radeon_fence_unref(&fence);
                return -EINVAL;
        }
 
        BUILD_BUG_ON((PAGE_SIZE % RADEON_GPU_PAGE_SIZE) != 0);
 
        /* sync other rings */
-       if (rdev->family >= CHIP_R600) {
-               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-                       /* no need to sync to our own or unused rings */
-                       if (i == radeon_copy_ring_index(rdev) || !rdev->ring[i].ready)
-                               continue;
-
-                       if (!fence->semaphore) {
-                               r = radeon_semaphore_create(rdev, &fence->semaphore);
-                               /* FIXME: handle semaphore error */
-                               if (r)
-                                       continue;
-                       }
+       old_fence = bo->sync_obj;
+       if (old_fence && old_fence->ring != fence->ring
+           && !radeon_fence_signaled(old_fence)) {
+               bool sync_to_ring[RADEON_NUM_RINGS] = { };
+               sync_to_ring[old_fence->ring] = true;
+
+               r = radeon_semaphore_create(rdev, &fence->semaphore);
+               if (r) {
+                       radeon_fence_unref(&fence);
+                       return r;
+               }
 
-                       r = radeon_ring_lock(rdev, &rdev->ring[i], 3);
-                       /* FIXME: handle ring lock error */
-                       if (r)
-                               continue;
-                       radeon_semaphore_emit_signal(rdev, i, fence->semaphore);
-                       radeon_ring_unlock_commit(rdev, &rdev->ring[i]);
-
-                       r = radeon_ring_lock(rdev, &rdev->ring[radeon_copy_ring_index(rdev)], 3);
-                       /* FIXME: handle ring lock error */
-                       if (r)
-                               continue;
-                       radeon_semaphore_emit_wait(rdev, radeon_copy_ring_index(rdev), fence->semaphore);
-                       radeon_ring_unlock_commit(rdev, &rdev->ring[radeon_copy_ring_index(rdev)]);
+               r = radeon_semaphore_sync_rings(rdev, fence->semaphore,
+                                               sync_to_ring, fence->ring);
+               if (r) {
+                       radeon_fence_unref(&fence);
+                       return r;
                }
        }