Re: [PATCH i-g-t v8 2/3] lib/xe: Add failable variant of xe_vm_bind_lr_sync()

["Hellstrom, Thomas" <thomas.hellstrom@intel.com>]

On Fri, 2026-03-27 at 13:24 +0000, Sobin Thomas wrote:
> Add __xe_vm_bind_lr_sync helper function which returns standard error
> codes instead of asserting on failure. This allows calling function
> to handle VM bind failures explicitly while preserving the existing
> xe_vm_bind_lr_sync() wrapper for tests.
> 
> Refactor the implementation to use proper return-code handling and
> user-fence synchronization, and add xe_vm_bind_lr_failable() for
> callers that expect bind / overcommit failures.
> 
> v7: Introduced xe_vm_bind_lr_sync_failable (Thomas)
> v8: Modified xe_vm_bind_lr_sync_failable and xe_vm_bind_lr_sync to
> call
>     __xe_vm_bind_lr_sync
> 
> Signed-off-by: Sobin Thomas <sobin.thomas@intel.com>
> ---
>  lib/xe/xe_ioctl.c | 36 ++++++++++++++++++++++++++++++------
>  lib/xe/xe_ioctl.h |  4 ++++
>  2 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/xe/xe_ioctl.c b/lib/xe/xe_ioctl.c
> index ea3f2fcaa..a8d315dfb 100644
> --- a/lib/xe/xe_ioctl.c
> +++ b/lib/xe/xe_ioctl.c
> @@ -822,23 +822,47 @@ void xe_vm_madvise(int fd, uint32_t vm,
> uint64_t addr, uint64_t range,
>  }
>  
>  #define	BIND_SYNC_VAL	0x686868
> -void xe_vm_bind_lr_sync(int fd, uint32_t vm, uint32_t bo, uint64_t
> offset,
> -			uint64_t addr, uint64_t size, uint32_t
> flags)
> +int __xe_vm_bind_lr_sync(int fd, uint32_t vm, uint32_t bo, uint64_t
> offset,
> +			 uint64_t addr, uint64_t size, uint32_t
> flags)
>  {
> -	volatile uint64_t *sync_addr = malloc(sizeof(*sync_addr));
> +	uint64_t *sync_addr = malloc(sizeof(*sync_addr));
>  	struct drm_xe_sync sync = {
>  		.flags = DRM_XE_SYNC_FLAG_SIGNAL,
>  		.type = DRM_XE_SYNC_TYPE_USER_FENCE,
>  		.addr = to_user_pointer((uint64_t *)sync_addr),

No need to typecast

>  		.timeline_value = BIND_SYNC_VAL,
>  	};
> +	int ret = 0;
>  
> -	igt_assert(!!sync_addr);
> -	xe_vm_bind_async_flags(fd, vm, 0, bo, 0, addr, size, &sync,
> 1, flags);
> -	if (*sync_addr != BIND_SYNC_VAL)
> +	if (!sync_addr)
> +		return -ENOMEM;
> +
> +	WRITE_ONCE(*sync_addr, 0);
> +
> +	ret = __xe_vm_bind(fd, vm, 0, bo, 0, addr, size,

It seems like offset got lost there. Pre-existing bug.


> DRM_XE_VM_BIND_OP_MAP, flags,
> +			   &sync, 1, 0,  DEFAULT_PAT_INDEX, 0);
> +
> +	if (ret)
> +		goto out;
> +
> +	if (READ_ONCE(*sync_addr) != BIND_SYNC_VAL)
>  		xe_wait_ufence(fd, (uint64_t *)sync_addr,
> BIND_SYNC_VAL, 0,

No typecast!

> NSEC_PER_SEC * 10);
>  	/* Only free if the wait succeeds */
> +out:
>  	free((void *)sync_addr);
> +	return ret;
> +}
> +
> +void xe_vm_bind_lr_sync(int fd, uint32_t vm, uint32_t bo, uint64_t
> offset,
> +			uint64_t addr, uint64_t size, uint32_t
> flags)
> +{
> +	igt_assert_eq(__xe_vm_bind_lr_sync(fd, vm, bo, offset, addr,
> size, flags), 0);
> +}
> +
> +int xe_vm_bind_lr_failable(int fd, uint32_t vm, uint32_t bo,
> uint64_t offset,
> +			   uint64_t addr, uint64_t size, uint32_t
> flags)
> +{
> +	return __xe_vm_bind_lr_sync(fd, vm, bo, offset, addr, size,
> flags);

Why did you add two functions with the same functionality but different
names?

/Thomas


>  }
>  
>  void xe_vm_unbind_lr_sync(int fd, uint32_t vm, uint64_t offset,
> diff --git a/lib/xe/xe_ioctl.h b/lib/xe/xe_ioctl.h
> index b62d259fd..a7e38ea04 100644
> --- a/lib/xe/xe_ioctl.h
> +++ b/lib/xe/xe_ioctl.h
> @@ -117,6 +117,10 @@ struct drm_xe_mem_range_attr
>  void xe_vm_bind_lr_sync(int fd, uint32_t vm, uint32_t bo,
>  			uint64_t offset, uint64_t addr,
>  			uint64_t size, uint32_t flags);
> +int __xe_vm_bind_lr_sync(int fd, uint32_t vm, uint32_t bo, uint64_t
> offset,
> +			 uint64_t addr, uint64_t size, uint32_t
> flags);
> +int xe_vm_bind_lr_failable(int fd, uint32_t vm, uint32_t bo,
> uint64_t offset,
> +			   uint64_t addr, uint64_t size, uint32_t
> flags);
>  void xe_vm_unbind_lr_sync(int fd, uint32_t vm, uint64_t offset,
>  			  uint64_t addr, uint64_t size);
>  #endif /* XE_IOCTL_H */