Re: [PATCH v4 02/20] drm/xe/uapi: Add madvise interface

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Sun, 2025-06-22 at 21:30 -0700, Matthew Brost wrote:
> On Fri, Jun 13, 2025 at 06:25:40PM +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.
> > 
> > v2 (Matthew/Thomas)
> > - Drop num_ops support
> > - Drop purgeable support
> > - Add kernel-docs
> > - IOWR/IOW
> > 
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Signed-off-by: Himal Prasad Ghimiray
> > <himal.prasad.ghimiray@intel.com>
> > Acked-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  include/uapi/drm/xe_drm.h | 118
> > ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 118 insertions(+)
> > 
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 6a702ba7817c..b5f8d11faaa8 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,structdrm_xe_wait_user_fence)
> >  #define
> > DRM_IOCTL_XE_OBSERVATION		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_OBSERVATION,structdrm_xe_observation_param)
> > +#define
> > DRM_IOCTL_XE_MADVISE			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_MADVISE, structdrm_xe_madvise)
> >  
> >  /**
> >   * DOC: Xe IOCTL Extensions
> > @@ -1970,6 +1973,121 @@ struct drm_xe_query_eu_stall {
> >  	__u64 sampling_rates[];
> >  };
> >  
> > +/**
> > + * struct drm_xe_madvise - Input of &DRM_IOCTL_XE_MADVISE
> > + *
> > + * This structure is used to set memory attributes for a virtual
> > address range
> > + * in a VM. The type of attribute is specified by @type, and the
> > corresponding
> > + * union member is used to provide additional parameters for
> > @type.
> > + *
> > + * Supported attribute types:
> > + * - DRM_XE_VMA_ATTR_PREFERRED_LOC: Set preferred memory location.
> > + * - DRM_XE_VMA_ATTR_ATOMIC: Set atomic access policy.
> > + * - DRM_XE_VMA_ATTR_PAT: Set page attribute table index.
> > + *
> > + * Example:
> > + *
> > + * .. code-block:: C
> > + *
> > + * struct drm_xe_madvise madvise = {
> > + *          .vm_id = vm_id,
> > + *          .start = 0x100000,
> > + *          .range = 0x2000,
> > + *          .type = DRM_XE_VMA_ATTR_ATOMIC,
> > + *          .atomic_val = DRM_XE_VMA_ATOMIC_DEVICE,
> > + *         };
> > + *
> > + * ioctl(fd, DRM_IOCTL_XE_MADVISE, &madvise);
> > + *
> > + */
> > +struct drm_xe_madvise {
> > +	/** @extensions: Pointer to the first extension struct, if
> > any */
> > +	__u64 extensions;
> > +
> > +	/** @start: start of the virtual address range */
> > +	__u64 start;
> > +
> > +	/** @size: size of the virtual address range */
> > +	__u64 range;
> > +
> > +	/** @vm_id: vm_id of the virtual range */
> > +	__u32 vm_id;
> > +
> > +#define DRM_XE_VMA_ATTR_PREFERRED_LOC	0
> > +#define DRM_XE_VMA_ATTR_ATOMIC		1
> > +#define DRM_XE_VMA_ATTR_PAT		2
> > +	/** @type: type of attribute */
> > +	__u32 type;
> > +
> > +	union {
> 
> Nit: I'd make this union is same order as the defines (e.g.,
> preferred location
> first, atomic second, pat third).
> 
> > +		/**
> > +		 * @atomic: Atomic access policy
> > +		 *
> > +		 * Used when @type == DRM_XE_VMA_ATTR_ATOMIC.
> > +		 *
> > +		 * Supported values for @atomic.val:
> > +		 * - DRM_XE_VMA_ATOMIC_UNDEFINED: Undefined or
> > default behaviour
> > +		 *   Support both GPU and CPU atomic operations
> > for system allocator
> > +		 *   Support GPU atomic operations for normal(bo)
> > allocator
> > +		 * - DRM_XE_VMA_ATOMIC_DEVICE: Support GPU atomic
> > operations
> > +		 * - DRM_XE_VMA_ATOMIC_GLOBAL: Support both GPU
> > and CPU atomic operations
> > +		 * - DRM_XE_VMA_ATOMIC_CPU: Support CPU atomic
> > +		 */
> > +		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
> > +			/** @atomic.val: value of atomic operation
> > */
> > +			__u32 val;
> > +
> > +			/** @atomic.reserved: Reserved */
> > +			__u32 reserved;
> > +		} atomic;
> > +
> > +		/**
> > +		 * @pat_index: Page attribute table index
> > +		 *
> > +		 * Used when @type == DRM_XE_VMA_ATTR_PAT.
> > +		 */
> > +		struct {
> > +			/** @pat_index.val: PAT index value */
> > +			__u32 val;
> > +
> > +			/** @pat_index.reserved: Reserved */
> > +			__u32 reserved;
> > +		} pat_index;
> > +
> > +		/**
> > +		 * @preferred_mem_loc: preferred memory location
> > +		 *
> > +		 * Used when @type ==
> > DRM_XE_VMA_ATTR_PREFERRED_LOC
> > +		 *
> > +		 * Supported values for
> > @preferred_mem_loc.devmem_fd:
> > +		 * - DRM_XE_PREFERRED_LOC_DEFAULT_DEVICE: set vram
> > of faulting tile as preferred loc
> > +		 * - DRM_XE_PREFERRED_LOC_DEFAULT_SYSTEM: set smem
> > as preferred loc
> > +		 *
> > +		 * Supported values for
> > @preferred_mem_loc.migration_policy:
> > +		 * - DRM_XE_MIGRATE_ALL_PAGES
> > +		 * - DRM_XE_MIGRATE_ONLY_SYSTEM_PAGES
> > +		 */
> > +		struct {
> > +#define DRM_XE_PREFERRED_LOC_DEFAULT_DEVICE	0
> > +#define DRM_XE_PREFERRED_LOC_DEFAULT_SYSTEM	-1
> > +			/** @preferred_mem_loc.devmem_fd: fd for
> > preferred loc */
> > +			__u32 devmem_fd;
> > +
> > +#define DRM_XE_MIGRATE_ALL_PAGES		0
> > +#define DRM_XE_MIGRATE_ONLY_SYSTEM_PAGES	1
> 
> I'd double check with Thomas / maintainers if they want this
> (migration_policy)
> to be included in this version as still more or less unused in this
> series.
> 
> > +			/** @preferred_mem_loc.migration_policy:
> > Page migration policy */
> > +			__u32 migration_policy;
> 
> Could we future proof a little, maybe migration_policy is a __u16 (or
> __u8?) and
> stick a reserved __u16 in here?
> 
> Matt

Yeah, I think we need to have a strategy for extensions here. Perhaps
we could add new madvise types by extending the union and add
additional types, but then I think we need to expand the union size to
accomodate reasonable future additions.

Additions could also be made using extensions, so that if a suitable
extension is present, the union is never used?

/Thomas



> 
> > +		} preferred_mem_loc;
> > +	};
> > +
> > +	/** @reserved: Reserved */
> > +	__u64 reserved[2];
> > +};
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > -- 
> > 2.34.1
> >