[TESTING] fix for IB & fence association

[Jerome Glisse <glisse@freedesktop.org>]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

Attached is 2 patch which fix quite bad issue in radeon kms
code for IB allocation and for fence association to ttm object.
They should fix race. I am still in the process of testing them,
i want to give the opportunity to others to test them too (Dave
don't apply them yet :))

Cheers,
Jerome

[-- Attachment #2: 0001-drm-radeon-kms-fix-indirect-buffer-management.patch --]
[-- Type: text/plain, Size: 7049 bytes --]

>From 5a9d7c4a4fa16bfe3148f161989411b4fe2bed77 Mon Sep 17 00:00:00 2001
From: Jerome Glisse <jglisse@redhat.com>
Date: Mon, 15 Feb 2010 14:06:11 +0100
Subject: [PATCH 1/2] drm/radeon/kms: fix indirect buffer management

There is 3 different distinct states for an indirect buffer (IB) :
  1- free with no fence
  2- free with a fence
  3- non free (fence doesn't matter)
Previous code mixed case 2 & 3 in a single one leading to possible
catastrophique failure. This patch rework the handling and properly
separate each case. So when you get ib we set the ib as non free and
fence status doesn't matter. Fence become active (ie has a meaning
for the ib code) once the ib is scheduled or free. This patch also
get rid of the alloc bitmap as it was overkill, we know go through
IB pool list like in a ring buffer as the oldest IB is the first
one the will be free.

Fix :
https://bugs.freedesktop.org/show_bug.cgi?id=26438
and likely other bugs.

Signed-off-by: Jerome Glisse <jglisse@redhat.com>
---
 drivers/gpu/drm/radeon/radeon.h      |    6 ++-
 drivers/gpu/drm/radeon/radeon_ring.c |   97 +++++++++++++--------------------
 2 files changed, 42 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 993cdf2..5617f44 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -97,6 +97,7 @@ extern int radeon_audio;
  * symbol;
  */
 #define RADEON_MAX_USEC_TIMEOUT		100000	/* 100 ms */
+/* RADEON_IB_POOL_SIZE must be a power of 2 */
 #define RADEON_IB_POOL_SIZE		16
 #define RADEON_DEBUGFS_MAX_NUM_FILES	32
 #define RADEONFB_CONN_LIMIT		4
@@ -374,8 +375,9 @@ struct radeon_ib {
 	unsigned long		idx;
 	uint64_t		gpu_addr;
 	struct radeon_fence	*fence;
-	uint32_t	*ptr;
+	uint32_t		*ptr;
 	uint32_t		length_dw;
+	bool			free;
 };
 
 /*
@@ -389,7 +391,7 @@ struct radeon_ib_pool {
 	struct list_head	bogus_ib;
 	struct radeon_ib	ibs[RADEON_IB_POOL_SIZE];
 	bool			ready;
-	DECLARE_BITMAP(alloc_bm, RADEON_IB_POOL_SIZE);
+	unsigned		head_id;
 };
 
 struct radeon_cp {
diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
index e3bee59..5fbc819 100644
--- a/drivers/gpu/drm/radeon/radeon_ring.c
+++ b/drivers/gpu/drm/radeon/radeon_ring.c
@@ -71,68 +71,56 @@ int radeon_ib_get(struct radeon_device *rdev, struct radeon_ib **ib)
 {
 	struct radeon_fence *fence;
 	struct radeon_ib *nib;
-	unsigned long i;
 	int r = 0;
 
 	*ib = NULL;
 	r = radeon_fence_create(rdev, &fence);
 	if (r) {
-		DRM_ERROR("failed to create fence for new IB\n");
+		dev_err(rdev->dev, "failed to create fence for new IB\n");
 		return r;
 	}
 	mutex_lock(&rdev->ib_pool.mutex);
-	i = find_first_zero_bit(rdev->ib_pool.alloc_bm, RADEON_IB_POOL_SIZE);
-	if (i < RADEON_IB_POOL_SIZE) {
-		set_bit(i, rdev->ib_pool.alloc_bm);
-		rdev->ib_pool.ibs[i].length_dw = 0;
-		*ib = &rdev->ib_pool.ibs[i];
+	nib = &rdev->ib_pool.ibs[rdev->ib_pool.head_id++];
+	rdev->ib_pool.head_id &= (RADEON_IB_POOL_SIZE - 1);
+	if (!nib->free) {
+		/* This should never happen, it means we allocated all
+		 * IB and haven't scheduled one yet, return EBUSY to
+		 * userspace hoping that on ioctl recall we get better
+		 * luck;
+		 */
 		mutex_unlock(&rdev->ib_pool.mutex);
-		goto out;
+		radeon_fence_unref(&fence);
+		return -EBUSY;
 	}
-	if (list_empty(&rdev->ib_pool.scheduled_ibs)) {
-		/* we go do nothings here */
-		mutex_unlock(&rdev->ib_pool.mutex);
-		DRM_ERROR("all IB allocated none scheduled.\n");
-		r = -EINVAL;
-		goto out;
+	nib->free = false;
+	if (!nib->fence && list_empty(&nib->list)) {
+		/* This should never happen it means somehow an ib get
+		 * scheduled without a fence.
+		 */
+		dev_err(rdev->dev, "IB[%lu] scheduled without fence\n", nib->idx);
+		list_del_init(&nib->list);
 	}
-	/* get the first ib on the scheduled list */
-	nib = list_entry(rdev->ib_pool.scheduled_ibs.next,
-			 struct radeon_ib, list);
-	if (nib->fence == NULL) {
-		/* we go do nothings here */
+	if (nib->fence) {
 		mutex_unlock(&rdev->ib_pool.mutex);
-		DRM_ERROR("IB %lu scheduled without a fence.\n", nib->idx);
-		r = -EINVAL;
-		goto out;
-	}
-	mutex_unlock(&rdev->ib_pool.mutex);
-
-	r = radeon_fence_wait(nib->fence, false);
-	if (r) {
-		DRM_ERROR("radeon: IB(%lu:0x%016lX:%u)\n", nib->idx,
-			  (unsigned long)nib->gpu_addr, nib->length_dw);
-		DRM_ERROR("radeon: GPU lockup detected, fail to get a IB\n");
-		goto out;
+		r = radeon_fence_wait(nib->fence, false);
+		if (r) {
+			dev_err(rdev->dev, "error waiting fence of IB(%lu:0x%016lX:%u)\n",
+				nib->idx, (unsigned long)nib->gpu_addr, nib->length_dw);
+			mutex_lock(&rdev->ib_pool.mutex);
+			nib->free = true;
+			mutex_unlock(&rdev->ib_pool.mutex);
+			radeon_fence_unref(&fence);
+			return r;
+		}
+		mutex_lock(&rdev->ib_pool.mutex);
 	}
 	radeon_fence_unref(&nib->fence);
-
+	nib->fence = fence;
 	nib->length_dw = 0;
-
-	/* scheduled list is accessed here */
-	mutex_lock(&rdev->ib_pool.mutex);
-	list_del(&nib->list);
-	INIT_LIST_HEAD(&nib->list);
+	list_del_init(&nib->list);
 	mutex_unlock(&rdev->ib_pool.mutex);
-
 	*ib = nib;
-out:
-	if (r) {
-		radeon_fence_unref(&fence);
-	} else {
-		(*ib)->fence = fence;
-	}
-	return r;
+	return 0;
 }
 
 void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib **ib)
@@ -144,18 +132,7 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib **ib)
 		return;
 	}
 	mutex_lock(&rdev->ib_pool.mutex);
