[Mesa-dev] [PATCH 8/9] winsys/amdgpu: clean up error handling in amdgpu_cs_submit_ib

[Samuel Pitoiset]

With Timothy suggestions applied, patches 1-8 are:

Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

On 07/12/2018 07:26 AM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
> 
> ---
>   src/gallium/winsys/amdgpu/drm/amdgpu_cs.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
> index 872e67a790a..ac7160a5e51 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_cs.c
> @@ -1305,31 +1305,28 @@ void amdgpu_cs_submit_ib(void *job, int thread_index)
>         LIST_FOR_EACH_ENTRY(bo, &ws->global_bo_list, u.real.global_list_item) {
>            assert(num < ws->num_buffers);
>            handles[num++] = bo->bo;
>         }
>   
>         r = amdgpu_bo_list_create(ws->dev, ws->num_buffers,
>                                   handles, NULL, &bo_list);
>         simple_mtx_unlock(&ws->global_bo_list_lock);
>         if (r) {
>            fprintf(stderr, "amdgpu: buffer list creation failed (%d)\n", r);
> -         amdgpu_fence_signalled(cs->fence);
> -         cs->error_code = r;
>            goto cleanup;
>         }
>      } else {
>         unsigned num_handles;
>   
>         if (!amdgpu_add_sparse_backing_buffers(cs)) {
>            fprintf(stderr, "amdgpu: amdgpu_add_sparse_backing_buffers failed\n");
> -         amdgpu_fence_signalled(cs->fence);
> -         cs->error_code = -ENOMEM;
> +         r = -ENOMEM;
>            goto cleanup;
>         }
>   
>         amdgpu_bo_handle *handles = alloca(sizeof(*handles) * cs->num_real_buffers);
>         uint8_t *flags = alloca(sizeof(*flags) * cs->num_real_buffers);
>   
>         num_handles = 0;
>         for (i = 0; i < cs->num_real_buffers; ++i) {
>            struct amdgpu_cs_buffer *buffer = &cs->real_buffers[i];
>   
> @@ -1341,22 +1338,20 @@ void amdgpu_cs_submit_ib(void *job, int thread_index)
>            handles[num_handles] = buffer->bo->bo;
>            flags[num_handles] = (util_last_bit(buffer->u.real.priority_usage) - 1) / 2;
>   	 ++num_handles;
>         }
>   
>         if (num_handles) {
>            r = amdgpu_bo_list_create(ws->dev, num_handles,
>                                      handles, flags, &bo_list);
>            if (r) {
>               fprintf(stderr, "amdgpu: buffer list creation failed (%d)\n", r);
> -            amdgpu_fence_signalled(cs->fence);
> -            cs->error_code = r;
>               goto cleanup;
>            }
>         }
>      }
>   
>      if (acs->ring_type == RING_GFX)
>         ws->gfx_bo_list_counter += cs->num_real_buffers;
>   
>      if (acs->ctx->num_rejected_cs) {
>         r = -ECANCELED;
> @@ -1451,48 +1446,52 @@ void amdgpu_cs_submit_ib(void *job, int thread_index)
>            chunks[num_chunks].chunk_data = (uintptr_t)sem_chunk;
>            num_chunks++;
>         }
>   
>         assert(num_chunks <= ARRAY_SIZE(chunks));
>   
>         r = amdgpu_cs_submit_raw(ws->dev, acs->ctx->ctx, bo_list,
>                                  num_chunks, chunks, &seq_no);
>      }
>   
> -   cs->error_code = r;
>      if (r) {
>         if (r == -ENOMEM)
>            fprintf(stderr, "amdgpu: Not enough memory for command submission.\n");
>         else if (r == -ECANCELED)
>            fprintf(stderr, "amdgpu: The CS has been cancelled because the context is lost.\n");
>         else
>            fprintf(stderr, "amdgpu: The CS has been rejected, "
>                    "see dmesg for more information (%i).\n", r);
>   
> -      amdgpu_fence_signalled(cs->fence);
> -
>         acs->ctx->num_rejected_cs++;
>         ws->num_total_rejected_cs++;
>      } else {
>         /* Success. */
>         uint64_t *user_fence = NULL;
>   
>         if (has_user_fence)
>            user_fence = acs->ctx->user_fence_cpu_address_base + acs->ring_type;
>         amdgpu_fence_submitted(cs->fence, seq_no, user_fence);
>      }
>   
>      /* Cleanup. */
>      if (bo_list)
>         amdgpu_bo_list_destroy(bo_list);
>   
>   cleanup:
> +   /* If there was an error, signal the fence, because it won't be signalled
> +    * by the hardware. */
> +   if (r)
> +      amdgpu_fence_signalled(cs->fence);
> +
> +   cs->error_code = r;
> +
>      for (i = 0; i < cs->num_real_buffers; i++)
>         p_atomic_dec(&cs->real_buffers[i].bo->num_active_ioctls);
>      for (i = 0; i < cs->num_slab_buffers; i++)
>         p_atomic_dec(&cs->slab_buffers[i].bo->num_active_ioctls);
>      for (i = 0; i < cs->num_sparse_buffers; i++)
>         p_atomic_dec(&cs->sparse_buffers[i].bo->num_active_ioctls);
>   
>      amdgpu_cs_context_cleanup(cs);
>   }
>   
>