Re: [PATCH v2 17/32] drm/xe/uapi: Add madvise interface

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2025-04-22 at 08:40 -0700, Matthew Brost wrote:
> On Thu, Apr 17, 2025 at 12:19:54PM -0600, Souza, Jose wrote:
> > On Mon, 2025-04-07 at 15:47 +0530, Himal Prasad Ghimiray wrote:
> > > This commit introduces a new madvise interface to support
> > > driver-specific ioctl operations. The madvise interface allows for more
> > > efficient memory management by providing hints to the driver about the
> > > expected memory usage and pte update policy for gpuvma.
> > > 
> > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > >  include/uapi/drm/xe_drm.h | 97 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 97 insertions(+)
> > > 
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 9c08738c3b91..aaf515df3a83 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -81,6 +81,7 @@ extern "C" {
> > >   *  - &DRM_IOCTL_XE_EXEC
> > >   *  - &DRM_IOCTL_XE_WAIT_USER_FENCE
> > >   *  - &DRM_IOCTL_XE_OBSERVATION
> > > + *  - &DRM_IOCTL_XE_MADVISE
> > >   */
> > >  
> > >  /*
> > > @@ -102,6 +103,7 @@ extern "C" {
> > >  #define DRM_XE_EXEC			0x09
> > >  #define DRM_XE_WAIT_USER_FENCE		0x0a
> > >  #define DRM_XE_OBSERVATION		0x0b
> > > +#define DRM_XE_MADVISE			0x0c
> > >  
> > >  /* Must be kept compact -- no holes */
> > >  
> > > @@ -117,6 +119,7 @@ extern "C" {
> > >  #define DRM_IOCTL_XE_EXEC			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> > >  #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
> > >  #define DRM_IOCTL_XE_OBSERVATION		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_OBSERVATION, struct drm_xe_observation_param)
> > > +#define DRM_IOCTL_XE_MADVISE			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_MADVISE, struct drm_xe_madvise)
> > >  
> > >  /**
> > >   * DOC: Xe IOCTL Extensions
> > > @@ -1965,6 +1968,100 @@ struct drm_xe_query_eu_stall {
> > >  	__u64 sampling_rates[];
> > >  };
> > >  
> > > +struct drm_xe_madvise_ops {
> > > +	/** @start: start of the virtual address range */
> > > +	__u64 start;
> > > +
> > > +	/** @size: size of the virtual address range */
> > > +	__u64 range;
> > > +
> > > +#define DRM_XE_VMA_ATTR_PREFERRED_LOC	0
> > > +#define DRM_XE_VMA_ATTR_ATOMIC		1
> > > +#define DRM_XE_VMA_ATTR_PAT		2
> > 
> > In my opinion we should drop DRM_XE_VMA_ATTR_PAT as we can already change the PAT index by unbinding and binding the VM with the wanted PAT index.
> > Similar for DRM_XE_VMA_ATTR_ATOMIC and DRM_XE_VMA_ATTR_PREFERRED_LOC, we could have those properties in vm bind uAPI and if needed to change we could
> > unbind and bind the VMA with the updated attributes.
> > 
> 
> I had thought about making madvise patch of VM bind too. I think it
> could work but I do think it further complicate an already highly
> complicated VM bind KMD path. Also the idea with madvise, is it doesn't
> actually update the GPU page tables rather just the KMD VM/VMA
> bookkeeping. My prefence here would to be keep this seperate.

Looks like of those 32 pages 25ish are just refactors to allow madvise support set these attributes, maybe if those were supported in VM bind uAPI it
would take less changes...

> 
> All of the above attribute are really only intended to be set by
> faulting VM which Mesa doesn't use.

We have plans to drop scratch flag from VM in Xe2+ platforms to support a Vulkan extension but that will take some time...

> 
> > > +#define DRM_XE_VMA_ATTR_PURGEABLE_STATE	3
> > 
> > Mesa can make use of DRM_XE_VMA_ATTR_PURGEABLE_STATE but not the other ones, so you will need other UMDs to land those.
> > 
> 
> I could see Mesa using this one, but again this just flips a bit in KMD
> internal state - it does touch GPU page tables so would be goofy to
> implement this in a very complex VM bind IOCTL.

This one I also don't think it belongs to VM bind uAPI but I did not knew it required a fault VM, we plan to remove the scratch flag from Vulkan
driver but the usage of this attribute would be in the OpenGL driver, so I don't think we can make use of it then.

> 
> Matt
> 
> > > +	/** @type: type of attribute */
> > > +	__u32 type;
> > > +
> > > +	/** @pad: MBZ */
> > > +	__u32 pad;
> > > +
> > > +	union {
> > > +		struct {
> > > +#define DRM_XE_VMA_ATOMIC_UNDEFINED	0
> > > +#define DRM_XE_VMA_ATOMIC_DEVICE	1
> > > +#define DRM_XE_VMA_ATOMIC_GLOBAL	2
> > > +#define DRM_XE_VMA_ATOMIC_CPU		3
> > > +		/** @val: value of atomic operation*/
> > > +			__u32 val;
> > > +
> > > +		/** @reserved: Reserved */
> > > +			__u32 reserved;
> > > +		} atomic;
> > > +
> > > +		struct {
> > > +#define DRM_XE_VMA_PURGEABLE_STATE_WILLNEED	0
> > > +#define DRM_XE_VMA_PURGEABLE_STATE_DONTNEED	1
> > > +#define DRM_XE_VMA_PURGEABLE_STATE_PURGED	2
> > > +		/** @val: value for DRM_XE_VMA_ATTR_PURGEABLE_STATE */
> > > +			__u32 val;
> > > +
> > > +		/** @reserved: Reserved */
> > > +			__u32 reserved;
> > > +		} purge_state_val;
> > > +
> > > +		struct {
> > > +			/** @pat_index */
> > > +			__u32 val;
> > > +
> > > +			/** @reserved: Reserved */
> > > +			__u32 reserved;
> > > +		} pat_index;
> > > +
> > > +		/** @preferred_mem_loc: preferred memory location */
> > > +		struct {
> > > +			__u32 devmem_fd;
> > > +
> > > +#define MIGRATE_ALL_PAGES 0
> > > +#define MIGRATE_ONLY_SYSTEM_PAGES 1
> > > +			__u32 migration_policy;
> > > +		} preferred_mem_loc;
> > > +	};
> > > +
> > > +	/** @reserved: Reserved */
> > > +	__u64 reserved[2];
> > > +};
> > > +
> > > +/**
> > > + * struct drm_xe_madvise - Input of &DRM_IOCTL_XE_MADVISE
> > > + *
> > > + * Set memory attributes to a virtual address range
> > > + */
> > > +struct drm_xe_madvise {
> > > +	/** @extensions: Pointer to the first extension struct, if any */
> > > +	__u64 extensions;
> > > +
> > > +	/** @vm_id: vm_id of the virtual range */
> > > +	__u32 vm_id;
> > > +
> > > +	/** @num_ops: number of madvises in ioctl */
> > > +	__u32 num_ops;
> > > +
> > > +	union {
> > > +		/** @ops: used if num_ops == 1 */
> > > +		struct drm_xe_madvise_ops ops;
> > > +
> > > +		/**
> > > +		 * @vector_of_ops: userptr to array of struct
> > > +		 * drm_xe_vm_madvise_op if num_ops > 1
> > > +		 */
> > > +		__u64 vector_of_ops;
> > > +	};
> > > +
> > > +	/** @reserved: Reserved */
> > > +	__u64 reserved[2];
> > > +
> > > +};
> > > +
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif