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

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

On Wed, Mar 26, 2025 at 07:51:15PM +0530, Cavitt, Jonathan wrote:
> > From: Jadav, Raag <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.
> 
> 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.

Yeah, that's one of the drm things which adds on to the confusion instead
of making things easier.

The usual practice (atleast in other subsystems) is to not differentiate
between patch and series version, which is quite easier to follow.

https://kernelnewbies.org/FirstKernelPatch -> "Versioning patchsets"

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

Yeah, mixing/matching types usual leads to bugs (atleast overtime), but
if you're trying to mimic xe_query then I think you should stick to it.

Raag