Re: [PATCH v8 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/13/25 11:34, Jonathan Cavitt wrote:
Add support for userspace to request a list of observed faults
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)

v5:
- Rename ioctl to xe_vm_get_faults_ioctl (jcavitt)
- Update fill_property_pfs to eliminate need for kzalloc (Jianxun)

v6:
- Repair and move fill_faults break condition (Dan Carpenter)
- Free vm after use (jcavitt)
- Combine assertions (jcavitt)
- Expand size check in xe_vm_get_faults_ioctl (jcavitt)
- Remove return mask from fill_faults, as return is already -EFAULT or 0
   (jcavitt)

v7:
- Revert back to using xe_vm_get_property_ioctl
- Apply better copy_to_user logic (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     | 134 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/xe/xe_vm.h     |   2 +
  3 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index b2f656b2a563..74e510cb0e47 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -194,6 +194,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 067a9cedcad9..521f0032d9e2 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -42,6 +42,14 @@
  #include "xe_wa.h"
  #include "xe_hmm.h"
+static const u16 xe_to_user_engine_class[] = {
+	[XE_ENGINE_CLASS_RENDER] = DRM_XE_ENGINE_CLASS_RENDER,
+	[XE_ENGINE_CLASS_COPY] = DRM_XE_ENGINE_CLASS_COPY,
+	[XE_ENGINE_CLASS_VIDEO_DECODE] = DRM_XE_ENGINE_CLASS_VIDEO_DECODE,
+	[XE_ENGINE_CLASS_VIDEO_ENHANCE] = DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE,
+	[XE_ENGINE_CLASS_COMPUTE] = DRM_XE_ENGINE_CLASS_COMPUTE,
+};
+
  static struct drm_gem_object *xe_vm_obj(struct xe_vm *vm)
  {
  	return vm->gpuvm.r_obj;
@@ -3551,6 +3559,132 @@ 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)
+{
+	int size = -EINVAL;
+
+	switch (property) {
+	case DRM_XE_VM_GET_PROPERTY_FAULTS:
+		spin_lock(&vm->faults.lock);
+		size = vm->faults.len * sizeof(struct xe_vm_fault);
+		spin_unlock(&vm->faults.lock);
+		break;
+	default:
+		break;
+	}
+	return size;
+}
+
+static int xe_vm_get_property_verify_size(struct xe_vm *vm, u32 property,
+					  int expected, int actual)
+{
+	switch (property) {
+	case DRM_XE_VM_GET_PROPERTY_FAULTS:
+		/*
+		 * Number of faults may increase between calls to
+		 * xe_vm_get_property_ioctl, so just report the
+		 * number of faults the user requests if it's less
+		 * than or equal to the number of faults in the VM
+		 * fault array.
+		 */
+		if (actual < expected)
+			return -EINVAL;
Application should be allowed to provide more memory than the needed. In return, the driver should update the arg->size with number of bytes actually written to the memory.
+		break;
+	default:
+		if (actual != expected)
+			return -EINVAL;
+		break;
+	}
+	return 0;
+}
+
+static int fill_faults(struct xe_vm *vm,
+		       struct drm_xe_vm_get_property *args)
+{
+	struct xe_vm_fault __user *usr_ptr = u64_to_user_ptr(args->data);
+	struct xe_vm_fault store = { 0 };
+	struct xe_vm_fault_entry *entry;
+	int ret = 0, i = 0, count;
+
+	count = args->size / sizeof(struct xe_vm_fault);
+
+	spin_lock(&vm->faults.lock);
+	list_for_each_entry(entry, &vm->faults.list, list) {
+		if (i++ == count)
+			break;
+
+		memset(&store, 0, sizeof(struct xe_vm_fault));
+
+		store.address = entry->address;
+		store.address_type = entry->address_type;
+		store.address_precision = entry->address_precision;
+		store.fault_level = entry->fault_level;
+		store.engine_class = xe_to_user_engine_class[entry->engine_class];
+		store.engine_instance = entry->engine_instance;
+
+		ret = copy_to_user(usr_ptr, &store, sizeof(struct xe_vm_fault));
+		if (ret)
+			break;
+
+		usr_ptr++;
+	}
+	spin_unlock(&vm->faults.lock);
+
Update the args->size with the amount of the written. Refer to my another comment above. Also document this in the comment of the interface.
+	return ret;
+}
+
+static int xe_vm_get_property_fill_data(struct xe_vm *vm,
+					struct drm_xe_vm_get_property *args)
+{
+	switch (args->property) {
+	case DRM_XE_VM_GET_PROPERTY_FAULTS:
+		return fill_faults(vm, args);
+	default:
+		break;
+	}
+	return -EINVAL;
+}
+
+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, ret = 0;
+
+	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) {
+		ret = size;
+		goto put_vm;
+	} else if (!args->size) {
+		args->size = size;
+		goto put_vm;
+	}
+
+	ret = xe_vm_get_property_verify_size(vm, args->property,
+					     args->size, size);
+	if (XE_IOCTL_DBG(xe, ret)) {
+		ret = -EINVAL;
+		goto put_vm;
+	}
+
+	ret = xe_vm_get_property_fill_data(vm, args);
+
+put_vm:
+	xe_vm_put(vm);
+	return ret;
+}
+
  /**
   * 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 9bd7e93824da..63ec22458e04 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -196,6 +196,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);




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux