-----Original Message-----
From: Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>
Sent: Wednesday, March 19, 2025 4:58 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; Wajdeczko, Michal <Michal.Wajdeczko@xxxxxxxxx>; michal.mzorek@xxxxxxxxx
Subject: Re: [PATCH v8 6/6] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
>
> 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,
> Not sure if this will be used to interpret engine_class that will be
> reported to UMD, so just a heads up. In bspec 77412 the compute engine
> class id is 5, instead of 4 as this macro defined.
xe_hw_engine_types.h
XE_ENGINE_CLASS_COMPUTE = 5,
-Jonathan Cavitt
>
> There are other values so perhaps you can just report raw value of
> engine class and let UMD deals with it according to the bspec.
> > +};
> > +
> > 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;
> > + 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);
> > +
> > + 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);
> >
>
>
