[Intel-xe] [PATCH 2/5] drm/xe: Fix VM bind out-sync signaling ordering
Mishra, Pallavi
pallavi.mishra at intel.com
Fri Jul 28 17:32:58 UTC 2023
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> Matthew Brost
> Sent: Wednesday, July 26, 2023 6:40 PM
> To: intel-xe at lists.freedesktop.org
> Subject: [Intel-xe] [PATCH 2/5] drm/xe: Fix VM bind out-sync signaling
> ordering
>
> A case existed where an out-sync of a later VM bind operation could signal
> before a previous one if the later operation results in a NOP (e.g. a unbind or
> prefetch to a VA range without any mappings). This breaks the ordering
> rules, fix this. This patch also lays the groundwork for users to pass in
> num_binds == 0 and out-syncs.
>
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> ---
> drivers/gpu/drm/xe/xe_engine.c | 75 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_engine.h | 7 +++
> drivers/gpu/drm/xe/xe_engine_types.h | 6 +++
> drivers/gpu/drm/xe/xe_vm.c | 44 +++++++++++++---
> 4 files changed, 124 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_engine.c
> b/drivers/gpu/drm/xe/xe_engine.c index 1c6b84f32823..bef82b40a097
> 100644
> --- a/drivers/gpu/drm/xe/xe_engine.c
> +++ b/drivers/gpu/drm/xe/xe_engine.c
> @@ -135,6 +135,7 @@ void xe_engine_destroy(struct kref *ref)
> struct xe_engine *e = container_of(ref, struct xe_engine, refcount);
> struct xe_engine *engine, *next;
>
> + xe_engine_last_fence_put_unlocked(e);
> if (!(e->flags & ENGINE_FLAG_BIND_ENGINE_CHILD)) {
> list_for_each_entry_safe(engine, next, &e->multi_gt_list,
> multi_gt_link)
> @@ -834,3 +835,77 @@ int xe_engine_set_property_ioctl(struct drm_device
> *dev, void *data,
>
> return ret;
> }
> +
> +static void xe_engine_last_fence_lockdep_assert(struct xe_engine *e,
> + struct xe_vm *vm)
> +{
> + lockdep_assert_held_write(&vm->lock);
> +}
Why do we need to pass xe_engine above? Do we intend to use this later?
Pallavi
> +
> +/**
> + * xe_engine_last_fence_put() - Drop ref to last fence
> + * @e: The engine
> + * @vm: The VM the engine does a bind or exec for */ void
> +xe_engine_last_fence_put(struct xe_engine *e, struct xe_vm *vm) {
> + xe_engine_last_fence_lockdep_assert(e, vm);
> +
> + if (e->last_fence) {
> + dma_fence_put(e->last_fence);
> + e->last_fence = NULL;
> + }
> +}
> +
> +/**
> + * xe_engine_last_fence_put_unlocked() - Drop ref to last fence
> +unlocked
> + * @e: The engine
> + *
> + * Only safe to be called from xe_engine_destroy().
> + */
> +void xe_engine_last_fence_put_unlocked(struct xe_engine *e) {
> + if (e->last_fence) {
> + dma_fence_put(e->last_fence);
> + e->last_fence = NULL;
> + }
> +}
> +
> +/**
> + * xe_engine_last_fence_get() - Get last fence
> + * @e: The engine
> + * @vm: The VM the engine does a bind or exec for
> + *
> + * Get last fence, does not take a ref
> + *
> + * Returns: last fence if not signaled, dma fence stub if signaled */
> +struct dma_fence *xe_engine_last_fence_get(struct xe_engine *e,
> + struct xe_vm *vm)
> +{
> + xe_engine_last_fence_lockdep_assert(e, vm);
> +
> + if (e->last_fence &&
> + test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &e->last_fence->flags))
> + xe_engine_last_fence_put(e, vm);
> +
> + return e->last_fence ? e->last_fence : dma_fence_get_stub(); }
> +
> +/**
> + * xe_engine_last_fence_set() - Set last fence
> + * @e: The engine
> + * @vm: The VM the engine does a bind or exec for
> + * @fence: The fence
> + *
> + * Set the last fence for the engine. Increases reference count for
> +fence, when
> + * closing engine xe_engine_last_fence_put should be called.
> + */
> +void xe_engine_last_fence_set(struct xe_engine *e, struct xe_vm *vm,
> + struct dma_fence *fence)
> +{
> + xe_engine_last_fence_lockdep_assert(e, vm);
> +
> + xe_engine_last_fence_put(e, vm);
> + e->last_fence = dma_fence_get(fence);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_engine.h
> b/drivers/gpu/drm/xe/xe_engine.h index 2e60f6d90226..0aae12ed06ef
> 100644
> --- a/drivers/gpu/drm/xe/xe_engine.h
> +++ b/drivers/gpu/drm/xe/xe_engine.h
> @@ -60,4 +60,11 @@ int xe_engine_get_property_ioctl(struct drm_device
> *dev, void *data,
> struct drm_file *file);
> enum drm_sched_priority xe_engine_device_get_max_priority(struct
> xe_device *xe);
>
> +void xe_engine_last_fence_put(struct xe_engine *e, struct xe_vm *vm);
> +void xe_engine_last_fence_put_unlocked(struct xe_engine *e); struct
> +dma_fence *xe_engine_last_fence_get(struct xe_engine *e,
> + struct xe_vm *vm);
> +void xe_engine_last_fence_set(struct xe_engine *e, struct xe_vm *vm,
> + struct dma_fence *fence);
> +
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_engine_types.h
> b/drivers/gpu/drm/xe/xe_engine_types.h
> index 4949edfa0980..9ca19942d72b 100644
> --- a/drivers/gpu/drm/xe/xe_engine_types.h
> +++ b/drivers/gpu/drm/xe/xe_engine_types.h
> @@ -52,6 +52,12 @@ struct xe_engine {
> /** @fence_irq: fence IRQ used to signal job completion */
> struct xe_hw_fence_irq *fence_irq;
>
> + /**
> + * @last_fence: last fence on engine, protected by vm->lock in write
> + * mode if bind engine
> + */
> + struct dma_fence *last_fence;
> +
> #define ENGINE_FLAG_BANNED BIT(0)
> #define ENGINE_FLAG_KERNEL BIT(1)
> #define ENGINE_FLAG_PERSISTENT BIT(2)
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 77723f680f37..3e420b2c12b4 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1366,6 +1366,13 @@ void xe_vm_close_and_put(struct xe_vm *vm)
> if (xe_vm_in_compute_mode(vm))
> flush_work(&vm->preempt.rebind_work);
>
> + down_write(&vm->lock);
> + for_each_tile(tile, xe, id) {
> + if (vm->eng[id])
> + xe_engine_last_fence_put(vm->eng[id], vm);
> + }
> + up_write(&vm->lock);
> +
> for_each_tile(tile, xe, id) {
> if (vm->eng[id]) {
> xe_engine_kill(vm->eng[id]);
> @@ -1518,16 +1525,22 @@ u64 xe_vm_pdp4_descriptor(struct xe_vm *vm,
> struct xe_tile *tile)
> XE_CACHE_WB);
> }
>
> +static struct xe_engine *to_wait_engine(struct xe_vm *vm, struct
> +xe_engine *e) {
> + return e ? e : vm->eng[0];
> +}
> +
> static struct dma_fence *
> xe_vm_unbind_vma(struct xe_vma *vma, struct xe_engine *e,
> struct xe_sync_entry *syncs, u32 num_syncs,
> bool first_op, bool last_op)
> {
> + struct xe_vm *vm = xe_vma_vm(vma);
> + struct xe_engine *wait_engine = to_wait_engine(vm, e);
> struct xe_tile *tile;
> struct dma_fence *fence = NULL;
> struct dma_fence **fences = NULL;
> struct dma_fence_array *cf = NULL;
> - struct xe_vm *vm = xe_vma_vm(vma);
> int cur_fence = 0, i;
> int number_tiles = hweight8(vma->tile_present);
> int err;
> @@ -1579,7 +1592,8 @@ xe_vm_unbind_vma(struct xe_vma *vma, struct
> xe_engine *e,
> cf ? &cf->base : fence);
> }
>
> - return cf ? &cf->base : !fence ? dma_fence_get_stub() : fence;
> + return cf ? &cf->base : !fence ?
> + xe_engine_last_fence_get(wait_engine, vm) : fence;
>
> err_fences:
> if (fences) {
> @@ -1678,6 +1692,7 @@ static int __xe_vm_bind(struct xe_vm *vm, struct
> xe_vma *vma,
> bool last_op)
> {
> struct dma_fence *fence;
> + struct xe_engine *wait_engine = to_wait_engine(vm, e);
>
> xe_vm_assert_held(vm);
>
> @@ -1691,13 +1706,15 @@ static int __xe_vm_bind(struct xe_vm *vm,
> struct xe_vma *vma,
>
> XE_BUG_ON(!xe_vm_in_fault_mode(vm));
>
> - fence = dma_fence_get_stub();
> + fence = xe_engine_last_fence_get(wait_engine, vm);
> if (last_op) {
> for (i = 0; i < num_syncs; i++)
> xe_sync_entry_signal(&syncs[i], NULL, fence);
> }
> }
>
> + if (last_op)
> + xe_engine_last_fence_set(wait_engine, vm, fence);
> if (last_op && xe_vm_sync_mode(vm, e))
> dma_fence_wait(fence, true);
> dma_fence_put(fence);
> @@ -1730,6 +1747,7 @@ static int xe_vm_unbind(struct xe_vm *vm, struct
> xe_vma *vma,
> u32 num_syncs, bool first_op, bool last_op) {
> struct dma_fence *fence;
> + struct xe_engine *wait_engine = to_wait_engine(vm, e);
>
> xe_vm_assert_held(vm);
> xe_bo_assert_held(xe_vma_bo(vma));
> @@ -1739,6 +1757,8 @@ static int xe_vm_unbind(struct xe_vm *vm, struct
> xe_vma *vma,
> return PTR_ERR(fence);
>
> xe_vma_destroy(vma, fence);
> + if (last_op)
> + xe_engine_last_fence_set(wait_engine, vm, fence);
> if (last_op && xe_vm_sync_mode(vm, e))
> dma_fence_wait(fence, true);
> dma_fence_put(fence);
> @@ -1875,6 +1895,7 @@ static int xe_vm_prefetch(struct xe_vm *vm, struct
> xe_vma *vma,
> struct xe_sync_entry *syncs, u32 num_syncs,
> bool first_op, bool last_op)
> {
> + struct xe_engine *wait_engine = to_wait_engine(vm, e);
> int err;
>
> XE_BUG_ON(region > ARRAY_SIZE(region_to_mem_type)); @@ -
> 1893,9 +1914,12 @@ static int xe_vm_prefetch(struct xe_vm *vm, struct
> xe_vma *vma,
>
> /* Nothing to do, signal fences now */
> if (last_op) {
> - for (i = 0; i < num_syncs; i++)
> - xe_sync_entry_signal(&syncs[i], NULL,
> - dma_fence_get_stub());
> + for (i = 0; i < num_syncs; i++) {
> + struct dma_fence *fence =
> +
> xe_engine_last_fence_get(wait_engine, vm);
> +
> + xe_sync_entry_signal(&syncs[i], NULL, fence);
> + }
> }
>
> return 0;
> @@ -3028,8 +3052,12 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file)
> unwind_ops:
> vm_bind_ioctl_ops_unwind(vm, ops, args->num_binds);
> free_syncs:
> - for (i = 0; err == -ENODATA && i < num_syncs; i++)
> - xe_sync_entry_signal(&syncs[i], NULL,
> dma_fence_get_stub());
> + for (i = 0; err == -ENODATA && i < num_syncs; i++) {
> + struct dma_fence *fence =
> + xe_engine_last_fence_get(to_wait_engine(vm, e),
> vm);
> +
> + xe_sync_entry_signal(&syncs[i], NULL, fence);
> + }
> while (num_syncs--)
> xe_sync_entry_cleanup(&syncs[num_syncs]);
>
> --
> 2.34.1
More information about the Intel-xe
mailing list