-	if (!list_empty(&tmp->list) && !radeon_fence_signaled(tmp->fence)) {
-		/* IB is scheduled & not signaled don't do anythings */
-		mutex_unlock(&rdev->ib_pool.mutex);
-		return;
-	}
-	list_del(&tmp->list);
-	INIT_LIST_HEAD(&tmp->list);
-	if (tmp->fence)
-		radeon_fence_unref(&tmp->fence);
-
-	tmp->length_dw = 0;
-	clear_bit(tmp->idx, rdev->ib_pool.alloc_bm);
+	tmp->free = true;
 	mutex_unlock(&rdev->ib_pool.mutex);
 }
 
@@ -179,6 +156,8 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
 	radeon_fence_emit(rdev, ib->fence);
 	mutex_lock(&rdev->ib_pool.mutex);
 	list_add_tail(&ib->list, &rdev->ib_pool.scheduled_ibs);
+	/* once scheduled IB is considered free and protected by the fence */
+	ib->free = true;
 	mutex_unlock(&rdev->ib_pool.mutex);
 	radeon_ring_unlock_commit(rdev);
 	return 0;
@@ -226,9 +205,10 @@ int radeon_ib_pool_init(struct radeon_device *rdev)
 		rdev->ib_pool.ibs[i].ptr = ptr + offset;
 		rdev->ib_pool.ibs[i].idx = i;
 		rdev->ib_pool.ibs[i].length_dw = 0;
