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

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

Hi!

Patch looks mostly correct, but some questions / comments below.

On Wed, 2026-03-25 at 06:03 +0000, Sobin Thomas wrote:
> Introduce a new failable variant that returns error codes instead of
> asserting, allowing callers to handle errors explicitly.
> 
> Signed-off-by: Sobin Thomas <sobin.thomas@intel.com>
> ---
>  lib/xe/xe_ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
>  lib/xe/xe_ioctl.h |  2 ++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/lib/xe/xe_ioctl.c b/lib/xe/xe_ioctl.c
> index 16aae05c9..30c8a15f6 100644
> --- a/lib/xe/xe_ioctl.c
> +++ b/lib/xe/xe_ioctl.c
> @@ -795,6 +795,42 @@ void xe_vm_bind_lr_sync(int fd, uint32_t vm,
> uint32_t bo, uint64_t offset,
>  	free((void *)sync_addr);
>  }
>  
> +int xe_vm_bind_lr_failable(int fd, uint32_t vm, uint32_t exec_queue,
> uint32_t bo, uint64_t offset,
> +			   uint64_t addr, uint64_t size, uint32_t
> flags)

Typically for versions of a function that can fail, we're using
__function_name().

Like __xe_vm_bind_lr_sync().  AI?


> +{
> +	volatile uint64_t *sync_addr = malloc(sizeof(*sync_addr));

Here we should actually use uint64_t without the volatile and use
READ_ONCE() when reading the value pointed to instead. I know the
original code has volatile as well, but the READ_ONCE is the better
choice.

> +	struct drm_xe_sync sync = {0};
> +	int ret = 0;
> +	int64_t timeout = NSEC_PER_SEC * 10;

We're typically using "chistmas tree" layout in the declaration if
possible. Longer lines on top.


> +
> +	if (!sync_addr)
> +		return -ENOMEM;
> +
> +	*sync_addr = 0;

WRITE_ONCE() when the volatile is gone.

> +
> +	sync.flags = DRM_XE_SYNC_FLAG_SIGNAL;
> +	sync.type = DRM_XE_SYNC_TYPE_USER_FENCE;
> +	sync.addr = to_user_pointer((uint64_t *)sync_addr);

Type-cast can be removed if we ditch "volatile"-

> +	sync.timeline_value = BIND_SYNC_VAL;

Use an initializer like in the original.

> +
> +	ret = __xe_vm_bind(fd, vm, 0, bo, 0, addr, size,
> DRM_XE_VM_BIND_OP_MAP, flags,
> +			   &sync, 1, 0,  DEFAULT_PAT_INDEX, 0);
> +
> +	if (ret)
> +		goto out;
> +
> +	if (*sync_addr != BIND_SYNC_VAL) {

READ_ONCE()

> +		ret = __xe_wait_ufence(fd, (uint64_t *)sync_addr,

Drop type-cast.

>  BIND_SYNC_VAL,
> +				       0, &timeout);
> +	if (ret)
> +		goto out;

Indentation looks off, but this if-statement doesn't actually do
anything, right? Both branches end up at the same place?

> +	}
> +
> +out:
> +	free((void *)sync_addr);

Drop type-cast.

This should *only* be freed if the wait didn't succeed. If we timed out
and free here, then KMD can still write to sync_addr and corrupt the
heap, so it's better to intentionally leak memory on failure here.

> +	return ret;
> +}
> +
>  void xe_vm_unbind_lr_sync(int fd, uint32_t vm, uint64_t offset,
>  			  uint64_t addr, uint64_t size)
>  {

This function should be calling the new function and assert that the
return value is 0. Potential to remove some code. AI?

Thanks,
Thomas


> diff --git a/lib/xe/xe_ioctl.h b/lib/xe/xe_ioctl.h
> index 3ea651063..eba10bd34 100644
> --- a/lib/xe/xe_ioctl.h
> +++ b/lib/xe/xe_ioctl.h
> @@ -116,6 +116,8 @@ 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_failable(int fd, uint32_t vm, uint32_t exec_queue,
> 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 */