Mesa (master): winsys/amdgpu: fix a deadlock when waiting for submission_in_progress

[Marek Olšák]

Module: Mesa
Branch: master
Commit: 58af1f6bb074168669aaec2755c7f369a8b58d62
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=58af1f6bb074168669aaec2755c7f369a8b58d62

Author: Marek Olšák <marek.olsak at amd.com>
Date:   Mon Jun 19 19:39:05 2017 +0200

winsys/amdgpu: fix a deadlock when waiting for submission_in_progress

First this happens:

1) amdgpu_cs_flush (lock bo_fence_lock)
   -> amdgpu_add_fence_dependency
   -> os_wait_until_zero (wait for submission_in_progress) - WAITING

2) amdgpu_bo_create
   -> pb_cache_reclaim_buffer (lock pb_cache::mutex)
   -> pb_cache_is_buffer_compat
   -> amdgpu_bo_wait (lock bo_fence_lock) - WAITING

So both bo_fence_lock and pb_cache::mutex are held. amdgpu_bo_create can't
continue. amdgpu_cs_flush is waiting for the CS ioctl to finish the job,
but the CS ioctl is trying to release a buffer:

3) amdgpu_cs_submit_ib (CS thread - job entrypoint)
   -> amdgpu_cs_context_cleanup
   -> pb_reference
   -> pb_destroy
   -> amdgpu_bo_destroy_or_cache
   -> pb_cache_add_buffer (lock pb_cache::mutex) - DEADLOCK

The simple solution is not to wait for submission_in_progress, which we
need in order to create the list of dependencies for the CS ioctl. Instead
of building the list of dependencies as a direct input to the CS ioctl,
build the list of dependencies as a list of fences, and make the final list
of dependencies in the CS thread itself.

Therefore, amdgpu_cs_flush doesn't have to wait and can continue.
Then, amdgpu_bo_create can continue and return. And then amdgpu_cs_submit_ib
can continue.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101294

Cc: 17.1 <mesa-stable at lists.freedesktop.org>
Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

---

 src/gallium/winsys/amdgpu/drm/amdgpu_cs.c | 55 ++++++++++++++++++++++---------
 src/gallium/winsys/amdgpu/drm/amdgpu_cs.h |  4 ++-
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
index a0ef826296..c88be069d0 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
@@ -752,10 +752,13 @@ static void amdgpu_cs_context_cleanup(struct amdgpu_cs_context *cs)
       p_atomic_dec(&cs->sparse_buffers[i].bo->num_cs_references);
       amdgpu_winsys_bo_reference(&cs->sparse_buffers[i].bo, NULL);
    }
+   for (i = 0; i < cs->num_fence_dependencies; i++)
+      amdgpu_fence_reference(&cs->fence_dependencies[i], NULL);
 
    cs->num_real_buffers = 0;
    cs->num_slab_buffers = 0;
    cs->num_sparse_buffers = 0;
+   cs->num_fence_dependencies = 0;
    amdgpu_fence_reference(&cs->fence, NULL);
 
    memset(cs->buffer_indices_hashlist, -1, sizeof(cs->buffer_indices_hashlist));
@@ -770,7 +773,7 @@ static void amdgpu_destroy_cs_context(struct amdgpu_cs_context *cs)
    FREE(cs->handles);
    FREE(cs->slab_buffers);
    FREE(cs->sparse_buffers);
-   FREE(cs->request.dependencies);
+   FREE(cs->fence_dependencies);
 }
 
 
@@ -981,7 +984,6 @@ static void amdgpu_add_fence_dependency(struct amdgpu_cs *acs,
 {
    struct amdgpu_cs_context *cs = acs->csc;
    struct amdgpu_winsys_bo *bo = buffer->bo;
-   struct amdgpu_cs_fence *dep;
    unsigned new_num_fences = 0;
 
    for (unsigned j = 0; j < bo->num_fences; ++j) {
@@ -1003,21 +1005,21 @@ static void amdgpu_add_fence_dependency(struct amdgpu_cs *acs,
       if (!(buffer->usage & RADEON_USAGE_SYNCHRONIZED))
          continue;
 
-      if (bo_fence->submission_in_progress)
-         os_wait_until_zero(&bo_fence->submission_in_progress,
-                            PIPE_TIMEOUT_INFINITE);
-
-      idx = cs->request.number_of_dependencies++;
-      if (idx >= cs->max_dependencies) {
+      idx = cs->num_fence_dependencies++;
+      if (idx >= cs->max_fence_dependencies) {
          unsigned size;
-
-         cs->max_dependencies = idx + 8;
-         size = cs->max_dependencies * sizeof(struct amdgpu_cs_fence);
-         cs->request.dependencies = realloc(cs->request.dependencies, size);
+         const unsigned increment = 8;
+
+         cs->max_fence_dependencies = idx + increment;
+         size = cs->max_fence_dependencies * sizeof(cs->fence_dependencies[0]);
+         cs->fence_dependencies = realloc(cs->fence_dependencies, size);
+         /* Clear the newly-allocated elements. */
+         memset(cs->fence_dependencies + idx, 0,
+                increment * sizeof(cs->fence_dependencies[0]));
       }
 
-      dep = &cs->request.dependencies[idx];
-      memcpy(dep, &bo_fence->fence, sizeof(*dep));
+      amdgpu_fence_reference(&cs->fence_dependencies[idx],
+                             (struct pipe_fence_handle*)bo_fence);
    }
 
    for (unsigned j = new_num_fences; j < bo->num_fences; ++j)
@@ -1088,7 +1090,7 @@ static void amdgpu_add_fence_dependencies(struct amdgpu_cs *acs)
 {
    struct amdgpu_cs_context *cs = acs->csc;
 
-   cs->request.number_of_dependencies = 0;
+   cs->num_fence_dependencies = 0;
 
    amdgpu_add_fence_dependencies_list(acs, cs->fence, cs->num_real_buffers, cs->real_buffers);
    amdgpu_add_fence_dependencies_list(acs, cs->fence, cs->num_slab_buffers, cs->slab_buffers);
@@ -1136,7 +1138,30 @@ void amdgpu_cs_submit_ib(void *job, int thread_index)
    struct amdgpu_winsys *ws = acs->ctx->ws;
    struct amdgpu_cs_context *cs = acs->cst;
    int i, r;
+   struct amdgpu_cs_fence *dependencies = NULL;
+
+   /* Set dependencies (input fences). */
+   if (cs->num_fence_dependencies) {
+      dependencies = alloca(sizeof(dependencies[0]) *
+                            cs->num_fence_dependencies);
+      unsigned num = 0;
+
+      for (i = 0; i < cs->num_fence_dependencies; i++) {
+         struct amdgpu_fence *fence =
+            (struct amdgpu_fence*)cs->fence_dependencies[i];
+
+         /* Past fences can't be unsubmitted because we have only 1 CS thread. */
+         assert(!fence->submission_in_progress);
+         memcpy(&dependencies[num++], &fence->fence, sizeof(dependencies[0]));
+      }
+      cs->request.dependencies = dependencies;
+      cs->request.number_of_dependencies = num;
+   } else {
+      cs->request.dependencies = NULL;
+      cs->request.number_of_dependencies = 0;
+   }
 
+   /* Set the output fence. */
    cs->request.fence_info.handle = NULL;
    if (amdgpu_cs_has_user_fence(cs)) {
 	cs->request.fence_info.handle = acs->ctx->user_fence_bo;
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.h b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.h
index d700b8c4cb..d83c1e0fe1 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.h
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.h
@@ -105,7 +105,9 @@ struct amdgpu_cs_context {
    unsigned                    last_added_bo_usage;
    uint64_t                    last_added_bo_priority_usage;
 
-   unsigned                    max_dependencies;
+   struct pipe_fence_handle    **fence_dependencies;
+   unsigned                    num_fence_dependencies;
+   unsigned                    max_fence_dependencies;
 
    struct pipe_fence_handle    *fence;