-----Original Message-----
From: Jadav, Raag <raag.ja...@intel.com>
Sent: Friday, March 21, 2025 4:37 PM
To: Cavitt, Jonathan <jonathan.cav...@intel.com>
Cc: intel...@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gu...@intel.com>;
Zuo, Alex <alex....@intel.com>; joonas.lahti...@linux.intel.com; Brost, Matthew
<matthew.br...@intel.com>; Zhang, Jianxun <jianxun.zh...@intel.com>; Lin,
Shuicheng <shuicheng....@intel.com>; dri-devel@lists.freedesktop.org;
Wajdeczko, Michal <michal.wajdec...@intel.com>; Mrozek, Michal
<michal.mro...@intel.com>
Subject: Re: [PATCH v10 5/5] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
>
> On Thu, Mar 20, 2025 at 03:26:15PM +0000, Jonathan Cavitt wrote:
> > Add support for userspace to request a list of observed faults
> > from a specified VM.
>
> ...
>
> > +static int xe_vm_get_property_size(struct xe_vm *vm, u32 property)
> > +{
> > + int size = -EINVAL;
>
> Mixing size and error codes is usually received with mixed feelings.
>
> > +
> > + switch (property) {
> > + case DRM_XE_VM_GET_PROPERTY_FAULTS:
> > + spin_lock(&vm->faults.lock);
> > + size = vm->faults.len * sizeof(struct xe_vm_fault);
>
> size_mul() and,
> [1] perhaps fill it up into the pointer passed by the caller here?
"The pointer passed by the caller". You mean the args pointer?
We'd still need to check that the args->size value is empty here before
overwriting
it, and we'd also still need to return the size to the ioctl so we can verify
it's
acceptable later in xe_vm_get_property_verify_size.
Unless you want to merge those two processes together into here?
"""
static int xe_vm_get_property_report_size(struct xe_vm *vm,
struct drm_xe_vm_get_property *args)
{
int size;
switch(args->property) {
case DRM_XE_VM_GET_PROPERTY_FAULTS:
spin_lock(&vm->faults.lock);
size = size_mul(sizeof(struct xe_vm_fault), vm->faults.len);
spin_unlock(&vm->faults.lock);
if (args->size)
/*
* 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.
*/
return args->size <= size ? 0 : -EINVAL;
else
args->size = size;
return 0;
}
return -EINVAL;
}
"""
Then, below, we'd need to branch based on the initial state of args->size:
"""
vm = xe_vm_lookup(xef, args->vm_id);
if (XE_IOCTL_DBG(xe, !vm))
return -ENOENT;
size = args->size;
ret = xe_vm_get_property_report_size(vm, args);
/*
* Either the xe_vm_get_property_report_size function failed, or
* userspace is expected to provide a memory allocation for the
* property. In either case, exit early.
*/
if ((args->size && !size) || ret)
goto put_vm;
"""
Something about this seems a bit cluttered, and it'll only get worse if
we need to add more properties, but maybe this would work.
...
I just looked below. When you're referring to "the pointer", you're
referring to a new pointer to store the size in, not "the args pointer"...
I guess that would also work, though we'd still need to branch execution
so we can store the new size in args if a size is requested/reported.
>
> > + spin_unlock(&vm->faults.lock);
> > + break;
> > + default:
> > + break;
>
> Do we need the default case?
That's a fair point. I thought that if I didn't include the default case,
either checkpatch would complain or the switch case would not run
properly, but I double-checked and it seems like it's not necessary.
>
> > + }
> > + 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:
>
> Unless we're expecting more cases (that we confidently know of), there's
> not much point of single case switch.
I guess one could argue that if the property value was anything other than
DRM_XE_VM_GET_PROPERTY_FAULTS, the test would have failed by
xe_vm_get_property_size, so any further checks are unnecessary.
Though given a previous ask (or at least a misinterpretation of a previous ask),
this function probably won't exist for much longer anyways.
>
> > + /*
> > + * 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 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;
>
> Same as above.
"above" as in "unless we expect more properties, having a single switch case is
pointless",
or "above" as in "we don't need a default case if all it's doing is breaking
out of the switch"?
>
> > + }
> > + 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 && size) {
> > + args->size = size;
> > + goto put_vm;
> > + }
>
> With [1] in place, this gymnastics can be dropped
>
> ret = xe_vm_get_property_size(vm, args->property, &size);
> if (ret)
> goto put_vm;
We'd still need to branch execution so we can store the new size in args if a
size is requested/reported.
So, it'd actually look something more like:
"""
ret = xe_vm_get_property_size(vm, args->property, &size);
if (ret) {
goto put_vm;
} else if (!args->size && size) {
args->size = size;
goto put_vm;
}
"""
I won't deny it's cleaner to look at, but it's not particularly better
compressed
than before.
-Jonathan Cavitt
>
> > +
> > + ret = xe_vm_get_property_verify_size(vm, args->property,
> > + args->size, size);
> > + if (XE_IOCTL_DBG(xe, ret))
> > + goto put_vm;
> > +
> > + ret = xe_vm_get_property_fill_data(vm, args);
> > +
> > +put_vm:
> > + xe_vm_put(vm);
> > + return ret;
> > +}
>
> Raag
>
