[PATCH v5 20/23] drm/xe: Reset VMA attributes to default in SVM garbage collector
Matthew Brost
matthew.brost at intel.com
Tue Jul 29 05:27:38 UTC 2025
On Thu, Jul 24, 2025 at 02:50:47PM -0700, Matthew Brost wrote:
> On Tue, Jul 22, 2025 at 07:05:23PM +0530, Himal Prasad Ghimiray wrote:
> > Restore default memory attributes for VMAs during garbage collection
> > if they were modified by madvise. Reuse existing VMA if fully overlapping;
> > otherwise, allocate a new mirror VMA.
> >
> > Suggested-by: Matthew Brost <matthew.brost at intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_svm.c | 34 +++++++++
> > drivers/gpu/drm/xe/xe_vm.c | 140 +++++++++++++++++++++++++-----------
> > drivers/gpu/drm/xe/xe_vm.h | 2 +
> > 3 files changed, 135 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> > index ba1233d0d5a2..79709dc066b9 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.c
> > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > @@ -255,7 +255,18 @@ static int __xe_svm_garbage_collector(struct xe_vm *vm,
> > static int xe_svm_garbage_collector(struct xe_vm *vm)
> > {
> > struct xe_svm_range *range;
> > + struct xe_vma *vma;
> > + u64 range_start;
> > + u64 range_size;
> > + u64 range_end;
> > int err;
> > + struct xe_vma_mem_attr default_attr = {
> > + .preferred_loc = {
> > + .devmem_fd = DRM_XE_PREFERRED_LOC_DEFAULT_DEVICE,
> > + .migration_policy = DRM_XE_MIGRATE_ALL_PAGES,
> > + },
> > + .atomic_access = DRM_XE_ATOMIC_UNDEFINED,
> > + };
> >
> > lockdep_assert_held_write(&vm->lock);
> >
> > @@ -270,6 +281,12 @@ static int xe_svm_garbage_collector(struct xe_vm *vm)
> > if (!range)
> > break;
> >
> > + range_start = xe_svm_range_start(range);
> > + range_size = xe_svm_range_size(range);
> > + range_end = xe_svm_range_end(range);
> > +
> > + vma = xe_vm_find_vma_by_addr(vm, xe_svm_range_start(range));
> > +
>
> I'd find the VMA outside of the svm.garbage_collector.lock.
>
> > list_del(&range->garbage_collector_link);
> > spin_unlock(&vm->svm.garbage_collector.lock);
> >
> > @@ -282,7 +299,24 @@ static int xe_svm_garbage_collector(struct xe_vm *vm)
> > return err;
> > }
> >
> > + if (!xe_vma_has_default_mem_attrs(vma)) {
>
> It seems possible the VMA could be NULL in error cases. I'd check for
> NULL and error out.
>
> Also could this code be moved to a helper? Internal SVM seems ok, in
> that case xe_vm_find_vma_by_addr could also be in the helper.
>
> > + vm_dbg(&vm->xe->drm, "Existing VMA start=0x%016llx, vma_end=0x%016llx",
> > + xe_vma_start(vma), xe_vma_end(vma));
> > +
> > + if (xe_vma_start(vma) == range_start && xe_vma_end(vma) == range_end) {
> > + default_attr.pat_index = vma->attr.default_pat_index;
> > + default_attr.default_pat_index = vma->attr.default_pat_index;
> > + vma->attr = default_attr;
> > + } else {
> > + vm_dbg(&vm->xe->drm, "Split VMA start=0x%016llx, vma_end=0x%016llx",
> > + range_start, range_end);
> > + err = xe_vm_alloc_cpu_addr_mirror_vma(vm, range_start, range_size);
> > + if (err)
>
> On error, I'd print a message and kill the VM as it shouldn't be
> possible to fail aside from a memory allocation failure and we can't
> code with errors given this can be inside a worker.
>
> I'll circle back to the rest of the patch a bit later.
>
I think the rest of the patch makes sense.
Matt
> Matt
>
> > + return err;
> > + }
> > + }
> > spin_lock(&vm->svm.garbage_collector.lock);
> > +
> > }
> > spin_unlock(&vm->svm.garbage_collector.lock);
> >
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index d3f08bf9a3ee..003c8209f8bd 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -4254,34 +4254,24 @@ bool xe_vma_need_vram_for_atomic(struct xe_device *xe, struct xe_vma *vma, bool
> > }
> > }
> >
> > -/**
> > - * xe_vm_alloc_madvise_vma - Allocate VMA's with madvise ops
> > - * @vm: Pointer to the xe_vm structure
> > - * @start: Starting input address
> > - * @range: Size of the input range
> > - *
> > - * This function splits existing vma to create new vma for user provided input range
> > - *
> > - * Return: 0 if success
> > - */
> > -int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > +static int xe_vm_alloc_vma(struct xe_vm *vm,
> > + u64 start, u64 range,
> > + enum drm_gpuvm_sm_map_ops_flags flags)
> > {
> > struct xe_vma_ops vops;
> > struct drm_gpuva_ops *ops = NULL;
> > struct drm_gpuva_op *__op;
> > bool is_cpu_addr_mirror = false;
> > bool remap_op = false;
> > + bool is_madvise = flags == DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE;
> > struct xe_vma_mem_attr tmp_attr;
> > + u16 default_pat;
> > int err;
> >
> > - vm_dbg(&vm->xe->drm, "MADVISE IN: addr=0x%016llx, size=0x%016llx", start, range);
> > -
> > lockdep_assert_held_write(&vm->lock);
> >
> > - vm_dbg(&vm->xe->drm, "MADVISE_OPS_CREATE: addr=0x%016llx, size=0x%016llx", start, range);
> > ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, start, range,
> > - DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE,
> > - NULL, start);
> > + flags, NULL, start);
> > if (IS_ERR(ops))
> > return PTR_ERR(ops);
> >
> > @@ -4292,33 +4282,56 @@ int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> >
> > drm_gpuva_for_each_op(__op, ops) {
> > struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> > + struct xe_vma *vma = NULL;
> >
> > - if (__op->op == DRM_GPUVA_OP_REMAP) {
> > - xe_assert(vm->xe, !remap_op);
> > - remap_op = true;
> > + if (!is_madvise) {
> > + if (__op->op == DRM_GPUVA_OP_UNMAP) {
> > + vma = gpuva_to_vma(op->base.unmap.va);
> > + XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
> > + default_pat = vma->attr.default_pat_index;
> > + }
> >
> > - if (xe_vma_is_cpu_addr_mirror(gpuva_to_vma(op->base.remap.unmap->va)))
> > - is_cpu_addr_mirror = true;
> > - else
> > - is_cpu_addr_mirror = false;
> > - }
> > + if (__op->op == DRM_GPUVA_OP_REMAP) {
> > + vma = gpuva_to_vma(op->base.remap.unmap->va);
> > + default_pat = vma->attr.default_pat_index;
> > + }
> >
> > - if (__op->op == DRM_GPUVA_OP_MAP) {
> > - xe_assert(vm->xe, remap_op);
> > - remap_op = false;
> > + if (__op->op == DRM_GPUVA_OP_MAP) {
> > + op->map.is_cpu_addr_mirror = true;
> > + op->map.pat_index = default_pat;
> > + }
> > + } else {
> > + if (__op->op == DRM_GPUVA_OP_REMAP) {
> > + vma = gpuva_to_vma(op->base.remap.unmap->va);
> > + xe_assert(vm->xe, !remap_op);
> > + remap_op = true;
> >
> > - /* In case of madvise ops DRM_GPUVA_OP_MAP is always after
> > - * DRM_GPUVA_OP_REMAP, so ensure we assign op->map.is_cpu_addr_mirror true
> > - * if REMAP is for xe_vma_is_cpu_addr_mirror vma
> > - */
> > - op->map.is_cpu_addr_mirror = is_cpu_addr_mirror;
> > - }
> > + if (xe_vma_is_cpu_addr_mirror(vma))
> > + is_cpu_addr_mirror = true;
> > + else
> > + is_cpu_addr_mirror = false;
> > + }
> >
> > + if (__op->op == DRM_GPUVA_OP_MAP) {
> > + xe_assert(vm->xe, remap_op);
> > + remap_op = false;
> > + /*
> > + * In case of madvise ops DRM_GPUVA_OP_MAP is
> > + * always after DRM_GPUVA_OP_REMAP, so ensure
> > + * we assign op->map.is_cpu_addr_mirror true
> > + * if REMAP is for xe_vma_is_cpu_addr_mirror vma
> > + */
> > + op->map.is_cpu_addr_mirror = is_cpu_addr_mirror;
> > + }
> > + }
> > print_op(vm->xe, __op);
> > }
> >
> > xe_vma_ops_init(&vops, vm, NULL, NULL, 0);
> > - vops.flags |= XE_VMA_OPS_FLAG_MADVISE;
> > +
> > + if (is_madvise)
> > + vops.flags |= XE_VMA_OPS_FLAG_MADVISE;
> > +
> > err = vm_bind_ioctl_ops_parse(vm, ops, &vops);
> > if (err)
> > goto unwind_ops;
> > @@ -4330,15 +4343,20 @@ int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > struct xe_vma *vma;
> >
> > if (__op->op == DRM_GPUVA_OP_UNMAP) {
> > - /* There should be no unmap */
> > - XE_WARN_ON("UNEXPECTED UNMAP");
> > - xe_vma_destroy(gpuva_to_vma(op->base.unmap.va), NULL);
> > + vma = gpuva_to_vma(op->base.unmap.va);
> > + /* There should be no unmap for madvise */
> > + if (is_madvise)
> > + XE_WARN_ON("UNEXPECTED UNMAP");
> > +
> > + xe_vma_destroy(vma, NULL);
> > } else if (__op->op == DRM_GPUVA_OP_REMAP) {
> > vma = gpuva_to_vma(op->base.remap.unmap->va);
> > - /* Store attributes for REMAP UNMAPPED VMA, so they can be assigned
> > - * to newly MAP created vma.
> > + /* In case of madvise ops Store attributes for REMAP UNMAPPED
> > + * VMA, so they can be assigned to newly MAP created vma.
> > */
> > - tmp_attr = vma->attr;
> > + if (is_madvise)
> > + tmp_attr = vma->attr;
> > +
> > xe_vma_destroy(gpuva_to_vma(op->base.remap.unmap->va), NULL);
> > } else if (__op->op == DRM_GPUVA_OP_MAP) {
> > vma = op->map.vma;
> > @@ -4346,7 +4364,8 @@ int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > * Therefore temp_attr will always have sane values, making it safe to
> > * copy them to new vma.
> > */
> > - vma->attr = tmp_attr;
> > + if (is_madvise)
> > + vma->attr = tmp_attr;
> > }
> > }
> >
> > @@ -4360,3 +4379,42 @@ int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > drm_gpuva_ops_free(&vm->gpuvm, ops);
> > return err;
> > }
> > +
> > +/**
> > + * xe_vm_alloc_madvise_vma - Allocate VMA's with madvise ops
> > + * @vm: Pointer to the xe_vm structure
> > + * @start: Starting input address
> > + * @range: Size of the input range
> > + *
> > + * This function splits existing vma to create new vma for user provided input range
> > + *
> > + * Return: 0 if success
> > + */
> > +int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > +{
> > + lockdep_assert_held_write(&vm->lock);
> > +
> > + vm_dbg(&vm->xe->drm, "MADVISE_OPS_CREATE: addr=0x%016llx, size=0x%016llx", start, range);
> > +
> > + return xe_vm_alloc_vma(vm, start, range, DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE);
> > +}
> > +
> > +/**
> > + * xe_vm_alloc_cpu_addr_mirror_vma - Allocate CPU addr mirror vma
> > + * @vm: Pointer to the xe_vm structure
> > + * @start: Starting input address
> > + * @range: Size of the input range
> > + *
> > + * This function splits/merges existing vma to create new vma for user provided input range
> > + *
> > + * Return: 0 if success
> > + */
> > +int xe_vm_alloc_cpu_addr_mirror_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > +{
> > + lockdep_assert_held_write(&vm->lock);
> > +
> > + vm_dbg(&vm->xe->drm, "CPU_ADDR_MIRROR_VMA_OPS_CREATE: addr=0x%016llx, size=0x%016llx",
> > + start, range);
> > +
> > + return xe_vm_alloc_vma(vm, start, range, DRM_GPUVM_SM_MAP_NOT_MADVISE);
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> > index a4db843de540..f7b9ad83685a 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.h
> > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > @@ -177,6 +177,8 @@ bool xe_vma_need_vram_for_atomic(struct xe_device *xe, struct xe_vma *vma, bool
> >
> > int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t addr, uint64_t size);
> >
> > +int xe_vm_alloc_cpu_addr_mirror_vma(struct xe_vm *vm, uint64_t addr, uint64_t size);
> > +
> > /**
> > * to_userptr_vma() - Return a pointer to an embedding userptr vma
> > * @vma: Pointer to the embedded struct xe_vma
> > --
> > 2.34.1
> >
More information about the Intel-xe
mailing list