Re: [PATCH 2/2] drm/xe/uapi: Enable madvise to pass multiple pat indeces

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

On Thu, 2025-09-18 at 13:30 +0530, Himal Prasad Ghimiray wrote:
> Allow users to pass multiple PAT indices via madvise, which can be
> used
> to encode PTEs based on the actual memory location system memory,
> local
> VRAM, or remote GPU memory.
> 
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Himal Prasad Ghimiray
> <himal.prasad.ghimiray@intel.com>

Based on the feedback from UMD, it might make sense to add two special
values,
XE_PAT_INDEX_INITIAL - that sets  the pat index to the initial value of
the VM_BIND
XE_PAT_INDEX_KEEP - Keep the current value.

And then we perhaps we can use __s16 values where needed.
Let's see where the discussion lands. If we're not able to resolve this
before your vacation, I can pick up the patches from where you left
off.

Thanks,
Thomas



> ---
>  drivers/gpu/drm/xe/xe_vm.c         |  7 +++--
>  drivers/gpu/drm/xe/xe_vm_madvise.c | 33 ++++++++++++++++-------
>  include/uapi/drm/xe_drm.h          | 42 +++++++++++++++++++++++++---
> --
>  3 files changed, 62 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 6122061786f6..a4906ae94bd4 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2003,10 +2003,9 @@ static int get_mem_attrs(struct xe_vm *vm, u32
> *num_vmas, u64 start,
>  		attrs[i].start = xe_vma_start(vma);
>  		attrs[i].end = xe_vma_end(vma);
>  		attrs[i].atomic.val = vma->attr.atomic_access;
> -
> -		/* TODO: Modify drm_xe_mem_range_attr for all pats
> */
> -		attrs[i].pat_index.val = vma-
> >attr.pat_index.initial;
> -
> +		attrs[i].pat_index.smem = vma->attr.pat_index.smem;
> +		attrs[i].pat_index.devmem = vma-
> >attr.pat_index.devmem;
> +		attrs[i].pat_index.remote = vma-
> >attr.pat_index.remote;
>  		attrs[i].preferred_mem_loc.devmem_fd = vma-
> >attr.preferred_loc.devmem_fd;
>  		attrs[i].preferred_mem_loc.migration_policy =
>  		vma->attr.preferred_loc.migration_policy;
> diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.c
> b/drivers/gpu/drm/xe/xe_vm_madvise.c
> index e3f0cf23a3a9..3116036e6d24 100644
> --- a/drivers/gpu/drm/xe/xe_vm_madvise.c
> +++ b/drivers/gpu/drm/xe/xe_vm_madvise.c
> @@ -149,16 +149,15 @@ static void madvise_pat_index(struct xe_device
> *xe, struct xe_vm *vm,
>  	xe_assert(vm->xe, op->type == DRM_XE_MEM_RANGE_ATTR_PAT);
>  
>  	for (i = 0; i < num_vmas; i++) {
> -		/*TODO : Pass different pat_indexes from ioctl */
> -		if (vmas[i]->attr.pat_index.smem == op-
> >pat_index.val &&
> -		    vmas[i]->attr.pat_index.devmem == op-
> >pat_index.val &&
> -		    vmas[i]->attr.pat_index.remote == op-
> >pat_index.val) {
> +		if (vmas[i]->attr.pat_index.smem == op-
> >pat_index.smem &&
> +		    vmas[i]->attr.pat_index.devmem == op-
> >pat_index.devmem &&
> +		    vmas[i]->attr.pat_index.remote == op-
> >pat_index.remote) {
>  			vmas[i]->skip_invalidation = true;
>  		} else {
>  			vmas[i]->skip_invalidation = false;
> -			vmas[i]->attr.pat_index.smem = op-
> >pat_index.val;
> -			vmas[i]->attr.pat_index.devmem = op-
> >pat_index.val;
> -			vmas[i]->attr.pat_index.remote =  op-
> >pat_index.val;
> +			vmas[i]->attr.pat_index.smem = op-
> >pat_index.smem;
> +			vmas[i]->attr.pat_index.devmem = op-
> >pat_index.devmem;
> +			vmas[i]->attr.pat_index.remote =  op-
> >pat_index.remote;
>  		}
>  	}
>  }
> @@ -273,12 +272,26 @@ static bool madvise_args_are_sane(struct
> xe_device *xe, const struct drm_xe_madv
>  		break;
>  	case DRM_XE_MEM_RANGE_ATTR_PAT:
>  	{
> -		u16 coh_mode = xe_pat_index_get_coh_mode(xe, args-
> >pat_index.val);
> +		u16 coh_mode_smem = xe_pat_index_get_coh_mode(xe,
> args->pat_index.smem);
> +		u16 coh_mode_devmem = xe_pat_index_get_coh_mode(xe,
> args->pat_index.devmem);
> +		u16 coh_mode_remote = xe_pat_index_get_coh_mode(xe,
> args->pat_index.remote);
>  
> -		if (XE_IOCTL_DBG(xe, !coh_mode))
> +		if (XE_IOCTL_DBG(xe, !coh_mode_smem))
>  			return false;
>  
> -		if (XE_WARN_ON(coh_mode > XE_COH_AT_LEAST_1WAY))
> +		if (XE_IOCTL_DBG(xe, !coh_mode_devmem))
> +			return false;
> +
> +		if (XE_IOCTL_DBG(xe, !coh_mode_remote))
> +			return false;
> +
> +		if (XE_WARN_ON(coh_mode_smem >
> XE_COH_AT_LEAST_1WAY))
> +			return false;
> +
> +		if (XE_WARN_ON(coh_mode_devmem >
> XE_COH_AT_LEAST_1WAY))
> +			return false;
> +
> +		if (XE_WARN_ON(coh_mode_remote >
> XE_COH_AT_LEAST_1WAY))
>  			return false;
>  
>  		if (XE_IOCTL_DBG(xe, args->pat_index.pad))
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 40ff19f52a8d..09d9a9cf02e8 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -2101,11 +2101,26 @@ struct drm_xe_madvise {
>  		 * Used when @type == DRM_XE_MEM_RANGE_ATTR_PAT.
>  		 */
>  		struct {
> -			/** @pat_index.val: PAT index value */
> -			__u32 val;
> +			/**
> +			 * @pat_index.smem: PAT index value to be
> used when
> +			 * location is system memory during GPU
> access time.
> +			 */
> +			__u16 smem;
> +
> +			/**
> +			 * @pat_index.devmem: PAT index value to be
> used when
> +			 * location is local vram during GPU access
> time.
> +			 */
> +			__u16 devmem;
> +
> +			/**
> +			 * @pat_index.remote: PAT index value to be
> used when
> +			 * location is remote gpu vram during GPU
> access time.
> +			 */
> +			__u16 remote;
>  
>  			/** @pat_index.pad: MBZ */
> -			__u32 pad;
> +			__u16 pad;
>  
>  			/** @pat_index.reserved: Reserved */
>  			__u64 reserved;
> @@ -2163,11 +2178,26 @@ struct drm_xe_mem_range_attr {
>  
>  	 /** @pat_index: Page attribute table index */
>  	struct {
> -		/** @pat_index.val: PAT index */
> -		__u32 val;
> +		/**
> +		 * @pat_index.smem: PAT index value to be used when
> +		 * location is system memory during GPU access time.
> +		 */
> +		__u16 smem;
> +
> +		/**
> +		 * @pat_index.devmem: PAT index value to be used
> when
> +		 * location is local vram during GPU access time.
> +		 */
> +		__u16 devmem;
> +
> +		/**
> +		 * @pat_index.remote: PAT index value to be used
> when
> +		 * location is remote gpu tiles during GPU access
> time.
> +		 */
> +		__u16 remote;
>  
>  		/** @pat_index.reserved: Reserved */
> -		__u32 reserved;
> +		__u16 reserved;
>  	} pat_index;
>  
>  	/** @reserved: Reserved */