Attachment 132069 Details for Bug 101294 – tentative fix

[patch] tentative fix

0001-winsys-amdgpu-fix-a-deadlock-when-waiting-for-submis.patch (text/plain), 6.70 KB, created by Marek Olšák on 2017-06-19 18:13:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marek Olšák

Created: 2017-06-19 18:13:43 UTC

Size: 6.70 KB

patch

obsolete

>From c1e129e6bb249bc3386656b7b4007fa1c33c2014 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak@amd.com>
>Date: Mon, 19 Jun 2017 19:39:05 +0200
>Subject: [PATCH] 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
>---
> 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 a0ef826..c88be06 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 d700b8c..d83c1e0 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;
> 
>-- 
>2.7.4
>

Actions: View | Splinter Review

Attachments on bug 101294: 131700 | 131721 | 132069

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.