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

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Mar 25, 2025 at 08:15:59PM +0530, Cavitt, Jonathan wrote:
> From: Jadav, Raag <raag.jadav@intel.com> 
> > 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?

I meant it belongs to v11.

> > > +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.

And what about the local one?

Raag