From: Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>
Sent: Monday, March 3, 2025 12:23 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>; dri-devel@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH v3 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)
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
Suggested-by: Matthew Brost <matthew.brost@xxxxxxxxx>
---
drivers/gpu/drm/xe/xe_device.c | 3 ++
drivers/gpu/drm/xe/xe_vm.c | 79 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/xe/xe_vm.h | 2 +
3 files changed, 84 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..00d2c62ccf53 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -3234,6 +3234,85 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
return err;
}
+static u32 xe_vm_get_property_size(struct xe_vm *vm, u32 property)
+{
+ u32 size = 0;
+
+ switch (property) {
+ case DRM_XE_VM_GET_PROPERTY_FAULTS:
+ spin_lock(&vm->pfs.lock);
+ size = vm->pfs.len * sizeof(struct drm_xe_pf);
+ spin_unlock(&vm->pfs.lock);
+ return size;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int fill_property_pfs(struct xe_vm *vm,
+ struct drm_xe_vm_get_property *args,
+ u32 size)
+{
+ 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 i = 0;
+
+ if (copy_from_user(&fault_list, usr_ptr, size))
+ return -EFAULT;
> Why is copying from user space memory needed in this query ioctl?
> Does copy_from_user() automatically allocate kernel memory for fault_list?
Hrmm… No, you have a point. It probably isn’t necessary to copy the memory
from userspace if we don’t need to read anything from it. Though I’m fairly certain
it does automatically allocate kernel memory for the fault_list, as we do similar
things in xe_query.c without allocating memory for first pointer.
+
+ 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 = SZ_4K;
+ }
+ spin_unlock(&vm->pfs.lock);
+
+ if (copy_to_user(usr_ptr, &fault_list, size))
+ return -EFAULT;
+
+ return 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;
+ u32 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;
> If there is a change in the size of property between the first and the second calls, say, more faults added in this case, the 2nd call will fail unintendedly.
I suppose the alternative would be to ask userspace to allocate memory for the maximum
possible number of pagefaults, then return the number of pagefaults in “value” while saving
the pagefaults to “*ptr”.
Hmm… Give me a moment to see what that would look like…
-Jonathan Cavitt
+ return 0;
+ }
+
+ switch (args->property) {
+ case DRM_XE_VM_GET_PROPERTY_FAULTS:
+ return fill_property_pfs(vm, args, size);
+ 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
|
