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

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.ca...

(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;

};    /**