>From 5a9d7c4a4fa16bfe3148f161989411b4fe2bed77 Mon Sep 17 00:00:00 2001 From: Jerome Glisse 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 --- 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