From 75a4d902806d9a947924267efdfeb65b3ba4532a Mon Sep 17 00:00:00 2001 From: Rob Clark Date: Thu, 8 Dec 2022 14:35:28 -0800 Subject: [PATCH] freedreno/drm: Start prying apart table_lock We want to get back to the point where table_lock is only doing what it's named for (ie. protecting handle/name tables). This is the first step. Add new locks to protect fencing and the bo caches. A single global lock for fences, to avoid atomic instructions in the loop per submit bo attaching fences. This couldn't be broken appart into different commits, otherwise there would have been an ABBA deadlock in BO cache's call to fd_bo_state() to check if a BO is idle. Signed-off-by: Rob Clark Part-of: --- src/freedreno/drm/freedreno_bo.c | 22 ++++++++------ src/freedreno/drm/freedreno_bo_cache.c | 23 +++++++++++---- src/freedreno/drm/freedreno_device.c | 8 ------ src/freedreno/drm/freedreno_pipe.c | 32 ++++++++------------- src/freedreno/drm/freedreno_priv.h | 8 ++++-- src/freedreno/drm/freedreno_ringbuffer_sp.c | 4 +-- src/freedreno/drm/msm/msm_ringbuffer.c | 4 +-- 7 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/freedreno/drm/freedreno_bo.c b/src/freedreno/drm/freedreno_bo.c index dc5fcd18d04..9719c0674f2 100644 --- a/src/freedreno/drm/freedreno_bo.c +++ b/src/freedreno/drm/freedreno_bo.c @@ -30,6 +30,7 @@ #include "freedreno_priv.h" simple_mtx_t table_lock = SIMPLE_MTX_INITIALIZER; +simple_mtx_t fence_lock = SIMPLE_MTX_INITIALIZER; void bo_del(struct fd_bo *bo); void bo_del_flush(struct fd_device *dev); @@ -347,7 +348,7 @@ bo_del(struct fd_bo *bo) simple_mtx_assert_locked(&table_lock); for (int i = 0; i < bo->nr_fences; i++) - fd_pipe_del_locked(bo->fences[i].pipe); + fd_pipe_del(bo->fences[i].pipe); if (bo->fences != &bo->_inline_fence) free(bo->fences); @@ -524,9 +525,7 @@ fd_bo_prefer_upload(struct fd_bo *bo, unsigned len) int fd_bo_cpu_prep(struct fd_bo *bo, struct fd_pipe *pipe, uint32_t op) { - simple_mtx_lock(&table_lock); enum fd_bo_state state = fd_bo_state(bo); - simple_mtx_unlock(&table_lock); if (state == FD_BO_STATE_IDLE) return 0; @@ -570,7 +569,7 @@ fd_bo_cpu_prep(struct fd_bo *bo, struct fd_pipe *pipe, uint32_t op) static void cleanup_fences(struct fd_bo *bo) { - simple_mtx_assert_locked(&table_lock); + simple_mtx_assert_locked(&fence_lock); for (int i = 0; i < bo->nr_fences; i++) { struct fd_bo_fence *f = &bo->fences[i]; @@ -595,7 +594,7 @@ cleanup_fences(struct fd_bo *bo) void fd_bo_add_fence(struct fd_bo *bo, struct fd_pipe *pipe, uint32_t fence) { - simple_mtx_assert_locked(&table_lock); + simple_mtx_assert_locked(&fence_lock); if (bo->nosync) return; @@ -634,13 +633,18 @@ fd_bo_add_fence(struct fd_bo *bo, struct fd_pipe *pipe, uint32_t fence) enum fd_bo_state fd_bo_state(struct fd_bo *bo) { - simple_mtx_assert_locked(&table_lock); - - cleanup_fences(bo); - + /* NOTE: check the nosync case before touching fence_lock in case we end + * up here recursively from dropping pipe reference in cleanup_fences(). + * The pipe's control buffer is specifically nosync to avoid recursive + * lock problems here. + */ if (bo->shared || bo->nosync) return FD_BO_STATE_UNKNOWN; + simple_mtx_lock(&fence_lock); + cleanup_fences(bo); + simple_mtx_unlock(&fence_lock); + if (!bo->nr_fences) return FD_BO_STATE_IDLE; diff --git a/src/freedreno/drm/freedreno_bo_cache.c b/src/freedreno/drm/freedreno_bo_cache.c index 3fe1fbf4354..2977a30ce52 100644 --- a/src/freedreno/drm/freedreno_bo_cache.c +++ b/src/freedreno/drm/freedreno_bo_cache.c @@ -37,7 +37,6 @@ void bo_del(struct fd_bo *bo); void bo_del_flush(struct fd_device *dev); -extern simple_mtx_t table_lock; static void bo_remove_from_bucket(struct fd_bo_bucket *bucket, struct fd_bo *bo) @@ -116,6 +115,7 @@ fd_bo_cache_init(struct fd_bo_cache *cache, int coarse, const char *name) unsigned long size, cache_max_size = 64 * 1024 * 1024; cache->name = name; + simple_mtx_init(&cache->lock, mtx_plain); /* OK, so power of two buckets was too wasteful of memory. * Give 3 other sizes between each power of two, to hopefully @@ -153,6 +153,11 @@ fd_bo_cache_cleanup(struct fd_bo_cache *cache, time_t time) if (cache->time == time) return; + struct list_head freelist; + + list_inithead(&freelist); + + simple_mtx_lock(&cache->lock); for (i = 0; i < cache->num_buckets; i++) { struct fd_bo_bucket *bucket = &cache->cache_bucket[i]; struct fd_bo *bo; @@ -174,11 +179,17 @@ fd_bo_cache_cleanup(struct fd_bo_cache *cache, time_t time) VG_BO_OBTAIN(bo); bo_remove_from_bucket(bucket, bo); bucket->expired++; - bo_del(bo); + list_addtail(&bo->node, &freelist); cnt++; } } + simple_mtx_unlock(&cache->lock); + + foreach_bo_safe (bo, &freelist) { + list_del(&bo->node); + bo_del(bo); + } /* Note: when called in bo_del_or_recycle() -> fd_bo_cache_free() path, * the caller will handle bo_del_flush(). @@ -224,7 +235,6 @@ find_in_bucket(struct fd_bo_bucket *bucket, uint32_t flags) * NOTE that intel takes ALLOC_FOR_RENDER bo's from the list tail * (MRU, since likely to be in GPU cache), rather than head (LRU).. */ - simple_mtx_lock(&table_lock); foreach_bo (entry, &bucket->list) { if (fd_bo_state(entry) != FD_BO_STATE_IDLE) { break; @@ -235,7 +245,6 @@ find_in_bucket(struct fd_bo_bucket *bucket, uint32_t flags) break; } } - simple_mtx_unlock(&table_lock); return bo; } @@ -254,7 +263,9 @@ fd_bo_cache_alloc(struct fd_bo_cache *cache, uint32_t *size, uint32_t flags) retry: if (bucket) { *size = bucket->size; + simple_mtx_lock(&cache->lock); bo = find_in_bucket(bucket, flags); + simple_mtx_unlock(&cache->lock); if (bo) { VG_BO_OBTAIN(bo); if (bo->funcs->madvise(bo, true) <= 0) { @@ -301,9 +312,11 @@ fd_bo_cache_free(struct fd_bo_cache *cache, struct fd_bo *bo) bo->free_time = time.tv_sec; VG_BO_RELEASE(bo); - list_addtail(&bo->node, &bucket->list); + simple_mtx_lock(&cache->lock); + list_addtail(&bo->node, &bucket->list); bucket->count++; + simple_mtx_unlock(&cache->lock); fd_bo_cache_cleanup(cache, time.tv_sec); diff --git a/src/freedreno/drm/freedreno_device.c b/src/freedreno/drm/freedreno_device.c index bff877ad419..3dd938d71f8 100644 --- a/src/freedreno/drm/freedreno_device.c +++ b/src/freedreno/drm/freedreno_device.c @@ -178,14 +178,6 @@ fd_device_del_impl(struct fd_device *dev) free(dev); } -void -fd_device_del_locked(struct fd_device *dev) -{ - if (!p_atomic_dec_zero(&dev->refcnt)) - return; - fd_device_del_impl(dev); -} - void fd_device_del(struct fd_device *dev) { diff --git a/src/freedreno/drm/freedreno_pipe.c b/src/freedreno/drm/freedreno_pipe.c index 5d52573a724..1f5aa3ba8c9 100644 --- a/src/freedreno/drm/freedreno_pipe.c +++ b/src/freedreno/drm/freedreno_pipe.c @@ -94,46 +94,38 @@ fd_pipe_new(struct fd_device *dev, enum fd_pipe_id id) struct fd_pipe * fd_pipe_ref(struct fd_pipe *pipe) { - simple_mtx_lock(&table_lock); + simple_mtx_lock(&fence_lock); fd_pipe_ref_locked(pipe); - simple_mtx_unlock(&table_lock); + simple_mtx_unlock(&fence_lock); return pipe; } struct fd_pipe * fd_pipe_ref_locked(struct fd_pipe *pipe) { - simple_mtx_assert_locked(&table_lock); + simple_mtx_assert_locked(&fence_lock); pipe->refcnt++; return pipe; } - -static void -pipe_del_locked(struct fd_pipe *pipe) -{ - fd_bo_del_locked(pipe->control_mem); - fd_device_del_locked(pipe->dev); - pipe->funcs->destroy(pipe); -} - void fd_pipe_del(struct fd_pipe *pipe) { - if (!p_atomic_dec_zero(&pipe->refcnt)) - return; - simple_mtx_lock(&table_lock); - pipe_del_locked(pipe); - simple_mtx_unlock(&table_lock); + simple_mtx_lock(&fence_lock); + fd_pipe_del_locked(pipe); + simple_mtx_unlock(&fence_lock); } void fd_pipe_del_locked(struct fd_pipe *pipe) { - simple_mtx_assert_locked(&table_lock); - if (!p_atomic_dec_zero(&pipe->refcnt)) + simple_mtx_assert_locked(&fence_lock); + if (--pipe->refcnt) return; - pipe_del_locked(pipe); + + fd_bo_del(pipe->control_mem); + fd_device_del(pipe->dev); + pipe->funcs->destroy(pipe); } /** diff --git a/src/freedreno/drm/freedreno_priv.h b/src/freedreno/drm/freedreno_priv.h index a731561bfbd..168661f0e49 100644 --- a/src/freedreno/drm/freedreno_priv.h +++ b/src/freedreno/drm/freedreno_priv.h @@ -52,6 +52,7 @@ #include "freedreno_ringbuffer.h" extern simple_mtx_t table_lock; +extern simple_mtx_t fence_lock; #define SUBALLOC_SIZE (32 * 1024) /* Maximum known alignment requirement is a6xx's TEX_CONST at 16 dwords */ @@ -119,6 +120,7 @@ struct fd_bo_bucket { struct fd_bo_cache { const char *name; + simple_mtx_t lock; struct fd_bo_bucket cache_bucket[14 * 4]; int num_buckets; time_t time; @@ -216,7 +218,7 @@ int fd_bo_cache_free(struct fd_bo_cache *cache, struct fd_bo *bo); /* for where @table_lock is already held: */ void fd_bo_del_locked(struct fd_bo *bo); -void fd_device_del_locked(struct fd_device *dev); +/* for where @fence_lock is already held: */ void fd_pipe_del_locked(struct fd_pipe *pipe); struct fd_pipe_funcs { @@ -251,8 +253,8 @@ struct fd_pipe { struct fd_dev_id dev_id; /** - * Note refcnt is *not* atomic, but protected by table_lock, since the - * table_lock is held in fd_bo_add_fence(), which is the hotpath. + * Note refcnt is *not* atomic, but protected by fence_lock, since the + * fence_lock is held in fd_bo_add_fence(), which is the hotpath. */ int32_t refcnt; diff --git a/src/freedreno/drm/freedreno_ringbuffer_sp.c b/src/freedreno/drm/freedreno_ringbuffer_sp.c index b707a10f8bd..0c8c0b25c19 100644 --- a/src/freedreno/drm/freedreno_ringbuffer_sp.c +++ b/src/freedreno/drm/freedreno_ringbuffer_sp.c @@ -179,12 +179,12 @@ fd_submit_sp_flush_prep(struct fd_submit *submit, int in_fence_fd, for (unsigned i = 0; i < primary->u.nr_cmds; i++) fd_submit_append_bo(fd_submit, primary->u.cmds[i].ring_bo); - simple_mtx_lock(&table_lock); + simple_mtx_lock(&fence_lock); for (unsigned i = 0; i < fd_submit->nr_bos; i++) { fd_bo_add_fence(fd_submit->bos[i], submit->pipe, submit->fence); has_shared |= fd_submit->bos[i]->shared; } - simple_mtx_unlock(&table_lock); + simple_mtx_unlock(&fence_lock); fd_submit->out_fence = out_fence; fd_submit->in_fence_fd = (in_fence_fd == -1) ? diff --git a/src/freedreno/drm/msm/msm_ringbuffer.c b/src/freedreno/drm/msm/msm_ringbuffer.c index 84ad20b41f8..8a09379430d 100644 --- a/src/freedreno/drm/msm/msm_ringbuffer.c +++ b/src/freedreno/drm/msm/msm_ringbuffer.c @@ -342,11 +342,11 @@ msm_submit_flush(struct fd_submit *submit, int in_fence_fd, } } - simple_mtx_lock(&table_lock); + simple_mtx_lock(&fence_lock); for (unsigned j = 0; j < msm_submit->nr_bos; j++) { fd_bo_add_fence(msm_submit->bos[j], submit->pipe, submit->fence); } - simple_mtx_unlock(&table_lock); + simple_mtx_unlock(&fence_lock); if (in_fence_fd != -1) { req.flags |= MSM_SUBMIT_FENCE_FD_IN | MSM_SUBMIT_NO_IMPLICIT;