From: Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>
Sent: Tuesday, March 4, 2025 12:38 PM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; Zuo, Alex <alex.zuo@xxxxxxxxx>; joonas.lahtinen@xxxxxxxxxxxxxxx; Brost, Matthew <matthew.brost@xxxxxxxxx>; Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [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);
> I still think returning the number of faults back to user space is better and clearer.
> But the intention is to use this ioctl on any properties, so perhaps it is why a size in byte is chosen.
This is a workaround for the issue you pointed out in the previous revision that caused the get
property ioctl to fail if the number of faults in the system changed between the first and second
calls. Getting the precise size of the data field would reintroduce that issue, and keeping the lock
between the first and second calls to prevent the list from changing between calls would violate
several design principles (E.G., what happens if the user only calls the ioctl once?)
> But first of all, should we regard faults as a property?
I think this question would be better answered by Matthew Brost, as this was at least
in part his suggestion. Though I suppose we could make this an extension of xe_query
and just ask userspace to pass in additional information W.R.T. the VM ID as a part of
the query structure. On the other hand, we’ll probably want the property ioctl extended
in the future to be able to report additional information, which has no precedence in
the query ioctl and thus would bloat the query ioctl with additional queries.
+ 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;
+}
> How about this to get rid of fault_list? The assumption is put_user is sufficient on types of data to return.
> It also deletes the copied data though KMD may still want to keep them.
> This is Just an idea, It makes copied size is unclear so more refactor would be needed.)
> 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 xe_vm_pf_entry *entry;
> struct xe_vm_pf_entry *tmp_entry;
> spin_lock(&vm->pfs.lock);
> list_for_each_entry_safe(entry, tmp_entry, &vm->pfs.list, list) {
> ret = put_user(entry->pf->page_addr, &usr_ptr->address);
> ret = put_user(entry->pf->address_type, &usr_ptr->address_type);
> ret = put_user(1, &usr_ptr->address_precision);
> spin_unlock(&vm->pfs.lock);
> }
I’ll consider it once we can get a finalization on whether this needs to be a separate ioctl
or an extension of xe_query. Though either way I’ll have to pass on clearing the list on query.
The information should stay persistent until the VM is closed or until it’s lost due to hitting
the list size limit.
-Jonathan Cavitt
+
+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
