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>

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

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) {
+	if (!need_sync) {
-		/* no need to sync to our own or unused rings */
+		return 0;
-		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]);
-
-		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]);
 	}
-	return 0;
+
+	r = radeon_semaphore_create(p->rdev, &p->ib->fence->semaphore);
+	if (r) {
+		return r;
+	}
+
+	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)

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

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;
+	struct radeon_fence *fence, *old_fence;
-	int r, i;
+	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) {
+	old_fence = bo->sync_obj;
-		for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+	if (old_fence && old_fence->ring != fence->ring
-			/* no need to sync to our own or unused rings */
+	    && !radeon_fence_signaled(old_fence)) {
-			if (i == radeon_copy_ring_index(rdev) || !rdev->ring[i].ready)
+		bool sync_to_ring[RADEON_NUM_RINGS] = { };
-				continue;
+		sync_to_ring[old_fence->ring] = true;
 
-			if (!fence->semaphore) {
+		r = radeon_semaphore_create(rdev, &fence->semaphore);
-				r = radeon_semaphore_create(rdev, &fence->semaphore);
+		if (r) {
-				/* FIXME: handle semaphore error */
+			radeon_fence_unref(&fence);
-				if (r)
+			return r;
-					continue;
+		}
-			}
 
-			r = radeon_ring_lock(rdev, &rdev->ring[i], 3);
+		r = radeon_semaphore_sync_rings(rdev, fence->semaphore,
-			/* FIXME: handle ring lock error */
+						sync_to_ring, fence->ring);
-			if (r)
+		if (r) {
-				continue;
+			radeon_fence_unref(&fence);
-			radeon_semaphore_emit_signal(rdev, i, fence->semaphore);
+			return r;
-			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)]);
 		}
 	}