+		rdev->ib_pool.ibs[i].free = true;
 		INIT_LIST_HEAD(&rdev->ib_pool.ibs[i].list);
 	}
-	bitmap_zero(rdev->ib_pool.alloc_bm, RADEON_IB_POOL_SIZE);
+	rdev->ib_pool.head_id = 0;
 	rdev->ib_pool.ready = true;
 	DRM_INFO("radeon: ib pool ready.\n");
 	if (radeon_debugfs_ib_init(rdev)) {
@@ -246,7 +226,6 @@ void radeon_ib_pool_fini(struct radeon_device *rdev)
 	}
 	mutex_lock(&rdev->ib_pool.mutex);
 	radeon_ib_bogus_cleanup(rdev);
-	bitmap_zero(rdev->ib_pool.alloc_bm, RADEON_IB_POOL_SIZE);
 	if (rdev->ib_pool.robj) {
 		r = radeon_bo_reserve(rdev->ib_pool.robj, false);
 		if (likely(r == 0)) {
-- 
1.6.6


[-- Attachment #3: 0002-drm-radeon-kms-fix-bo-s-fence-association.patch --]
[-- Type: text/plain, Size: 4757 bytes --]

>From afec0d2ff7ce6c8569201cc852ce914403b6979b Mon Sep 17 00:00:00 2001
From: Jerome Glisse <jglisse@redhat.com>
Date: Mon, 15 Feb 2010 14:14:50 +0100
Subject: [PATCH 2/2] drm/radeon/kms: fix bo's fence association

Previous code did associate fence to bo before the fence was emited
and it also didn't lock protected access to ttm sync_obj member.
Both of this flaw leads to possible race between different code
path. This patch fix this by associating fence only once the fence
is emitted and properly lock protect access to sync_obj member of
ttm.

Fix:
https://bugs.freedesktop.org/show_bug.cgi?id=26438
and likely similar others bugs
Signed-off-by: Jerome Glisse <jglisse@redhat.com>
---
 drivers/gpu/drm/radeon/radeon_cs.c     |   10 +++-----
 drivers/gpu/drm/radeon/radeon_object.c |   36 +++++++++++++------------------
 drivers/gpu/drm/radeon/radeon_object.h |    4 +-
 3 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
index 1190148..e9d0850 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -86,7 +86,7 @@ int radeon_cs_parser_relocs(struct radeon_cs_parser *p)
 						&p->validated);
 		}
 	}
-	return radeon_bo_list_validate(&p->validated, p->ib->fence);
+	return radeon_bo_list_validate(&p->validated);
 }
 
 int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data)
