[PATCH v6 01/20] drm/gpusvm: Introduce devmem_only flag for allocation

[PATCH v6 01/20] drm/gpusvm: Introduce devmem_only flag for allocation

[Himal Prasad Ghimiray]

This commit adds a new flag, devmem_only, to the drm_gpusvm structure. The
purpose of this flag is to ensure that the get_pages function allocates
memory exclusively from the device's memory. If the allocation from
device memory fails, the function will return an -EFAULT error.

Required for shared CPU and GPU atomics on certain devices.

v3:
 - s/vram_only/devmem_only/

Fixes: 99624bdff867 ("drm/gpusvm: Add support for GPU Shared Virtual Memory")
Cc: stable@vger.kernel.org
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 5 +++++
 include/drm/drm_gpusvm.h     | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index de424e670995..a58d03e6cac2 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1454,6 +1454,11 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
 				goto err_unmap;
 			}
 
+			if (ctx->devmem_only) {
+				err = -EFAULT;
+				goto err_unmap;
+			}
+
 			addr = dma_map_page(gpusvm->drm->dev,
 					    page, 0,
 					    PAGE_SIZE << order,
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index df120b4d1f83..9fd25fc880a4 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -286,6 +286,7 @@ struct drm_gpusvm {
  * @in_notifier: entering from a MMU notifier
  * @read_only: operating on read-only memory
  * @devmem_possible: possible to use device memory
+ * @devmem_only: use only device memory
  *
  * Context that is DRM GPUSVM is operating in (i.e. user arguments).
  */
@@ -294,6 +295,7 @@ struct drm_gpusvm_ctx {
 	unsigned int in_notifier :1;
 	unsigned int read_only :1;
 	unsigned int devmem_possible :1;
+	unsigned int devmem_only :1;
 };
 
 int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
-- 
2.34.1

[PATCH v6 02/20] drm/xe: Strict migration policy for atomic SVM faults

[Himal Prasad Ghimiray]

From: Matthew Brost <matthew.brost@intel.com>

Mixing GPU and CPU atomics does not work unless a strict migration
policy of GPU atomics must be device memory. Enforce a policy of must be
in VRAM with a retry loop of 3 attempts, if retry loop fails abort
fault.

Removing always_migrate_to_vram modparam as we now have real migration
policy.

v2:
 - Only retry migration on atomics
 - Drop alway migrate modparam
v3:
 - Only set vram_only on DGFX (Himal)
 - Bail on get_pages failure if vram_only and retry count exceeded (Himal)
 - s/vram_only/devmem_only
 - Update xe_svm_range_is_valid to accept devmem_only argument
v4:
 - Fix logic bug get_pages failure
v5:
 - Fix commit message (Himal)
 - Mention removing always_migrate_to_vram in commit message (Lucas)
 - Fix xe_svm_range_is_valid to check for devmem pages
 - Bail on devmem_only && !migrate_devmem (Thomas)
v6:
 - Add READ_ONCE barriers for opportunistic checks (Thomas)

Fixes: 2f118c949160 ("drm/xe: Add SVM VRAM migration")
Cc: stable@vger.kernel.org
Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Acked-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
---
 drivers/gpu/drm/xe/xe_module.c |   3 -
 drivers/gpu/drm/xe/xe_module.h |   1 -
 drivers/gpu/drm/xe/xe_svm.c    | 103 ++++++++++++++++++++++++---------
 drivers/gpu/drm/xe/xe_svm.h    |   5 --
 include/drm/drm_gpusvm.h       |  40 ++++++++-----
 5 files changed, 103 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
index 64bf46646544..e4742e27e2cd 100644
--- a/drivers/gpu/drm/xe/xe_module.c
+++ b/drivers/gpu/drm/xe/xe_module.c
@@ -30,9 +30,6 @@ struct xe_modparam xe_modparam = {
 module_param_named(svm_notifier_size, xe_modparam.svm_notifier_size, uint, 0600);
 MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier size(in MiB), must be power of 2");
 
-module_param_named(always_migrate_to_vram, xe_modparam.always_migrate_to_vram, bool, 0444);
-MODULE_PARM_DESC(always_migrate_to_vram, "Always migrate to VRAM on GPU fault");
-
 module_param_named_unsafe(force_execlist, xe_modparam.force_execlist, bool, 0444);
 MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
 
diff --git a/drivers/gpu/drm/xe/xe_module.h b/drivers/gpu/drm/xe/xe_module.h
index 84339e509c80..5a3bfea8b7b4 100644
--- a/drivers/gpu/drm/xe/xe_module.h
+++ b/drivers/gpu/drm/xe/xe_module.h
@@ -12,7 +12,6 @@
 struct xe_modparam {
 	bool force_execlist;
 	bool probe_display;
-	bool always_migrate_to_vram;
 	u32 force_vram_bar_size;
 	int guc_log_level;
 	char *guc_firmware_path;
diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
index 890f6b2f40e9..dcc84e65ca96 100644
--- a/drivers/gpu/drm/xe/xe_svm.c
+++ b/drivers/gpu/drm/xe/xe_svm.c
@@ -16,8 +16,12 @@
 
 static bool xe_svm_range_in_vram(struct xe_svm_range *range)
 {
-	/* Not reliable without notifier lock */
-	return range->base.flags.has_devmem_pages;
+	/* Not reliable without notifier lock, opportunistic only */
+	struct drm_gpusvm_range_flags flags = {
+		.__flags = READ_ONCE(range->base.flags.__flags),
+	};
+
+	return flags.has_devmem_pages;
 }
 
 static bool xe_svm_range_has_vram_binding(struct xe_svm_range *range)
@@ -650,9 +654,13 @@ void xe_svm_fini(struct xe_vm *vm)
 }
 
 static bool xe_svm_range_is_valid(struct xe_svm_range *range,
-				  struct xe_tile *tile)
+				  struct xe_tile *tile,
+				  bool devmem_only)
 {
-	return (range->tile_present & ~range->tile_invalidated) & BIT(tile->id);
+	/* Not reliable without notifier lock, opportunistic only */
+	return ((READ_ONCE(range->tile_present) &
+		 ~READ_ONCE(range->tile_invalidated)) & BIT(tile->id)) &&
+		(!devmem_only || xe_svm_range_in_vram(range));
 }
 
 #if IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
@@ -726,6 +734,36 @@ static int xe_svm_alloc_vram(struct xe_vm *vm, struct xe_tile *tile,
 }
 #endif
 
+static bool supports_4K_migration(struct xe_device *xe)
+{
+	if (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
+		return false;
+
+	return true;
+}
+
+static bool xe_svm_range_needs_migrate_to_vram(struct xe_svm_range *range,
+					       struct xe_vma *vma)
+{
+	struct xe_vm *vm = range_to_vm(&range->base);
+	u64 range_size = xe_svm_range_size(range);
+
+	if (!range->base.flags.migrate_devmem)
+		return false;
+
+	/* Not reliable without notifier lock, opportunistic only */
+	if (xe_svm_range_in_vram(range)) {
+		drm_dbg(&vm->xe->drm, "Range is already in VRAM\n");
+		return false;
+	}
+
+	if (range_size <= SZ_64K && !supports_4K_migration(vm->xe)) {
+		drm_dbg(&vm->xe->drm, "Platform doesn't support SZ_4K range migration\n");
+		return false;
+	}
+
+	return true;
+}
 
 /**
  * xe_svm_handle_pagefault() - SVM handle page fault
@@ -750,12 +788,15 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
 		.check_pages_threshold = IS_DGFX(vm->xe) &&
 			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR) ? SZ_64K : 0,
+		.devmem_only = atomic && IS_DGFX(vm->xe) &&
+			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
 	};
 	struct xe_svm_range *range;
 	struct drm_gpusvm_range *r;
 	struct drm_exec exec;
 	struct dma_fence *fence;
 	struct xe_tile *tile = gt_to_tile(gt);
+	int migrate_try_count = ctx.devmem_only ? 3 : 1;
 	ktime_t end = 0;
 	int err;
 
@@ -776,24 +817,30 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 	if (IS_ERR(r))
 		return PTR_ERR(r);
 
+	if (ctx.devmem_only && !r->flags.migrate_devmem)
+		return -EACCES;
+
 	range = to_xe_range(r);
-	if (xe_svm_range_is_valid(range, tile))
+	if (xe_svm_range_is_valid(range, tile, ctx.devmem_only))
 		return 0;
 
 	range_debug(range, "PAGE FAULT");
 
-	/* XXX: Add migration policy, for now migrate range once */
-	if (!range->skip_migrate && range->base.flags.migrate_devmem &&
-	    xe_svm_range_size(range) >= SZ_64K) {
-		range->skip_migrate = true;
-
+	if (--migrate_try_count >= 0 &&
+	    xe_svm_range_needs_migrate_to_vram(range, vma)) {
 		err = xe_svm_alloc_vram(vm, tile, range, &ctx);
 		if (err) {
-			drm_dbg(&vm->xe->drm,
-				"VRAM allocation failed, falling back to "
-				"retrying fault, asid=%u, errno=%pe\n",
-				vm->usm.asid, ERR_PTR(err));
-			goto retry;
+			if (migrate_try_count || !ctx.devmem_only) {
+				drm_dbg(&vm->xe->drm,
+					"VRAM allocation failed, falling back to retrying fault, asid=%u, errno=%pe\n",
+					vm->usm.asid, ERR_PTR(err));
+				goto retry;
+			} else {
+				drm_err(&vm->xe->drm,
+					"VRAM allocation failed, retry count exceeded, asid=%u, errno=%pe\n",
+					vm->usm.asid, ERR_PTR(err));
+				return err;
+			}
 		}
 	}
 
@@ -801,15 +848,22 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 	err = drm_gpusvm_range_get_pages(&vm->svm.gpusvm, r, &ctx);
 	/* Corner where CPU mappings have changed */
 	if (err == -EOPNOTSUPP || err == -EFAULT || err == -EPERM) {
-		if (err == -EOPNOTSUPP) {
-			range_debug(range, "PAGE FAULT - EVICT PAGES");
-			drm_gpusvm_range_evict(&vm->svm.gpusvm, &range->base);
+		if (migrate_try_count > 0 || !ctx.devmem_only) {
+			if (err == -EOPNOTSUPP) {
+				range_debug(range, "PAGE FAULT - EVICT PAGES");
+				drm_gpusvm_range_evict(&vm->svm.gpusvm,
+						       &range->base);
+			}
+			drm_dbg(&vm->xe->drm,
+				"Get pages failed, falling back to retrying, asid=%u, gpusvm=%p, errno=%pe\n",
+				vm->usm.asid, &vm->svm.gpusvm, ERR_PTR(err));
+			range_debug(range, "PAGE FAULT - RETRY PAGES");
+			goto retry;
+		} else {
+			drm_err(&vm->xe->drm,
+				"Get pages failed, retry count exceeded, asid=%u, gpusvm=%p, errno=%pe\n",
+				vm->usm.asid, &vm->svm.gpusvm, ERR_PTR(err));
 		}
-		drm_dbg(&vm->xe->drm,
-			"Get pages failed, falling back to retrying, asid=%u, gpusvm=%p, errno=%pe\n",
-			vm->usm.asid, &vm->svm.gpusvm, ERR_PTR(err));
-		range_debug(range, "PAGE FAULT - RETRY PAGES");
-		goto retry;
 	}
 	if (err) {
 		range_debug(range, "PAGE FAULT - FAIL PAGE COLLECT");
@@ -843,9 +897,6 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 	}
 	drm_exec_fini(&exec);
 
-	if (xe_modparam.always_migrate_to_vram)
-		range->skip_migrate = false;
-
 	dma_fence_wait(fence, false);
 	dma_fence_put(fence);
 
diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
index 3d441eb1f7ea..0e1f376a7471 100644
--- a/drivers/gpu/drm/xe/xe_svm.h
+++ b/drivers/gpu/drm/xe/xe_svm.h
@@ -39,11 +39,6 @@ struct xe_svm_range {
 	 * range. Protected by GPU SVM notifier lock.
 	 */
 	u8 tile_invalidated;
-	/**
-	 * @skip_migrate: Skip migration to VRAM, protected by GPU fault handler
-	 * locking.
-	 */
-	u8 skip_migrate	:1;
 };
 
 /**
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index 9fd25fc880a4..653d48dbe1c1 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -185,6 +185,31 @@ struct drm_gpusvm_notifier {
 	} flags;
 };
 
+/**
+ * struct drm_gpusvm_range_flags - Structure representing a GPU SVM range flags
+ *
+ * @migrate_devmem: Flag indicating whether the range can be migrated to device memory
+ * @unmapped: Flag indicating if the range has been unmapped
+ * @partial_unmap: Flag indicating if the range has been partially unmapped
+ * @has_devmem_pages: Flag indicating if the range has devmem pages
+ * @has_dma_mapping: Flag indicating if the range has a DMA mapping
+ * @__flags: Flags for range in u16 form (used for READ_ONCE)
+ */
+struct drm_gpusvm_range_flags {
+	union {
+		struct {
+			/* All flags below must be set upon creation */
+			u16 migrate_devmem : 1;
+			/* All flags below must be set / cleared under notifier lock */
+			u16 unmapped : 1;
+			u16 partial_unmap : 1;
+			u16 has_devmem_pages : 1;
+			u16 has_dma_mapping : 1;
+		};
+		u16 __flags;
+	};
+};
+
 /**
  * struct drm_gpusvm_range - Structure representing a GPU SVM range
  *
@@ -198,11 +223,6 @@ struct drm_gpusvm_notifier {
  * @dpagemap: The struct drm_pagemap of the device pages we're dma-mapping.
  *            Note this is assuming only one drm_pagemap per range is allowed.
  * @flags: Flags for range
- * @flags.migrate_devmem: Flag indicating whether the range can be migrated to device memory
- * @flags.unmapped: Flag indicating if the range has been unmapped
- * @flags.partial_unmap: Flag indicating if the range has been partially unmapped
- * @flags.has_devmem_pages: Flag indicating if the range has devmem pages
- * @flags.has_dma_mapping: Flag indicating if the range has a DMA mapping
  *
  * This structure represents a GPU SVM range used for tracking memory ranges
  * mapped in a DRM device.
@@ -216,15 +236,7 @@ struct drm_gpusvm_range {
 	unsigned long notifier_seq;
 	struct drm_pagemap_device_addr *dma_addr;
 	struct drm_pagemap *dpagemap;
-	struct {
-		/* All flags below must be set upon creation */
-		u16 migrate_devmem : 1;
-		/* All flags below must be set / cleared under notifier lock */
-		u16 unmapped : 1;
-		u16 partial_unmap : 1;
-		u16 has_devmem_pages : 1;
-		u16 has_dma_mapping : 1;
-	} flags;
+	struct drm_gpusvm_range_flags flags;
 };
 
 /**
-- 
2.34.1

Re: [PATCH v6 02/20] drm/xe: Strict migration policy for atomic SVM faults

[Thomas Hellström]

Hi, Himal,

On Wed, 2025-04-30 at 17:48 +0530, Himal Prasad Ghimiray wrote:
> From: Matthew Brost <matthew.brost@intel.com>
> 
> Mixing GPU and CPU atomics does not work unless a strict migration
> policy of GPU atomics must be device memory. Enforce a policy of must
> be
> in VRAM with a retry loop of 3 attempts, if retry loop fails abort
> fault.
> 
> Removing always_migrate_to_vram modparam as we now have real
> migration
> policy.
> 
> v2:
>  - Only retry migration on atomics
>  - Drop alway migrate modparam
> v3:
>  - Only set vram_only on DGFX (Himal)
>  - Bail on get_pages failure if vram_only and retry count exceeded
> (Himal)
>  - s/vram_only/devmem_only
>  - Update xe_svm_range_is_valid to accept devmem_only argument
> v4:
>  - Fix logic bug get_pages failure
> v5:
>  - Fix commit message (Himal)
>  - Mention removing always_migrate_to_vram in commit message (Lucas)
>  - Fix xe_svm_range_is_valid to check for devmem pages
>  - Bail on devmem_only && !migrate_devmem (Thomas)
> v6:
>  - Add READ_ONCE barriers for opportunistic checks (Thomas)
> 
> Fixes: 2f118c949160 ("drm/xe: Add SVM VRAM migration")
> Cc: stable@vger.kernel.org
> Signed-off-by: Himal Prasad Ghimiray
> <himal.prasad.ghimiray@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Acked-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_module.c |   3 -
>  drivers/gpu/drm/xe/xe_module.h |   1 -
>  drivers/gpu/drm/xe/xe_svm.c    | 103 ++++++++++++++++++++++++-------
> --
>  drivers/gpu/drm/xe/xe_svm.h    |   5 --
>  include/drm/drm_gpusvm.h       |  40 ++++++++-----
>  5 files changed, 103 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_module.c
> b/drivers/gpu/drm/xe/xe_module.c
> index 64bf46646544..e4742e27e2cd 100644
> --- a/drivers/gpu/drm/xe/xe_module.c
> +++ b/drivers/gpu/drm/xe/xe_module.c
> @@ -30,9 +30,6 @@ struct xe_modparam xe_modparam = {
>  module_param_named(svm_notifier_size, xe_modparam.svm_notifier_size,
> uint, 0600);
>  MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier size(in
> MiB), must be power of 2");
>  
> -module_param_named(always_migrate_to_vram,
> xe_modparam.always_migrate_to_vram, bool, 0444);
> -MODULE_PARM_DESC(always_migrate_to_vram, "Always migrate to VRAM on
> GPU fault");
> -
>  module_param_named_unsafe(force_execlist,
> xe_modparam.force_execlist, bool, 0444);
>  MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
>  
> diff --git a/drivers/gpu/drm/xe/xe_module.h
> b/drivers/gpu/drm/xe/xe_module.h
> index 84339e509c80..5a3bfea8b7b4 100644
> --- a/drivers/gpu/drm/xe/xe_module.h
> +++ b/drivers/gpu/drm/xe/xe_module.h
> @@ -12,7 +12,6 @@
>  struct xe_modparam {
>  	bool force_execlist;
>  	bool probe_display;
> -	bool always_migrate_to_vram;
>  	u32 force_vram_bar_size;
>  	int guc_log_level;
>  	char *guc_firmware_path;
> diff --git a/drivers/gpu/drm/xe/xe_svm.c
> b/drivers/gpu/drm/xe/xe_svm.c
> index 890f6b2f40e9..dcc84e65ca96 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
> @@ -16,8 +16,12 @@
>  
>  static bool xe_svm_range_in_vram(struct xe_svm_range *range)
>  {
> -	/* Not reliable without notifier lock */
> -	return range->base.flags.has_devmem_pages;
> +	/* Not reliable without notifier lock, opportunistic only */
> +	struct drm_gpusvm_range_flags flags = {
> +		.__flags = READ_ONCE(range->base.flags.__flags),
> +	};
> +
> +	return flags.has_devmem_pages;
>  }
>  
>  static bool xe_svm_range_has_vram_binding(struct xe_svm_range
> *range)
> @@ -650,9 +654,13 @@ void xe_svm_fini(struct xe_vm *vm)
>  }
>  
>  static bool xe_svm_range_is_valid(struct xe_svm_range *range,
> -				  struct xe_tile *tile)
> +				  struct xe_tile *tile,
> +				  bool devmem_only)
>  {
> -	return (range->tile_present & ~range->tile_invalidated) &
> BIT(tile->id);
> +	/* Not reliable without notifier lock, opportunistic only */
> +	return ((READ_ONCE(range->tile_present) &
> +		 ~READ_ONCE(range->tile_invalidated)) & BIT(tile-
> >id)) &&
> +		(!devmem_only || xe_svm_range_in_vram(range));
>  }

Hmm, TBH I had something more elaborate in mind:

https://lore.kernel.org/intel-xe/b5569de8cc036e23b976f21a51c4eb5ca104d4bb.camel@linux.intel.com/

(Separate function for lockless access and a lockdep assert for locked
access + similar documentation as the functions I mentioned there + a
"Pairs with" comment.

Thanks,
Thomas



>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
> @@ -726,6 +734,36 @@ static int xe_svm_alloc_vram(struct xe_vm *vm,
> struct xe_tile *tile,
>  }
>  #endif
>  
> +static bool supports_4K_migration(struct xe_device *xe)
> +{
> +	if (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool xe_svm_range_needs_migrate_to_vram(struct xe_svm_range
> *range,
> +					       struct xe_vma *vma)
> +{
> +	struct xe_vm *vm = range_to_vm(&range->base);
> +	u64 range_size = xe_svm_range_size(range);
> +
> +	if (!range->base.flags.migrate_devmem)
> +		return false;
> +
> +	/* Not reliable without notifier lock, opportunistic only */
> +	if (xe_svm_range_in_vram(range)) {
> +		drm_dbg(&vm->xe->drm, "Range is already in VRAM\n");
> +		return false;
> +	}
> +
> +	if (range_size <= SZ_64K && !supports_4K_migration(vm->xe))
> {
> +		drm_dbg(&vm->xe->drm, "Platform doesn't support
> SZ_4K range migration\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
>  
>  /**
>   * xe_svm_handle_pagefault() - SVM handle page fault
> @@ -750,12 +788,15 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> struct xe_vma *vma,
>  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
>  		.check_pages_threshold = IS_DGFX(vm->xe) &&
>  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR) ?
> SZ_64K : 0,
> +		.devmem_only = atomic && IS_DGFX(vm->xe) &&
> +			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
>  	};
>  	struct xe_svm_range *range;
>  	struct drm_gpusvm_range *r;
>  	struct drm_exec exec;
>  	struct dma_fence *fence;
>  	struct xe_tile *tile = gt_to_tile(gt);
> +	int migrate_try_count = ctx.devmem_only ? 3 : 1;
>  	ktime_t end = 0;
>  	int err;
>  
> @@ -776,24 +817,30 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> struct xe_vma *vma,
>  	if (IS_ERR(r))
>  		return PTR_ERR(r);
>  
> +	if (ctx.devmem_only && !r->flags.migrate_devmem)
> +		return -EACCES;
> +
>  	range = to_xe_range(r);
> -	if (xe_svm_range_is_valid(range, tile))
> +	if (xe_svm_range_is_valid(range, tile, ctx.devmem_only))
>  		return 0;
>  
>  	range_debug(range, "PAGE FAULT");
>  
> -	/* XXX: Add migration policy, for now migrate range once */
> -	if (!range->skip_migrate && range->base.flags.migrate_devmem
> &&
> -	    xe_svm_range_size(range) >= SZ_64K) {
> -		range->skip_migrate = true;
> -
> +	if (--migrate_try_count >= 0 &&
> +	    xe_svm_range_needs_migrate_to_vram(range, vma)) {
>  		err = xe_svm_alloc_vram(vm, tile, range, &ctx);
>  		if (err) {
> -			drm_dbg(&vm->xe->drm,
> -				"VRAM allocation failed, falling
> back to "
> -				"retrying fault, asid=%u,
> errno=%pe\n",
> -				vm->usm.asid, ERR_PTR(err));
> -			goto retry;
> +			if (migrate_try_count || !ctx.devmem_only) {
> +				drm_dbg(&vm->xe->drm,
> +					"VRAM allocation failed,
> falling back to retrying fault, asid=%u, errno=%pe\n",
> +					vm->usm.asid, ERR_PTR(err));
> +				goto retry;
> +			} else {
> +				drm_err(&vm->xe->drm,
> +					"VRAM allocation failed,
> retry count exceeded, asid=%u, errno=%pe\n",
> +					vm->usm.asid, ERR_PTR(err));
> +				return err;
> +			}
>  		}
>  	}
>  
> @@ -801,15 +848,22 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> struct xe_vma *vma,
>  	err = drm_gpusvm_range_get_pages(&vm->svm.gpusvm, r, &ctx);
>  	/* Corner where CPU mappings have changed */
>  	if (err == -EOPNOTSUPP || err == -EFAULT || err == -EPERM) {
> -		if (err == -EOPNOTSUPP) {
> -			range_debug(range, "PAGE FAULT - EVICT
> PAGES");
> -			drm_gpusvm_range_evict(&vm->svm.gpusvm,
> &range->base);
> +		if (migrate_try_count > 0 || !ctx.devmem_only) {
> +			if (err == -EOPNOTSUPP) {
> +				range_debug(range, "PAGE FAULT -
> EVICT PAGES");
> +				drm_gpusvm_range_evict(&vm-
> >svm.gpusvm,
> +						       &range-
> >base);
> +			}
> +			drm_dbg(&vm->xe->drm,
> +				"Get pages failed, falling back to
> retrying, asid=%u, gpusvm=%p, errno=%pe\n",
> +				vm->usm.asid, &vm->svm.gpusvm,
> ERR_PTR(err));
> +			range_debug(range, "PAGE FAULT - RETRY
> PAGES");
> +			goto retry;
> +		} else {
> +			drm_err(&vm->xe->drm,
> +				"Get pages failed, retry count
> exceeded, asid=%u, gpusvm=%p, errno=%pe\n",
> +				vm->usm.asid, &vm->svm.gpusvm,
> ERR_PTR(err));
>  		}
> -		drm_dbg(&vm->xe->drm,
> -			"Get pages failed, falling back to retrying,
> asid=%u, gpusvm=%p, errno=%pe\n",
> -			vm->usm.asid, &vm->svm.gpusvm,
> ERR_PTR(err));
> -		range_debug(range, "PAGE FAULT - RETRY PAGES");
> -		goto retry;
>  	}
>  	if (err) {
>  		range_debug(range, "PAGE FAULT - FAIL PAGE
> COLLECT");
> @@ -843,9 +897,6 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> struct xe_vma *vma,
>  	}
>  	drm_exec_fini(&exec);
>  
> -	if (xe_modparam.always_migrate_to_vram)
> -		range->skip_migrate = false;
> -
>  	dma_fence_wait(fence, false);
>  	dma_fence_put(fence);
>  
> diff --git a/drivers/gpu/drm/xe/xe_svm.h
> b/drivers/gpu/drm/xe/xe_svm.h
> index 3d441eb1f7ea..0e1f376a7471 100644
> --- a/drivers/gpu/drm/xe/xe_svm.h
> +++ b/drivers/gpu/drm/xe/xe_svm.h
> @@ -39,11 +39,6 @@ struct xe_svm_range {
>  	 * range. Protected by GPU SVM notifier lock.
>  	 */
>  	u8 tile_invalidated;
> -	/**
> -	 * @skip_migrate: Skip migration to VRAM, protected by GPU
> fault handler
> -	 * locking.
> -	 */
> -	u8 skip_migrate	:1;
>  };
>  
>  /**
> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index 9fd25fc880a4..653d48dbe1c1 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h
> @@ -185,6 +185,31 @@ struct drm_gpusvm_notifier {
>  	} flags;
>  };
>  
> +/**
> + * struct drm_gpusvm_range_flags - Structure representing a GPU SVM
> range flags
> + *
> + * @migrate_devmem: Flag indicating whether the range can be
> migrated to device memory
> + * @unmapped: Flag indicating if the range has been unmapped
> + * @partial_unmap: Flag indicating if the range has been partially
> unmapped
> + * @has_devmem_pages: Flag indicating if the range has devmem pages
> + * @has_dma_mapping: Flag indicating if the range has a DMA mapping
> + * @__flags: Flags for range in u16 form (used for READ_ONCE)
> + */
> +struct drm_gpusvm_range_flags {
> +	union {
> +		struct {
> +			/* All flags below must be set upon creation
> */
> +			u16 migrate_devmem : 1;
> +			/* All flags below must be set / cleared
> under notifier lock */
> +			u16 unmapped : 1;
> +			u16 partial_unmap : 1;
> +			u16 has_devmem_pages : 1;
> +			u16 has_dma_mapping : 1;
> +		};
> +		u16 __flags;
> +	};
> +};
> +
>  /**
>   * struct drm_gpusvm_range - Structure representing a GPU SVM range
>   *
> @@ -198,11 +223,6 @@ struct drm_gpusvm_notifier {
>   * @dpagemap: The struct drm_pagemap of the device pages we're dma-
> mapping.
>   *            Note this is assuming only one drm_pagemap per range
> is allowed.
>   * @flags: Flags for range
> - * @flags.migrate_devmem: Flag indicating whether the range can be
> migrated to device memory
> - * @flags.unmapped: Flag indicating if the range has been unmapped
> - * @flags.partial_unmap: Flag indicating if the range has been
> partially unmapped
> - * @flags.has_devmem_pages: Flag indicating if the range has devmem
> pages
> - * @flags.has_dma_mapping: Flag indicating if the range has a DMA
> mapping
>   *
>   * This structure represents a GPU SVM range used for tracking
> memory ranges
>   * mapped in a DRM device.
> @@ -216,15 +236,7 @@ struct drm_gpusvm_range {
>  	unsigned long notifier_seq;
>  	struct drm_pagemap_device_addr *dma_addr;
>  	struct drm_pagemap *dpagemap;
> -	struct {
> -		/* All flags below must be set upon creation */
> -		u16 migrate_devmem : 1;
> -		/* All flags below must be set / cleared under
> notifier lock */
> -		u16 unmapped : 1;
> -		u16 partial_unmap : 1;
> -		u16 has_devmem_pages : 1;
> -		u16 has_dma_mapping : 1;
> -	} flags;
> +	struct drm_gpusvm_range_flags flags;
>  };
>  
>  /**

Re: [PATCH v6 02/20] drm/xe: Strict migration policy for atomic SVM faults

[Matthew Brost]

On Mon, May 05, 2025 at 05:20:00PM +0200, Thomas Hellström wrote:
> Hi, Himal,
> 
> On Wed, 2025-04-30 at 17:48 +0530, Himal Prasad Ghimiray wrote:
> > From: Matthew Brost <matthew.brost@intel.com>
> > 
> > Mixing GPU and CPU atomics does not work unless a strict migration
> > policy of GPU atomics must be device memory. Enforce a policy of must
> > be
> > in VRAM with a retry loop of 3 attempts, if retry loop fails abort
> > fault.
> > 
> > Removing always_migrate_to_vram modparam as we now have real
> > migration
> > policy.
> > 
> > v2:
> >  - Only retry migration on atomics
> >  - Drop alway migrate modparam
> > v3:
> >  - Only set vram_only on DGFX (Himal)
> >  - Bail on get_pages failure if vram_only and retry count exceeded
> > (Himal)
> >  - s/vram_only/devmem_only
> >  - Update xe_svm_range_is_valid to accept devmem_only argument
> > v4:
> >  - Fix logic bug get_pages failure
> > v5:
> >  - Fix commit message (Himal)
> >  - Mention removing always_migrate_to_vram in commit message (Lucas)
> >  - Fix xe_svm_range_is_valid to check for devmem pages
> >  - Bail on devmem_only && !migrate_devmem (Thomas)
> > v6:
> >  - Add READ_ONCE barriers for opportunistic checks (Thomas)
> > 
> > Fixes: 2f118c949160 ("drm/xe: Add SVM VRAM migration")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Himal Prasad Ghimiray
> > <himal.prasad.ghimiray@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Acked-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_module.c |   3 -
> >  drivers/gpu/drm/xe/xe_module.h |   1 -
> >  drivers/gpu/drm/xe/xe_svm.c    | 103 ++++++++++++++++++++++++-------
> > --
> >  drivers/gpu/drm/xe/xe_svm.h    |   5 --
> >  include/drm/drm_gpusvm.h       |  40 ++++++++-----
> >  5 files changed, 103 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_module.c
> > b/drivers/gpu/drm/xe/xe_module.c
> > index 64bf46646544..e4742e27e2cd 100644
> > --- a/drivers/gpu/drm/xe/xe_module.c
> > +++ b/drivers/gpu/drm/xe/xe_module.c
> > @@ -30,9 +30,6 @@ struct xe_modparam xe_modparam = {
> >  module_param_named(svm_notifier_size, xe_modparam.svm_notifier_size,
> > uint, 0600);
> >  MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier size(in
> > MiB), must be power of 2");
> >  
> > -module_param_named(always_migrate_to_vram,
> > xe_modparam.always_migrate_to_vram, bool, 0444);
> > -MODULE_PARM_DESC(always_migrate_to_vram, "Always migrate to VRAM on
> > GPU fault");
> > -
> >  module_param_named_unsafe(force_execlist,
> > xe_modparam.force_execlist, bool, 0444);
> >  MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_module.h
> > b/drivers/gpu/drm/xe/xe_module.h
> > index 84339e509c80..5a3bfea8b7b4 100644
> > --- a/drivers/gpu/drm/xe/xe_module.h
> > +++ b/drivers/gpu/drm/xe/xe_module.h
> > @@ -12,7 +12,6 @@
> >  struct xe_modparam {
> >  	bool force_execlist;
> >  	bool probe_display;
> > -	bool always_migrate_to_vram;
> >  	u32 force_vram_bar_size;
> >  	int guc_log_level;
> >  	char *guc_firmware_path;
> > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > b/drivers/gpu/drm/xe/xe_svm.c
> > index 890f6b2f40e9..dcc84e65ca96 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.c
> > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > @@ -16,8 +16,12 @@
> >  
> >  static bool xe_svm_range_in_vram(struct xe_svm_range *range)
> >  {
> > -	/* Not reliable without notifier lock */
> > -	return range->base.flags.has_devmem_pages;
> > +	/* Not reliable without notifier lock, opportunistic only */
> > +	struct drm_gpusvm_range_flags flags = {
> > +		.__flags = READ_ONCE(range->base.flags.__flags),
> > +	};
> > +
> > +	return flags.has_devmem_pages;
> >  }
> >  
> >  static bool xe_svm_range_has_vram_binding(struct xe_svm_range
> > *range)
> > @@ -650,9 +654,13 @@ void xe_svm_fini(struct xe_vm *vm)
> >  }
> >  
> >  static bool xe_svm_range_is_valid(struct xe_svm_range *range,
> > -				  struct xe_tile *tile)
> > +				  struct xe_tile *tile,
> > +				  bool devmem_only)
> >  {
> > -	return (range->tile_present & ~range->tile_invalidated) &
> > BIT(tile->id);
> > +	/* Not reliable without notifier lock, opportunistic only */
> > +	return ((READ_ONCE(range->tile_present) &
> > +		 ~READ_ONCE(range->tile_invalidated)) & BIT(tile-
> > >id)) &&
> > +		(!devmem_only || xe_svm_range_in_vram(range));
> >  }
> 
> Hmm, TBH I had something more elaborate in mind:
> 
> https://lore.kernel.org/intel-xe/b5569de8cc036e23b976f21a51c4eb5ca104d4bb.camel@linux.intel.com/
> 
> (Separate function for lockless access and a lockdep assert for locked
> access + similar documentation as the functions I mentioned there + a
> "Pairs with" comment.
> 

But if the locked functions are unused wouldn't the compiler complain?

Matt

> Thanks,
> Thomas
> 
> 
> 
> >  
> >  #if IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
> > @@ -726,6 +734,36 @@ static int xe_svm_alloc_vram(struct xe_vm *vm,
> > struct xe_tile *tile,
> >  }
> >  #endif
> >  
> > +static bool supports_4K_migration(struct xe_device *xe)
> > +{
> > +	if (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static bool xe_svm_range_needs_migrate_to_vram(struct xe_svm_range
> > *range,
> > +					       struct xe_vma *vma)
> > +{
> > +	struct xe_vm *vm = range_to_vm(&range->base);
> > +	u64 range_size = xe_svm_range_size(range);
> > +
> > +	if (!range->base.flags.migrate_devmem)
> > +		return false;
> > +
> > +	/* Not reliable without notifier lock, opportunistic only */
> > +	if (xe_svm_range_in_vram(range)) {
> > +		drm_dbg(&vm->xe->drm, "Range is already in VRAM\n");
> > +		return false;
> > +	}
> > +
> > +	if (range_size <= SZ_64K && !supports_4K_migration(vm->xe))
> > {
> > +		drm_dbg(&vm->xe->drm, "Platform doesn't support
> > SZ_4K range migration\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> >  
> >  /**
> >   * xe_svm_handle_pagefault() - SVM handle page fault
> > @@ -750,12 +788,15 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> > struct xe_vma *vma,
> >  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
> >  		.check_pages_threshold = IS_DGFX(vm->xe) &&
> >  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR) ?
> > SZ_64K : 0,
> > +		.devmem_only = atomic && IS_DGFX(vm->xe) &&
> > +			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
> >  	};
> >  	struct xe_svm_range *range;
> >  	struct drm_gpusvm_range *r;
> >  	struct drm_exec exec;
> >  	struct dma_fence *fence;
> >  	struct xe_tile *tile = gt_to_tile(gt);
> > +	int migrate_try_count = ctx.devmem_only ? 3 : 1;
> >  	ktime_t end = 0;
> >  	int err;
> >  
> > @@ -776,24 +817,30 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> > struct xe_vma *vma,
> >  	if (IS_ERR(r))
> >  		return PTR_ERR(r);
> >  
> > +	if (ctx.devmem_only && !r->flags.migrate_devmem)
> > +		return -EACCES;
> > +
> >  	range = to_xe_range(r);
> > -	if (xe_svm_range_is_valid(range, tile))
> > +	if (xe_svm_range_is_valid(range, tile, ctx.devmem_only))
> >  		return 0;
> >  
> >  	range_debug(range, "PAGE FAULT");
> >  
> > -	/* XXX: Add migration policy, for now migrate range once */
> > -	if (!range->skip_migrate && range->base.flags.migrate_devmem
> > &&
> > -	    xe_svm_range_size(range) >= SZ_64K) {
> > -		range->skip_migrate = true;
> > -
> > +	if (--migrate_try_count >= 0 &&
> > +	    xe_svm_range_needs_migrate_to_vram(range, vma)) {
> >  		err = xe_svm_alloc_vram(vm, tile, range, &ctx);
> >  		if (err) {
> > -			drm_dbg(&vm->xe->drm,
> > -				"VRAM allocation failed, falling
> > back to "
> > -				"retrying fault, asid=%u,
> > errno=%pe\n",
> > -				vm->usm.asid, ERR_PTR(err));
> > -			goto retry;
> > +			if (migrate_try_count || !ctx.devmem_only) {
> > +				drm_dbg(&vm->xe->drm,
> > +					"VRAM allocation failed,
> > falling back to retrying fault, asid=%u, errno=%pe\n",
> > +					vm->usm.asid, ERR_PTR(err));
> > +				goto retry;
> > +			} else {
> > +				drm_err(&vm->xe->drm,
> > +					"VRAM allocation failed,
> > retry count exceeded, asid=%u, errno=%pe\n",
> > +					vm->usm.asid, ERR_PTR(err));
> > +				return err;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -801,15 +848,22 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> > struct xe_vma *vma,
> >  	err = drm_gpusvm_range_get_pages(&vm->svm.gpusvm, r, &ctx);
> >  	/* Corner where CPU mappings have changed */
> >  	if (err == -EOPNOTSUPP || err == -EFAULT || err == -EPERM) {
> > -		if (err == -EOPNOTSUPP) {
> > -			range_debug(range, "PAGE FAULT - EVICT
> > PAGES");
> > -			drm_gpusvm_range_evict(&vm->svm.gpusvm,
> > &range->base);
> > +		if (migrate_try_count > 0 || !ctx.devmem_only) {
> > +			if (err == -EOPNOTSUPP) {
> > +				range_debug(range, "PAGE FAULT -
> > EVICT PAGES");
> > +				drm_gpusvm_range_evict(&vm-
> > >svm.gpusvm,
> > +						       &range-
> > >base);
> > +			}
> > +			drm_dbg(&vm->xe->drm,
> > +				"Get pages failed, falling back to
> > retrying, asid=%u, gpusvm=%p, errno=%pe\n",
> > +				vm->usm.asid, &vm->svm.gpusvm,
> > ERR_PTR(err));
> > +			range_debug(range, "PAGE FAULT - RETRY
> > PAGES");
> > +			goto retry;
> > +		} else {
> > +			drm_err(&vm->xe->drm,
> > +				"Get pages failed, retry count
> > exceeded, asid=%u, gpusvm=%p, errno=%pe\n",
> > +				vm->usm.asid, &vm->svm.gpusvm,
> > ERR_PTR(err));
> >  		}
> > -		drm_dbg(&vm->xe->drm,
> > -			"Get pages failed, falling back to retrying,
> > asid=%u, gpusvm=%p, errno=%pe\n",
> > -			vm->usm.asid, &vm->svm.gpusvm,
> > ERR_PTR(err));
> > -		range_debug(range, "PAGE FAULT - RETRY PAGES");
> > -		goto retry;
> >  	}
> >  	if (err) {
> >  		range_debug(range, "PAGE FAULT - FAIL PAGE
> > COLLECT");
> > @@ -843,9 +897,6 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> > struct xe_vma *vma,
> >  	}
> >  	drm_exec_fini(&exec);
> >  
> > -	if (xe_modparam.always_migrate_to_vram)
> > -		range->skip_migrate = false;
> > -
> >  	dma_fence_wait(fence, false);
> >  	dma_fence_put(fence);
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_svm.h
> > b/drivers/gpu/drm/xe/xe_svm.h
> > index 3d441eb1f7ea..0e1f376a7471 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.h
> > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > @@ -39,11 +39,6 @@ struct xe_svm_range {
> >  	 * range. Protected by GPU SVM notifier lock.
> >  	 */
> >  	u8 tile_invalidated;
> > -	/**
> > -	 * @skip_migrate: Skip migration to VRAM, protected by GPU
> > fault handler
> > -	 * locking.
> > -	 */
> > -	u8 skip_migrate	:1;
> >  };
> >  
> >  /**
> > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> > index 9fd25fc880a4..653d48dbe1c1 100644
> > --- a/include/drm/drm_gpusvm.h
> > +++ b/include/drm/drm_gpusvm.h
> > @@ -185,6 +185,31 @@ struct drm_gpusvm_notifier {
> >  	} flags;
> >  };
> >  
> > +/**
> > + * struct drm_gpusvm_range_flags - Structure representing a GPU SVM
> > range flags
> > + *
> > + * @migrate_devmem: Flag indicating whether the range can be
> > migrated to device memory
> > + * @unmapped: Flag indicating if the range has been unmapped
> > + * @partial_unmap: Flag indicating if the range has been partially
> > unmapped
> > + * @has_devmem_pages: Flag indicating if the range has devmem pages
> > + * @has_dma_mapping: Flag indicating if the range has a DMA mapping
> > + * @__flags: Flags for range in u16 form (used for READ_ONCE)
> > + */
> > +struct drm_gpusvm_range_flags {
> > +	union {
> > +		struct {
> > +			/* All flags below must be set upon creation
> > */
> > +			u16 migrate_devmem : 1;
> > +			/* All flags below must be set / cleared
> > under notifier lock */
> > +			u16 unmapped : 1;
> > +			u16 partial_unmap : 1;
> > +			u16 has_devmem_pages : 1;
> > +			u16 has_dma_mapping : 1;
> > +		};
> > +		u16 __flags;
> > +	};
> > +};
> > +
> >  /**
> >   * struct drm_gpusvm_range - Structure representing a GPU SVM range
> >   *
> > @@ -198,11 +223,6 @@ struct drm_gpusvm_notifier {
> >   * @dpagemap: The struct drm_pagemap of the device pages we're dma-
> > mapping.
> >   *            Note this is assuming only one drm_pagemap per range
> > is allowed.
> >   * @flags: Flags for range
> > - * @flags.migrate_devmem: Flag indicating whether the range can be
> > migrated to device memory
> > - * @flags.unmapped: Flag indicating if the range has been unmapped
> > - * @flags.partial_unmap: Flag indicating if the range has been
> > partially unmapped
> > - * @flags.has_devmem_pages: Flag indicating if the range has devmem
> > pages
> > - * @flags.has_dma_mapping: Flag indicating if the range has a DMA
> > mapping
> >   *
> >   * This structure represents a GPU SVM range used for tracking
> > memory ranges
> >   * mapped in a DRM device.
> > @@ -216,15 +236,7 @@ struct drm_gpusvm_range {
> >  	unsigned long notifier_seq;
> >  	struct drm_pagemap_device_addr *dma_addr;
> >  	struct drm_pagemap *dpagemap;
> > -	struct {
> > -		/* All flags below must be set upon creation */
> > -		u16 migrate_devmem : 1;
> > -		/* All flags below must be set / cleared under
> > notifier lock */
> > -		u16 unmapped : 1;
> > -		u16 partial_unmap : 1;
> > -		u16 has_devmem_pages : 1;
> > -		u16 has_dma_mapping : 1;
> > -	} flags;
> > +	struct drm_gpusvm_range_flags flags;
> >  };
> >  
> >  /**
> 

Re: [PATCH v6 02/20] drm/xe: Strict migration policy for atomic SVM faults

[Thomas Hellström]

On Mon, 2025-05-05 at 13:18 -0700, Matthew Brost wrote:
> On Mon, May 05, 2025 at 05:20:00PM +0200, Thomas Hellström wrote:
> > Hi, Himal,
> > 
> > On Wed, 2025-04-30 at 17:48 +0530, Himal Prasad Ghimiray wrote:
> > > From: Matthew Brost <matthew.brost@intel.com>
> > > 
> > > Mixing GPU and CPU atomics does not work unless a strict
> > > migration
> > > policy of GPU atomics must be device memory. Enforce a policy of
> > > must
> > > be
> > > in VRAM with a retry loop of 3 attempts, if retry loop fails
> > > abort
> > > fault.
> > > 
> > > Removing always_migrate_to_vram modparam as we now have real
> > > migration
> > > policy.
> > > 
> > > v2:
> > >  - Only retry migration on atomics
> > >  - Drop alway migrate modparam
> > > v3:
> > >  - Only set vram_only on DGFX (Himal)
> > >  - Bail on get_pages failure if vram_only and retry count
> > > exceeded
> > > (Himal)
> > >  - s/vram_only/devmem_only
> > >  - Update xe_svm_range_is_valid to accept devmem_only argument
> > > v4:
> > >  - Fix logic bug get_pages failure
> > > v5:
> > >  - Fix commit message (Himal)
> > >  - Mention removing always_migrate_to_vram in commit message
> > > (Lucas)
> > >  - Fix xe_svm_range_is_valid to check for devmem pages
> > >  - Bail on devmem_only && !migrate_devmem (Thomas)
> > > v6:
> > >  - Add READ_ONCE barriers for opportunistic checks (Thomas)
> > > 
> > > Fixes: 2f118c949160 ("drm/xe: Add SVM VRAM migration")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Himal Prasad Ghimiray
> > > <himal.prasad.ghimiray@intel.com>
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > Acked-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_module.c |   3 -
> > >  drivers/gpu/drm/xe/xe_module.h |   1 -
> > >  drivers/gpu/drm/xe/xe_svm.c    | 103 ++++++++++++++++++++++++---
> > > ----
> > > --
> > >  drivers/gpu/drm/xe/xe_svm.h    |   5 --
> > >  include/drm/drm_gpusvm.h       |  40 ++++++++-----
> > >  5 files changed, 103 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_module.c
> > > b/drivers/gpu/drm/xe/xe_module.c
> > > index 64bf46646544..e4742e27e2cd 100644
> > > --- a/drivers/gpu/drm/xe/xe_module.c
> > > +++ b/drivers/gpu/drm/xe/xe_module.c
> > > @@ -30,9 +30,6 @@ struct xe_modparam xe_modparam = {
> > >  module_param_named(svm_notifier_size,
> > > xe_modparam.svm_notifier_size,
> > > uint, 0600);
> > >  MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier
> > > size(in
> > > MiB), must be power of 2");
> > >  
> > > -module_param_named(always_migrate_to_vram,
> > > xe_modparam.always_migrate_to_vram, bool, 0444);
> > > -MODULE_PARM_DESC(always_migrate_to_vram, "Always migrate to VRAM
> > > on
> > > GPU fault");
> > > -
> > >  module_param_named_unsafe(force_execlist,
> > > xe_modparam.force_execlist, bool, 0444);
> > >  MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
> > >  
> > > diff --git a/drivers/gpu/drm/xe/xe_module.h
> > > b/drivers/gpu/drm/xe/xe_module.h
> > > index 84339e509c80..5a3bfea8b7b4 100644
> > > --- a/drivers/gpu/drm/xe/xe_module.h
> > > +++ b/drivers/gpu/drm/xe/xe_module.h
> > > @@ -12,7 +12,6 @@
> > >  struct xe_modparam {
> > >  	bool force_execlist;
> > >  	bool probe_display;
> > > -	bool always_migrate_to_vram;
> > >  	u32 force_vram_bar_size;
> > >  	int guc_log_level;
> > >  	char *guc_firmware_path;
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > > b/drivers/gpu/drm/xe/xe_svm.c
> > > index 890f6b2f40e9..dcc84e65ca96 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > @@ -16,8 +16,12 @@
> > >  
> > >  static bool xe_svm_range_in_vram(struct xe_svm_range *range)
> > >  {
> > > -	/* Not reliable without notifier lock */
> > > -	return range->base.flags.has_devmem_pages;
> > > +	/* Not reliable without notifier lock, opportunistic
> > > only */
> > > +	struct drm_gpusvm_range_flags flags = {
> > > +		.__flags = READ_ONCE(range->base.flags.__flags),
> > > +	};
> > > +
> > > +	return flags.has_devmem_pages;
> > >  }
> > >  
> > >  static bool xe_svm_range_has_vram_binding(struct xe_svm_range
> > > *range)
> > > @@ -650,9 +654,13 @@ void xe_svm_fini(struct xe_vm *vm)
> > >  }
> > >  
> > >  static bool xe_svm_range_is_valid(struct xe_svm_range *range,
> > > -				  struct xe_tile *tile)
> > > +				  struct xe_tile *tile,
> > > +				  bool devmem_only)
> > >  {
> > > -	return (range->tile_present & ~range->tile_invalidated)
> > > &
> > > BIT(tile->id);
> > > +	/* Not reliable without notifier lock, opportunistic
> > > only */
> > > +	return ((READ_ONCE(range->tile_present) &
> > > +		 ~READ_ONCE(range->tile_invalidated)) &
> > > BIT(tile-
> > > > id)) &&
> > > +		(!devmem_only || xe_svm_range_in_vram(range));
> > >  }
> > 
> > Hmm, TBH I had something more elaborate in mind:
> > 
> > https://lore.kernel.org/intel-xe/b5569de8cc036e23b976f21a51c4eb5ca104d4bb.camel@linux.intel.com/
> > 
> > (Separate function for lockless access and a lockdep assert for
> > locked
> > access + similar documentation as the functions I mentioned there +
> > a
> > "Pairs with" comment.
> > 
> 
> But if the locked functions are unused wouldn't the compiler
> complain?

Oh, I was under the impression we had multiple ose of those. My bad.

But still IMO we should move that comment about opportunistic from
within the function to the header to clarify the usage of the function
interface, and close to the READ_ONCE we should add a comment about a
pairing WRITE_ONCE.

It actually looks like (for a future patch unless done in this one) the
atomic bitops is a good fit for this.

/Thomas



> 
> Matt
> 
> > Thanks,
> > Thomas
> > 
> > 
> > 
> > >  
> > >  #if IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
> > > @@ -726,6 +734,36 @@ static int xe_svm_alloc_vram(struct xe_vm
> > > *vm,
> > > struct xe_tile *tile,
> > >  }
> > >  #endif
> > >  
> > > +static bool supports_4K_migration(struct xe_device *xe)
> > > +{
> > > +	if (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static bool xe_svm_range_needs_migrate_to_vram(struct
> > > xe_svm_range
> > > *range,
> > > +					       struct xe_vma
> > > *vma)
> > > +{
> > > +	struct xe_vm *vm = range_to_vm(&range->base);
> > > +	u64 range_size = xe_svm_range_size(range);
> > > +
> > > +	if (!range->base.flags.migrate_devmem)
> > > +		return false;
> > > +
> > > +	/* Not reliable without notifier lock, opportunistic
> > > only */
> > > +	if (xe_svm_range_in_vram(range)) {
> > > +		drm_dbg(&vm->xe->drm, "Range is already in
> > > VRAM\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	if (range_size <= SZ_64K && !supports_4K_migration(vm-
> > > >xe))
> > > {
> > > +		drm_dbg(&vm->xe->drm, "Platform doesn't support
> > > SZ_4K range migration\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > >  
> > >  /**
> > >   * xe_svm_handle_pagefault() - SVM handle page fault
> > > @@ -750,12 +788,15 @@ int xe_svm_handle_pagefault(struct xe_vm
> > > *vm,
> > > struct xe_vma *vma,
> > >  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
> > >  		.check_pages_threshold = IS_DGFX(vm->xe) &&
> > >  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
> > > ?
> > > SZ_64K : 0,
> > > +		.devmem_only = atomic && IS_DGFX(vm->xe) &&
> > > +			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
> > >  	};
> > >  	struct xe_svm_range *range;
> > >  	struct drm_gpusvm_range *r;
> > >  	struct drm_exec exec;
> > >  	struct dma_fence *fence;
> > >  	struct xe_tile *tile = gt_to_tile(gt);
> > > +	int migrate_try_count = ctx.devmem_only ? 3 : 1;
> > >  	ktime_t end = 0;
> > >  	int err;
> > >  
> > > @@ -776,24 +817,30 @@ int xe_svm_handle_pagefault(struct xe_vm
> > > *vm,
> > > struct xe_vma *vma,
> > >  	if (IS_ERR(r))
> > >  		return PTR_ERR(r);
> > >  
> > > +	if (ctx.devmem_only && !r->flags.migrate_devmem)
> > > +		return -EACCES;
> > > +
> > >  	range = to_xe_range(r);
> > > -	if (xe_svm_range_is_valid(range, tile))
> > > +	if (xe_svm_range_is_valid(range, tile, ctx.devmem_only))
> > >  		return 0;
> > >  
> > >  	range_debug(range, "PAGE FAULT");
> > >  
> > > -	/* XXX: Add migration policy, for now migrate range once
> > > */
> > > -	if (!range->skip_migrate && range-
> > > >base.flags.migrate_devmem
> > > &&
> > > -	    xe_svm_range_size(range) >= SZ_64K) {
> > > -		range->skip_migrate = true;
> > > -
> > > +	if (--migrate_try_count >= 0 &&
> > > +	    xe_svm_range_needs_migrate_to_vram(range, vma)) {
> > >  		err = xe_svm_alloc_vram(vm, tile, range, &ctx);
> > >  		if (err) {
> > > -			drm_dbg(&vm->xe->drm,
> > > -				"VRAM allocation failed, falling
> > > back to "
> > > -				"retrying fault, asid=%u,
> > > errno=%pe\n",
> > > -				vm->usm.asid, ERR_PTR(err));
> > > -			goto retry;
> > > +			if (migrate_try_count ||
> > > !ctx.devmem_only) {
> > > +				drm_dbg(&vm->xe->drm,
> > > +					"VRAM allocation failed,
> > > falling back to retrying fault, asid=%u, errno=%pe\n",
> > > +					vm->usm.asid,
> > > ERR_PTR(err));
> > > +				goto retry;
> > > +			} else {
> > > +				drm_err(&vm->xe->drm,
> > > +					"VRAM allocation failed,
> > > retry count exceeded, asid=%u, errno=%pe\n",
> > > +					vm->usm.asid,
> > > ERR_PTR(err));
> > > +				return err;
> > > +			}
> > >  		}
> > >  	}
> > >  
> > > @@ -801,15 +848,22 @@ int xe_svm_handle_pagefault(struct xe_vm
> > > *vm,
> > > struct xe_vma *vma,
> > >  	err = drm_gpusvm_range_get_pages(&vm->svm.gpusvm, r,
> > > &ctx);
> > >  	/* Corner where CPU mappings have changed */
> > >  	if (err == -EOPNOTSUPP || err == -EFAULT || err == -
> > > EPERM) {
> > > -		if (err == -EOPNOTSUPP) {
> > > -			range_debug(range, "PAGE FAULT - EVICT
> > > PAGES");
> > > -			drm_gpusvm_range_evict(&vm->svm.gpusvm,
> > > &range->base);
> > > +		if (migrate_try_count > 0 || !ctx.devmem_only) {
> > > +			if (err == -EOPNOTSUPP) {
> > > +				range_debug(range, "PAGE FAULT -
> > > EVICT PAGES");
> > > +				drm_gpusvm_range_evict(&vm-
> > > > svm.gpusvm,
> > > +						       &range-
> > > > base);
> > > +			}
> > > +			drm_dbg(&vm->xe->drm,
> > > +				"Get pages failed, falling back
> > > to
> > > retrying, asid=%u, gpusvm=%p, errno=%pe\n",
> > > +				vm->usm.asid, &vm->svm.gpusvm,
> > > ERR_PTR(err));
> > > +			range_debug(range, "PAGE FAULT - RETRY
> > > PAGES");
> > > +			goto retry;
> > > +		} else {
> > > +			drm_err(&vm->xe->drm,
> > > +				"Get pages failed, retry count
> > > exceeded, asid=%u, gpusvm=%p, errno=%pe\n",
> > > +				vm->usm.asid, &vm->svm.gpusvm,
> > > ERR_PTR(err));
> > >  		}
> > > -		drm_dbg(&vm->xe->drm,
> > > -			"Get pages failed, falling back to
> > > retrying,
> > > asid=%u, gpusvm=%p, errno=%pe\n",
> > > -			vm->usm.asid, &vm->svm.gpusvm,
> > > ERR_PTR(err));
> > > -		range_debug(range, "PAGE FAULT - RETRY PAGES");
> > > -		goto retry;
> > >  	}
> > >  	if (err) {
> > >  		range_debug(range, "PAGE FAULT - FAIL PAGE
> > > COLLECT");
> > > @@ -843,9 +897,6 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> > > struct xe_vma *vma,
> > >  	}
> > >  	drm_exec_fini(&exec);
> > >  
> > > -	if (xe_modparam.always_migrate_to_vram)
> > > -		range->skip_migrate = false;
> > > -
> > >  	dma_fence_wait(fence, false);
> > >  	dma_fence_put(fence);
> > >  
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.h
> > > b/drivers/gpu/drm/xe/xe_svm.h
> > > index 3d441eb1f7ea..0e1f376a7471 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.h
> > > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > > @@ -39,11 +39,6 @@ struct xe_svm_range {
> > >  	 * range. Protected by GPU SVM notifier lock.
> > >  	 */
> > >  	u8 tile_invalidated;
> > > -	/**
> > > -	 * @skip_migrate: Skip migration to VRAM, protected by
> > > GPU
> > > fault handler
> > > -	 * locking.
> > > -	 */
> > > -	u8 skip_migrate	:1;
> > >  };
> > >  
> > >  /**
> > > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> > > index 9fd25fc880a4..653d48dbe1c1 100644
> > > --- a/include/drm/drm_gpusvm.h
> > > +++ b/include/drm/drm_gpusvm.h
> > > @@ -185,6 +185,31 @@ struct drm_gpusvm_notifier {
> > >  	} flags;
> > >  };
> > >  
> > > +/**
> > > + * struct drm_gpusvm_range_flags - Structure representing a GPU
> > > SVM
> > > range flags
> > > + *
> > > + * @migrate_devmem: Flag indicating whether the range can be
> > > migrated to device memory
> > > + * @unmapped: Flag indicating if the range has been unmapped
> > > + * @partial_unmap: Flag indicating if the range has been
> > > partially
> > > unmapped
> > > + * @has_devmem_pages: Flag indicating if the range has devmem
> > > pages
> > > + * @has_dma_mapping: Flag indicating if the range has a DMA
> > > mapping
> > > + * @__flags: Flags for range in u16 form (used for READ_ONCE)
> > > + */
> > > +struct drm_gpusvm_range_flags {
> > > +	union {
> > > +		struct {
> > > +			/* All flags below must be set upon
> > > creation
> > > */
> > > +			u16 migrate_devmem : 1;
> > > +			/* All flags below must be set / cleared
> > > under notifier lock */
> > > +			u16 unmapped : 1;
> > > +			u16 partial_unmap : 1;
> > > +			u16 has_devmem_pages : 1;
> > > +			u16 has_dma_mapping : 1;
> > > +		};
> > > +		u16 __flags;
> > > +	};
> > > +};
> > > +
> > >  /**
> > >   * struct drm_gpusvm_range - Structure representing a GPU SVM
> > > range
> > >   *
> > > @@ -198,11 +223,6 @@ struct drm_gpusvm_notifier {
> > >   * @dpagemap: The struct drm_pagemap of the device pages we're
> > > dma-
> > > mapping.
> > >   *            Note this is assuming only one drm_pagemap per
> > > range
> > > is allowed.
> > >   * @flags: Flags for range
> > > - * @flags.migrate_devmem: Flag indicating whether the range can
> > > be
> > > migrated to device memory
> > > - * @flags.unmapped: Flag indicating if the range has been
> > > unmapped
> > > - * @flags.partial_unmap: Flag indicating if the range has been
> > > partially unmapped
> > > - * @flags.has_devmem_pages: Flag indicating if the range has
> > > devmem
> > > pages
> > > - * @flags.has_dma_mapping: Flag indicating if the range has a
> > > DMA
> > > mapping
> > >   *
> > >   * This structure represents a GPU SVM range used for tracking
> > > memory ranges
> > >   * mapped in a DRM device.
> > > @@ -216,15 +236,7 @@ struct drm_gpusvm_range {
> > >  	unsigned long notifier_seq;
> > >  	struct drm_pagemap_device_addr *dma_addr;
> > >  	struct drm_pagemap *dpagemap;
> > > -	struct {
> > > -		/* All flags below must be set upon creation */
> > > -		u16 migrate_devmem : 1;
> > > -		/* All flags below must be set / cleared under
> > > notifier lock */
> > > -		u16 unmapped : 1;
> > > -		u16 partial_unmap : 1;
> > > -		u16 has_devmem_pages : 1;
> > > -		u16 has_dma_mapping : 1;
> > > -	} flags;
> > > +	struct drm_gpusvm_range_flags flags;
> > >  };
> > >  
> > >  /**
> > 

Re: [PATCH v6 02/20] drm/xe: Strict migration policy for atomic SVM faults

[Matthew Brost]

On Tue, May 06, 2025 at 12:13:34PM +0200, Thomas Hellström wrote:
> On Mon, 2025-05-05 at 13:18 -0700, Matthew Brost wrote:
> > On Mon, May 05, 2025 at 05:20:00PM +0200, Thomas Hellström wrote:
> > > Hi, Himal,
> > > 
> > > On Wed, 2025-04-30 at 17:48 +0530, Himal Prasad Ghimiray wrote:
> > > > From: Matthew Brost <matthew.brost@intel.com>
> > > > 
> > > > Mixing GPU and CPU atomics does not work unless a strict
> > > > migration
> > > > policy of GPU atomics must be device memory. Enforce a policy of
> > > > must
> > > > be
> > > > in VRAM with a retry loop of 3 attempts, if retry loop fails
> > > > abort
> > > > fault.
> > > > 
> > > > Removing always_migrate_to_vram modparam as we now have real
> > > > migration
> > > > policy.
> > > > 
> > > > v2:
> > > >  - Only retry migration on atomics
> > > >  - Drop alway migrate modparam
> > > > v3:
> > > >  - Only set vram_only on DGFX (Himal)
> > > >  - Bail on get_pages failure if vram_only and retry count
> > > > exceeded
> > > > (Himal)
> > > >  - s/vram_only/devmem_only
> > > >  - Update xe_svm_range_is_valid to accept devmem_only argument
> > > > v4:
> > > >  - Fix logic bug get_pages failure
> > > > v5:
> > > >  - Fix commit message (Himal)
> > > >  - Mention removing always_migrate_to_vram in commit message
> > > > (Lucas)
> > > >  - Fix xe_svm_range_is_valid to check for devmem pages
> > > >  - Bail on devmem_only && !migrate_devmem (Thomas)
> > > > v6:
> > > >  - Add READ_ONCE barriers for opportunistic checks (Thomas)
> > > > 
> > > > Fixes: 2f118c949160 ("drm/xe: Add SVM VRAM migration")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Himal Prasad Ghimiray
> > > > <himal.prasad.ghimiray@intel.com>
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > Acked-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_module.c |   3 -
> > > >  drivers/gpu/drm/xe/xe_module.h |   1 -
> > > >  drivers/gpu/drm/xe/xe_svm.c    | 103 ++++++++++++++++++++++++---
> > > > ----
> > > > --
> > > >  drivers/gpu/drm/xe/xe_svm.h    |   5 --
> > > >  include/drm/drm_gpusvm.h       |  40 ++++++++-----
> > > >  5 files changed, 103 insertions(+), 49 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_module.c
> > > > b/drivers/gpu/drm/xe/xe_module.c
> > > > index 64bf46646544..e4742e27e2cd 100644
> > > > --- a/drivers/gpu/drm/xe/xe_module.c
> > > > +++ b/drivers/gpu/drm/xe/xe_module.c
> > > > @@ -30,9 +30,6 @@ struct xe_modparam xe_modparam = {
> > > >  module_param_named(svm_notifier_size,
> > > > xe_modparam.svm_notifier_size,
> > > > uint, 0600);
> > > >  MODULE_PARM_DESC(svm_notifier_size, "Set the svm notifier
> > > > size(in
> > > > MiB), must be power of 2");
> > > >  
> > > > -module_param_named(always_migrate_to_vram,
> > > > xe_modparam.always_migrate_to_vram, bool, 0444);
> > > > -MODULE_PARM_DESC(always_migrate_to_vram, "Always migrate to VRAM
> > > > on
> > > > GPU fault");
> > > > -
> > > >  module_param_named_unsafe(force_execlist,
> > > > xe_modparam.force_execlist, bool, 0444);
> > > >  MODULE_PARM_DESC(force_execlist, "Force Execlist submission");
> > > >  
> > > > diff --git a/drivers/gpu/drm/xe/xe_module.h
> > > > b/drivers/gpu/drm/xe/xe_module.h
> > > > index 84339e509c80..5a3bfea8b7b4 100644
> > > > --- a/drivers/gpu/drm/xe/xe_module.h
> > > > +++ b/drivers/gpu/drm/xe/xe_module.h
> > > > @@ -12,7 +12,6 @@
> > > >  struct xe_modparam {
> > > >  	bool force_execlist;
> > > >  	bool probe_display;
> > > > -	bool always_migrate_to_vram;
> > > >  	u32 force_vram_bar_size;
> > > >  	int guc_log_level;
> > > >  	char *guc_firmware_path;
> > > > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > > > b/drivers/gpu/drm/xe/xe_svm.c
> > > > index 890f6b2f40e9..dcc84e65ca96 100644
> > > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > > @@ -16,8 +16,12 @@
> > > >  
> > > >  static bool xe_svm_range_in_vram(struct xe_svm_range *range)
> > > >  {
> > > > -	/* Not reliable without notifier lock */
> > > > -	return range->base.flags.has_devmem_pages;
> > > > +	/* Not reliable without notifier lock, opportunistic
> > > > only */
> > > > +	struct drm_gpusvm_range_flags flags = {
> > > > +		.__flags = READ_ONCE(range->base.flags.__flags),
> > > > +	};
> > > > +
> > > > +	return flags.has_devmem_pages;
> > > >  }
> > > >  
> > > >  static bool xe_svm_range_has_vram_binding(struct xe_svm_range
> > > > *range)
> > > > @@ -650,9 +654,13 @@ void xe_svm_fini(struct xe_vm *vm)
> > > >  }
> > > >  
> > > >  static bool xe_svm_range_is_valid(struct xe_svm_range *range,
> > > > -				  struct xe_tile *tile)
> > > > +				  struct xe_tile *tile,
> > > > +				  bool devmem_only)
> > > >  {
> > > > -	return (range->tile_present & ~range->tile_invalidated)
> > > > &
> > > > BIT(tile->id);
> > > > +	/* Not reliable without notifier lock, opportunistic
> > > > only */
> > > > +	return ((READ_ONCE(range->tile_present) &
> > > > +		 ~READ_ONCE(range->tile_invalidated)) &
> > > > BIT(tile-
> > > > > id)) &&
> > > > +		(!devmem_only || xe_svm_range_in_vram(range));
> > > >  }
> > > 
> > > Hmm, TBH I had something more elaborate in mind:
> > > 
> > > https://lore.kernel.org/intel-xe/b5569de8cc036e23b976f21a51c4eb5ca104d4bb.camel@linux.intel.com/
> > > 
> > > (Separate function for lockless access and a lockdep assert for
> > > locked
> > > access + similar documentation as the functions I mentioned there +
> > > a
> > > "Pairs with" comment.
> > > 
> > 
> > But if the locked functions are unused wouldn't the compiler
> > complain?
> 
> Oh, I was under the impression we had multiple ose of those. My bad.
> 
> But still IMO we should move that comment about opportunistic from
> within the function to the header to clarify the usage of the function
> interface, and close to the READ_ONCE we should add a comment about a
> pairing WRITE_ONCE.
> 

Ah, ok. I missed the WRITE_ONCE side of this + comment.

> It actually looks like (for a future patch unless done in this one) the
> atomic bitops is a good fit for this.

Maybe? I'll probably stick with WRITE_ONCE pairing for now.

Matt

> 
> /Thomas
> 
> 
> 
> > 
> > Matt
> > 
> > > Thanks,
> > > Thomas
> > > 
> > > 
> > > 
> > > >  
> > > >  #if IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
> > > > @@ -726,6 +734,36 @@ static int xe_svm_alloc_vram(struct xe_vm
> > > > *vm,
> > > > struct xe_tile *tile,
> > > >  }
> > > >  #endif
> > > >  
> > > > +static bool supports_4K_migration(struct xe_device *xe)
> > > > +{
> > > > +	if (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
> > > > +		return false;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static bool xe_svm_range_needs_migrate_to_vram(struct
> > > > xe_svm_range
> > > > *range,
> > > > +					       struct xe_vma
> > > > *vma)
> > > > +{
> > > > +	struct xe_vm *vm = range_to_vm(&range->base);
> > > > +	u64 range_size = xe_svm_range_size(range);
> > > > +
> > > > +	if (!range->base.flags.migrate_devmem)
> > > > +		return false;
> > > > +
> > > > +	/* Not reliable without notifier lock, opportunistic
> > > > only */
> > > > +	if (xe_svm_range_in_vram(range)) {
> > > > +		drm_dbg(&vm->xe->drm, "Range is already in
> > > > VRAM\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	if (range_size <= SZ_64K && !supports_4K_migration(vm-
> > > > >xe))
> > > > {
> > > > +		drm_dbg(&vm->xe->drm, "Platform doesn't support
> > > > SZ_4K range migration\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > >  
> > > >  /**
> > > >   * xe_svm_handle_pagefault() - SVM handle page fault
> > > > @@ -750,12 +788,15 @@ int xe_svm_handle_pagefault(struct xe_vm
> > > > *vm,
> > > > struct xe_vma *vma,
> > > >  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
> > > >  		.check_pages_threshold = IS_DGFX(vm->xe) &&
> > > >  			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
> > > > ?
> > > > SZ_64K : 0,
> > > > +		.devmem_only = atomic && IS_DGFX(vm->xe) &&
> > > > +			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
> > > >  	};
> > > >  	struct xe_svm_range *range;
> > > >  	struct drm_gpusvm_range *r;
> > > >  	struct drm_exec exec;
> > > >  	struct dma_fence *fence;
> > > >  	struct xe_tile *tile = gt_to_tile(gt);
> > > > +	int migrate_try_count = ctx.devmem_only ? 3 : 1;
> > > >  	ktime_t end = 0;
> > > >  	int err;
> > > >  
> > > > @@ -776,24 +817,30 @@ int xe_svm_handle_pagefault(struct xe_vm
> > > > *vm,
> > > > struct xe_vma *vma,
> > > >  	if (IS_ERR(r))
> > > >  		return PTR_ERR(r);
> > > >  
> > > > +	if (ctx.devmem_only && !r->flags.migrate_devmem)
> > > > +		return -EACCES;
> > > > +
> > > >  	range = to_xe_range(r);
> > > > -	if (xe_svm_range_is_valid(range, tile))
> > > > +	if (xe_svm_range_is_valid(range, tile, ctx.devmem_only))
> > > >  		return 0;
> > > >  
> > > >  	range_debug(range, "PAGE FAULT");
> > > >  
> > > > -	/* XXX: Add migration policy, for now migrate range once
> > > > */
> > > > -	if (!range->skip_migrate && range-
> > > > >base.flags.migrate_devmem
> > > > &&
> > > > -	    xe_svm_range_size(range) >= SZ_64K) {
> > > > -		range->skip_migrate = true;
> > > > -
> > > > +	if (--migrate_try_count >= 0 &&
> > > > +	    xe_svm_range_needs_migrate_to_vram(range, vma)) {
> > > >  		err = xe_svm_alloc_vram(vm, tile, range, &ctx);
> > > >  		if (err) {
> > > > -			drm_dbg(&vm->xe->drm,
> > > > -				"VRAM allocation failed, falling
> > > > back to "
> > > > -				"retrying fault, asid=%u,
> > > > errno=%pe\n",
> > > > -				vm->usm.asid, ERR_PTR(err));
> > > > -			goto retry;
> > > > +			if (migrate_try_count ||
> > > > !ctx.devmem_only) {
> > > > +				drm_dbg(&vm->xe->drm,
> > > > +					"VRAM allocation failed,
> > > > falling back to retrying fault, asid=%u, errno=%pe\n",
> > > > +					vm->usm.asid,
> > > > ERR_PTR(err));
> > > > +				goto retry;
> > > > +			} else {
> > > > +				drm_err(&vm->xe->drm,
> > > > +					"VRAM allocation failed,
> > > > retry count exceeded, asid=%u, errno=%pe\n",
> > > > +					vm->usm.asid,
> > > > ERR_PTR(err));
> > > > +				return err;
> > > > +			}
> > > >  		}
> > > >  	}
> > > >  
> > > > @@ -801,15 +848,22 @@ int xe_svm_handle_pagefault(struct xe_vm
> > > > *vm,
> > > > struct xe_vma *vma,
> > > >  	err = drm_gpusvm_range_get_pages(&vm->svm.gpusvm, r,
> > > > &ctx);
> > > >  	/* Corner where CPU mappings have changed */
> > > >  	if (err == -EOPNOTSUPP || err == -EFAULT || err == -
> > > > EPERM) {
> > > > -		if (err == -EOPNOTSUPP) {
> > > > -			range_debug(range, "PAGE FAULT - EVICT
> > > > PAGES");
> > > > -			drm_gpusvm_range_evict(&vm->svm.gpusvm,
> > > > &range->base);
> > > > +		if (migrate_try_count > 0 || !ctx.devmem_only) {
> > > > +			if (err == -EOPNOTSUPP) {
> > > > +				range_debug(range, "PAGE FAULT -
> > > > EVICT PAGES");
> > > > +				drm_gpusvm_range_evict(&vm-
> > > > > svm.gpusvm,
> > > > +						       &range-
> > > > > base);
> > > > +			}
> > > > +			drm_dbg(&vm->xe->drm,
> > > > +				"Get pages failed, falling back
> > > > to
> > > > retrying, asid=%u, gpusvm=%p, errno=%pe\n",
> > > > +				vm->usm.asid, &vm->svm.gpusvm,
> > > > ERR_PTR(err));
> > > > +			range_debug(range, "PAGE FAULT - RETRY
> > > > PAGES");
> > > > +			goto retry;
> > > > +		} else {
> > > > +			drm_err(&vm->xe->drm,
> > > > +				"Get pages failed, retry count
> > > > exceeded, asid=%u, gpusvm=%p, errno=%pe\n",
> > > > +				vm->usm.asid, &vm->svm.gpusvm,
> > > > ERR_PTR(err));
> > > >  		}
> > > > -		drm_dbg(&vm->xe->drm,
> > > > -			"Get pages failed, falling back to
> > > > retrying,
> > > > asid=%u, gpusvm=%p, errno=%pe\n",
> > > > -			vm->usm.asid, &vm->svm.gpusvm,
> > > > ERR_PTR(err));
> > > > -		range_debug(range, "PAGE FAULT - RETRY PAGES");
> > > > -		goto retry;
> > > >  	}
> > > >  	if (err) {
> > > >  		range_debug(range, "PAGE FAULT - FAIL PAGE
> > > > COLLECT");
> > > > @@ -843,9 +897,6 @@ int xe_svm_handle_pagefault(struct xe_vm *vm,
> > > > struct xe_vma *vma,
> > > >  	}
> > > >  	drm_exec_fini(&exec);
> > > >  
> > > > -	if (xe_modparam.always_migrate_to_vram)
> > > > -		range->skip_migrate = false;
> > > > -
> > > >  	dma_fence_wait(fence, false);
> > > >  	dma_fence_put(fence);
> > > >  
> > > > diff --git a/drivers/gpu/drm/xe/xe_svm.h
> > > > b/drivers/gpu/drm/xe/xe_svm.h
> > > > index 3d441eb1f7ea..0e1f376a7471 100644
> > > > --- a/drivers/gpu/drm/xe/xe_svm.h
> > > > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > > > @@ -39,11 +39,6 @@ struct xe_svm_range {
> > > >  	 * range. Protected by GPU SVM notifier lock.
> > > >  	 */
> > > >  	u8 tile_invalidated;
> > > > -	/**
> > > > -	 * @skip_migrate: Skip migration to VRAM, protected by
> > > > GPU
> > > > fault handler
> > > > -	 * locking.
> > > > -	 */
> > > > -	u8 skip_migrate	:1;
> > > >  };
> > > >  
> > > >  /**
> > > > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> > > > index 9fd25fc880a4..653d48dbe1c1 100644
> > > > --- a/include/drm/drm_gpusvm.h
> > > > +++ b/include/drm/drm_gpusvm.h
> > > > @@ -185,6 +185,31 @@ struct drm_gpusvm_notifier {
> > > >  	} flags;
> > > >  };
> > > >  
> > > > +/**
> > > > + * struct drm_gpusvm_range_flags - Structure representing a GPU
> > > > SVM
> > > > range flags
> > > > + *
> > > > + * @migrate_devmem: Flag indicating whether the range can be
> > > > migrated to device memory
> > > > + * @unmapped: Flag indicating if the range has been unmapped
> > > > + * @partial_unmap: Flag indicating if the range has been
> > > > partially
> > > > unmapped
> > > > + * @has_devmem_pages: Flag indicating if the range has devmem
> > > > pages
> > > > + * @has_dma_mapping: Flag indicating if the range has a DMA
> > > > mapping
> > > > + * @__flags: Flags for range in u16 form (used for READ_ONCE)
> > > > + */
> > > > +struct drm_gpusvm_range_flags {
> > > > +	union {
> > > > +		struct {
> > > > +			/* All flags below must be set upon
> > > > creation
> > > > */
> > > > +			u16 migrate_devmem : 1;
> > > > +			/* All flags below must be set / cleared
> > > > under notifier lock */
> > > > +			u16 unmapped : 1;
> > > > +			u16 partial_unmap : 1;
> > > > +			u16 has_devmem_pages : 1;
> > > > +			u16 has_dma_mapping : 1;
> > > > +		};
> > > > +		u16 __flags;
> > > > +	};
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct drm_gpusvm_range - Structure representing a GPU SVM
> > > > range
> > > >   *
> > > > @@ -198,11 +223,6 @@ struct drm_gpusvm_notifier {
> > > >   * @dpagemap: The struct drm_pagemap of the device pages we're
> > > > dma-
> > > > mapping.
> > > >   *            Note this is assuming only one drm_pagemap per
> > > > range
> > > > is allowed.
> > > >   * @flags: Flags for range
> > > > - * @flags.migrate_devmem: Flag indicating whether the range can
> > > > be
> > > > migrated to device memory
> > > > - * @flags.unmapped: Flag indicating if the range has been
> > > > unmapped
> > > > - * @flags.partial_unmap: Flag indicating if the range has been
> > > > partially unmapped
> > > > - * @flags.has_devmem_pages: Flag indicating if the range has
> > > > devmem
> > > > pages
> > > > - * @flags.has_dma_mapping: Flag indicating if the range has a
> > > > DMA
> > > > mapping
> > > >   *
> > > >   * This structure represents a GPU SVM range used for tracking
> > > > memory ranges
> > > >   * mapped in a DRM device.
> > > > @@ -216,15 +236,7 @@ struct drm_gpusvm_range {
> > > >  	unsigned long notifier_seq;
> > > >  	struct drm_pagemap_device_addr *dma_addr;
> > > >  	struct drm_pagemap *dpagemap;
> > > > -	struct {
> > > > -		/* All flags below must be set upon creation */
> > > > -		u16 migrate_devmem : 1;
> > > > -		/* All flags below must be set / cleared under
> > > > notifier lock */
> > > > -		u16 unmapped : 1;
> > > > -		u16 partial_unmap : 1;
> > > > -		u16 has_devmem_pages : 1;
> > > > -		u16 has_dma_mapping : 1;
> > > > -	} flags;
> > > > +	struct drm_gpusvm_range_flags flags;
> > > >  };
> > > >  
> > > >  /**
> > > 
> 

[PATCH v6 03/20] drm/gpusvm: Add timeslicing support to GPU SVM

[Himal Prasad Ghimiray]

From: Matthew Brost <matthew.brost@intel.com>

Add timeslicing support to GPU SVM which will guarantee the GPU a
minimum execution time on piece of physical memory before migration back
to CPU. Intended to implement strict migration policies which require
memory to be in a certain placement for correct execution.

Required for shared CPU and GPU atomics on certain devices.

Fixes: 99624bdff867 ("drm/gpusvm: Add support for GPU Shared Virtual Memory")
Cc: stable@vger.kernel.org
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 9 +++++++++
 include/drm/drm_gpusvm.h     | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index a58d03e6cac2..c94a8d7a293d 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1770,6 +1770,8 @@ int drm_gpusvm_migrate_to_devmem(struct drm_gpusvm *gpusvm,
 		goto err_finalize;
 
 	/* Upon success bind devmem allocation to range and zdd */
+	devmem_allocation->timeslice_expiration = get_jiffies_64() +
+		msecs_to_jiffies(ctx->timeslice_ms);
 	zdd->devmem_allocation = devmem_allocation;	/* Owns ref */
 
 err_finalize:
@@ -1990,6 +1992,13 @@ static int __drm_gpusvm_migrate_to_ram(struct vm_area_struct *vas,
 	void *buf;
 	int i, err = 0;
 
+	if (page) {
+		zdd = page->zone_device_data;
+		if (time_before64(get_jiffies_64(),
+				  zdd->devmem_allocation->timeslice_expiration))
+			return 0;
+	}
+
 	start = ALIGN_DOWN(fault_addr, size);
 	end = ALIGN(fault_addr + 1, size);
 
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index 653d48dbe1c1..eaf704d3d05e 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -89,6 +89,7 @@ struct drm_gpusvm_devmem_ops {
  * @ops: Pointer to the operations structure for GPU SVM device memory
  * @dpagemap: The struct drm_pagemap of the pages this allocation belongs to.
  * @size: Size of device memory allocation
+ * @timeslice_expiration: Timeslice expiration in jiffies
  */
 struct drm_gpusvm_devmem {
 	struct device *dev;
@@ -97,6 +98,7 @@ struct drm_gpusvm_devmem {
 	const struct drm_gpusvm_devmem_ops *ops;
 	struct drm_pagemap *dpagemap;
 	size_t size;
+	u64 timeslice_expiration;
 };
 
 /**
@@ -295,6 +297,8 @@ struct drm_gpusvm {
  * @check_pages_threshold: Check CPU pages for present if chunk is less than or
  *                         equal to threshold. If not present, reduce chunk
  *                         size.
+ * @timeslice_ms: The timeslice MS which in minimum time a piece of memory
+ *		  remains with either exclusive GPU or CPU access.
  * @in_notifier: entering from a MMU notifier
  * @read_only: operating on read-only memory
  * @devmem_possible: possible to use device memory
@@ -304,6 +308,7 @@ struct drm_gpusvm {
  */
 struct drm_gpusvm_ctx {
 	unsigned long check_pages_threshold;
+	unsigned long timeslice_ms;
 	unsigned int in_notifier :1;
 	unsigned int read_only :1;
 	unsigned int devmem_possible :1;
-- 
2.34.1

[PATCH v6 04/20] drm/xe: Timeslice GPU on atomic SVM fault

[Himal Prasad Ghimiray]

From: Matthew Brost <matthew.brost@intel.com>

Ensure GPU can make forward progress on an atomic SVM GPU fault by
giving the GPU a timeslice of 5ms

v2:
 - Reduce timeslice to 5ms
 - Double timeslice on retry
 - Split out GPU SVM changes into independent patch
v5:
 - Double timeslice in a few more places

Fixes: 2f118c949160 ("drm/xe: Add SVM VRAM migration")
Cc: stable@vger.kernel.org
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_svm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
index dcc84e65ca96..f967caabc895 100644
--- a/drivers/gpu/drm/xe/xe_svm.c
+++ b/drivers/gpu/drm/xe/xe_svm.c
@@ -790,6 +790,8 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR) ? SZ_64K : 0,
 		.devmem_only = atomic && IS_DGFX(vm->xe) &&
 			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR),
+		.timeslice_ms = atomic && IS_DGFX(vm->xe) &&
+			IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR) ? 5 : 0,
 	};
 	struct xe_svm_range *range;
 	struct drm_gpusvm_range *r;
@@ -829,6 +831,7 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 	if (--migrate_try_count >= 0 &&
 	    xe_svm_range_needs_migrate_to_vram(range, vma)) {
 		err = xe_svm_alloc_vram(vm, tile, range, &ctx);
+		ctx.timeslice_ms <<= 1;	/* Double timeslice if we have to retry */
 		if (err) {
 			if (migrate_try_count || !ctx.devmem_only) {
 				drm_dbg(&vm->xe->drm,
@@ -848,6 +851,7 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 	err = drm_gpusvm_range_get_pages(&vm->svm.gpusvm, r, &ctx);
 	/* Corner where CPU mappings have changed */
 	if (err == -EOPNOTSUPP || err == -EFAULT || err == -EPERM) {
+		ctx.timeslice_ms <<= 1;	/* Double timeslice if we have to retry */
 		if (migrate_try_count > 0 || !ctx.devmem_only) {
 			if (err == -EOPNOTSUPP) {
 				range_debug(range, "PAGE FAULT - EVICT PAGES");
@@ -887,6 +891,7 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
 			drm_exec_fini(&exec);
 			err = PTR_ERR(fence);
 			if (err == -EAGAIN) {
+				ctx.timeslice_ms <<= 1;	/* Double timeslice if we have to retry */
 				range_debug(range, "PAGE FAULT - RETRY BIND");
 				goto retry;
 			}
-- 
2.34.1