[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 4:45 PM
> 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 Tue, Mar 25, 2025 at 08:15:59PM +0530, Cavitt, Jonathan wrote:
> > From: Jadav, Raag <raag.jadav at 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.

Apologies for the confusion.  Not every patch is modified each revision cycle, so
removing unnecessary switch case logic was the 10th actual revision of the patch,
whereas the series as a whole had been modified 11 times at that point.

The change is correctly labeled as a part of revision 11 in the cover letter.

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

Do you mean the size variable used in xe_query functions?  Because that's
size_t.  Though the initial question was about the size variables in the structs,
AFAICT.

I need to solve a rogue locking issue with the series, so I'll change the size variable
to be a size_t in xe_vm_get_property_helper while I'm taking care of that.

-Jonathan Cavitt

> 
> Raag
>