@@ -189,12 +189,10 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error)
 {
 	unsigned i;
 
-	if (error && parser->ib) {
-		radeon_bo_list_unvalidate(&parser->validated,
-						parser->ib->fence);
-	} else {
-		radeon_bo_list_unreserve(&parser->validated);
+	if (!error && parser->ib) {
+		radeon_bo_list_fence(&parser->validated, parser->ib->fence);
 	}
+	radeon_bo_list_unreserve(&parser->validated);
 	for (i = 0; i < parser->nrelocs; i++) {
 		if (parser->relocs[i].gobj) {
 			mutex_lock(&parser->rdev->ddev->struct_mutex);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index d72a71b..f1da370 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -306,11 +306,10 @@ void radeon_bo_list_unreserve(struct list_head *head)
 	}
 }
 
-int radeon_bo_list_validate(struct list_head *head, void *fence)
+int radeon_bo_list_validate(struct list_head *head)
 {
 	struct radeon_bo_list *lobj;
 	struct radeon_bo *bo;
-	struct radeon_fence *old_fence = NULL;
 	int r;
 
 	r = radeon_bo_list_reserve(head);
@@ -334,32 +333,27 @@ int radeon_bo_list_validate(struct list_head *head, void *fence)
 		}
 		lobj->gpu_offset = radeon_bo_gpu_offset(bo);
 		lobj->tiling_flags = bo->tiling_flags;
-		if (fence) {
-			old_fence = (struct radeon_fence *)bo->tbo.sync_obj;
-			bo->tbo.sync_obj = radeon_fence_ref(fence);
-			bo->tbo.sync_obj_arg = NULL;
-		}
-		if (old_fence) {
-			radeon_fence_unref(&old_fence);
-		}
 	}
 	return 0;
 }
 
-void radeon_bo_list_unvalidate(struct list_head *head, void *fence)
+void radeon_bo_list_fence(struct list_head *head, void *fence)
 {
 	struct radeon_bo_list *lobj;
-	struct radeon_fence *old_fence;
-
-	if (fence)
-		list_for_each_entry(lobj, head, list) {
-			old_fence = to_radeon_fence(lobj->bo->tbo.sync_obj);
-			if (old_fence == fence) {
-				lobj->bo->tbo.sync_obj = NULL;
-				radeon_fence_unref(&old_fence);
-			}
+	struct radeon_bo *bo;
+	struct radeon_fence *old_fence = NULL;
+
+	list_for_each_entry(lobj, head, list) {
+		bo = lobj->bo;
+		spin_lock(&bo->tbo.lock);
+		old_fence = (struct radeon_fence *)bo->tbo.sync_obj;
+		bo->tbo.sync_obj = radeon_fence_ref(fence);
+		bo->tbo.sync_obj_arg = NULL;
+		spin_unlock(&bo->tbo.lock);
+		if (old_fence) {
+			radeon_fence_unref(&old_fence);
 		}
-	radeon_bo_list_unreserve(head);
+	}
 }
 
 int radeon_bo_fbdev_mmap(struct radeon_bo *bo,
diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h
index a02f180..7ab43de 100644
--- a/drivers/gpu/drm/radeon/radeon_object.h
+++ b/drivers/gpu/drm/radeon/radeon_object.h
@@ -156,8 +156,8 @@ extern void radeon_bo_list_add_object(struct radeon_bo_list *lobj,
 				struct list_head *head);
 extern int radeon_bo_list_reserve(struct list_head *head);
 extern void radeon_bo_list_unreserve(struct list_head *head);
-extern int radeon_bo_list_validate(struct list_head *head, void *fence);
-extern void radeon_bo_list_unvalidate(struct list_head *head, void *fence);
+extern int radeon_bo_list_validate(struct list_head *head);
+extern void radeon_bo_list_fence(struct list_head *head, void *fence);
 extern int radeon_bo_fbdev_mmap(struct radeon_bo *bo,
 				struct vm_area_struct *vma);
 extern int radeon_bo_set_tiling_flags(struct radeon_bo *bo,
-- 
1.6.6


[-- Attachment #4: Type: text/plain, Size: 254 bytes --]

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev

[-- Attachment #5: Type: text/plain, Size: 161 bytes --]

--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel