[PATCH 0/4] drm/xe: Support optional pinning of userptr pages

[PATCH 0/4] drm/xe: Support optional pinning of userptr pages

[Thomas Hellström]

This series add a flag at VM_BIND time to pin the memory backing a VMA.
Initially this is needed for long-running workloads on hardware that
neither support mid-thread preemption nor pagefaults, since without it
the userptr MMU notifier will wait for preemption until preemption times
out.

Moving forward this could be supported also for bo-backed VMAs given
a proper accounting takes place. A sysadmin could then optionally configure
a system to be optimized for dealing with a single GPU application
at a time.

The series will be followed up with an igt series to exercise the uAPI.

Thomas Hellström (4):
  drm/xe/vm: Use onion unwind for xe_vma_userptr_pin_pages()
  drm/xe/vm: Implement userptr page pinning
  drm/xe/vm: Perform accounting of userptr pinned pages
  drm/xe/uapi: Support pinning of userptr vmas

 drivers/gpu/drm/xe/xe_vm.c       | 181 +++++++++++++++++++++++--------
 drivers/gpu/drm/xe/xe_vm.h       |   9 ++
 drivers/gpu/drm/xe/xe_vm_types.h |  14 +++
 include/uapi/drm/xe_drm.h        |  18 +++
 4 files changed, 176 insertions(+), 46 deletions(-)

-- 
2.41.0

[PATCH 1/4] drm/xe/vm: Use onion unwind for xe_vma_userptr_pin_pages()

[Thomas Hellström]

Use onion error unwind since that makes the function easier to read
and extend. No functional change.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_vm.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 2e99f865d7ec..8bf7f62e6548 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -116,19 +116,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 		kthread_unuse_mm(vma->userptr.notifier.mm);
 		mmput(vma->userptr.notifier.mm);
 	}
-mm_closed:
 	if (ret)
-		goto out;
+		goto out_release_pages;
 
 	ret = sg_alloc_table_from_pages_segment(&vma->userptr.sgt, pages,
 						pinned, 0,
 						(u64)pinned << PAGE_SHIFT,
 						xe_sg_segment_size(xe->drm.dev),
 						GFP_KERNEL);
-	if (ret) {
-		vma->userptr.sg = NULL;
-		goto out;
-	}
+	if (ret)
+		goto out_release_pages;
+
 	vma->userptr.sg = &vma->userptr.sgt;
 
 	ret = dma_map_sgtable(xe->drm.dev, vma->userptr.sg,
@@ -136,11 +134,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 			      DMA_BIDIRECTIONAL,
 			      DMA_ATTR_SKIP_CPU_SYNC |
 			      DMA_ATTR_NO_KERNEL_MAPPING);
-	if (ret) {
-		sg_free_table(vma->userptr.sg);
-		vma->userptr.sg = NULL;
-		goto out;
-	}
+	if (ret)
+		goto out_free_sg;
 
 	for (i = 0; i < pinned; ++i) {
 		if (!read_only) {
@@ -152,17 +147,23 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 		mark_page_accessed(pages[i]);
 	}
 
-out:
 	release_pages(pages, pinned);
 	kvfree(pages);
 
-	if (!(ret < 0)) {
-		vma->userptr.notifier_seq = notifier_seq;
-		if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
-			goto retry;
-	}
+	vma->userptr.notifier_seq = notifier_seq;
+	if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
+		goto retry;
+
+	return 0;
 
-	return ret < 0 ? ret : 0;
+out_free_sg:
+	sg_free_table(vma->userptr.sg);
+	vma->userptr.sg = NULL;
+out_release_pages:
+	release_pages(pages, pinned);
+mm_closed:
+	kvfree(pages);
+	return ret;
 }
 
 static bool preempt_fences_waiting(struct xe_vm *vm)
-- 
2.41.0

Re: [PATCH 1/4] drm/xe/vm: Use onion unwind for xe_vma_userptr_pin_pages()

[Matthew Brost]

On Fri, Aug 18, 2023 at 05:08:42PM +0200, Thomas Hellström wrote:
> Use onion error unwind since that makes the function easier to read
> and extend. No functional change.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_vm.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 2e99f865d7ec..8bf7f62e6548 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -116,19 +116,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  		kthread_unuse_mm(vma->userptr.notifier.mm);
>  		mmput(vma->userptr.notifier.mm);
>  	}
> -mm_closed:
>  	if (ret)
> -		goto out;
> +		goto out_release_pages;
>  
>  	ret = sg_alloc_table_from_pages_segment(&vma->userptr.sgt, pages,
>  						pinned, 0,
>  						(u64)pinned << PAGE_SHIFT,
>  						xe_sg_segment_size(xe->drm.dev),
>  						GFP_KERNEL);
> -	if (ret) {
> -		vma->userptr.sg = NULL;
> -		goto out;
> -	}
> +	if (ret)
> +		goto out_release_pages;
> +
>  	vma->userptr.sg = &vma->userptr.sgt;
>  
>  	ret = dma_map_sgtable(xe->drm.dev, vma->userptr.sg,
> @@ -136,11 +134,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  			      DMA_BIDIRECTIONAL,
>  			      DMA_ATTR_SKIP_CPU_SYNC |
>  			      DMA_ATTR_NO_KERNEL_MAPPING);
> -	if (ret) {
> -		sg_free_table(vma->userptr.sg);
> -		vma->userptr.sg = NULL;
> -		goto out;
> -	}
> +	if (ret)
> +		goto out_free_sg;
>  
>  	for (i = 0; i < pinned; ++i) {
>  		if (!read_only) {
> @@ -152,17 +147,23 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  		mark_page_accessed(pages[i]);
>  	}
>  
> -out:
>  	release_pages(pages, pinned);
>  	kvfree(pages);
>  
> -	if (!(ret < 0)) {
> -		vma->userptr.notifier_seq = notifier_seq;
> -		if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
> -			goto retry;
> -	}
> +	vma->userptr.notifier_seq = notifier_seq;
> +	if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
> +		goto retry;
> +
> +	return 0;
>  
> -	return ret < 0 ? ret : 0;
> +out_free_sg:
> +	sg_free_table(vma->userptr.sg);
> +	vma->userptr.sg = NULL;
> +out_release_pages:
> +	release_pages(pages, pinned);
> +mm_closed:
> +	kvfree(pages);
> +	return ret;
>  }
>  
>  static bool preempt_fences_waiting(struct xe_vm *vm)
> -- 
> 2.41.0
> 

[PATCH 2/4] drm/xe/vm: Implement userptr page pinning

[Thomas Hellström]

Implement pinning of userptrs between VM_BIND and VM_UNBIND, which will
facilitate avoiding long hangs on non-preemptible workloads. But don't
hook it up to userspace just yet.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_vm.c       | 76 ++++++++++++++++++++++----------
 drivers/gpu/drm/xe/xe_vm.h       |  9 ++++
 drivers/gpu/drm/xe/xe_vm_types.h | 12 +++++
 3 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 8bf7f62e6548..ecbcad696b60 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -74,10 +74,6 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 	if (notifier_seq == vma->userptr.notifier_seq)
 		return 0;
 
