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

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Jun 27, 2025 at 04:29:02PM +0200, Thomas Hellström wrote:
> 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.
> 

I agree that adding some extra bits in the member for the union is a
reasonable idea. Maybe add an extra __u64 to each union member as well.

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

Implementing this purely as extensions could work too.

That said, these two ideas are not mutually exclusive. I’d suggest
starting by adding some extra bits to the union, and if we later back
ourselves into a corner where there aren’t enough bits for a future
property, we can introduce an extension for that property if needed.

Matt

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