Hello Lucas, Rodrigo, and Thomas. Just querying the room on something.
I need help deciding if this new xe_vm_get_property_ioctl should actually just be an extension
of the currently existing xe_query functionality. On one hand, there's a decent chance that this
ioctl will need to be extended in the future to allow for querying additional information on the
VM (E.G. engine reset counts), which has no precedent in xe_query and would make this type
of query better suited for a new ioctl. On the other hand, there's also no anti-precedence for
such a type of query, and it's also debatable if the currently returned data from the
xe_vm_get_property_ioctl is even a "property" of the vm itself.
Any input welcome. Thank you for your time.
-Jonathan Cavitt
-----Original Message-----
From: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>
Sent: Tuesday, March 4, 2025 9:09 AM
To: intel-xe@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; Zuo, Alex <alex.zuo@xxxxxxxxx>; Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; joonas.lahtinen@xxxxxxxxxxxxxxx; Brost, Matthew <matthew.brost@xxxxxxxxx>; Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>; Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx
Subject: [PATCH v5 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
>
> Add support for userspace to request a list of observed failed
> pagefaults from a specified VM.
>
> v2:
> - Only allow querying of failed pagefaults (Matt Brost)
>
> v3:
> - Remove unnecessary size parameter from helper function, as it
> is a property of the arguments. (jcavitt)
> - Remove unnecessary copy_from_user (Jainxun)
> - Set address_precision to 1 (Jainxun)
> - Report max size instead of dynamic size for memory allocation
> purposes. Total memory usage is reported separately.
>
> v4:
> - Return int from xe_vm_get_property_size (Shuicheng)
> - Fix memory leak (Shuicheng)
> - Remove unnecessary size variable (jcavitt)
>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> Suggested-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> CC: Jainxun Zhang <jianxun.zhang@xxxxxxxxx>
> CC: Shuicheng Lin <shuicheng.lin@xxxxxxxxx>
> ---
> drivers/gpu/drm/xe/xe_device.c | 3 ++
> drivers/gpu/drm/xe/xe_vm.c | 75 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_vm.h | 2 +
> 3 files changed, 80 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 9454b51f7ad8..43accae152ff 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -193,6 +193,9 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
> DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(XE_OBSERVATION, xe_observation_ioctl, DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(XE_VM_GET_PROPERTY, xe_vm_get_property_ioctl,
> + DRM_RENDER_ALLOW),
> +
> };
>
> static long xe_drm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 6211b971bbbd..1e78103fb0f6 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3234,6 +3234,81 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> return err;
> }
>
> +static int xe_vm_get_property_size(struct xe_vm *vm, u32 property)
> +{
> + switch (property) {
> + case DRM_XE_VM_GET_PROPERTY_FAULTS:
> + return MAX_PFS * sizeof(struct drm_xe_pf);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int fill_property_pfs(struct xe_vm *vm,
> + struct drm_xe_vm_get_property *args)
> +{
> + struct drm_xe_pf __user *usr_ptr = u64_to_user_ptr(args->ptr);
> + struct drm_xe_pf *fault_list;
> + struct drm_xe_pf *fault;
> + struct xe_vm_pf_entry *entry;
> + int ret, i = 0;
> +
> + fault_list = kzalloc(args->size, GFP_KERNEL);
> + if (!fault_list)
> + return -ENOMEM;
> +
> + spin_lock(&vm->pfs.lock);
> + list_for_each_entry(entry, &vm->pfs.list, list) {
> + struct xe_pagefault *pf = entry->pf;
> +
> + fault = &fault_list[i++];
> + fault->address = pf->page_addr;
> + fault->address_type = pf->address_type;
> + fault->address_precision = 1;
> + }
> + args->value = vm->pfs.len;
> + spin_unlock(&vm->pfs.lock);
> +
> + ret = copy_to_user(usr_ptr, &fault_list, args->size);
> + kfree(fault_list);
> +
> + return ret ? -EFAULT : 0;
> +}
> +
> +int xe_vm_get_property_ioctl(struct drm_device *drm, void *data,
> + struct drm_file *file)
> +{
> + struct xe_device *xe = to_xe_device(drm);
> + struct xe_file *xef = to_xe_file(file);
> + struct drm_xe_vm_get_property *args = data;
> + struct xe_vm *vm;
> + int size;
> +
> + if (XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> + return -EINVAL;
> +
> + vm = xe_vm_lookup(xef, args->vm_id);
> + if (XE_IOCTL_DBG(xe, !vm))
> + return -ENOENT;
> +
> + size = xe_vm_get_property_size(vm, args->property);
> + if (size < 0) {
> + return size;
> + } else if (args->size != size) {
> + if (args->size)
> + return -EINVAL;
> + args->size = size;
> + return 0;
> + }
> +
> + switch (args->property) {
> + case DRM_XE_VM_GET_PROPERTY_FAULTS:
> + return fill_property_pfs(vm, args);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> /**
> * xe_vm_bind_kernel_bo - bind a kernel BO to a VM
> * @vm: VM to bind the BO to
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 4d94ab5c8ea4..bf6604465aa3 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -184,6 +184,8 @@ int xe_vm_destroy_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> int xe_vm_bind_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> +int xe_vm_get_property_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file);
>
> void xe_vm_close_and_put(struct xe_vm *vm);
>
> --
> 2.43.0
>
>
