[PATCH v12 5/5] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Cavitt, Jonathan]

-----Original Message-----
From: Jadav, Raag <raag.jadav at intel.com> 
Sent: Tuesday, March 25, 2025 12:40 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Cc: intel-xe at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; joonas.lahtinen at linux.intel.com; Brost, Matthew <matthew.brost at intel.com>; Zhang, Jianxun <jianxun.zhang at intel.com>; Lin, Shuicheng <shuicheng.lin at intel.com>; dri-devel at lists.freedesktop.org; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Mrozek, Michal <michal.mrozek at intel.com>
Subject: Re: [PATCH v12 5/5] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl
> 
> On Mon, Mar 24, 2025 at 11:09:28PM +0000, Jonathan Cavitt wrote:
> > Add support for userspace to request a list of observed faults
> > from a specified VM.
> 
> ...
> 
> > v10:
> > - Remove unnecessary switch case logic (Raag)
> 
> This is usually "changes present in version" and not "comments received
> in version" but I guess this must be one of those drm things.

I'm fairly certain change logs are supposed to be written in the future tense.
Or at the very least, I think I picked up the habit in college.  Is this not correct?

> 
> ...
> 
> > +static int xe_vm_get_property_helper(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 ? fill_faults(vm, args) : -EINVAL;
> 
> You're comparing an int with u32 and I'm not sure how this plays out.
> The usual practice is to use size_t (even in the struct) 

Size is a u32 in struct drm_xe_device_query.

> but I'm not aware of its userspace counterpart.

You mean the associated IGT tests?  The interface has been changing so much
that I've been putting off making the IGT tests until the interface has been
finalized.

Or do you mean the use case?  Because that's
https://registry.khronos.org/vulkan/specs/latest/man/html/VK_EXT_device_fault.html
-Jonathan Cavitt

> 
> Raag
>