Mesa (main): freedreno/drm: Move suballoc_bo to device

[GitLab Mirror]

Module: Mesa
Branch: main
Commit: d2a7afe34d8a3bba51035fce1d8b8e40c7a521d8
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=d2a7afe34d8a3bba51035fce1d8b8e40c7a521d8

Author: Rob Clark <robdclark at chromium.org>
Date:   Mon Oct 25 15:59:23 2021 -0700

freedreno/drm: Move suballoc_bo to device

Having it in msm_pipe isn't saving any locking.  But it does mean that
cleanup_fences() can drop the last pipe reference, which in turn drops
the last suballoc_bo reference, which can cause recursion back into the
bo cache.

Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/5562
Signed-off-by: Rob Clark <robdclark at chromium.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/13521>

---

 src/freedreno/drm/freedreno_device.c  |  4 ++++
 src/freedreno/drm/freedreno_priv.h    | 17 +++++++++++++++++
 src/freedreno/drm/msm_pipe.c          |  3 ---
 src/freedreno/drm/msm_priv.h          |  4 ----
 src/freedreno/drm/msm_ringbuffer_sp.c | 25 ++++++++++++-------------
 5 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/src/freedreno/drm/freedreno_device.c b/src/freedreno/drm/freedreno_device.c
index dc01f65abd6..31c306fb04b 100644
--- a/src/freedreno/drm/freedreno_device.c
+++ b/src/freedreno/drm/freedreno_device.c
@@ -86,6 +86,7 @@ out:
 
    list_inithead(&dev->deferred_submits);
    simple_mtx_init(&dev->submit_lock, mtx_plain);
+   simple_mtx_init(&dev->suballoc_lock, mtx_plain);
 
    return dev;
 }
@@ -130,6 +131,9 @@ fd_device_del_impl(struct fd_device *dev)
 
    assert(list_is_empty(&dev->deferred_submits));
 
+   if (dev->suballoc_bo)
+      fd_bo_del_locked(dev->suballoc_bo);
+
    fd_bo_cache_cleanup(&dev->bo_cache, 0);
    fd_bo_cache_cleanup(&dev->ring_cache, 0);
    _mesa_hash_table_destroy(dev->handle_table, NULL);
diff --git a/src/freedreno/drm/freedreno_priv.h b/src/freedreno/drm/freedreno_priv.h
index b1616f68d95..bb165b87285 100644
--- a/src/freedreno/drm/freedreno_priv.h
+++ b/src/freedreno/drm/freedreno_priv.h
@@ -148,6 +148,23 @@ struct fd_device {
    struct list_head deferred_submits;
    unsigned deferred_cmds;
    simple_mtx_t submit_lock;
+
+   /**
+    * BO for suballocating long-lived state objects.
+    *
+    * Note: one would be tempted to put this in fd_pipe to avoid locking.
+    * But that is a bad idea for a couple of reasons:
+    *
+    *  1) With TC, stateobj allocation can happen in either frontend thread
+    *     (ie. most CSOs), and also driver thread (a6xx cached tex state)
+    *  2) It is best for fd_pipe to not hold a reference to a BO that can
+    *     be free'd to bo cache, as that can cause unexpected re-entrancy
+    *     (fd_bo_cache_alloc() -> find_in_bucket() -> fd_bo_state() ->
+    *     cleanup_fences() -> drop pipe ref which free's bo's).
+    */
+   struct fd_bo *suballoc_bo;
+   uint32_t suballoc_offset;
+   simple_mtx_t suballoc_lock;
 };
 
 #define foreach_submit(name, list) \
