|
From: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>
Sent: Tuesday, March 4, 2025 9:08 AM To: intel-xe@xxxxxxxxxxxxxxxxxxxxx <intel-xe@xxxxxxxxxxxxxxxxxxxxx> Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; Zuo, Alex <alex.zuo@xxxxxxxxx>; Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; joonas.lahtinen@xxxxxxxxxxxxxxx <joonas.lahtinen@xxxxxxxxxxxxxxx>; Brost, Matthew <matthew.brost@xxxxxxxxx>; Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>; Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx <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); 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.
But first of all, should we regard faults as a property?
+ 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;
int ret = 0;
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);
if (ret)
break;
ret = put_user(entry->pf->address_type, &usr_ptr->address_type);
if (ret)
break;
ret = put_user(1, &usr_ptr->address_precision);
if (ret)
break;
usr_ptr++;
list_del(entry->list);
}
spin_unlock(&vm->pfs.lock);
return ret;
}
+ +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 |