-	pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
-	if (!pages)
-		return -ENOMEM;
-
 	if (vma->userptr.sg) {
 		dma_unmap_sgtable(xe->drm.dev,
 				  vma->userptr.sg,
@@ -87,6 +83,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 		vma->userptr.sg = NULL;
 	}
 
+	if (vma->userptr.pinned_pages) {
+		unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
+					    vma->userptr.num_pinned,
+					    !read_only);
+		pages = vma->userptr.pinned_pages;
+	} else {
+		pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
+		if (!pages)
+			return -ENOMEM;
+	}
+
 	pinned = ret = 0;
 	if (in_kthread) {
 		if (!mmget_not_zero(vma->userptr.notifier.mm)) {
@@ -97,11 +104,18 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 	}
 
 	while (pinned < num_pages) {
-		ret = get_user_pages_fast(xe_vma_userptr(vma) +
-					  pinned * PAGE_SIZE,
-					  num_pages - pinned,
-					  read_only ? 0 : FOLL_WRITE,
-					  &pages[pinned]);
+		if (xe_vma_is_pinned(vma))
+			ret = pin_user_pages_fast(xe_vma_userptr(vma) +
+						  pinned * PAGE_SIZE,
+						  num_pages - pinned,
+						  read_only ? 0 : FOLL_WRITE,
+						  &pages[pinned]);
+		else
+			ret = get_user_pages_fast(xe_vma_userptr(vma) +
+						  pinned * PAGE_SIZE,
+						  num_pages - pinned,
+						  read_only ? 0 : FOLL_WRITE,
+						  &pages[pinned]);
 		if (ret < 0) {
 			if (in_kthread)
 				ret = 0;
@@ -137,19 +151,24 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 	if (ret)
 		goto out_free_sg;
 
-	for (i = 0; i < pinned; ++i) {
-		if (!read_only) {
-			lock_page(pages[i]);
-			set_page_dirty(pages[i]);
-			unlock_page(pages[i]);
+	if (!xe_vma_is_pinned(vma)) {
+		for (i = 0; i < pinned; ++i) {
+			if (!read_only) {
+				lock_page(pages[i]);
+				set_page_dirty(pages[i]);
+				unlock_page(pages[i]);
+			}
+
+			mark_page_accessed(pages[i]);
 		}
 
-		mark_page_accessed(pages[i]);
+		release_pages(pages, pinned);
+		kvfree(pages);
+	} else {
+		vma->userptr.pinned_pages = pages;
+		vma->userptr.num_pinned = pinned;
 	}
 
-	release_pages(pages, pinned);
-	kvfree(pages);
-
 	vma->userptr.notifier_seq = notifier_seq;
 	if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
 		goto retry;
@@ -160,9 +179,14 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 	sg_free_table(vma->userptr.sg);
 	vma->userptr.sg = NULL;
 out_release_pages:
-	release_pages(pages, pinned);
+	if (!xe_vma_is_pinned(vma))
+		release_pages(pages, pinned);
+	else
+		unpin_user_pages(pages, pinned);
+	vma->userptr.num_pinned = 0;
 mm_closed:
 	kvfree(pages);
+	vma->userptr.pinned_pages = NULL;
 	return ret;
 }
 
@@ -721,7 +745,7 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni,
 	mmu_interval_set_seq(mni, cur_seq);
 
 	/* No need to stop gpu access if the userptr is not yet bound. */
-	if (!vma->userptr.initial_bind) {
+	if (xe_vma_is_pinned(vma) || !vma->userptr.initial_bind) {
 		up_write(&vm->userptr.notifier_lock);
 		return true;
 	}
@@ -976,10 +1000,16 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
 			vma->userptr.sg = NULL;
 		}
 
+		if (vma->userptr.pinned_pages) {
+			unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
+						    vma->userptr.num_pinned,
+						    !read_only);
+			kvfree(vma->userptr.pinned_pages);
+		}
+
 		/*
-		 * Since userptr pages are not pinned, we can't remove
-		 * the notifer until we're sure the GPU is not accessing
-		 * them anymore
+		 * We can't remove the notifer until we're sure the GPU is
+		 * not accessing the pages anymore
 		 */
 		mmu_interval_notifier_remove(&vma->userptr.notifier);
 		xe_vm_put(vm);
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 6de6e3edb24a..913544d7d995 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -139,6 +139,15 @@ static inline bool xe_vma_is_userptr(struct xe_vma *vma)
 	return xe_vma_has_no_bo(vma) && !xe_vma_is_null(vma);
 }
 
+/**
+ * xe_vma_is_pinned() - User has requested the backing store of this vma
+ * to be pinned.
+ */
+static inline bool xe_vma_is_pinned(struct xe_vma *vma)
+{
+	return xe_vma_is_userptr(vma) && (vma->gpuva.flags & XE_VMA_PINNED);
+}
+
 #define xe_vm_assert_held(vm) dma_resv_assert_held(&(vm)->resv)
 
 u64 xe_vm_pdp4_descriptor(struct xe_vm *vm, struct xe_tile *tile);
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 3681a5ff588b..9b90e649cd69 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -33,6 +33,8 @@ struct xe_vm;
 #define XE_VMA_PTE_4K		(DRM_GPUVA_USERBITS << 5)
 #define XE_VMA_PTE_2M		(DRM_GPUVA_USERBITS << 6)
 #define XE_VMA_PTE_1G		(DRM_GPUVA_USERBITS << 7)
+/* User requested backing store to be pinned */
+#define XE_VMA_PINNED           (DRM_GPUVA_USERBITS << 8)
 
 /** struct xe_userptr - User pointer */
 struct xe_userptr {
@@ -54,6 +56,16 @@ struct xe_userptr {
 	 * read: vm->userptr.notifier_lock in write mode or vm->resv held.
 	 */
 	bool initial_bind;
+	/**
+	 * @pinned_pages: List of pinned pages if xe_vma_pinned(),
+	 * NULL otherwise. protected by the vm lock.
+	 */
+	struct page **pinned_pages;
+	/**
+	 * @num_pinned: Number of pointers to pinned pages in @pinned_pages.
+	 * protected by the vm lock.
+	 */
+	unsigned long num_pinned;
 #if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
 	u32 divisor;
 #endif
-- 
2.41.0

Re: [PATCH 2/4] drm/xe/vm: Implement userptr page pinning

[Matthew Brost]

On Fri, Aug 18, 2023 at 05:08:43PM +0200, Thomas Hellström wrote:
> Implement pinning of userptrs between VM_BIND and VM_UNBIND, which will
> facilitate avoiding long hangs on non-preemptible workloads. But don't
> hook it up to userspace just yet.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vm.c       | 76 ++++++++++++++++++++++----------
>  drivers/gpu/drm/xe/xe_vm.h       |  9 ++++
>  drivers/gpu/drm/xe/xe_vm_types.h | 12 +++++
>  3 files changed, 74 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 8bf7f62e6548..ecbcad696b60 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -74,10 +74,6 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  	if (notifier_seq == vma->userptr.notifier_seq)
>  		return 0;
>  
> -	pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
> -	if (!pages)
> -		return -ENOMEM;
> -
>  	if (vma->userptr.sg) {
>  		dma_unmap_sgtable(xe->drm.dev,
>  				  vma->userptr.sg,
> @@ -87,6 +83,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  		vma->userptr.sg = NULL;
>  	}
>  
> +	if (vma->userptr.pinned_pages) {
> +		unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
> +					    vma->userptr.num_pinned,
> +					    !read_only);
> +		pages = vma->userptr.pinned_pages;

This implies that we can repin already pinned pages, I don't think that
should be possible, right? We shouldn't call this function twice nor
should the retry loop be trigger - both of these cases require a
invalidation to occur which shouldn't be possible if the pages are
pinned. So we likely should have warning if vma->userptr.pinned_pages is
set, right?

> +	} else {
> +		pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
> +		if (!pages)
> +			return -ENOMEM;
> +	}
> +
>  	pinned = ret = 0;
>  	if (in_kthread) {
>  		if (!mmget_not_zero(vma->userptr.notifier.mm)) {
> @@ -97,11 +104,18 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  	}
>  
>  	while (pinned < num_pages) {
> -		ret = get_user_pages_fast(xe_vma_userptr(vma) +
> -					  pinned * PAGE_SIZE,
> -					  num_pages - pinned,
> -					  read_only ? 0 : FOLL_WRITE,
> -					  &pages[pinned]);
> +		if (xe_vma_is_pinned(vma))
> +			ret = pin_user_pages_fast(xe_vma_userptr(vma) +
> +						  pinned * PAGE_SIZE,
> +						  num_pages - pinned,
> +						  read_only ? 0 : FOLL_WRITE,
> +						  &pages[pinned]);
> +		else
> +			ret = get_user_pages_fast(xe_vma_userptr(vma) +
> +						  pinned * PAGE_SIZE,
> +						  num_pages - pinned,
> +						  read_only ? 0 : FOLL_WRITE,
> +						  &pages[pinned]);
>  		if (ret < 0) {
>  			if (in_kthread)
>  				ret = 0;
> @@ -137,19 +151,24 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  	if (ret)
>  		goto out_free_sg;
>  
> -	for (i = 0; i < pinned; ++i) {
> -		if (!read_only) {
> -			lock_page(pages[i]);
> -			set_page_dirty(pages[i]);
> -			unlock_page(pages[i]);
> +	if (!xe_vma_is_pinned(vma)) {
> +		for (i = 0; i < pinned; ++i) {
> +			if (!read_only) {
> +				lock_page(pages[i]);
> +				set_page_dirty(pages[i]);
> +				unlock_page(pages[i]);
> +			}
> +
> +			mark_page_accessed(pages[i]);
>  		}
>  
> -		mark_page_accessed(pages[i]);
> +		release_pages(pages, pinned);
> +		kvfree(pages);
> +	} else {
> +		vma->userptr.pinned_pages = pages;
> +		vma->userptr.num_pinned = pinned;
>  	}
>  
> -	release_pages(pages, pinned);
> -	kvfree(pages);
> -
>  	vma->userptr.notifier_seq = notifier_seq;
>  	if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
>  		goto retry;
> @@ -160,9 +179,14 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  	sg_free_table(vma->userptr.sg);
>  	vma->userptr.sg = NULL;
>  out_release_pages:
> -	release_pages(pages, pinned);
> +	if (!xe_vma_is_pinned(vma))
> +		release_pages(pages, pinned);
> +	else
> +		unpin_user_pages(pages, pinned);
> +	vma->userptr.num_pinned = 0;
>  mm_closed:
>  	kvfree(pages);
> +	vma->userptr.pinned_pages = NULL;
>  	return ret;
>  }
>  
> @@ -721,7 +745,7 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni,
>  	mmu_interval_set_seq(mni, cur_seq);
>  
>  	/* No need to stop gpu access if the userptr is not yet bound. */
> -	if (!vma->userptr.initial_bind) {
> +	if (xe_vma_is_pinned(vma) || !vma->userptr.initial_bind) {
>  		up_write(&vm->userptr.notifier_lock);
>  		return true;
>  	}
> @@ -976,10 +1000,16 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>  			vma->userptr.sg = NULL;
>  		}
>  
> +		if (vma->userptr.pinned_pages) {
> +			unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
> +						    vma->userptr.num_pinned,
> +						    !read_only);
> +			kvfree(vma->userptr.pinned_pages);
> +		}
> +
>  		/*
> -		 * Since userptr pages are not pinned, we can't remove
> -		 * the notifer until we're sure the GPU is not accessing
> -		 * them anymore
> +		 * We can't remove the notifer until we're sure the GPU is
> +		 * not accessing the pages anymore
>  		 */
>  		mmu_interval_notifier_remove(&vma->userptr.notifier);
>  		xe_vm_put(vm);
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 6de6e3edb24a..913544d7d995 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -139,6 +139,15 @@ static inline bool xe_vma_is_userptr(struct xe_vma *vma)
>  	return xe_vma_has_no_bo(vma) && !xe_vma_is_null(vma);
>  }
>  
> +/**
> + * xe_vma_is_pinned() - User has requested the backing store of this vma
> + * to be pinned.
> + */
> +static inline bool xe_vma_is_pinned(struct xe_vma *vma)
> +{
> +	return xe_vma_is_userptr(vma) && (vma->gpuva.flags & XE_VMA_PINNED);

Nit, I'd name this xe_vma_is_userptr_pinned based checking both userptr
and pinned. Or just check XE_VMA_PINNED here with the current name.

Matt

> +}
> +
>  #define xe_vm_assert_held(vm) dma_resv_assert_held(&(vm)->resv)
>  
>  u64 xe_vm_pdp4_descriptor(struct xe_vm *vm, struct xe_tile *tile);
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> index 3681a5ff588b..9b90e649cd69 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -33,6 +33,8 @@ struct xe_vm;
>  #define XE_VMA_PTE_4K		(DRM_GPUVA_USERBITS << 5)
>  #define XE_VMA_PTE_2M		(DRM_GPUVA_USERBITS << 6)
>  #define XE_VMA_PTE_1G		(DRM_GPUVA_USERBITS << 7)
> +/* User requested backing store to be pinned */
> +#define XE_VMA_PINNED           (DRM_GPUVA_USERBITS << 8)
>  
>  /** struct xe_userptr - User pointer */
>  struct xe_userptr {
> @@ -54,6 +56,16 @@ struct xe_userptr {
>  	 * read: vm->userptr.notifier_lock in write mode or vm->resv held.
>  	 */
>  	bool initial_bind;
> +	/**
> +	 * @pinned_pages: List of pinned pages if xe_vma_pinned(),
> +	 * NULL otherwise. protected by the vm lock.
> +	 */
> +	struct page **pinned_pages;
> +	/**
> +	 * @num_pinned: Number of pointers to pinned pages in @pinned_pages.
> +	 * protected by the vm lock.
> +	 */
> +	unsigned long num_pinned;
>  #if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
>  	u32 divisor;
>  #endif
> -- 
> 2.41.0
> 

Re: [PATCH 2/4] drm/xe/vm: Implement userptr page pinning

[Thomas Hellström]

On 8/20/23 06:06, Matthew Brost wrote:
> On Fri, Aug 18, 2023 at 05:08:43PM +0200, Thomas Hellström wrote:
>> Implement pinning of userptrs between VM_BIND and VM_UNBIND, which will
>> facilitate avoiding long hangs on non-preemptible workloads. But don't
>> hook it up to userspace just yet.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_vm.c       | 76 ++++++++++++++++++++++----------
>>   drivers/gpu/drm/xe/xe_vm.h       |  9 ++++
>>   drivers/gpu/drm/xe/xe_vm_types.h | 12 +++++
>>   3 files changed, 74 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 8bf7f62e6548..ecbcad696b60 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -74,10 +74,6 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	if (notifier_seq == vma->userptr.notifier_seq)
>>   		return 0;
>>   
>> -	pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
>> -	if (!pages)
>> -		return -ENOMEM;
>> -
>>   	if (vma->userptr.sg) {
>>   		dma_unmap_sgtable(xe->drm.dev,
>>   				  vma->userptr.sg,
>> @@ -87,6 +83,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   		vma->userptr.sg = NULL;
>>   	}
>>   
>> +	if (vma->userptr.pinned_pages) {
>> +		unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
>> +					    vma->userptr.num_pinned,
>> +					    !read_only);
>> +		pages = vma->userptr.pinned_pages;
> This implies that we can repin already pinned pages, I don't think that
> should be possible, right? We shouldn't call this function twice nor
> should the retry loop be trigger - both of these cases require a
> invalidation to occur which shouldn't be possible if the pages are
> pinned. So we likely should have warning if vma->userptr.pinned_pages is
> set, right?

Good catch. Currently since we still allow the userptr sequence number 
to be bumped, the next exec will release the old pages and pin new pages 
(which may be the same), but the GPU might still be accessing the old 
pages. Need to make sure this doesn't happen.

/Thomas

>> +	} else {
>> +		pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
>> +		if (!pages)
>> +			return -ENOMEM;
>> +	}
>> +
>>   	pinned = ret = 0;
>>   	if (in_kthread) {
>>   		if (!mmget_not_zero(vma->userptr.notifier.mm)) {
>> @@ -97,11 +104,18 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	}
>>   
>>   	while (pinned < num_pages) {
>> -		ret = get_user_pages_fast(xe_vma_userptr(vma) +
>> -					  pinned * PAGE_SIZE,
>> -					  num_pages - pinned,
>> -					  read_only ? 0 : FOLL_WRITE,
>> -					  &pages[pinned]);
>> +		if (xe_vma_is_pinned(vma))
>> +			ret = pin_user_pages_fast(xe_vma_userptr(vma) +
>> +						  pinned * PAGE_SIZE,
>> +						  num_pages - pinned,
>> +						  read_only ? 0 : FOLL_WRITE,
>> +						  &pages[pinned]);
>> +		else
>> +			ret = get_user_pages_fast(xe_vma_userptr(vma) +
>> +						  pinned * PAGE_SIZE,
>> +						  num_pages - pinned,
>> +						  read_only ? 0 : FOLL_WRITE,
>> +						  &pages[pinned]);
>>   		if (ret < 0) {
>>   			if (in_kthread)
>>   				ret = 0;
>> @@ -137,19 +151,24 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	if (ret)
>>   		goto out_free_sg;
>>   
>> -	for (i = 0; i < pinned; ++i) {
>> -		if (!read_only) {
>> -			lock_page(pages[i]);
>> -			set_page_dirty(pages[i]);
>> -			unlock_page(pages[i]);
>> +	if (!xe_vma_is_pinned(vma)) {
>> +		for (i = 0; i < pinned; ++i) {
>> +			if (!read_only) {
>> +				lock_page(pages[i]);
>> +				set_page_dirty(pages[i]);
>> +				unlock_page(pages[i]);
>> +			}
>> +
>> +			mark_page_accessed(pages[i]);
>>   		}
>>   
>> -		mark_page_accessed(pages[i]);
>> +		release_pages(pages, pinned);
>> +		kvfree(pages);
>> +	} else {
>> +		vma->userptr.pinned_pages = pages;
>> +		vma->userptr.num_pinned = pinned;
>>   	}
>>   
>> -	release_pages(pages, pinned);
>> -	kvfree(pages);
>> -
>>   	vma->userptr.notifier_seq = notifier_seq;
>>   	if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
>>   		goto retry;
>> @@ -160,9 +179,14 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	sg_free_table(vma->userptr.sg);
>>   	vma->userptr.sg = NULL;
>>   out_release_pages:
>> -	release_pages(pages, pinned);
>> +	if (!xe_vma_is_pinned(vma))
>> +		release_pages(pages, pinned);
>> +	else
>> +		unpin_user_pages(pages, pinned);
>> +	vma->userptr.num_pinned = 0;
>>   mm_closed:
>>   	kvfree(pages);
>> +	vma->userptr.pinned_pages = NULL;
>>   	return ret;
>>   }
>>   
>> @@ -721,7 +745,7 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni,
>>   	mmu_interval_set_seq(mni, cur_seq);
>>   
>>   	/* No need to stop gpu access if the userptr is not yet bound. */
>> -	if (!vma->userptr.initial_bind) {
>> +	if (xe_vma_is_pinned(vma) || !vma->userptr.initial_bind) {
>>   		up_write(&vm->userptr.notifier_lock);
>>   		return true;
>>   	}
>> @@ -976,10 +1000,16 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>>   			vma->userptr.sg = NULL;
>>   		}
>>   
>> +		if (vma->userptr.pinned_pages) {
>> +			unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
>> +						    vma->userptr.num_pinned,
>> +						    !read_only);
>> +			kvfree(vma->userptr.pinned_pages);
>> +		}
>> +
>>   		/*
>> -		 * Since userptr pages are not pinned, we can't remove
>> -		 * the notifer until we're sure the GPU is not accessing
>> -		 * them anymore
>> +		 * We can't remove the notifer until we're sure the GPU is
>> +		 * not accessing the pages anymore
>>   		 */
>>   		mmu_interval_notifier_remove(&vma->userptr.notifier);
>>   		xe_vm_put(vm);
>> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
>> index 6de6e3edb24a..913544d7d995 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.h
>> +++ b/drivers/gpu/drm/xe/xe_vm.h
>> @@ -139,6 +139,15 @@ static inline bool xe_vma_is_userptr(struct xe_vma *vma)
>>   	return xe_vma_has_no_bo(vma) && !xe_vma_is_null(vma);
>>   }
>>   
>> +/**
>> + * xe_vma_is_pinned() - User has requested the backing store of this vma
>> + * to be pinned.
>> + */
>> +static inline bool xe_vma_is_pinned(struct xe_vma *vma)
>> +{
>> +	return xe_vma_is_userptr(vma) && (vma->gpuva.flags & XE_VMA_PINNED);
> Nit, I'd name this xe_vma_is_userptr_pinned based checking both userptr
> and pinned. Or just check XE_VMA_PINNED here with the current name.
>
> Matt
>
>> +}
>> +
>>   #define xe_vm_assert_held(vm) dma_resv_assert_held(&(vm)->resv)
>>   
>>   u64 xe_vm_pdp4_descriptor(struct xe_vm *vm, struct xe_tile *tile);
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
>> index 3681a5ff588b..9b90e649cd69 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -33,6 +33,8 @@ struct xe_vm;
>>   #define XE_VMA_PTE_4K		(DRM_GPUVA_USERBITS << 5)
>>   #define XE_VMA_PTE_2M		(DRM_GPUVA_USERBITS << 6)
>>   #define XE_VMA_PTE_1G		(DRM_GPUVA_USERBITS << 7)
>> +/* User requested backing store to be pinned */
>> +#define XE_VMA_PINNED           (DRM_GPUVA_USERBITS << 8)
>>   
>>   /** struct xe_userptr - User pointer */
>>   struct xe_userptr {
>> @@ -54,6 +56,16 @@ struct xe_userptr {
>>   	 * read: vm->userptr.notifier_lock in write mode or vm->resv held.
>>   	 */
>>   	bool initial_bind;
>> +	/**
>> +	 * @pinned_pages: List of pinned pages if xe_vma_pinned(),
>> +	 * NULL otherwise. protected by the vm lock.
>> +	 */
>> +	struct page **pinned_pages;
>> +	/**
>> +	 * @num_pinned: Number of pointers to pinned pages in @pinned_pages.
>> +	 * protected by the vm lock.
>> +	 */
>> +	unsigned long num_pinned;
>>   #if IS_ENABLED(CONFIG_DRM_XE_USERPTR_INVAL_INJECT)
>>   	u32 divisor;
>>   #endif
>> -- 
>> 2.41.0
>>

[PATCH 3/4] drm/xe/vm: Perform accounting of userptr pinned pages

[Thomas Hellström]

Account these pages against RLIMIT_MEMLOCK following how RDMA does this
with CAP_IPC_LOCK bypassing the limit.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_vm.c | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index ecbcad696b60..d9c000689002 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -34,6 +34,33 @@
 
 #define TEST_VM_ASYNC_OPS_ERROR
 
+/*
+ * Perform userptr PIN accounting against RLIMIT_MEMLOCK for now, similarly
+ * to how RDMA does this.
+ */
+static int xe_vma_mlock_alloc(struct xe_vma *vma, unsigned long num_pages)
+{
+	unsigned long lock_limit, new_pinned;
+	struct mm_struct *mm = vma->userptr.notifier.mm;
+
+	if (!can_do_mlock())
+		return -EPERM;
+
+	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
+	new_pinned = atomic64_add_return(num_pages, &mm->pinned_vm);
+	if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {
+		atomic64_sub(num_pages, &mm->pinned_vm);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void xe_vma_mlock_free(struct xe_vma *vma, unsigned long num_pages)
+{
+	atomic64_sub(num_pages, &vma->userptr.notifier.mm->pinned_vm);
+}
+
 /**
  * xe_vma_userptr_check_repin() - Advisory check for repin needed
  * @vma: The userptr vma
@@ -89,9 +116,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 					    !read_only);
 		pages = vma->userptr.pinned_pages;
 	} else {
+		if (xe_vma_is_pinned(vma)) {
+			ret = xe_vma_mlock_alloc(vma, num_pages);
+			if (ret)
+				return ret;
+		}
+
 		pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
-		if (!pages)
-			return -ENOMEM;
+		if (!pages) {
+			ret = -ENOMEM;
+			goto out_account;
+		}
 	}
 
 	pinned = ret = 0;
@@ -187,6 +222,9 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
 mm_closed:
 	kvfree(pages);
 	vma->userptr.pinned_pages = NULL;
+out_account:
+	if (xe_vma_is_pinned(vma))
+		xe_vma_mlock_free(vma, num_pages);
 	return ret;
 }
 
@@ -1004,6 +1042,7 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
 			unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
 						    vma->userptr.num_pinned,
 						    !read_only);
+			xe_vma_mlock_free(vma, xe_vma_size(vma) >> PAGE_SHIFT);
 			kvfree(vma->userptr.pinned_pages);
 		}
 
-- 
2.41.0

Re: [PATCH 3/4] drm/xe/vm: Perform accounting of userptr pinned pages

[Matthew Brost]

On Fri, Aug 18, 2023 at 05:08:44PM +0200, Thomas Hellström wrote:
> Account these pages against RLIMIT_MEMLOCK following how RDMA does this
> with CAP_IPC_LOCK bypassing the limit.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Patch LGTM but nits on naming + possible assert.

> ---
>  drivers/gpu/drm/xe/xe_vm.c | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index ecbcad696b60..d9c000689002 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -34,6 +34,33 @@
>  
>  #define TEST_VM_ASYNC_OPS_ERROR
>  
> +/*
> + * Perform userptr PIN accounting against RLIMIT_MEMLOCK for now, similarly
> + * to how RDMA does this.
> + */
> +static int xe_vma_mlock_alloc(struct xe_vma *vma, unsigned long num_pages)
> +{

xe_vma_userptr_mlock_alloc? or maybe even xe_vma_userptr_mlock_reserve?

> +	unsigned long lock_limit, new_pinned;
> +	struct mm_struct *mm = vma->userptr.notifier.mm;
> +

This be a candidate to use the new aseert macros to ensure that the vma
is a userptr + pinned? Not sure if that merged yet.

> +	if (!can_do_mlock())
> +		return -EPERM;
> +
> +	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> +	new_pinned = atomic64_add_return(num_pages, &mm->pinned_vm);
> +	if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {
> +		atomic64_sub(num_pages, &mm->pinned_vm);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static void xe_vma_mlock_free(struct xe_vma *vma, unsigned long num_pages)
> +{

xe_vma_userptr_mlock_free? or maybe even xe_vma_userptr_mlock_release?

Same for the assert here.

Anyways, I'll leave addressing these nits up to you, with that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +	atomic64_sub(num_pages, &vma->userptr.notifier.mm->pinned_vm);
> +}
> +
>  /**
>   * xe_vma_userptr_check_repin() - Advisory check for repin needed
>   * @vma: The userptr vma
> @@ -89,9 +116,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  					    !read_only);
>  		pages = vma->userptr.pinned_pages;
>  	} else {
> +		if (xe_vma_is_pinned(vma)) {
> +			ret = xe_vma_mlock_alloc(vma, num_pages);
> +			if (ret)
> +				return ret;
> +		}
> +
>  		pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
> -		if (!pages)
> -			return -ENOMEM;
> +		if (!pages) {
> +			ret = -ENOMEM;
> +			goto out_account;
> +		}
>  	}
>  
>  	pinned = ret = 0;
> @@ -187,6 +222,9 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>  mm_closed:
>  	kvfree(pages);
>  	vma->userptr.pinned_pages = NULL;
> +out_account:
> +	if (xe_vma_is_pinned(vma))
> +		xe_vma_mlock_free(vma, num_pages);
>  	return ret;
>  }
>  
> @@ -1004,6 +1042,7 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>  			unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
>  						    vma->userptr.num_pinned,
>  						    !read_only);
> +			xe_vma_mlock_free(vma, xe_vma_size(vma) >> PAGE_SHIFT);
>  			kvfree(vma->userptr.pinned_pages);
>  		}
>  
> -- 
> 2.41.0
> 

Re: [PATCH 3/4] drm/xe/vm: Perform accounting of userptr pinned pages

[Thomas Hellström]

On 8/20/23 05:43, Matthew Brost wrote:
> On Fri, Aug 18, 2023 at 05:08:44PM +0200, Thomas Hellström wrote:
>> Account these pages against RLIMIT_MEMLOCK following how RDMA does this
>> with CAP_IPC_LOCK bypassing the limit.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Patch LGTM but nits on naming + possible assert.
>
>> ---
>>   drivers/gpu/drm/xe/xe_vm.c | 43 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index ecbcad696b60..d9c000689002 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -34,6 +34,33 @@
>>   
>>   #define TEST_VM_ASYNC_OPS_ERROR
>>   
>> +/*
>> + * Perform userptr PIN accounting against RLIMIT_MEMLOCK for now, similarly
>> + * to how RDMA does this.
>> + */
>> +static int xe_vma_mlock_alloc(struct xe_vma *vma, unsigned long num_pages)
>> +{
> xe_vma_userptr_mlock_alloc? or maybe even xe_vma_userptr_mlock_reserve?
>
>> +	unsigned long lock_limit, new_pinned;
>> +	struct mm_struct *mm = vma->userptr.notifier.mm;
>> +
> This be a candidate to use the new aseert macros to ensure that the vma
> is a userptr + pinned? Not sure if that merged yet.
>
>> +	if (!can_do_mlock())
>> +		return -EPERM;
>> +
>> +	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> +	new_pinned = atomic64_add_return(num_pages, &mm->pinned_vm);
>> +	if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {
>> +		atomic64_sub(num_pages, &mm->pinned_vm);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void xe_vma_mlock_free(struct xe_vma *vma, unsigned long num_pages)
>> +{
> xe_vma_userptr_mlock_free? or maybe even xe_vma_userptr_mlock_release?
>
> Same for the assert here.
>
> Anyways, I'll leave addressing these nits up to you, with that:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>

OK, thanks. I'll take a look at addressing those.

>
>> +	atomic64_sub(num_pages, &vma->userptr.notifier.mm->pinned_vm);
>> +}
>> +
>>   /**
>>    * xe_vma_userptr_check_repin() - Advisory check for repin needed
>>    * @vma: The userptr vma
>> @@ -89,9 +116,17 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   					    !read_only);
>>   		pages = vma->userptr.pinned_pages;
>>   	} else {
>> +		if (xe_vma_is_pinned(vma)) {
>> +			ret = xe_vma_mlock_alloc(vma, num_pages);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>>   		pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
>> -		if (!pages)
>> -			return -ENOMEM;
>> +		if (!pages) {
>> +			ret = -ENOMEM;
>> +			goto out_account;
>> +		}
>>   	}
>>   
>>   	pinned = ret = 0;
>> @@ -187,6 +222,9 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   mm_closed:
>>   	kvfree(pages);
>>   	vma->userptr.pinned_pages = NULL;
>> +out_account:
>> +	if (xe_vma_is_pinned(vma))
>> +		xe_vma_mlock_free(vma, num_pages);
>>   	return ret;
>>   }
>>   
>> @@ -1004,6 +1042,7 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>>   			unpin_user_pages_dirty_lock(vma->userptr.pinned_pages,
>>   						    vma->userptr.num_pinned,
>>   						    !read_only);
>> +			xe_vma_mlock_free(vma, xe_vma_size(vma) >> PAGE_SHIFT);
>>   			kvfree(vma->userptr.pinned_pages);
>>   		}
>>   
>> -- 
>> 2.41.0
>>

[PATCH 4/4] drm/xe/uapi: Support pinning of userptr vmas

[Thomas Hellström]

Support pinning of vmas using XE_VM_BIND_FLAG_PIN, initially for userptr
only. Pinned memory becomes accounted against RLIMIT_MEMLOCK and processes
with CAP_IPC_LOCK will not apply the limit. This is pretty similar to
mlock()'ing userptr memory with the added benefit that the driver is
aware and can ignore some actions in the MMU invalidation notifier.

This will initially become useful for compute VMs on hardware without
mid-thread-preemption capability since with pinned pages, the MMU
invalidation notifier never tries to preempt a running compute kernel.

If that were the only usage we could restrict this to a flag that always
pins userptr VMAs on compute VMs on such hardware, but there are
indications that this may become needed in other situations as well.

From a more general point of view, the usage pattern of a system may be
such that in most cases it only ever runs a single workload per system
and then the sysadmin would want to configure the system to allow
extensive pinning for performance reasons.

Hence we might want to extend the pinning capability to bo-backed VMAs
as well. How that pinning will be accounted remains an open but to build
on the current drm CGROUP work would be an option.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_vm.c       | 33 +++++++++++++++++++++++++-------
 drivers/gpu/drm/xe/xe_vm_types.h |  2 ++
 include/uapi/drm/xe_drm.h        | 18 +++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index d9c000689002..3832f1f21def 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -936,6 +936,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
 				    u64 start, u64 end,
 				    bool read_only,
 				    bool is_null,
+				    bool pin,
 				    u8 tile_mask)
 {
 	struct xe_vma *vma;
@@ -967,6 +968,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
 		vma->gpuva.flags |= XE_VMA_READ_ONLY;
 	if (is_null)
 		vma->gpuva.flags |= DRM_GPUVA_SPARSE;
+	if (pin)
+		vma->gpuva.flags |= XE_VMA_PINNED;
 
 	if (tile_mask) {
 		vma->tile_mask = tile_mask;
@@ -2367,6 +2370,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
 			op->map.read_only =
 				operation & XE_VM_BIND_FLAG_READONLY;
 			op->map.is_null = operation & XE_VM_BIND_FLAG_NULL;
+			op->map.pin = operation & XE_VM_BIND_FLAG_PIN;
 		}
 		break;
 	case XE_VM_BIND_OP_UNMAP:
@@ -2431,7 +2435,8 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
 }
 
 static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
-			      u8 tile_mask, bool read_only, bool is_null)
+			      u8 tile_mask, bool read_only, bool is_null,
+			      bool pin)
 {
 	struct xe_bo *bo = op->gem.obj ? gem_to_xe_bo(op->gem.obj) : NULL;
 	struct xe_vma *vma;
@@ -2447,7 +2452,7 @@ static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
 	}
 	vma = xe_vma_create(vm, bo, op->gem.offset,
 			    op->va.addr, op->va.addr +
-			    op->va.range - 1, read_only, is_null,
+			    op->va.range - 1, read_only, is_null, pin,
 			    tile_mask);
 	if (bo)
 		xe_bo_unlock(bo, &ww);
@@ -2562,7 +2567,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
 
 				vma = new_vma(vm, &op->base.map,
 					      op->tile_mask, op->map.read_only,
-					      op->map.is_null);
+					      op->map.is_null, op->map.pin);
 				if (IS_ERR(vma)) {
 					err = PTR_ERR(vma);
 					goto free_fence;
@@ -2587,10 +2592,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
 					bool is_null =
 						op->base.remap.unmap->va->flags &
 						DRM_GPUVA_SPARSE;
+					bool pin =
+						op->base.remap.unmap->va->flags &
+						XE_VMA_PINNED;
 
 					vma = new_vma(vm, op->base.remap.prev,
 						      op->tile_mask, read_only,
-						      is_null);
+						      is_null, pin);
 					if (IS_ERR(vma)) {
 						err = PTR_ERR(vma);
 						goto free_fence;
@@ -2623,10 +2631,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
 					bool is_null =
 						op->base.remap.unmap->va->flags &
 						DRM_GPUVA_SPARSE;
+					bool pin =
+						op->base.remap.unmap->va->flags &
+						XE_VMA_PINNED;
 
 					vma = new_vma(vm, op->base.remap.next,
 						      op->tile_mask, read_only,
-						      is_null);
+						      is_null, pin);
 					if (IS_ERR(vma)) {
 						err = PTR_ERR(vma);
 						goto free_fence;
@@ -3131,11 +3142,12 @@ static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
 #define SUPPORTED_FLAGS	\
 	(FORCE_ASYNC_OP_ERROR | XE_VM_BIND_FLAG_ASYNC | \
 	 XE_VM_BIND_FLAG_READONLY | XE_VM_BIND_FLAG_IMMEDIATE | \
-	 XE_VM_BIND_FLAG_NULL | 0xffff)
+	 XE_VM_BIND_FLAG_NULL | XE_VM_BIND_FLAG_PIN | 0xffff)
 #else
 #define SUPPORTED_FLAGS	\
 	(XE_VM_BIND_FLAG_ASYNC | XE_VM_BIND_FLAG_READONLY | \
-	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | 0xffff)
+	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | \
+	 XE_VM_BIND_FLAG_PIN | 0xffff)
 #endif
 #define XE_64K_PAGE_MASK 0xffffull
 
@@ -3205,6 +3217,13 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
 			goto free_bind_ops;
 		}
 
+		/* TODO: Support OP_PREFETCH, OP_MAP */
+		if (XE_IOCTL_DBG(xe, (op & XE_VM_BIND_FLAG_PIN) &&
+				 VM_BIND_OP(op) != XE_VM_BIND_OP_MAP_USERPTR)) {
+			err = -EINVAL;
+			goto free_bind_ops;
+		}
+
 		if (XE_IOCTL_DBG(xe, VM_BIND_OP(op) >
 				 XE_VM_BIND_OP_PREFETCH) ||
 		    XE_IOCTL_DBG(xe, op & ~SUPPORTED_FLAGS) ||
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 9b90e649cd69..024ccabadd12 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -360,6 +360,8 @@ struct xe_vma_op_map {
 	bool read_only;
 	/** @is_null: is NULL binding */
 	bool is_null;
+	/** @pin: pin underlying memory */
+	bool pin;
 };
 
 /** struct xe_vma_op_remap - VMA remap operation */
diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index 86f16d50e9cc..fc3d9cd4f8d0 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -631,6 +631,24 @@ struct drm_xe_vm_bind_op {
 	 * intended to implement VK sparse bindings.
 	 */
 #define XE_VM_BIND_FLAG_NULL		(0x1 << 19)
+	 /*
+	  * When the PIN flag is set, the user requests the underlying
+	  * backing store of the vma to be pinned, that is, it will be
+	  * resident while bound and the underlying physical memory
+	  * will not change. For userptr VMAs this means that if the
+	  * user performs an operation that changes the underlying
+	  * pages of the CPU virtual space, the corresponding pinned
+	  * GPU virtual space will not pick up the new memory unless
+	  * an OP_UNMAP followed by a OP_MAP_USERPTR is performed.
+	  * Pinned userptr memory is accounted in the same way as
+	  * mlock(2), and if pinning fails the following error codes
+	  * may be returned:
+	  * -EINVAL: The memory region does not support pinning.
+	  * -EPERM: The process is not permitted to pin.
+	  * -ENOMEM: The pinning limit does not allow pinning.
+	  * For userptr memory, CAP_IPC_LOCK will bypass the limit checking.
+	  */
+#define XE_VM_BIND_FLAG_PIN		(0x1 << 20)
 	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
 	__u32 op;
 
-- 
2.41.0

Re: [PATCH 4/4] drm/xe/uapi: Support pinning of userptr vmas

[Matthew Brost]

On Fri, Aug 18, 2023 at 05:08:45PM +0200, Thomas Hellström wrote:
> Support pinning of vmas using XE_VM_BIND_FLAG_PIN, initially for userptr
> only. Pinned memory becomes accounted against RLIMIT_MEMLOCK and processes
> with CAP_IPC_LOCK will not apply the limit. This is pretty similar to
> mlock()'ing userptr memory with the added benefit that the driver is
> aware and can ignore some actions in the MMU invalidation notifier.
> 
> This will initially become useful for compute VMs on hardware without
> mid-thread-preemption capability since with pinned pages, the MMU
> invalidation notifier never tries to preempt a running compute kernel.
> 
> If that were the only usage we could restrict this to a flag that always
> pins userptr VMAs on compute VMs on such hardware, but there are
> indications that this may become needed in other situations as well.
> 
> From a more general point of view, the usage pattern of a system may be
> such that in most cases it only ever runs a single workload per system
> and then the sysadmin would want to configure the system to allow
> extensive pinning for performance reasons.
> 
> Hence we might want to extend the pinning capability to bo-backed VMAs
> as well. How that pinning will be accounted remains an open but to build
> on the current drm CGROUP work would be an option.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Patch LGTM but a few comments that are currently out of scope but want
to get out there for future work.

> ---
>  drivers/gpu/drm/xe/xe_vm.c       | 33 +++++++++++++++++++++++++-------
>  drivers/gpu/drm/xe/xe_vm_types.h |  2 ++
>  include/uapi/drm/xe_drm.h        | 18 +++++++++++++++++
>  3 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index d9c000689002..3832f1f21def 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -936,6 +936,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>  				    u64 start, u64 end,
>  				    bool read_only,
>  				    bool is_null,
> +				    bool pin,
>  				    u8 tile_mask)
>  {
>  	struct xe_vma *vma;
> @@ -967,6 +968,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>  		vma->gpuva.flags |= XE_VMA_READ_ONLY;
>  	if (is_null)
>  		vma->gpuva.flags |= DRM_GPUVA_SPARSE;
> +	if (pin)
> +		vma->gpuva.flags |= XE_VMA_PINNED;
>  
>  	if (tile_mask) {
>  		vma->tile_mask = tile_mask;
> @@ -2367,6 +2370,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
>  			op->map.read_only =
>  				operation & XE_VM_BIND_FLAG_READONLY;
>  			op->map.is_null = operation & XE_VM_BIND_FLAG_NULL;
> +			op->map.pin = operation & XE_VM_BIND_FLAG_PIN;
>  		}
>  		break;
>  	case XE_VM_BIND_OP_UNMAP:
> @@ -2431,7 +2435,8 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
>  }
>  
>  static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
> -			      u8 tile_mask, bool read_only, bool is_null)
> +			      u8 tile_mask, bool read_only, bool is_null,
> +			      bool pin)
>  {
>  	struct xe_bo *bo = op->gem.obj ? gem_to_xe_bo(op->gem.obj) : NULL;
>  	struct xe_vma *vma;
> @@ -2447,7 +2452,7 @@ static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
>  	}
>  	vma = xe_vma_create(vm, bo, op->gem.offset,
>  			    op->va.addr, op->va.addr +
> -			    op->va.range - 1, read_only, is_null,
> +			    op->va.range - 1, read_only, is_null, pin,
>  			    tile_mask);
>  	if (bo)
>  		xe_bo_unlock(bo, &ww);
> @@ -2562,7 +2567,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>  
>  				vma = new_vma(vm, &op->base.map,
>  					      op->tile_mask, op->map.read_only,
> -					      op->map.is_null);
> +					      op->map.is_null, op->map.pin);
>  				if (IS_ERR(vma)) {
>  					err = PTR_ERR(vma);
>  					goto free_fence;
> @@ -2587,10 +2592,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>  					bool is_null =
>  						op->base.remap.unmap->va->flags &
>  						DRM_GPUVA_SPARSE;
> +					bool pin =
> +						op->base.remap.unmap->va->flags &
> +						XE_VMA_PINNED;

We probably should move the read_only, is_null, and pin check out of the
next / prev if statements to just below the DRM_GPUVA_OP_REMAP case
statement. 

>  
>  					vma = new_vma(vm, op->base.remap.prev,
>  						      op->tile_mask, read_only,
> -						      is_null);
> +						      is_null, pin);
>  					if (IS_ERR(vma)) {
>  						err = PTR_ERR(vma);
>  						goto free_fence;
> @@ -2623,10 +2631,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>  					bool is_null =
>  						op->base.remap.unmap->va->flags &
>  						DRM_GPUVA_SPARSE;
> +					bool pin =
> +						op->base.remap.unmap->va->flags &
> +						XE_VMA_PINNED;
>  
>  					vma = new_vma(vm, op->base.remap.next,
>  						      op->tile_mask, read_only,
> -						      is_null);
> +						      is_null, pin);
>  					if (IS_ERR(vma)) {
>  						err = PTR_ERR(vma);
>  						goto free_fence;
> @@ -3131,11 +3142,12 @@ static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
>  #define SUPPORTED_FLAGS	\
>  	(FORCE_ASYNC_OP_ERROR | XE_VM_BIND_FLAG_ASYNC | \
>  	 XE_VM_BIND_FLAG_READONLY | XE_VM_BIND_FLAG_IMMEDIATE | \
> -	 XE_VM_BIND_FLAG_NULL | 0xffff)
> +	 XE_VM_BIND_FLAG_NULL | XE_VM_BIND_FLAG_PIN | 0xffff)
>  #else
>  #define SUPPORTED_FLAGS	\
>  	(XE_VM_BIND_FLAG_ASYNC | XE_VM_BIND_FLAG_READONLY | \
> -	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | 0xffff)
> +	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | \
> +	 XE_VM_BIND_FLAG_PIN | 0xffff)
>  #endif
>  #define XE_64K_PAGE_MASK 0xffffull
>  
> @@ -3205,6 +3217,13 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>  			goto free_bind_ops;
>  		}
>  
> +		/* TODO: Support OP_PREFETCH, OP_MAP */
> +		if (XE_IOCTL_DBG(xe, (op & XE_VM_BIND_FLAG_PIN) &&
> +				 VM_BIND_OP(op) != XE_VM_BIND_OP_MAP_USERPTR)) {
> +			err = -EINVAL;
> +			goto free_bind_ops;
> +		}
> +
>  		if (XE_IOCTL_DBG(xe, VM_BIND_OP(op) >
>  				 XE_VM_BIND_OP_PREFETCH) ||
>  		    XE_IOCTL_DBG(xe, op & ~SUPPORTED_FLAGS) ||
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> index 9b90e649cd69..024ccabadd12 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -360,6 +360,8 @@ struct xe_vma_op_map {
>  	bool read_only;
>  	/** @is_null: is NULL binding */
>  	bool is_null;
> +	/** @pin: pin underlying memory */
> +	bool pin;
>  };
>  
>  /** struct xe_vma_op_remap - VMA remap operation */
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 86f16d50e9cc..fc3d9cd4f8d0 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -631,6 +631,24 @@ struct drm_xe_vm_bind_op {
>  	 * intended to implement VK sparse bindings.
>  	 */
>  #define XE_VM_BIND_FLAG_NULL		(0x1 << 19)
> +	 /*
> +	  * When the PIN flag is set, the user requests the underlying
> +	  * backing store of the vma to be pinned, that is, it will be
> +	  * resident while bound and the underlying physical memory
> +	  * will not change. For userptr VMAs this means that if the
> +	  * user performs an operation that changes the underlying
> +	  * pages of the CPU virtual space, the corresponding pinned
> +	  * GPU virtual space will not pick up the new memory unless
> +	  * an OP_UNMAP followed by a OP_MAP_USERPTR is performed.
> +	  * Pinned userptr memory is accounted in the same way as
> +	  * mlock(2), and if pinning fails the following error codes
> +	  * may be returned:
> +	  * -EINVAL: The memory region does not support pinning.
> +	  * -EPERM: The process is not permitted to pin.
> +	  * -ENOMEM: The pinning limit does not allow pinning.
> +	  * For userptr memory, CAP_IPC_LOCK will bypass the limit checking.
> +	  */
> +#define XE_VM_BIND_FLAG_PIN		(0x1 << 20)

We are quickly using a lot of the upper bits, maybe we change the op
field to a __u64 soon? We have to break the VM bind api when removing
the async worker + updating sync mode to align with VM bind doc, maybe
we change this then too?

Anyways this patch LGTM:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
>  	__u32 op;
>  
> -- 
> 2.41.0
> 

Re: [PATCH 4/4] drm/xe/uapi: Support pinning of userptr vmas

[Thomas Hellström]

On 8/20/23 05:54, Matthew Brost wrote:
> On Fri, Aug 18, 2023 at 05:08:45PM +0200, Thomas Hellström wrote:
>> Support pinning of vmas using XE_VM_BIND_FLAG_PIN, initially for userptr
>> only. Pinned memory becomes accounted against RLIMIT_MEMLOCK and processes
>> with CAP_IPC_LOCK will not apply the limit. This is pretty similar to
>> mlock()'ing userptr memory with the added benefit that the driver is
>> aware and can ignore some actions in the MMU invalidation notifier.
>>
>> This will initially become useful for compute VMs on hardware without
>> mid-thread-preemption capability since with pinned pages, the MMU
>> invalidation notifier never tries to preempt a running compute kernel.
>>
>> If that were the only usage we could restrict this to a flag that always
>> pins userptr VMAs on compute VMs on such hardware, but there are
>> indications that this may become needed in other situations as well.
>>
>>  From a more general point of view, the usage pattern of a system may be
>> such that in most cases it only ever runs a single workload per system
>> and then the sysadmin would want to configure the system to allow
>> extensive pinning for performance reasons.
>>
>> Hence we might want to extend the pinning capability to bo-backed VMAs
>> as well. How that pinning will be accounted remains an open but to build
>> on the current drm CGROUP work would be an option.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Patch LGTM but a few comments that are currently out of scope but want
> to get out there for future work.
>
>> ---
>>   drivers/gpu/drm/xe/xe_vm.c       | 33 +++++++++++++++++++++++++-------
>>   drivers/gpu/drm/xe/xe_vm_types.h |  2 ++
>>   include/uapi/drm/xe_drm.h        | 18 +++++++++++++++++
>>   3 files changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index d9c000689002..3832f1f21def 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -936,6 +936,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   				    u64 start, u64 end,
>>   				    bool read_only,
>>   				    bool is_null,
>> +				    bool pin,
>>   				    u8 tile_mask)
>>   {
>>   	struct xe_vma *vma;
>> @@ -967,6 +968,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   		vma->gpuva.flags |= XE_VMA_READ_ONLY;
>>   	if (is_null)
>>   		vma->gpuva.flags |= DRM_GPUVA_SPARSE;
>> +	if (pin)
>> +		vma->gpuva.flags |= XE_VMA_PINNED;
>>   
>>   	if (tile_mask) {
>>   		vma->tile_mask = tile_mask;
>> @@ -2367,6 +2370,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
>>   			op->map.read_only =
>>   				operation & XE_VM_BIND_FLAG_READONLY;
>>   			op->map.is_null = operation & XE_VM_BIND_FLAG_NULL;
>> +			op->map.pin = operation & XE_VM_BIND_FLAG_PIN;
>>   		}
>>   		break;
>>   	case XE_VM_BIND_OP_UNMAP:
>> @@ -2431,7 +2435,8 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
>>   }
>>   
>>   static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
>> -			      u8 tile_mask, bool read_only, bool is_null)
>> +			      u8 tile_mask, bool read_only, bool is_null,
>> +			      bool pin)
>>   {
>>   	struct xe_bo *bo = op->gem.obj ? gem_to_xe_bo(op->gem.obj) : NULL;
>>   	struct xe_vma *vma;
>> @@ -2447,7 +2452,7 @@ static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
>>   	}
>>   	vma = xe_vma_create(vm, bo, op->gem.offset,
>>   			    op->va.addr, op->va.addr +
>> -			    op->va.range - 1, read_only, is_null,
>> +			    op->va.range - 1, read_only, is_null, pin,
>>   			    tile_mask);
>>   	if (bo)
>>   		xe_bo_unlock(bo, &ww);
>> @@ -2562,7 +2567,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>>   
>>   				vma = new_vma(vm, &op->base.map,
>>   					      op->tile_mask, op->map.read_only,
>> -					      op->map.is_null);
>> +					      op->map.is_null, op->map.pin);
>>   				if (IS_ERR(vma)) {
>>   					err = PTR_ERR(vma);
>>   					goto free_fence;
>> @@ -2587,10 +2592,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>>   					bool is_null =
>>   						op->base.remap.unmap->va->flags &
>>   						DRM_GPUVA_SPARSE;
>> +					bool pin =
>> +						op->base.remap.unmap->va->flags &
>> +						XE_VMA_PINNED;
> We probably should move the read_only, is_null, and pin check out of the
> next / prev if statements to just below the DRM_GPUVA_OP_REMAP case
> statement.
>
>>   
>>   					vma = new_vma(vm, op->base.remap.prev,
>>   						      op->tile_mask, read_only,
>> -						      is_null);
>> +						      is_null, pin);
>>   					if (IS_ERR(vma)) {
>>   						err = PTR_ERR(vma);
>>   						goto free_fence;
>> @@ -2623,10 +2631,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>>   					bool is_null =
>>   						op->base.remap.unmap->va->flags &
>>   						DRM_GPUVA_SPARSE;
>> +					bool pin =
>> +						op->base.remap.unmap->va->flags &
>> +						XE_VMA_PINNED;
>>   
>>   					vma = new_vma(vm, op->base.remap.next,
>>   						      op->tile_mask, read_only,
>> -						      is_null);
>> +						      is_null, pin);
>>   					if (IS_ERR(vma)) {
>>   						err = PTR_ERR(vma);
>>   						goto free_fence;
>> @@ -3131,11 +3142,12 @@ static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
>>   #define SUPPORTED_FLAGS	\
>>   	(FORCE_ASYNC_OP_ERROR | XE_VM_BIND_FLAG_ASYNC | \
>>   	 XE_VM_BIND_FLAG_READONLY | XE_VM_BIND_FLAG_IMMEDIATE | \
>> -	 XE_VM_BIND_FLAG_NULL | 0xffff)
>> +	 XE_VM_BIND_FLAG_NULL | XE_VM_BIND_FLAG_PIN | 0xffff)
>>   #else
>>   #define SUPPORTED_FLAGS	\
>>   	(XE_VM_BIND_FLAG_ASYNC | XE_VM_BIND_FLAG_READONLY | \
>> -	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | 0xffff)
>> +	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | \
>> +	 XE_VM_BIND_FLAG_PIN | 0xffff)
>>   #endif
>>   #define XE_64K_PAGE_MASK 0xffffull
>>   
>> @@ -3205,6 +3217,13 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>>   			goto free_bind_ops;
>>   		}
>>   
>> +		/* TODO: Support OP_PREFETCH, OP_MAP */
>> +		if (XE_IOCTL_DBG(xe, (op & XE_VM_BIND_FLAG_PIN) &&
>> +				 VM_BIND_OP(op) != XE_VM_BIND_OP_MAP_USERPTR)) {
>> +			err = -EINVAL;
>> +			goto free_bind_ops;
>> +		}
>> +
>>   		if (XE_IOCTL_DBG(xe, VM_BIND_OP(op) >
>>   				 XE_VM_BIND_OP_PREFETCH) ||
>>   		    XE_IOCTL_DBG(xe, op & ~SUPPORTED_FLAGS) ||
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
>> index 9b90e649cd69..024ccabadd12 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -360,6 +360,8 @@ struct xe_vma_op_map {
>>   	bool read_only;
>>   	/** @is_null: is NULL binding */
>>   	bool is_null;
>> +	/** @pin: pin underlying memory */
>> +	bool pin;
>>   };
>>   
>>   /** struct xe_vma_op_remap - VMA remap operation */
>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>> index 86f16d50e9cc..fc3d9cd4f8d0 100644
>> --- a/include/uapi/drm/xe_drm.h
>> +++ b/include/uapi/drm/xe_drm.h
>> @@ -631,6 +631,24 @@ struct drm_xe_vm_bind_op {
>>   	 * intended to implement VK sparse bindings.
>>   	 */
>>   #define XE_VM_BIND_FLAG_NULL		(0x1 << 19)
>> +	 /*
>> +	  * When the PIN flag is set, the user requests the underlying
>> +	  * backing store of the vma to be pinned, that is, it will be
>> +	  * resident while bound and the underlying physical memory
>> +	  * will not change. For userptr VMAs this means that if the
>> +	  * user performs an operation that changes the underlying
>> +	  * pages of the CPU virtual space, the corresponding pinned
>> +	  * GPU virtual space will not pick up the new memory unless
>> +	  * an OP_UNMAP followed by a OP_MAP_USERPTR is performed.
>> +	  * Pinned userptr memory is accounted in the same way as
>> +	  * mlock(2), and if pinning fails the following error codes
>> +	  * may be returned:
>> +	  * -EINVAL: The memory region does not support pinning.
>> +	  * -EPERM: The process is not permitted to pin.
>> +	  * -ENOMEM: The pinning limit does not allow pinning.
>> +	  * For userptr memory, CAP_IPC_LOCK will bypass the limit checking.
>> +	  */
>> +#define XE_VM_BIND_FLAG_PIN		(0x1 << 20)
> We are quickly using a lot of the upper bits, maybe we change the op
> field to a __u64 soon? We have to break the VM bind api when removing
> the async worker + updating sync mode to align with VM bind doc, maybe
> we change this then too?

What about a separate flags field?

/Thomas


>
> Anyways this patch LGTM:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
>>   	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
>>   	__u32 op;
>>   
>> -- 
>> 2.41.0
>>