On Tue, 2024-10-15 at 20:25 -0700, Matthew Brost wrote:
> Add unbind to SVM garbage collector. To facilitate add unbind support
> function to VM layer which unbinds a SVM range. Also teach PY layer
> to
> understand unbinds of SVM ranges.
>
> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> ---
> drivers/gpu/drm/xe/xe_pt.c | 84 ++++++++++++++++++++++++++----
> --
> drivers/gpu/drm/xe/xe_svm.c | 9 +++-
> drivers/gpu/drm/xe/xe_vm.c | 73 +++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_vm.h | 2 +
> drivers/gpu/drm/xe/xe_vm_types.h | 12 ++++-
> 5 files changed, 162 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 024e4eb83408..687abd1a5e74 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -925,10 +925,16 @@ static void xe_pt_cancel_bind(struct xe_vma
> *vma,
> }
> }
>
> +#define INVALID_VMA (struct xe_vma*)(0xdeaddeadull)
Please prefix with XE_ to avoid future name clashes.
> +
> static void xe_pt_commit_locks_assert(struct xe_vma *vma)
> {
> - struct xe_vm *vm = xe_vma_vm(vma);
> + struct xe_vm *vm;
>
> + if (vma == INVALID_VMA)
> + return;
> +
> + vm = xe_vma_vm(vma);
> lockdep_assert_held(&vm->lock);
>
> if (!xe_vma_has_no_bo(vma))
> @@ -954,7 +960,8 @@ static void xe_pt_commit(struct xe_vma *vma,
> for (j = 0; j < entries[i].qwords; j++) {
> struct xe_pt *oldpte =
> entries[i].pt_entries[j].pt;
>
> - xe_pt_destroy(oldpte, xe_vma_vm(vma)->flags,
> deferred);
> + xe_pt_destroy(oldpte, (vma == INVALID_VMA) ?
> 0 :
> + xe_vma_vm(vma)->flags,
> deferred);
> }
> }
> }
> @@ -1387,6 +1394,9 @@ static int xe_pt_svm_pre_commit(struct
> xe_migrate_pt_update *pt_update)
> list_for_each_entry(op, &vops->list, link) {
> struct xe_svm_range *range = op->map_range.range;
>
> + if (op->subop == XE_VMA_SUBOP_UNMAP_RANGE)
> + continue;
> +
> xe_assert(vm->xe, xe_vma_is_system_allocator(op-
> >map_range.vma));
> xe_assert(vm->xe, op->subop ==
> XE_VMA_SUBOP_MAP_RANGE);
>
> @@ -1585,7 +1595,9 @@ static const struct xe_pt_walk_ops
> xe_pt_stage_unbind_ops = {
> * xe_pt_stage_unbind() - Build page-table update structures for an
> unbind
> * operation
> * @tile: The tile we're unbinding for.
> + * @vm: The vm
> * @vma: The vma we're unbinding.
> + * @range: The range we're unbinding.
> * @entries: Caller-provided storage for the update structures.
> *
> * Builds page-table update structures for an unbind operation. The
> function
> @@ -1595,9 +1607,14 @@ static const struct xe_pt_walk_ops
> xe_pt_stage_unbind_ops = {
> *
> * Return: The number of entries used.
> */
> -static unsigned int xe_pt_stage_unbind(struct xe_tile *tile, struct
> xe_vma *vma,
> +static unsigned int xe_pt_stage_unbind(struct xe_tile *tile,
> + struct xe_vm *vm,
> + struct xe_vma *vma,
> + struct xe_svm_range *range,
> struct xe_vm_pgtable_update
> *entries)
> {
> + u64 start = range ? range->base.va.start :
> xe_vma_start(vma);
> + u64 end = range ? range->base.va.end : xe_vma_end(vma);
> struct xe_pt_stage_unbind_walk xe_walk = {
> .base = {
> .ops = &xe_pt_stage_unbind_ops,
> @@ -1605,14 +1622,14 @@ static unsigned int xe_pt_stage_unbind(struct
> xe_tile *tile, struct xe_vma *vma,
> .max_level = XE_PT_HIGHEST_LEVEL,
> },
> .tile = tile,
> - .modified_start = xe_vma_start(vma),
> - .modified_end = xe_vma_end(vma),
> + .modified_start = start,
> + .modified_end = end,
> .wupd.entries = entries,
> };
> - struct xe_pt *pt = xe_vma_vm(vma)->pt_root[tile->id];
> + struct xe_pt *pt = vm->pt_root[tile->id];
>
> - (void)xe_pt_walk_shared(&pt->base, pt->level,
> xe_vma_start(vma),
> - xe_vma_end(vma), &xe_walk.base);
> + (void)xe_pt_walk_shared(&pt->base, pt->level, start, end,
> + &xe_walk.base);
>
> return xe_walk.wupd.num_used_entries;
> }
> @@ -1854,13 +1871,6 @@ static int unbind_op_prepare(struct xe_tile
> *tile,
> "Preparing unbind, with range [%llx...%llx)\n",
> xe_vma_start(vma), xe_vma_end(vma) - 1);
>
> - /*
> - * Wait for invalidation to complete. Can corrupt internal
> page table
> - * state if an invalidation is running while preparing an
> unbind.
> - */
> - if (xe_vma_is_userptr(vma) &&
> xe_vm_in_fault_mode(xe_vma_vm(vma)))
> - mmu_interval_read_begin(&to_userptr_vma(vma)-
> >userptr.notifier);
> -
> pt_op->vma = vma;
> pt_op->bind = false;
> pt_op->rebind = false;
> @@ -1869,7 +1879,8 @@ static int unbind_op_prepare(struct xe_tile
> *tile,
> if (err)
> return err;
>
> - pt_op->num_entries = xe_pt_stage_unbind(tile, vma, pt_op-
> >entries);
> + pt_op->num_entries = xe_pt_stage_unbind(tile,
> xe_vma_vm(vma),
> + vma, NULL, pt_op-
> >entries);
>
> xe_vm_dbg_print_entries(tile_to_xe(tile), pt_op->entries,
> pt_op->num_entries, false);
> @@ -1884,6 +1895,42 @@ static int unbind_op_prepare(struct xe_tile
> *tile,
> return 0;
> }
>
> +static int unbind_range_prepare(struct xe_vm *vm,
> + struct xe_tile *tile,
> + struct xe_vm_pgtable_update_ops
> *pt_update_ops,
> + struct xe_svm_range *range)
> +{
> + u32 current_op = pt_update_ops->current_op;
> + struct xe_vm_pgtable_update_op *pt_op = &pt_update_ops-
> >ops[current_op];
> +
> + if (!(range->tile_present & BIT(tile->id)))
> + return 0;
> +
> + vm_dbg(&vm->xe->drm,
> + "Preparing unbind, with range [%llx...%llx)\n",
> + range->base.va.start, range->base.va.end - 1);
> +
> + pt_op->vma = INVALID_VMA;
> + pt_op->bind = false;
> + pt_op->rebind = false;
> +
> + pt_op->num_entries = xe_pt_stage_unbind(tile, vm, NULL,
> range,
> + pt_op->entries);
> +
> + xe_vm_dbg_print_entries(tile_to_xe(tile), pt_op->entries,
> + pt_op->num_entries, false);
> + xe_pt_update_ops_rfence_interval(pt_update_ops, range-
> >base.va.start,
> + range->base.va.end);
> + ++pt_update_ops->current_op;
> + pt_update_ops->needs_svm_lock = true;
> + pt_update_ops->needs_invalidation = true;
> +
> + xe_pt_commit_prepare_unbind(INVALID_VMA, pt_op->entries,
> + pt_op->num_entries);
> +
> + return 0;
> +}
> +
> static int op_prepare(struct xe_vm *vm,
> struct xe_tile *tile,
> struct xe_vm_pgtable_update_ops
> *pt_update_ops,
> @@ -1951,6 +1998,9 @@ static int op_prepare(struct xe_vm *vm,
> err = bind_range_prepare(vm, tile,
> pt_update_ops,
> op->map_range.vma,
> op-
> >map_range.range);
> + } else if (op->subop == XE_VMA_SUBOP_UNMAP_RANGE) {
> + err = unbind_range_prepare(vm, tile,
> pt_update_ops,
> + op-
> >unmap_range.range);
> }
> break;
> default:
> @@ -2139,6 +2189,8 @@ static void op_commit(struct xe_vm *vm,
> if (op->subop == XE_VMA_SUBOP_MAP_RANGE) {
> op->map_range.range->tile_present |=
> BIT(tile->id);
> op->map_range.range->tile_invalidated &=
> ~BIT(tile->id);
> + } else if (op->subop == XE_VMA_SUBOP_UNMAP_RANGE) {
> + op->unmap_range.range->tile_present &=
> ~BIT(tile->id);
> }
> break;
> }
I think this further stresses the need to provide a pt code interface
that is oblivious of vmas, userptr and ranges so that we can get rid of
all special-casing, but for now code looks good as is IMO.
> diff --git a/drivers/gpu/drm/xe/xe_svm.c
> b/drivers/gpu/drm/xe/xe_svm.c
> index 9c2f44cba166..0762126f65e0 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
> @@ -208,7 +208,14 @@ static void xe_svm_invalidate(struct drm_gpusvm
> *gpusvm,
> static int __xe_svm_garbage_collector(struct xe_vm *vm,
> struct xe_svm_range *range)
> {
> - /* TODO: Do unbind */
> + struct dma_fence *fence;
> +
> + xe_vm_lock(vm, false);
> + fence = xe_vm_range_unbind(vm, range);
> + xe_vm_unlock(vm);
> + if (IS_ERR(fence))
> + return PTR_ERR(fence);
> + dma_fence_put(fence);
>
> drm_gpusvm_range_remove(&vm->svm.gpusvm, &range->base);
>
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 399cbbdbddd5..76a20e96084e 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -972,6 +972,79 @@ struct dma_fence *xe_vm_range_rebind(struct
> xe_vm *vm,
> return fence;
> }
>
> +static void xe_vm_populate_range_unbind(struct xe_vma_op *op,
> + struct xe_svm_range *range)
> +{
> + INIT_LIST_HEAD(&op->link);
> + op->tile_mask = range->tile_present;
> + op->base.op = DRM_GPUVA_OP_USER;
> + op->subop = XE_VMA_SUBOP_UNMAP_RANGE;
> + op->unmap_range.range = range;
> +}
> +
> +static int
> +xe_vm_ops_add_range_unbind(struct xe_vma_ops *vops,
> + struct xe_svm_range *range)
> +{
> + struct xe_vma_op *op;
> +
> + op = kzalloc(sizeof(*op), GFP_KERNEL);
> + if (!op)
> + return -ENOMEM;
> +
> + xe_vm_populate_range_unbind(op, range);
> + list_add_tail(&op->link, &vops->list);
> + xe_vma_ops_incr_pt_update_ops(vops, range->tile_present);
> +
> + return 0;
> +}
> +
> +struct dma_fence *xe_vm_range_unbind(struct xe_vm *vm,
> + struct xe_svm_range *range)
Kerneldoc.
> +{
> + struct dma_fence *fence = NULL;
> + struct xe_vma_ops vops;
> + struct xe_vma_op *op, *next_op;
> + struct xe_tile *tile;
> + u8 id;
> + int err;
> +
> + lockdep_assert_held(&vm->lock);
> + xe_vm_assert_held(vm);
> + xe_assert(vm->xe, xe_vm_in_fault_mode(vm));
> +
> + if (!range->tile_present)
> + return dma_fence_get_stub();
> +
> + xe_vma_ops_init(&vops, vm, NULL, NULL, 0);
> + for_each_tile(tile, vm->xe, id) {
> + vops.pt_update_ops[id].wait_vm_bookkeep = true;
> + vops.pt_update_ops[tile->id].q =
> + xe_tile_migrate_exec_queue(tile);
> + }
> +
> + err = xe_vm_ops_add_range_unbind(&vops, range);
> + if (err)
> + return ERR_PTR(err);
> +
> + err = xe_vma_ops_alloc(&vops, false);
> + if (err) {
> + fence = ERR_PTR(err);
> + goto free_ops;
> + }
> +
> + fence = ops_execute(vm, &vops);
> +
> +free_ops:
> + list_for_each_entry_safe(op, next_op, &vops.list, link) {
> + list_del(&op->link);
> + kfree(op);
> + }
> + xe_vma_ops_fini(&vops);
> +
> + return fence;
> +}
> +
> static void xe_vma_free(struct xe_vma *vma)
> {
> if (xe_vma_is_userptr(vma))
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 8bd921b33090..d577ca9e3d65 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -222,6 +222,8 @@ struct dma_fence *xe_vm_range_rebind(struct xe_vm
> *vm,
> struct xe_vma *vma,
> struct xe_svm_range *range,
> u8 tile_mask);
> +struct dma_fence *xe_vm_range_unbind(struct xe_vm *vm,
> + struct xe_svm_range *range);
>
> int xe_vm_invalidate_vma(struct xe_vma *vma);
>
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> b/drivers/gpu/drm/xe/xe_vm_types.h
> index 2eae3575c409..d38cf7558f62 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -348,6 +348,12 @@ struct xe_vma_op_map_range {
> struct xe_svm_range *range;
> };
>
> +/** struct xe_vma_op_unmap_range - VMA unmap range operation */
> +struct xe_vma_op_unmap_range {
> + /** @range: SVM range to unmap */
> + struct xe_svm_range *range;
> +};
> +
> /** enum xe_vma_op_flags - flags for VMA operation */
> enum xe_vma_op_flags {
> /** @XE_VMA_OP_COMMITTED: VMA operation committed */
> @@ -362,6 +368,8 @@ enum xe_vma_op_flags {
> enum xe_vma_subop {
> /** @XE_VMA_SUBOP_MAP_RANGE: Map range */
> XE_VMA_SUBOP_MAP_RANGE,
> + /** @XE_VMA_SUBOP_UNMAP_RANGE: Unmap range */
> + XE_VMA_SUBOP_UNMAP_RANGE,
> };
>
> /** struct xe_vma_op - VMA operation */
> @@ -384,8 +392,10 @@ struct xe_vma_op {
> struct xe_vma_op_remap remap;
> /** @prefetch: VMA prefetch operation specific data
> */
> struct xe_vma_op_prefetch prefetch;
> - /** @map: VMA map range operation specific data */
> + /** @map_range: VMA map range operation specific
> data */
> struct xe_vma_op_map_range map_range;
> + /** @unmap_range: VMA unmap range operation specific
> data */
> + struct xe_vma_op_map_range unmap_range;
> };
> };
>
Thanks,
Thomas