diff --git a/src/freedreno/drm/msm_pipe.c b/src/freedreno/drm/msm_pipe.c
index 0c35063c35f..4a71a9f9400 100644
--- a/src/freedreno/drm/msm_pipe.c
+++ b/src/freedreno/drm/msm_pipe.c
@@ -172,9 +172,6 @@ msm_pipe_destroy(struct fd_pipe *pipe)
 {
    struct msm_pipe *msm_pipe = to_msm_pipe(pipe);
 
-   if (msm_pipe->suballoc_bo)
-      fd_bo_del_locked(msm_pipe->suballoc_bo);
-
    close_submitqueue(pipe, msm_pipe->queue_id);
    msm_pipe_sp_ringpool_init(msm_pipe);
    free(msm_pipe);
diff --git a/src/freedreno/drm/msm_priv.h b/src/freedreno/drm/msm_priv.h
index 5299b114fbe..9e0211d922b 100644
--- a/src/freedreno/drm/msm_priv.h
+++ b/src/freedreno/drm/msm_priv.h
@@ -58,10 +58,6 @@ struct msm_pipe {
    uint32_t queue_id;
    struct slab_parent_pool ring_pool;
 
-   /* BO for suballocating long-lived objects on the pipe. */
-   struct fd_bo *suballoc_bo;
-   uint32_t suballoc_offset;
-
    /**
     * The last fence seqno that was flushed to kernel (doesn't mean that it
     * is complete, just that the kernel knows about it)
diff --git a/src/freedreno/drm/msm_ringbuffer_sp.c b/src/freedreno/drm/msm_ringbuffer_sp.c
index 1d7e7b94967..e84e37d8944 100644
--- a/src/freedreno/drm/msm_ringbuffer_sp.c
+++ b/src/freedreno/drm/msm_ringbuffer_sp.c
@@ -825,34 +825,33 @@ msm_ringbuffer_sp_init(struct msm_ringbuffer_sp *msm_ring, uint32_t size,
 struct fd_ringbuffer *
 msm_ringbuffer_sp_new_object(struct fd_pipe *pipe, uint32_t size)
 {
-   struct msm_pipe *msm_pipe = to_msm_pipe(pipe);
+   struct fd_device *dev = pipe->dev;
    struct msm_ringbuffer_sp *msm_ring = malloc(sizeof(*msm_ring));
 
    /* Lock access to the msm_pipe->suballoc_* since ringbuffer object allocation
     * can happen both on the frontend (most CSOs) and the driver thread (a6xx
     * cached tex state, for example)
     */
-   static simple_mtx_t suballoc_lock = _SIMPLE_MTX_INITIALIZER_NP;
-   simple_mtx_lock(&suballoc_lock);
+   simple_mtx_lock(&dev->suballoc_lock);
 
    /* Maximum known alignment requirement is a6xx's TEX_CONST at 16 dwords */
-   msm_ring->offset = align(msm_pipe->suballoc_offset, 64);
-   if (!msm_pipe->suballoc_bo ||
-       msm_ring->offset + size > fd_bo_size(msm_pipe->suballoc_bo)) {
-      if (msm_pipe->suballoc_bo)
-         fd_bo_del(msm_pipe->suballoc_bo);
-      msm_pipe->suballoc_bo =
-         fd_bo_new_ring(pipe->dev, MAX2(SUBALLOC_SIZE, align(size, 4096)));
+   msm_ring->offset = align(dev->suballoc_offset, 64);
+   if (!dev->suballoc_bo ||
+       msm_ring->offset + size > fd_bo_size(dev->suballoc_bo)) {
+      if (dev->suballoc_bo)
+         fd_bo_del(dev->suballoc_bo);
+      dev->suballoc_bo =
+         fd_bo_new_ring(dev, MAX2(SUBALLOC_SIZE, align(size, 4096)));
       msm_ring->offset = 0;
    }
 
    msm_ring->u.pipe = pipe;
-   msm_ring->ring_bo = fd_bo_ref(msm_pipe->suballoc_bo);
+   msm_ring->ring_bo = fd_bo_ref(dev->suballoc_bo);
    msm_ring->base.refcnt = 1;
 
-   msm_pipe->suballoc_offset = msm_ring->offset + size;
+   dev->suballoc_offset = msm_ring->offset + size;
 
-   simple_mtx_unlock(&suballoc_lock);
+   simple_mtx_unlock(&dev->suballoc_lock);
 
    return msm_ringbuffer_sp_init(msm_ring, size, _FD_RINGBUFFER_OBJECT);
 }