Re: [PATCH v4 4/5] drm/xe/svm: Enable UNMAP for VMA merging operations

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Nov 24, 2025 at 11:31:07AM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 23-11-2025 03:31, Matthew Brost wrote:
> > On Mon, Nov 03, 2025 at 11:39:56AM +0530, Himal Prasad Ghimiray wrote:
> > > ALLOW UNMAP of VMAs associated with SVM mappings when the MAP operation
> > > is intended to merge adjacent CPU_ADDR_MIRROR VMAs.
> > > 
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_vm.c       | 10 +++++++---
> > >   drivers/gpu/drm/xe/xe_vm_types.h |  3 ++-
> > >   2 files changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index d78eb95a954f..12354e03c3e2 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -2244,8 +2244,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> > >   	switch (operation) {
> > >   	case DRM_XE_VM_BIND_OP_MAP:
> > > -		if (flags & DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR)
> > > +		if (flags & DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR) {
> > >   			xe_vm_find_cpu_addr_mirror_vma_range(vm, &range_start, &range_end);
> > > +			vops->flags |= XE_VMA_OPS_FLAG_ALLOW_SVM_UNMAP;
> > > +		}
> > >   		fallthrough;
> > >   	case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> > > @@ -2727,7 +2729,8 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
> > >   			if (xe_vma_is_cpu_addr_mirror(vma) &&
> > >   			    xe_svm_has_mapping(vm, xe_vma_start(vma),
> > > -					       xe_vma_end(vma)))
> > > +					       xe_vma_end(vma)) &&
> > > +			    !(vops->flags & XE_VMA_OPS_FLAG_ALLOW_SVM_UNMAP))
> > 
> > Do we need similar change to the DRM_GPUVA_OP_REMAP case statement which
> > has similar logic?
> 
> 
> As far as I can tell, REMAP is only possible within the
> svm_garbage_collector. In that function, we perform the REMAP operation
> after the range has been unmapped. Therefore, the REMAP should proceed
> safely without this check.
> 
> 

I believe this is right now that I think about this.

> > 
> > >   				return -EBUSY;
> > >   			if (!xe_vma_is_cpu_addr_mirror(vma))
> > > @@ -4295,6 +4298,8 @@ static int xe_vm_alloc_vma(struct xe_vm *vm,
> > >   	if (is_madvise)
> > >   		vops.flags |= XE_VMA_OPS_FLAG_MADVISE;
> > > +	else
> > > +		vops.flags |= XE_VMA_OPS_FLAG_ALLOW_SVM_UNMAP;
> > >   	err = vm_bind_ioctl_ops_parse(vm, ops, &vops);
> > >   	if (err)
> > > @@ -4371,7 +4376,6 @@ int xe_vm_alloc_madvise_vma(struct xe_vm *vm, uint64_t start, uint64_t range)
> > >   static bool is_cpu_addr_vma_with_default_attr(struct xe_vma *vma)
> > >   {
> > >   	return vma && xe_vma_is_cpu_addr_mirror(vma) &&
> > > -	       !xe_svm_has_mapping(xe_vma_vm(vma), xe_vma_start(vma), xe_vma_end(vma)) &&
> > 
> > Ah, ok this answers my question in patch #1 [1]. But then in this patch
> > should we also remove the similar check in patch #2 [2] in which I asked
> > the same question?
> 
> Thanks for pointing this. Will address in next version.
> 

+1

Matt

> > 
> > Matt
> > 
> > [1] https://patchwork.freedesktop.org/patch/685118/?series=154832&rev=4#comment_1267065
> > [2] https://patchwork.freedesktop.org/patch/685119/?series=154832&rev=4
> > 
> > >   	       xe_vma_has_default_mem_attrs(vma);
> > >   }
> > > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> > > index 830ed7b05c27..7578ccada2cf 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > > @@ -470,7 +470,8 @@ struct xe_vma_ops {
> > >   	/** @flag: signify the properties within xe_vma_ops*/
> > >   #define XE_VMA_OPS_FLAG_HAS_SVM_PREFETCH BIT(0)
> > >   #define XE_VMA_OPS_FLAG_MADVISE          BIT(1)
> > > -#define XE_VMA_OPS_ARRAY_OF_BINDS	 BIT(2)
> > > +#define XE_VMA_OPS_FLAG_ALLOW_SVM_UNMAP  BIT(2)
> > > +#define XE_VMA_OPS_ARRAY_OF_BINDS        BIT(3)
> > >   	u32 flags;
> > >   #ifdef TEST_VM_OPS_ERROR
> > >   	/** @inject_error: inject error to test error handling */
> > > -- 
> > > 2.34.1
> > > 
>