[PATCH] drm/amdgpu: fix user fence write race condition

[Alex Deucher]

On Fri, Jun 29, 2018 at 7:30 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> The buffer object backing the user fence is reserved using the non-user
> fence, i.e., as soon as the non-user fence is signaled, the user fence
> buffer object can be moved or even destroyed.
>
> Therefore, emit the user fence first.
>
> Both fences have the same cache invalidation behavior, so this should
> have no user-visible effect.
>
> Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> ---
> There is one aspect to this change that I'm a bit unsure about: what does
> insert_end do? It's only used by UVD & friends, and since those rings
> don't use user fences I guess this patch doesn't really change anything
> for them. And having the insert_end between those fences always looked
> a bit suspicious...

It should be fine I think.  some MM engines just need start and end
markers around a work batch.  They don't support user fences so it
shouldn't be an issue.  Patch looks good to me.

Reviewed-by: Alex Deucher <alexander.deucher at amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 829c4d2a33b9..8117b8c2113e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -227,38 +227,38 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>         if (ring->funcs->emit_tmz)
>                 amdgpu_ring_emit_tmz(ring, false);
>
>         if (ring->funcs->emit_hdp_invalidate
>  #ifdef CONFIG_X86_64
>             && !(adev->flags & AMD_IS_APU)
>  #endif
>            )
>                 amdgpu_ring_emit_hdp_invalidate(ring);
>
> +       /* wrap the last IB with fence */
> +       if (job && job->uf_addr) {
> +               amdgpu_ring_emit_fence(ring, job->uf_addr, job->uf_sequence,
> +                                      AMDGPU_FENCE_FLAG_64BIT);
> +       }
> +
>         r = amdgpu_fence_emit(ring, f);
>         if (r) {
>                 dev_err(adev->dev, "failed to emit fence (%d)\n", r);
>                 if (job && job->vmid)
>                         amdgpu_vmid_reset(adev, ring->funcs->vmhub, job->vmid);
>                 amdgpu_ring_undo(ring);
>                 return r;
>         }
>
>         if (ring->funcs->insert_end)
>                 ring->funcs->insert_end(ring);
>
> -       /* wrap the last IB with fence */
> -       if (job && job->uf_addr) {
> -               amdgpu_ring_emit_fence(ring, job->uf_addr, job->uf_sequence,
> -                                      AMDGPU_FENCE_FLAG_64BIT);
> -       }
> -
>         if (patch_offset != ~0 && ring->funcs->patch_cond_exec)
>                 amdgpu_ring_patch_cond_exec(ring, patch_offset);
>
>         ring->current_ctx = fence_ctx;
>         if (vm && ring->funcs->emit_switch_buffer)
>                 amdgpu_ring_emit_switch_buffer(ring);
>         amdgpu_ring_commit(ring);
>         return 0;
>  }
>
> --
> 2.14.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx