Re: [PATCH v2 3/3] drm/xe: implement VM_BIND decompression in vm_bind_ioctl

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

On Mon, Oct 13, 2025 at 09:10:18AM -0700, Matthew Brost wrote:
> On Mon, Oct 13, 2025 at 08:18:50PM +0530, Nitin Gote wrote:
> > Implement handling of VM_BIND(..., DECOMPRESS) in xe_vm_bind_ioctl.
> > 
> > Key changes:
> > - Parse and record per-op intent (op->map.request_decompress) when the
> >   DECOMPRESS flag is present.
> > - Validate DECOMPRESS preconditions in the ioctl path:
> >   - Only valid for MAP ops.
> >   - The provided pat_index must select the device's "no-compression" PAT.
> >   - Only meaningful for VRAM-backed BOs on devices with flat CCS and the
> >     required XE2+ hardware (reject with -EOPNOTSUPP otherwise).
> >   - Use XE_IOCTL_DBG for uAPI sanity checks.
> > - Implement xe_bo_schedule_decompress():
> >   - Locate and invalidate any overlapping VMA.
> >   - Schedule an in-place resolve via the migrate/resolve path.
> >   - Install the resulting dma_fence into the BO's kernel reservation
> >     (DMA_RESV_USAGE_KERNEL).
> > - Wire scheduling into vma_lock_and_validate() so VM_BIND will schedule
> >   decompression when request_decompress is set.
> > - Handle fault-mode VMs by performing decompression synchronously during
> >   the bind process, ensuring that the resolve is completed before the bind
> >   finishes.
> > 
> > This schedules an in-place GPU resolve (xe_migrate_resolve) for
> > decompression.
> > 
> > v2:
> >    - Move decompression work out of vm_bind ioctl. (Matt)
> >    - Put that work in a small helper at the BO/migrate layer invoke it
> >      from vma_lock_and_validate which already runs under drm_exec.
> >    - Move lightweight checks to vm_bind_ioctl_check_args (Matthew Auld)
> > 
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_bo.c       | 53 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_bo.h       |  3 ++
> >  drivers/gpu/drm/xe/xe_vm.c       | 48 ++++++++++++++++++++---------
> >  drivers/gpu/drm/xe/xe_vm_types.h |  2 ++
> >  4 files changed, 92 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index 7b6502081873..e9b28c8462c6 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -3307,6 +3307,59 @@ int xe_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * xe_bo_schedule_decompress - schedule in-place decompress and install fence
> > + * @bo:   buffer object (caller should hold drm_exec reservations for VM+BO)
> > + * @vm:   VM containing the VMA range
> > + * @addr: start GPU virtual address of the range
> > + * @range:length in bytes of the range
> > + *
> > + * Schedules an in-place resolve via the migrate layer and installs the
> > + * returned dma_fence into the BO kernel reservation slot (DMA_RESV_USAGE_KERNEL).
> > + * Returns 0 on success, negative errno on error.
> > + */
> > +int xe_bo_schedule_decompress(struct xe_bo *bo, struct xe_vm *vm,
> 
> s/xe_bo_schedule_decompress/xe_bo_decompress
> 
> > +			      u64 addr, u64 range)
> 
> The caller has the VMA, just pass it in rather than looking it up here
> but I don't even this the VMA is needed - more on that below.
> 
> > +{
> > +	struct xe_tile *tile = xe_device_get_root_tile(vm->xe);
> > +	struct xe_vma *existing_vma = NULL;
> > +	struct dma_fence *decomp_fence = NULL;
> > +	int err = 0;
> > +
> > +	/* Validate buffer is VRAM-backed  */
> > +	if (!bo->ttm.resource || !mem_type_is_vram(bo->ttm.resource->mem_type)) {
> > +		drm_err(&vm->xe->drm, "Decompression requires VRAM buffer\n");
> > +		return -EINVAL;
> > +	}
> 
> I think we should just slightly skip the decrompress step if the BO

Typo. s/slightly/silently 

By silently I mean just return 0.

Matt

> isn't in VRAM. It is possible we have evicted the BO system memory
> already behind the user and decrompression isn't required then.
> 
> > +
> > +	/* Find overlapping VMA */
> > +	existing_vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > +	if (existing_vma) {
> 
> See above a lookup isn't required as the caller has the VMA already. 
> 
> > +		drm_dbg(&vm->xe->drm,
> > +			"Found overlapping VMA - automatic invalidation will occur\n");
> > +
> > +		/* Invalidate the VMA */
> > +		err = xe_vm_invalidate_vma(existing_vma);
> 
> xe_vm_invalidate_vma is only valid on faulting VM. I think you want to
> call xe_bo_move_notify here as that will do all the correct things wrt
> to invalidtaions.
> 
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	/* Schedule the in-place decompression */
> > +	decomp_fence = xe_migrate_resolve(tile->migrate,
> > +					  bo, bo,
> > +					  bo->ttm.resource, bo->ttm.resource,
> > +					  false);
> > +
> > +	if (IS_ERR(decomp_fence))
> > +		return PTR_ERR(decomp_fence);
> > +
> > +	/* Install kernel-usage fence */
> > +	dma_resv_add_fence(bo->ttm.base.resv, decomp_fence, DMA_RESV_USAGE_KERNEL);
> 
> I believe you need to call dma_resv_reserve_fences before
> xe_migrate_resolve to ensure dma_resv_add_fence will succeed.
> 
> > +	dma_fence_put(decomp_fence);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * xe_bo_lock() - Lock the buffer object's dma_resv object
> >   * @bo: The struct xe_bo whose lock is to be taken
> > diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> > index 353d607d301d..b5dd8630cde5 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.h
> > +++ b/drivers/gpu/drm/xe/xe_bo.h
> > @@ -308,6 +308,9 @@ int xe_bo_dumb_create(struct drm_file *file_priv,
> >  
> >  bool xe_bo_needs_ccs_pages(struct xe_bo *bo);
> >  
> > +int xe_bo_schedule_decompress(struct xe_bo *bo, struct xe_vm *vm,
> > +			      u64 addr, u64 range);
> > +
> >  static inline size_t xe_bo_ccs_pages_start(struct xe_bo *bo)
> >  {
> >  	return PAGE_ALIGN(xe_bo_size(bo));
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 179758ca7cb8..32bc6d899c36 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -2304,6 +2304,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> >  			op->map.is_cpu_addr_mirror = flags &
> >  				DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR;
> >  			op->map.dumpable = flags & DRM_XE_VM_BIND_FLAG_DUMPABLE;
> > +			op->map.request_decompress = flags & DRM_XE_VM_BIND_FLAG_DECOMPRESS;
> >  			op->map.pat_index = pat_index;
> >  			op->map.invalidate_on_bind =
> >  				__xe_vm_needs_clear_scratch_pages(vm, flags);
> > @@ -2858,7 +2859,7 @@ static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
> >  }
> >  
> >  static int vma_lock_and_validate(struct drm_exec *exec, struct xe_vma *vma,
> > -				 bool res_evict, bool validate)
> > +				 bool res_evict, bool validate, bool request_decompress)
> 
> We have to many bool here but can clean this up in a follow up.
> 
> >  {
> >  	struct xe_bo *bo = xe_vma_bo(vma);
> >  	struct xe_vm *vm = xe_vma_vm(vma);
> > @@ -2871,6 +2872,17 @@ static int vma_lock_and_validate(struct drm_exec *exec, struct xe_vma *vma,
> >  			err = xe_bo_validate(bo, vm,
> >  					     !xe_vm_in_preempt_fence_mode(vm) &&
> >  					     res_evict, exec);
> > +
> > +		if (err)
> > +			return err;
> > +
> > +		if (request_decompress) {
> > +			int ret = xe_bo_schedule_decompress(bo, vm,
> > +							    xe_vma_start(vma),
> > +							    xe_vma_size(vma));
> 
> As stated above, start / size are not needed.
> 
> > +			if (ret)
> > +				return ret;
> > +		}
> 
> 
> I'd write this like:
> 
> if (request_decompress)
> 	err = xe_bo_schedule_decompress(...))
> 
> >  	}
> >  
> >  	return err;
> > @@ -2958,7 +2970,8 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
> >  			err = vma_lock_and_validate(exec, op->map.vma,
> >  						    res_evict,
> >  						    !xe_vm_in_fault_mode(vm) ||
> > -						    op->map.immediate);
> > +						    op->map.immediate,
> > +						    op->map.request_decompress);
> >  		break;
> >  	case DRM_GPUVA_OP_REMAP:
> >  		err = check_ufence(gpuva_to_vma(op->base.remap.unmap->va));
> > @@ -2967,13 +2980,13 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
> >  
> >  		err = vma_lock_and_validate(exec,
> >  					    gpuva_to_vma(op->base.remap.unmap->va),
> > -					    res_evict, false);
> > +					    res_evict, false, false);
> >  		if (!err && op->remap.prev)
> >  			err = vma_lock_and_validate(exec, op->remap.prev,
> > -						    res_evict, true);
> > +						    res_evict, true, false);
> >  		if (!err && op->remap.next)
> >  			err = vma_lock_and_validate(exec, op->remap.next,
> > -						    res_evict, true);
> > +						    res_evict, true, false);
> >  		break;
> >  	case DRM_GPUVA_OP_UNMAP:
> >  		err = check_ufence(gpuva_to_vma(op->base.unmap.va));
> > @@ -2982,7 +2995,7 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
> >  
> >  		err = vma_lock_and_validate(exec,
> >  					    gpuva_to_vma(op->base.unmap.va),
> > -					    res_evict, false);
> > +					    res_evict, false, false);
> >  		break;
> >  	case DRM_GPUVA_OP_PREFETCH:
> >  	{
> > @@ -2997,7 +3010,7 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
> >  
> >  		err = vma_lock_and_validate(exec,
> >  					    gpuva_to_vma(op->base.prefetch.va),
> > -					    res_evict, false);
> > +					    res_evict, false, false);
> >  		if (!err && !xe_vma_has_no_bo(vma))
> >  			err = xe_bo_migrate(xe_vma_bo(vma),
> >  					    region_to_mem_type[region],
> > @@ -3305,7 +3318,8 @@ ALLOW_ERROR_INJECTION(vm_bind_ioctl_ops_execute, ERRNO);
> >  	 DRM_XE_VM_BIND_FLAG_NULL | \
> >  	 DRM_XE_VM_BIND_FLAG_DUMPABLE | \
> >  	 DRM_XE_VM_BIND_FLAG_CHECK_PXP | \
> > -	 DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR)
> > +	 DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR | \
> > +	 DRM_XE_VM_BIND_FLAG_DECOMPRESS)
> >  
> >  #ifdef TEST_VM_OPS_ERROR
> >  #define SUPPORTED_FLAGS	(SUPPORTED_FLAGS_STUB | FORCE_OP_ERROR)
> > @@ -3322,7 +3336,6 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> >  {
> >  	int err;
> >  	int i;
> > -
> 
> No related.
> 
> >  	if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
> >  	    XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> >  		return -EINVAL;
> > @@ -3363,9 +3376,9 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> >  		bool is_null = flags & DRM_XE_VM_BIND_FLAG_NULL;
> >  		bool is_cpu_addr_mirror = flags &
> >  			DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR;
> > +		bool is_decompress = flags & DRM_XE_VM_BIND_FLAG_DECOMPRESS;
> >  		u16 pat_index = (*bind_ops)[i].pat_index;
> >  		u16 coh_mode;
> > -
> 
> Not related.
> 
> >  		if (XE_IOCTL_DBG(xe, is_cpu_addr_mirror &&
> >  				 (!xe_vm_in_fault_mode(vm) ||
> >  				 !IS_ENABLED(CONFIG_DRM_XE_GPUSVM)))) {
> > @@ -3397,7 +3410,9 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> >  		    XE_IOCTL_DBG(xe, obj_offset && (is_null ||
> >  						    is_cpu_addr_mirror)) ||
> >  		    XE_IOCTL_DBG(xe, op != DRM_XE_VM_BIND_OP_MAP &&
> > -				 (is_null || is_cpu_addr_mirror)) ||
> > +				 (is_decompress || is_null || is_cpu_addr_mirror)) ||
> > +		    XE_IOCTL_DBG(xe, is_decompress &&
> > +				 pat_index == xe->pat.idx[XE_CACHE_NONE_COMPRESSION]) ||
> 
> The above check doesn't look right. I think you may to check pat_index
> against enabling compression and reject it.
> 
> >  		    XE_IOCTL_DBG(xe, !obj &&
> >  				 op == DRM_XE_VM_BIND_OP_MAP &&
> >  				 !is_null && !is_cpu_addr_mirror) ||
> > @@ -3417,8 +3432,8 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> >  				 op == DRM_XE_VM_BIND_OP_PREFETCH) ||
> >  		    XE_IOCTL_DBG(xe, prefetch_region &&
> >  				 op != DRM_XE_VM_BIND_OP_PREFETCH) ||
> > -		    XE_IOCTL_DBG(xe,  (prefetch_region != DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC &&
> > -				       !(BIT(prefetch_region) & xe->info.mem_region_mask))) ||
> > +		    XE_IOCTL_DBG(xe, (prefetch_region != DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC &&
> > +				      !(BIT(prefetch_region) & xe->info.mem_region_mask))) ||
> 
> Not related but is a good cleanup.
> 
> >  		    XE_IOCTL_DBG(xe, obj &&
> >  				 op == DRM_XE_VM_BIND_OP_UNMAP)) {
> >  			err = -EINVAL;
> > @@ -3433,6 +3448,12 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> >  			err = -EINVAL;
> >  			goto free_bind_ops;
> >  		}
> > +
> > +		if (is_decompress && (XE_IOCTL_DBG(xe, !xe_device_has_flat_ccs(xe)) ||
> > +				      XE_IOCTL_DBG(xe, GRAPHICS_VER(xe) < 20))) {
> > +			err = -EOPNOTSUPP;
> > +			goto free_bind_ops;
> > +		}
> >  	}
> >  
> >  	return 0;
> > @@ -3686,7 +3707,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		u64 obj_offset = bind_ops[i].obj_offset;
> >  		u32 prefetch_region = bind_ops[i].prefetch_mem_region_instance;
> >  		u16 pat_index = bind_ops[i].pat_index;
> > -
> 
> Not related.
> 
> Matt
> 
> >  		ops[i] = vm_bind_ioctl_ops_create(vm, &vops, bos[i], obj_offset,
> >  						  addr, range, op, flags,
> >  						  prefetch_region, pat_index);
> > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> > index 413353e1c225..7d652d17b0dc 100644
> > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > @@ -357,6 +357,8 @@ struct xe_vma_op_map {
> >  	bool dumpable;
> >  	/** @invalidate: invalidate the VMA before bind */
> >  	bool invalidate_on_bind;
> > +	/** @request_decompress: schedule decompression for GPU map */
> > +	bool request_decompress;
> >  	/** @pat_index: The pat index to use for this operation. */
> >  	u16 pat_index;
> >  };
> > -- 
> > 2.25.1
> >