Re: [PATCH 4/7] drm/gpusvm: pull out drm_gpusvm_pages substructure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20/03/2025 20:43, Matthew Brost wrote:
On Thu, Mar 20, 2025 at 05:30:01PM +0000, Matthew Auld wrote:
Pull the pages stuff from the svm range into its own substructure, with
the idea of having the main pages related routines, like get_pages(),
unmap_pages() and free_pages() all operating on some lower level
structures, which can then be re-used for stuff like userptr which wants
to use basic stuff like get_pages(), but not all the other more complex
svm stuff.

Suggested-by: Matthew Brost <matthew.brost@xxxxxxxxx>
Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/drm_gpusvm.c | 95 +++++++++++++++++++-----------------
  drivers/gpu/drm/xe/xe_pt.c   |  2 +-
  drivers/gpu/drm/xe/xe_svm.c  |  6 +--
  drivers/gpu/drm/xe/xe_svm.h  |  2 +-
  include/drm/drm_gpusvm.h     | 43 +++++++++-------
  5 files changed, 82 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index 5b4ecd36dff1..f27731a51f34 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -812,7 +812,7 @@ drm_gpusvm_range_alloc(struct drm_gpusvm *gpusvm,
  	range->itree.last = ALIGN(fault_addr + 1, chunk_size) - 1;
  	INIT_LIST_HEAD(&range->entry);
  	range->notifier_seq = LONG_MAX;
-	range->flags.migrate_devmem = migrate_devmem ? 1 : 0;
+	range->pages.flags.migrate_devmem = migrate_devmem ? 1 : 0;
return range;
  }
@@ -1120,27 +1120,27 @@ drm_gpusvm_range_find_or_insert(struct drm_gpusvm *gpusvm,
  EXPORT_SYMBOL_GPL(drm_gpusvm_range_find_or_insert);
/**
- * __drm_gpusvm_range_unmap_pages() - Unmap pages associated with a GPU SVM range (internal)
+ * __drm_gpusvm_unmap_pages() - Unmap pages associated with a GPU SVM range (internal)
   * @gpusvm: Pointer to the GPU SVM structure
- * @range: Pointer to the GPU SVM range structure
+ * @svm_pages: Pointer to the GPU SVM pages structure
   * @npages: Number of pages to unmap
   *
   * This function unmap pages associated with a GPU SVM range. Assumes and
   * asserts correct locking is in place when called.
   */
-static void __drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
-					   struct drm_gpusvm_range *range,
-					   unsigned long npages)
+static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
+				     struct drm_gpusvm_pages *svm_pages,
+				     unsigned long npages)
  {
  	unsigned long i, j;
-	struct drm_pagemap *dpagemap = range->dpagemap;
+	struct drm_pagemap *dpagemap = svm_pages->dpagemap;
  	struct device *dev = gpusvm->drm->dev;
lockdep_assert_held(&gpusvm->notifier_lock); - if (range->flags.has_dma_mapping) {
+	if (svm_pages->flags.has_dma_mapping) {
  		for (i = 0, j = 0; i < npages; j++) {
-			struct drm_pagemap_device_addr *addr = &range->dma_addr[j];
+			struct drm_pagemap_device_addr *addr = &svm_pages->dma_addr[j];
if (addr->proto == DRM_INTERCONNECT_SYSTEM)
  				dma_unmap_page(dev,
@@ -1152,9 +1152,9 @@ static void __drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
  							    dev, *addr);
  			i += 1 << addr->order;
  		}
-		range->flags.has_devmem_pages = false;
-		range->flags.has_dma_mapping = false;
-		range->dpagemap = NULL;
+		svm_pages->flags.has_devmem_pages = false;
+		svm_pages->flags.has_dma_mapping = false;
+		svm_pages->dpagemap = NULL;
  	}
  }
@@ -1165,14 +1165,14 @@ static void __drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
   *
   * This function frees the dma address array associated with a GPU SVM range.
   */
-static void drm_gpusvm_range_free_pages(struct drm_gpusvm *gpusvm,
-					struct drm_gpusvm_range *range)
+static void drm_gpusvm_free_pages(struct drm_gpusvm *gpusvm,
+				  struct drm_gpusvm_pages *svm_pages)
  {
  	lockdep_assert_held(&gpusvm->notifier_lock);
- if (range->dma_addr) {
-		kvfree(range->dma_addr);
-		range->dma_addr = NULL;
+	if (svm_pages->dma_addr) {
+		kvfree(svm_pages->dma_addr);
+		svm_pages->dma_addr = NULL;
  	}
  }
@@ -1200,8 +1200,8 @@ void drm_gpusvm_range_remove(struct drm_gpusvm *gpusvm,
  		return;
drm_gpusvm_notifier_lock(gpusvm);
-	__drm_gpusvm_range_unmap_pages(gpusvm, range, npages);
-	drm_gpusvm_range_free_pages(gpusvm, range);
+	__drm_gpusvm_unmap_pages(gpusvm, &range->pages, npages);
+	drm_gpusvm_free_pages(gpusvm, &range->pages);
  	__drm_gpusvm_range_remove(notifier, range);
  	drm_gpusvm_notifier_unlock(gpusvm);
@@ -1266,6 +1266,14 @@ void drm_gpusvm_range_put(struct drm_gpusvm_range *range)
  }
  EXPORT_SYMBOL_GPL(drm_gpusvm_range_put);

Same comment as patch #1, let's aim to keep kernel doc for all functions in drm_gpusvm.c.

+static bool drm_gpusvm_pages_valid(struct drm_gpusvm *gpusvm,
+				   struct drm_gpusvm_pages *svm_pages)
+{
+	lockdep_assert_held(&gpusvm->notifier_lock);
+
+	return svm_pages->flags.has_devmem_pages || svm_pages->flags.has_dma_mapping;
+}
+
  /**
   * drm_gpusvm_range_pages_valid() - GPU SVM range pages valid
   * @gpusvm: Pointer to the GPU SVM structure
@@ -1283,9 +1291,7 @@ EXPORT_SYMBOL_GPL(drm_gpusvm_range_put);
  bool drm_gpusvm_range_pages_valid(struct drm_gpusvm *gpusvm,
  				  struct drm_gpusvm_range *range)
  {
-	lockdep_assert_held(&gpusvm->notifier_lock);
-
-	return range->flags.has_devmem_pages || range->flags.has_dma_mapping;
+	return drm_gpusvm_pages_valid(gpusvm, &range->pages);
  }
  EXPORT_SYMBOL_GPL(drm_gpusvm_range_pages_valid);
@@ -1301,17 +1307,17 @@ EXPORT_SYMBOL_GPL(drm_gpusvm_range_pages_valid);
   */
  static bool
  drm_gpusvm_range_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
-				      struct drm_gpusvm_range *range)
+				      struct drm_gpusvm_pages *svm_pages)
  {
  	bool pages_valid;
- if (!range->dma_addr)
+	if (!svm_pages->dma_addr)
  		return false;
drm_gpusvm_notifier_lock(gpusvm);
-	pages_valid = drm_gpusvm_range_pages_valid(gpusvm, range);
+	pages_valid = drm_gpusvm_pages_valid(gpusvm, svm_pages);
  	if (!pages_valid)
-		drm_gpusvm_range_free_pages(gpusvm, range);
+		drm_gpusvm_free_pages(gpusvm, svm_pages);
  	drm_gpusvm_notifier_unlock(gpusvm);
return pages_valid;
@@ -1332,6 +1338,7 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  			       struct drm_gpusvm_range *range,
  			       const struct drm_gpusvm_ctx *ctx)
  {
+	struct drm_gpusvm_pages *svm_pages = &range->pages;
  	struct mmu_interval_notifier *notifier = &range->notifier->notifier;
  	struct hmm_range hmm_range = {
  		.default_flags = HMM_PFN_REQ_FAULT | (ctx->read_only ? 0 :
@@ -1360,7 +1367,7 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
retry:
  	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
-	if (drm_gpusvm_range_pages_valid_unlocked(gpusvm, range))
+	if (drm_gpusvm_range_pages_valid_unlocked(gpusvm, svm_pages))
  		goto set_seqno;
pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
@@ -1401,7 +1408,7 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  	 */
  	drm_gpusvm_notifier_lock(gpusvm);
- if (range->flags.unmapped) {
+	if (svm_pages->flags.unmapped) {
  		drm_gpusvm_notifier_unlock(gpusvm);
  		err = -EFAULT;
  		goto err_free;
@@ -1413,13 +1420,12 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  		goto retry;
  	}
- if (!range->dma_addr) {
+	if (!svm_pages->dma_addr) {
  		/* Unlock and restart mapping to allocate memory. */
  		drm_gpusvm_notifier_unlock(gpusvm);
-		range->dma_addr = kvmalloc_array(npages,
-						 sizeof(*range->dma_addr),
-						 GFP_KERNEL);
-		if (!range->dma_addr) {
+		svm_pages->dma_addr =
+			kvmalloc_array(npages, sizeof(*svm_pages->dma_addr), GFP_KERNEL);
+		if (!svm_pages->dma_addr) {
  			err = -ENOMEM;
  			goto err_free;
  		}
@@ -1457,13 +1463,13 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  					goto err_unmap;
  				}
  			}
-			range->dma_addr[j] =
+			svm_pages->dma_addr[j] =
  				dpagemap->ops->device_map(dpagemap,
  							  gpusvm->drm->dev,
  							  page, order,
  							  dma_dir);
  			if (dma_mapping_error(gpusvm->drm->dev,
-					      range->dma_addr[j].addr)) {
+					      svm_pages->dma_addr[j].addr)) {
  				err = -EFAULT;
  				goto err_unmap;
  			}
@@ -1487,7 +1493,7 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  				goto err_unmap;
  			}
- range->dma_addr[j] = drm_pagemap_device_addr_encode
+			svm_pages->dma_addr[j] = drm_pagemap_device_addr_encode
  				(addr, DRM_INTERCONNECT_SYSTEM, order,
  				 dma_dir);
@@ -1503,10 +1509,10 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  		num_dma_mapped = i;
  	}
- range->flags.has_dma_mapping = true;
+	svm_pages->flags.has_dma_mapping = true;
  	if (zdd) {
-		range->flags.has_devmem_pages = true;
-		range->dpagemap = dpagemap;
+		svm_pages->flags.has_devmem_pages = true;
+		svm_pages->dpagemap = dpagemap;
  	}
drm_gpusvm_notifier_unlock(gpusvm);
@@ -1517,7 +1523,7 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
  	return 0;
err_unmap:
-	__drm_gpusvm_range_unmap_pages(gpusvm, range, num_dma_mapped);
+	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
  	drm_gpusvm_notifier_unlock(gpusvm);
  err_free:
  	kvfree(pfns);
@@ -1543,6 +1549,7 @@ void drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
  				  struct drm_gpusvm_range *range,
  				  const struct drm_gpusvm_ctx *ctx)
  {
+	struct drm_gpusvm_pages *svm_pages = &range->pages;
  	unsigned long npages = npages_in_range(drm_gpusvm_range_start(range),
  					       drm_gpusvm_range_end(range));
@@ -1551,7 +1558,7 @@ void drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
  	else
  		drm_gpusvm_notifier_lock(gpusvm);
- __drm_gpusvm_range_unmap_pages(gpusvm, range, npages);
+	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, npages);
if (!ctx->in_notifier)
  		drm_gpusvm_notifier_unlock(gpusvm);
@@ -1719,7 +1726,7 @@ int drm_gpusvm_migrate_to_devmem(struct drm_gpusvm *gpusvm,
mmap_assert_locked(gpusvm->mm); - if (!range->flags.migrate_devmem)
+	if (!range->pages.flags.migrate_devmem)
  		return -EINVAL;
if (!ops->populate_devmem_pfn || !ops->copy_to_devmem ||
@@ -2248,10 +2255,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range,
  {
  	lockdep_assert_held_write(&range->gpusvm->notifier_lock);
- range->flags.unmapped = true;
+	range->pages.flags.unmapped = true;
  	if (drm_gpusvm_range_start(range) < mmu_range->start ||
  	    drm_gpusvm_range_end(range) > mmu_range->end)
-		range->flags.partial_unmap = true;
+		range->pages.flags.partial_unmap = true;
  }
  EXPORT_SYMBOL_GPL(drm_gpusvm_range_set_unmapped);
diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
index ffaf0d02dc7d..c43e7619cb80 100644
--- a/drivers/gpu/drm/xe/xe_pt.c
+++ b/drivers/gpu/drm/xe/xe_pt.c
@@ -659,7 +659,7 @@ xe_pt_stage_bind(struct xe_tile *tile, struct xe_vma *vma,
  			return -EAGAIN;
  		}
  		if (xe_svm_range_has_dma_mapping(range)) {
-			xe_res_first_dma(range->base.dma_addr, 0,
+			xe_res_first_dma(range->base.pages.dma_addr, 0,
  					 range->base.itree.last + 1 - range->base.itree.start,
  					 &curs);
  			is_devmem = xe_res_is_vram(&curs);
diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
index 08617a62ab07..4e7f2e77b38d 100644
--- a/drivers/gpu/drm/xe/xe_svm.c
+++ b/drivers/gpu/drm/xe/xe_svm.c
@@ -17,7 +17,7 @@
  static bool xe_svm_range_in_vram(struct xe_svm_range *range)
  {
  	/* Not reliable without notifier lock */
-	return range->base.flags.has_devmem_pages;
+	return range->base.pages.flags.has_devmem_pages;
  }
static bool xe_svm_range_has_vram_binding(struct xe_svm_range *range)
@@ -135,7 +135,7 @@ xe_svm_range_notifier_event_begin(struct xe_vm *vm, struct drm_gpusvm_range *r,
  	range_debug(range, "NOTIFIER");
/* Skip if already unmapped or if no binding exist */
-	if (range->base.flags.unmapped || !range->tile_present)
+	if (range->base.pages.flags.unmapped || !range->tile_present)
  		return 0;
range_debug(range, "NOTIFIER - EXECUTE");
@@ -766,7 +766,7 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
  	range_debug(range, "PAGE FAULT");
/* XXX: Add migration policy, for now migrate range once */
-	if (!range->skip_migrate && range->base.flags.migrate_devmem &&
+	if (!range->skip_migrate && range->base.pages.flags.migrate_devmem &&
  	    xe_svm_range_size(range) >= SZ_64K) {
  		range->skip_migrate = true;
diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
index 93442738666e..79fbd4fec1fb 100644
--- a/drivers/gpu/drm/xe/xe_svm.h
+++ b/drivers/gpu/drm/xe/xe_svm.h
@@ -136,7 +136,7 @@ void xe_svm_range_debug(struct xe_svm_range *range, const char *operation)
  static inline bool xe_svm_range_has_dma_mapping(struct xe_svm_range *range)
  {
  	lockdep_assert_held(&range->base.gpusvm->notifier_lock);
-	return range->base.flags.has_dma_mapping;
+	return range->base.pages.flags.has_dma_mapping;
  }
#define xe_svm_assert_in_notifier(vm__) \
diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index df120b4d1f83..c15c733ef0ad 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -186,14 +186,8 @@ struct drm_gpusvm_notifier {
  };
/**
- * struct drm_gpusvm_range - Structure representing a GPU SVM range
+ * struct drm_gpusvm_pages - Structure representing a GPU SVM mapped pages
   *
- * @gpusvm: Pointer to the GPU SVM structure
- * @notifier: Pointer to the GPU SVM notifier
- * @refcount: Reference count for the range
- * @itree: Interval tree node for the range (inserted in GPU SVM notifier)
- * @entry: List entry to fast interval tree traversal
- * @notifier_seq: Notifier sequence number of the range's pages
   * @dma_addr: Device address array
   * @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.
@@ -203,17 +197,8 @@ struct drm_gpusvm_notifier {
   * @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.
   */
-struct drm_gpusvm_range {
-	struct drm_gpusvm *gpusvm;
-	struct drm_gpusvm_notifier *notifier;
-	struct kref refcount;
-	struct interval_tree_node itree;
-	struct list_head entry;
-	unsigned long notifier_seq;
+struct drm_gpusvm_pages {
  	struct drm_pagemap_device_addr *dma_addr;
  	struct drm_pagemap *dpagemap;
  	struct {
@@ -227,6 +212,30 @@ struct drm_gpusvm_range {
  	} flags;
  };
+/**
+ * struct drm_gpusvm_range - Structure representing a GPU SVM range
+ *
+ * @gpusvm: Pointer to the GPU SVM structure
+ * @notifier: Pointer to the GPU SVM notifier
+ * @refcount: Reference count for the range
+ * @itree: Interval tree node for the range (inserted in GPU SVM notifier)
+ * @entry: List entry to fast interval tree traversal
+ * @notifier_seq: Notifier sequence number of the range's pages
+ * @pages: The pages for this range.
+ *
+ * This structure represents a GPU SVM range used for tracking memory ranges
+ * mapped in a DRM device.
+ */
+struct drm_gpusvm_range {
+	struct drm_gpusvm *gpusvm;
+	struct drm_gpusvm_notifier *notifier;
+	struct kref refcount;
+	struct interval_tree_node itree;
+	struct list_head entry;
+	unsigned long notifier_seq;

Should the notifier seqno be in the pages?

Yeah, I think that makes sense. Thanks.


Matt

+	struct drm_gpusvm_pages pages;
+};
+
  /**
   * struct drm_gpusvm - GPU SVM structure
   *
--
2.48.1





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux