[PATCH v4 0/3] Add PASID to fpriv lookup infrastructure

[PATCH v4 0/3] Add PASID to fpriv lookup infrastructure

[Srinivasan Shanmugam]

v4: (per Christian)
- Extend amdgpu_pasid_alloc() to optionally store the DRM file-private
  owner directly.
- Allocate DRM PASIDs after fpriv initialization instead of registering
  the owner later.
- Drop the separate amdgpu_pasid_set_fpriv()/clear_fpriv() helpers.
- Squash PASID owner registration and VM lookup conversion into a
  single patch.
- Clear PASID ownership from the delayed PASID free path instead of
  using a separate unregister helper.

Only compilation tested.

Srinivasan Shanmugam (3):
  drm/amdgpu: Allow PASID allocator to store fpriv owner
  drm/amdgpu: Resolve VM through DRM PASID ownership
  drm/amdgpu: Drop vm_manager PASID to VM mapping

 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 82 +++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 16 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 65 +++++++++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  4 --
 drivers/gpu/drm/amd/amdgpu/mes_v12_1.c  |  2 +-
 6 files changed, 120 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] drm/amdgpu: Allow PASID allocator to store fpriv owner

[Srinivasan Shanmugam]

AMDGPU already has a global PASID xarray used by the PASID allocator.

Allow amdgpu_pasid_alloc() to optionally store the owning DRM
file-private object directly.

Initial callers pass NULL and keep the current dummy allocation marker
behavior. A later patch in this series passes the DRM file-private
object for DRM PASIDs.

This prepares for using:

	PASID -> fpriv -> VM

instead of:

	PASID -> VM

Also clear any stored owner from amdgpu_pasid_free_delayed() before
waiting for fences, so PASID lookups cannot observe a stale fpriv while
the PASID number itself is still pending delayed release.

v4: (per Christian)
- Add fpriv as an optional parameter to amdgpu_pasid_alloc().
- Drop separate amdgpu_pasid_set_fpriv()/clear_fpriv() helpers.
- Clear PASID owner from amdgpu_pasid_free_delayed().

Cc: Alex Deucher <alexander.deucher@amd.com>
Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 82 +++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/mes_v12_1.c  |  2 +-
 4 files changed, 85 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 684f40fce73f..669d0fff8cbc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -52,6 +52,7 @@ struct amdgpu_pasid_cb {
 /**
  * amdgpu_pasid_alloc - Allocate a PASID
  * @bits: Maximum width of the PASID in bits, must be at least 1
+ * @fpriv: optional DRM file-private owner
  *
  * Uses kernel's IDR cyclic allocator (same as PID allocation).
  * Allocates sequentially with automatic wrap-around.
@@ -60,17 +61,19 @@ struct amdgpu_pasid_cb {
  * Returns %-ENOSPC if no PASID was available. Returns %-ENOMEM on
  * memory allocation failure.
  */
-int amdgpu_pasid_alloc(unsigned int bits)
+int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv)
 {
 	u32 pasid;
 	int r;
+	void *entry;
 
 	if (bits == 0)
 		return -EINVAL;
 
-	r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, xa_mk_value(0),
-			    XA_LIMIT(1, (1U << bits) - 1),
-			    &amdgpu_pasid_xa_next, GFP_KERNEL);
+	entry = fpriv ? fpriv : xa_mk_value(0);
+	r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, entry,
+				XA_LIMIT(1, (1U << bits) - 1),
+				&amdgpu_pasid_xa_next, GFP_KERNEL);
 	if (r < 0)
 		return r;
 
@@ -78,6 +81,75 @@ int amdgpu_pasid_alloc(unsigned int bits)
 	return pasid;
 }
 
+/**
+ * amdgpu_pasid_clear_owner - Remove the owner associated with a PASID
+ * @pasid: PASID whose owner should be cleared
+ *
+ * Restore a PASID entry back to the allocation marker while keeping the
+ * PASID itself allocated.
+ *
+ * This is used by the delayed PASID free path so that future PASID
+ * lookups cannot resolve a stale DRM file-private object while the PASID
+ * is still waiting for outstanding fences before being released.
+ */
+static void amdgpu_pasid_clear_owner(u32 pasid)
+{
+	unsigned long flags;
+	void *entry;
+
+	if (!pasid)
+		return;
+
+	xa_lock_irqsave(&amdgpu_pasid_xa, flags);
+	entry = xa_load(&amdgpu_pasid_xa, pasid);
+	if (entry && !xa_is_value(entry))
+		__xa_store(&amdgpu_pasid_xa, pasid, xa_mk_value(0),
+			   GFP_ATOMIC);
+	xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
+}
+
+/**
+ * amdgpu_pasid_lock - acquire the global PASID xarray lock
+ * @flags: storage for interrupt state
+ *
+ * Acquire the global PASID xarray lock with interrupts disabled.
+ * The saved interrupt state must be passed to
+ * amdgpu_pasid_unlock().
+ */
+void amdgpu_pasid_lock(unsigned long *flags)
+{
+	xa_lock_irqsave(&amdgpu_pasid_xa, *flags);
+}
+
+/**
+ * amdgpu_pasid_unlock - release the global PASID xarray lock
+ * @flags: interrupt state returned by amdgpu_pasid_lock()
+ *
+ * Release the global PASID xarray lock and restore the previous
+ * interrupt state.
+ */
+void amdgpu_pasid_unlock(unsigned long flags)
+{
+	xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
+}
+
+/**
+ * amdgpu_pasid_get_fpriv_locked - get fpriv from PASID
+ * @pasid: PASID to resolve
+ *
+ * Caller must hold the PASID XA lock.
+ */
+struct amdgpu_fpriv *amdgpu_pasid_get_fpriv_locked(u32 pasid)
+{
+	void *entry;
+
+	entry = xa_load(&amdgpu_pasid_xa, pasid);
+	if (!entry || xa_is_value(entry))
+		return NULL;
+
+	return entry;
+}
+
 /**
  * amdgpu_pasid_free - Free a PASID
  * @pasid: PASID to free
@@ -121,6 +193,8 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
 	struct dma_fence *fence;
 	int r;
 
+	amdgpu_pasid_clear_owner(pasid);
+
 	r = dma_resv_get_singleton(resv, DMA_RESV_USAGE_BOOKKEEP, &fence);
 	if (r)
 		goto fallback;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
index a57919478d3b..c2be6f81d680 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
@@ -38,6 +38,7 @@ struct amdgpu_vm;
 struct amdgpu_ring;
 struct amdgpu_sync;
 struct amdgpu_job;
+struct amdgpu_fpriv;
 
 struct amdgpu_vmid {
 	struct list_head	list;
@@ -70,8 +71,11 @@ struct amdgpu_vmid_mgr {
 	bool			reserved_vmid;
 };
 
-int amdgpu_pasid_alloc(unsigned int bits);
+int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv);
 void amdgpu_pasid_free(u32 pasid);
+void amdgpu_pasid_lock(unsigned long *flags);
+void amdgpu_pasid_unlock(unsigned long flags);
+struct amdgpu_fpriv *amdgpu_pasid_get_fpriv_locked(u32 pasid);
 void amdgpu_pasid_free_delayed(struct dma_resv *resv,
 			       u32 pasid);
 void amdgpu_pasid_mgr_cleanup(void);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index cacdc99b3ad6..4610d6889e9b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1487,7 +1487,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 		goto out_suspend;
 	}
 
-	pasid = amdgpu_pasid_alloc(16);
+	pasid = amdgpu_pasid_alloc(16, NULL);
 	if (pasid < 0) {
 		dev_warn(adev->dev, "No more PASIDs available!");
 		pasid = 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
index f7d5879c6e44..65b824144a2f 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
@@ -2293,7 +2293,7 @@ static int mes_v12_1_self_test(struct amdgpu_device *adev, int xcc_id)
 	u64 meta_gpu_addr, ctx_gpu_addr;
 	int size, i, r, pasid;
 
-	pasid = amdgpu_pasid_alloc(16);
+	pasid = amdgpu_pasid_alloc(16, NULL);
 	if (pasid < 0)
 		pasid = 0;
 
-- 
2.34.1

[PATCH v4 2/3] drm/amdgpu: Resolve VM through DRM PASID ownership

[Srinivasan Shanmugam]

Allocate DRM PASIDs with fpriv and resolve VM lookup users through:

	PASID -> fpriv -> VM

This preserves the root BO reference and revalidation flow in
amdgpu_vm_lock_by_pasid().

v4:
- Allocate DRM PASIDs with fpriv directly.
- Squash ownership registration and PASID lookup conversion.

Cc: Alex Deucher <alexander.deucher@amd.com>
Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 16 +++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 ++++++++++++++++---------
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 4610d6889e9b..087ca7783d1b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1375,11 +1375,11 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 
 		memset(&gpuvm_fault, 0, sizeof(gpuvm_fault));
 
-		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+		amdgpu_pasid_lock(&flags);
 		gpuvm_fault.addr = vm->fault_info.addr;
 		gpuvm_fault.status = vm->fault_info.status;
 		gpuvm_fault.vmhub = vm->fault_info.vmhub;
-		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
+		amdgpu_pasid_unlock(flags);
 
 		return copy_to_user(out, &gpuvm_fault,
 				    min((size_t)size, sizeof(gpuvm_fault))) ? -EFAULT : 0;
@@ -1464,7 +1464,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 	struct amdgpu_device *adev = drm_to_adev(dev);
 	struct amdgpu_fpriv *fpriv;
 	struct drm_exec exec;
-	int r, pasid;
+	int r, pasid = 0;
 
 	/* Ensure IB tests are run on ring */
 	flush_delayed_work(&adev->delayed_init_work);
@@ -1487,16 +1487,16 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 		goto out_suspend;
 	}
 
-	pasid = amdgpu_pasid_alloc(16, NULL);
+	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
+	if (r)
+		goto error_pasid;
+
+	pasid = amdgpu_pasid_alloc(16, fpriv);
 	if (pasid < 0) {
 		dev_warn(adev->dev, "No more PASIDs available!");
 		pasid = 0;
 	}
 
-	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
-	if (r)
-		goto error_pasid;
-
 	amdgpu_debugfs_vm_init(file_priv);
 
 	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, pasid);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 32719f31b6c9..9092ff227a55 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2463,12 +2463,14 @@ static void amdgpu_vm_destroy_task_info(struct kref *kref)
 static inline struct amdgpu_vm *
 amdgpu_vm_get_vm_from_pasid(struct amdgpu_device *adev, u32 pasid)
 {
+	struct amdgpu_fpriv *fpriv;
 	struct amdgpu_vm *vm;
 	unsigned long flags;
 
-	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
-	vm = xa_load(&adev->vm_manager.pasids, pasid);
-	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
+	amdgpu_pasid_lock(&flags);
+	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
+	vm = fpriv ? &fpriv->vm : NULL;
+	amdgpu_pasid_unlock(flags);
 
 	return vm;
 }
@@ -2943,14 +2945,16 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
 					  u32 pasid, struct drm_exec *exec)
 {
 	unsigned long irqflags;
+	struct amdgpu_fpriv *fpriv;
 	struct amdgpu_bo *root;
 	struct amdgpu_vm *vm;
 	int r;
 
-	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
-	vm = xa_load(&adev->vm_manager.pasids, pasid);
-	root = vm ? amdgpu_bo_ref(vm->root.bo) : NULL;
-	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
+	amdgpu_pasid_lock(&irqflags);
+	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
+	vm = fpriv ? &fpriv->vm : NULL;
+	root = vm && vm->root.bo ? amdgpu_bo_ref(vm->root.bo) : NULL;
+	amdgpu_pasid_unlock(irqflags);
 
 	if (!root)
 		return NULL;
@@ -2962,11 +2966,17 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
 	}
 
 	/* Double check that the VM still exists */
-	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
-	vm = xa_load(&adev->vm_manager.pasids, pasid);
-	if (vm && vm->root.bo != root)
+	amdgpu_pasid_lock(&irqflags);
+	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
+	if (!fpriv) {
 		vm = NULL;
-	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
+	} else {
+		vm = &fpriv->vm;
+		if (vm->root.bo != root)
+			vm = NULL;
+	}
+	amdgpu_pasid_unlock(irqflags);
+
 	if (!vm) {
 		drm_exec_unlock_obj(exec, &root->tbo.base);
 		amdgpu_bo_unref(&root);
@@ -3163,12 +3173,14 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
 				  uint32_t status,
 				  unsigned int vmhub)
 {
+	struct amdgpu_fpriv *fpriv;
 	struct amdgpu_vm *vm;
 	unsigned long flags;
 
-	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+	amdgpu_pasid_lock(&flags);
 
-	vm = xa_load(&adev->vm_manager.pasids, pasid);
+	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
+	vm = fpriv ? &fpriv->vm : NULL;
 	/* Don't update the fault cache if status is 0.  In the multiple
 	 * fault case, subsequent faults will return a 0 status which is
 	 * useless for userspace and replaces the useful fault status, so
@@ -3201,7 +3213,7 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
 			WARN_ONCE(1, "Invalid vmhub %u\n", vmhub);
 		}
 	}
-	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
+	amdgpu_pasid_unlock(flags);
 }
 
 void amdgpu_vm_print_task_info(struct amdgpu_device *adev,
-- 
2.34.1

[PATCH v4 3/3] drm/amdgpu: Drop vm_manager PASID to VM mapping

[Srinivasan Shanmugam]

VM lookup users now resolve DRM PASIDs through the global PASID xarray:

	PASID -> fpriv -> VM

The per-device vm_manager.pasids xarray is no longer needed.

Remove PASID registration and teardown from VM init/fini paths, drop
vm_manager PASID initialization/cleanup, and remove the xarray from
struct amdgpu_vm_manager.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 ++-----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 ----
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9092ff227a55..74836240edbb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2647,14 +2647,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		dev_dbg(adev->dev, "Failed to create task info for VM\n");
 
-	/* Store new PASID in XArray (if non-zero) */
-	if (pasid != 0) {
-		r = xa_err(xa_store_irq(&adev->vm_manager.pasids, pasid, vm, GFP_KERNEL));
-		if (r < 0)
-			goto error_free_root;
-
-		vm->pasid = pasid;
-	}
+	vm->pasid = pasid;
 
 	amdgpu_bo_unreserve(vm->root.bo);
 	amdgpu_bo_unref(&root_bo);
@@ -2662,11 +2655,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	return 0;
 
 error_free_root:
-	/* If PASID was partially set, erase it from XArray before failing */
-	if (vm->pasid != 0) {
-		xa_erase_irq(&adev->vm_manager.pasids, vm->pasid);
-		vm->pasid = 0;
-	}
+	vm->pasid = 0;
 	amdgpu_vm_pt_free_root(adev, vm);
 	amdgpu_bo_unreserve(vm->root.bo);
 	amdgpu_bo_unref(&root_bo);
@@ -2773,11 +2762,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 
 	root = amdgpu_bo_ref(vm->root.bo);
 	amdgpu_bo_reserve(root, true);
-	/* Remove PASID mapping before destroying VM */
-	if (vm->pasid != 0) {
-		xa_erase_irq(&adev->vm_manager.pasids, vm->pasid);
-		vm->pasid = 0;
-	}
 	dma_fence_wait(vm->last_unlocked, false);
 	dma_fence_put(vm->last_unlocked);
 	dma_fence_wait(vm->last_tlb_flush, false);
@@ -2873,8 +2857,6 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
 #else
 	adev->vm_manager.vm_update_mode = 0;
 #endif
-
-	xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
 }
 
 /**
@@ -2886,9 +2868,6 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
  */
 void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
 {
-	WARN_ON(!xa_empty(&adev->vm_manager.pasids));
-	xa_destroy(&adev->vm_manager.pasids);
-
 	amdgpu_vmid_mgr_fini(adev);
 	amdgpu_pasid_mgr_cleanup();
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 939f639cd8bf..f63364f128bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -481,10 +481,6 @@ struct amdgpu_vm_manager {
 	 */
 	int					vm_update_mode;
 
-	/* PASID to VM mapping, will be used in interrupt context to
-	 * look up VM of a page fault
-	 */
-	struct xarray				pasids;
 	/* Global registration of recent page fault information */
 	struct amdgpu_vm_fault_info	fault_info;
 };
-- 
2.34.1

Re: [PATCH v4 1/3] drm/amdgpu: Allow PASID allocator to store fpriv owner

[Christian König]

On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> AMDGPU already has a global PASID xarray used by the PASID allocator.
> 
> Allow amdgpu_pasid_alloc() to optionally store the owning DRM
> file-private object directly.
> 
> Initial callers pass NULL and keep the current dummy allocation marker
> behavior. A later patch in this series passes the DRM file-private
> object for DRM PASIDs.
> 
> This prepares for using:
> 
> 	PASID -> fpriv -> VM
> 
> instead of:
> 
> 	PASID -> VM
> 
> Also clear any stored owner from amdgpu_pasid_free_delayed() before
> waiting for fences, so PASID lookups cannot observe a stale fpriv while
> the PASID number itself is still pending delayed release.
> 
> v4: (per Christian)
> - Add fpriv as an optional parameter to amdgpu_pasid_alloc().
> - Drop separate amdgpu_pasid_set_fpriv()/clear_fpriv() helpers.
> - Clear PASID owner from amdgpu_pasid_free_delayed().
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 82 +++++++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  6 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
>  drivers/gpu/drm/amd/amdgpu/mes_v12_1.c  |  2 +-
>  4 files changed, 85 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 684f40fce73f..669d0fff8cbc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -52,6 +52,7 @@ struct amdgpu_pasid_cb {
>  /**
>   * amdgpu_pasid_alloc - Allocate a PASID
>   * @bits: Maximum width of the PASID in bits, must be at least 1
> + * @fpriv: optional DRM file-private owner
>   *
>   * Uses kernel's IDR cyclic allocator (same as PID allocation).
>   * Allocates sequentially with automatic wrap-around.
> @@ -60,17 +61,19 @@ struct amdgpu_pasid_cb {
>   * Returns %-ENOSPC if no PASID was available. Returns %-ENOMEM on
>   * memory allocation failure.
>   */
> -int amdgpu_pasid_alloc(unsigned int bits)
> +int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv)
>  {
>  	u32 pasid;
>  	int r;
> +	void *entry;
>  
>  	if (bits == 0)
>  		return -EINVAL;
>  
> -	r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, xa_mk_value(0),
> -			    XA_LIMIT(1, (1U << bits) - 1),
> -			    &amdgpu_pasid_xa_next, GFP_KERNEL);
> +	entry = fpriv ? fpriv : xa_mk_value(0);

That's problematic I think. The xa_mk_value(0) value is not NULL and needs to be filtered out when somebody looks the fpriv up using the array.

But xa_insert() can handle NULL entries, but I'm not sure if xa_alloc_cyclic_irq() can do that as well.

> +	r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, entry,
> +				XA_LIMIT(1, (1U << bits) - 1),
> +				&amdgpu_pasid_xa_next, GFP_KERNEL);
>  	if (r < 0)
>  		return r;
>  
> @@ -78,6 +81,75 @@ int amdgpu_pasid_alloc(unsigned int bits)
>  	return pasid;
>  }
>  
> +/**
> + * amdgpu_pasid_clear_owner - Remove the owner associated with a PASID
> + * @pasid: PASID whose owner should be cleared
> + *
> + * Restore a PASID entry back to the allocation marker while keeping the
> + * PASID itself allocated.
> + *
> + * This is used by the delayed PASID free path so that future PASID
> + * lookups cannot resolve a stale DRM file-private object while the PASID
> + * is still waiting for outstanding fences before being released.
> + */
> +static void amdgpu_pasid_clear_owner(u32 pasid)
> +{
> +	unsigned long flags;
> +	void *entry;
> +
> +	if (!pasid)
> +		return;
> +
> +	xa_lock_irqsave(&amdgpu_pasid_xa, flags);

> +	entry = xa_load(&amdgpu_pasid_xa, pasid);
> +	if (entry && !xa_is_value(entry))

I think you should just skip this test and use xa_store to overwrite the fpriv value.

Alternative xa_cmpxchg() could be used to replace the value.

> +		__xa_store(&amdgpu_pasid_xa, pasid, xa_mk_value(0),
> +			   GFP_ATOMIC);
> +	xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
> +}
> +
> +/**
> + * amdgpu_pasid_lock - acquire the global PASID xarray lock
> + * @flags: storage for interrupt state
> + *
> + * Acquire the global PASID xarray lock with interrupts disabled.
> + * The saved interrupt state must be passed to
> + * amdgpu_pasid_unlock().
> + */
> +void amdgpu_pasid_lock(unsigned long *flags)
> +{
> +	xa_lock_irqsave(&amdgpu_pasid_xa, *flags);
> +}
> +
> +/**
> + * amdgpu_pasid_unlock - release the global PASID xarray lock
> + * @flags: interrupt state returned by amdgpu_pasid_lock()
> + *
> + * Release the global PASID xarray lock and restore the previous
> + * interrupt state.
> + */
> +void amdgpu_pasid_unlock(unsigned long flags)
> +{
> +	xa_unlock_irqrestore(&amdgpu_pasid_xa, flags);
> +}
> +
> +/**
> + * amdgpu_pasid_get_fpriv_locked - get fpriv from PASID
> + * @pasid: PASID to resolve
> + *
> + * Caller must hold the PASID XA lock.
> + */
> +struct amdgpu_fpriv *amdgpu_pasid_get_fpriv_locked(u32 pasid)
> +{
> +	void *entry;
> +
> +	entry = xa_load(&amdgpu_pasid_xa, pasid);
> +	if (!entry || xa_is_value(entry))
> +		return NULL;
> +
> +	return entry;
> +}
> +
>  /**
>   * amdgpu_pasid_free - Free a PASID
>   * @pasid: PASID to free
> @@ -121,6 +193,8 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
>  	struct dma_fence *fence;
>  	int r;
>  
> +	amdgpu_pasid_clear_owner(pasid);
> +
>  	r = dma_resv_get_singleton(resv, DMA_RESV_USAGE_BOOKKEEP, &fence);
>  	if (r)
>  		goto fallback;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> index a57919478d3b..c2be6f81d680 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> @@ -38,6 +38,7 @@ struct amdgpu_vm;
>  struct amdgpu_ring;
>  struct amdgpu_sync;
>  struct amdgpu_job;
> +struct amdgpu_fpriv;
>  
>  struct amdgpu_vmid {
>  	struct list_head	list;
> @@ -70,8 +71,11 @@ struct amdgpu_vmid_mgr {
>  	bool			reserved_vmid;
>  };
>  
> -int amdgpu_pasid_alloc(unsigned int bits);
> +int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv);
>  void amdgpu_pasid_free(u32 pasid);
> +void amdgpu_pasid_lock(unsigned long *flags);
> +void amdgpu_pasid_unlock(unsigned long flags);
> +struct amdgpu_fpriv *amdgpu_pasid_get_fpriv_locked(u32 pasid);
>  void amdgpu_pasid_free_delayed(struct dma_resv *resv,
>  			       u32 pasid);
>  void amdgpu_pasid_mgr_cleanup(void);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index cacdc99b3ad6..4610d6889e9b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1487,7 +1487,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  		goto out_suspend;
>  	}
>  
> -	pasid = amdgpu_pasid_alloc(16);
> +	pasid = amdgpu_pasid_alloc(16, NULL);
>  	if (pasid < 0) {
>  		dev_warn(adev->dev, "No more PASIDs available!");
>  		pasid = 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> index f7d5879c6e44..65b824144a2f 100644

> --- a/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_1.c
> @@ -2293,7 +2293,7 @@ static int mes_v12_1_self_test(struct amdgpu_device *adev, int xcc_id)
>  	u64 meta_gpu_addr, ctx_gpu_addr;
>  	int size, i, r, pasid;
>  
> -	pasid = amdgpu_pasid_alloc(16);
> +	pasid = amdgpu_pasid_alloc(16, NULL);
>  	if (pasid < 0)
>  		pasid = 0;
>  

Please prepare a patch to completely remove the self test for MES v12. We have already removed the self test for other MES versions because the IGT tests now take care of that.

Regards,
Christian.

Re: [PATCH v4 2/3] drm/amdgpu: Resolve VM through DRM PASID ownership

[Christian König]

On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> Allocate DRM PASIDs with fpriv and resolve VM lookup users through:
> 
> 	PASID -> fpriv -> VM
> 
> This preserves the root BO reference and revalidation flow in
> amdgpu_vm_lock_by_pasid().
> 
> v4:
> - Allocate DRM PASIDs with fpriv directly.
> - Squash ownership registration and PASID lookup conversion.
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 16 +++++-----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 ++++++++++++++++---------
>  2 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 4610d6889e9b..087ca7783d1b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1375,11 +1375,11 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  
>  		memset(&gpuvm_fault, 0, sizeof(gpuvm_fault));
>  
> -		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> +		amdgpu_pasid_lock(&flags);
>  		gpuvm_fault.addr = vm->fault_info.addr;
>  		gpuvm_fault.status = vm->fault_info.status;
>  		gpuvm_fault.vmhub = vm->fault_info.vmhub;
> -		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +		amdgpu_pasid_unlock(flags);
>  
>  		return copy_to_user(out, &gpuvm_fault,
>  				    min((size_t)size, sizeof(gpuvm_fault))) ? -EFAULT : 0;
> @@ -1464,7 +1464,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	struct amdgpu_device *adev = drm_to_adev(dev);
>  	struct amdgpu_fpriv *fpriv;
>  	struct drm_exec exec;
> -	int r, pasid;
> +	int r, pasid = 0;
>  
>  	/* Ensure IB tests are run on ring */
>  	flush_delayed_work(&adev->delayed_init_work);
> @@ -1487,16 +1487,16 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  		goto out_suspend;
>  	}
>  
> -	pasid = amdgpu_pasid_alloc(16, NULL);
> +	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
> +	if (r)
> +		goto error_pasid;
> +
> +	pasid = amdgpu_pasid_alloc(16, fpriv);

That needs to come even later, e.g. after the VM is initialized.

Otherwise we would run into a bunch of trouble should there be any stale entries for this PASID in the interrupt ring buffers.

Apart from that looks good to me,
Christian.

>  	if (pasid < 0) {
>  		dev_warn(adev->dev, "No more PASIDs available!");
>  		pasid = 0;
>  	}
>  
> -	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
> -	if (r)
> -		goto error_pasid;
> -
>  	amdgpu_debugfs_vm_init(file_priv);
>  
>  	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, pasid);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 32719f31b6c9..9092ff227a55 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2463,12 +2463,14 @@ static void amdgpu_vm_destroy_task_info(struct kref *kref)
>  static inline struct amdgpu_vm *
>  amdgpu_vm_get_vm_from_pasid(struct amdgpu_device *adev, u32 pasid)
>  {
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_vm *vm;
>  	unsigned long flags;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_lock(&flags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
> +	amdgpu_pasid_unlock(flags);
>  
>  	return vm;
>  }
> @@ -2943,14 +2945,16 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
>  					  u32 pasid, struct drm_exec *exec)
>  {
>  	unsigned long irqflags;
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_bo *root;
>  	struct amdgpu_vm *vm;
>  	int r;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> -	root = vm ? amdgpu_bo_ref(vm->root.bo) : NULL;
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> +	amdgpu_pasid_lock(&irqflags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
> +	root = vm && vm->root.bo ? amdgpu_bo_ref(vm->root.bo) : NULL;
> +	amdgpu_pasid_unlock(irqflags);
>  
>  	if (!root)
>  		return NULL;
> @@ -2962,11 +2966,17 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
>  	}
>  
>  	/* Double check that the VM still exists */
> -	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> -	if (vm && vm->root.bo != root)
> +	amdgpu_pasid_lock(&irqflags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	if (!fpriv) {
>  		vm = NULL;
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> +	} else {
> +		vm = &fpriv->vm;
> +		if (vm->root.bo != root)
> +			vm = NULL;
> +	}
> +	amdgpu_pasid_unlock(irqflags);
> +
>  	if (!vm) {
>  		drm_exec_unlock_obj(exec, &root->tbo.base);
>  		amdgpu_bo_unref(&root);
> @@ -3163,12 +3173,14 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>  				  uint32_t status,
>  				  unsigned int vmhub)
>  {
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_vm *vm;
>  	unsigned long flags;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_lock(&flags);
>  
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
>  	/* Don't update the fault cache if status is 0.  In the multiple
>  	 * fault case, subsequent faults will return a 0 status which is
>  	 * useless for userspace and replaces the useful fault status, so
> @@ -3201,7 +3213,7 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>  			WARN_ONCE(1, "Invalid vmhub %u\n", vmhub);
>  		}
>  	}
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_unlock(flags);
>  }
>  
>  void amdgpu_vm_print_task_info(struct amdgpu_device *adev,

Re: [PATCH v4 3/3] drm/amdgpu: Drop vm_manager PASID to VM mapping

[Christian König]

On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> VM lookup users now resolve DRM PASIDs through the global PASID xarray:
> 
> 	PASID -> fpriv -> VM
> 
> The per-device vm_manager.pasids xarray is no longer needed.
> 
> Remove PASID registration and teardown from VM init/fini paths, drop
> vm_manager PASID initialization/cleanup, and remove the xarray from
> struct amdgpu_vm_manager.
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 ++-----------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 ----
>  2 files changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9092ff227a55..74836240edbb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2647,14 +2647,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	if (r)
>  		dev_dbg(adev->dev, "Failed to create task info for VM\n");
>  
> -	/* Store new PASID in XArray (if non-zero) */
> -	if (pasid != 0) {
> -		r = xa_err(xa_store_irq(&adev->vm_manager.pasids, pasid, vm, GFP_KERNEL));
> -		if (r < 0)
> -			goto error_free_root;
> -
> -		vm->pasid = pasid;
> -	}
> +	vm->pasid = pasid;

What do we actually still need the pasid in the VM for?

Regards,
Christian.

>  
>  	amdgpu_bo_unreserve(vm->root.bo);
>  	amdgpu_bo_unref(&root_bo);
> @@ -2662,11 +2655,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	return 0;
>  
>  error_free_root:
> -	/* If PASID was partially set, erase it from XArray before failing */
> -	if (vm->pasid != 0) {
> -		xa_erase_irq(&adev->vm_manager.pasids, vm->pasid);
> -		vm->pasid = 0;
> -	}
> +	vm->pasid = 0;
>  	amdgpu_vm_pt_free_root(adev, vm);
>  	amdgpu_bo_unreserve(vm->root.bo);
>  	amdgpu_bo_unref(&root_bo);
> @@ -2773,11 +2762,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>  
>  	root = amdgpu_bo_ref(vm->root.bo);
>  	amdgpu_bo_reserve(root, true);
> -	/* Remove PASID mapping before destroying VM */
> -	if (vm->pasid != 0) {
> -		xa_erase_irq(&adev->vm_manager.pasids, vm->pasid);
> -		vm->pasid = 0;
> -	}
>  	dma_fence_wait(vm->last_unlocked, false);
>  	dma_fence_put(vm->last_unlocked);
>  	dma_fence_wait(vm->last_tlb_flush, false);
> @@ -2873,8 +2857,6 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
>  #else
>  	adev->vm_manager.vm_update_mode = 0;
>  #endif
> -
> -	xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>  }
>  
>  /**
> @@ -2886,9 +2868,6 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
>   */
>  void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>  {
> -	WARN_ON(!xa_empty(&adev->vm_manager.pasids));
> -	xa_destroy(&adev->vm_manager.pasids);
> -
>  	amdgpu_vmid_mgr_fini(adev);
>  	amdgpu_pasid_mgr_cleanup();
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 939f639cd8bf..f63364f128bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -481,10 +481,6 @@ struct amdgpu_vm_manager {
>  	 */
>  	int					vm_update_mode;
>  
> -	/* PASID to VM mapping, will be used in interrupt context to
> -	 * look up VM of a page fault
> -	 */
> -	struct xarray				pasids;
>  	/* Global registration of recent page fault information */
>  	struct amdgpu_vm_fault_info	fault_info;
>  };

RE: [PATCH v4 3/3] drm/amdgpu: Drop vm_manager PASID to VM mapping

[SHANMUGAM, SRINIVASAN]

AMD General

Hi Christian,

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 3, 2026 1:14 PM
> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>;
> Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v4 3/3] drm/amdgpu: Drop vm_manager PASID to VM
> mapping
>
> On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> > VM lookup users now resolve DRM PASIDs through the global PASID xarray:
> >
> >     PASID -> fpriv -> VM
> >
> > The per-device vm_manager.pasids xarray is no longer needed.
> >
> > Remove PASID registration and teardown from VM init/fini paths, drop
> > vm_manager PASID initialization/cleanup, and remove the xarray from
> > struct amdgpu_vm_manager.
> >
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 ++-----------------------
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 ----
> >  2 files changed, 2 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index 9092ff227a55..74836240edbb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -2647,14 +2647,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> >     if (r)
> >             dev_dbg(adev->dev, "Failed to create task info for VM\n");
> >
> > -   /* Store new PASID in XArray (if non-zero) */
> > -   if (pasid != 0) {
> > -           r = xa_err(xa_store_irq(&adev->vm_manager.pasids, pasid, vm,
> GFP_KERNEL));
> > -           if (r < 0)
> > -                   goto error_free_root;
> > -
> > -           vm->pasid = pasid;
> > -   }
> > +   vm->pasid = pasid;
>
> What do we actually still need the pasid in the VM for?

I checked the remaining vm->pasid users. It looks like vm->pasid is still needed for existing hardware programming paths (TLB flushes, PASID mapping packets, tracepoints, etc.), while this series only removes the separate vm_manager.pasids lookup table.

My understanding is therefore that:

vm->pasid
    remains as per-VM state

vm_manager.pasids
    can be removed because lookups now go
    PASID -> fpriv -> VM

May I kno pls, does that match your expectation, or were you thinking of removing vm->pasid as well?

Regards,
Srini

>
> Regards,
> Christian.

RE: [PATCH v4 1/3] drm/amdgpu: Allow PASID allocator to store fpriv owner

[SHANMUGAM, SRINIVASAN]

AMD General

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, July 3, 2026 1:08 PM
> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>;
> Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v4 1/3] drm/amdgpu: Allow PASID allocator to store fpriv
> owner
>
> On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> > AMDGPU already has a global PASID xarray used by the PASID allocator.
> >
> > Allow amdgpu_pasid_alloc() to optionally store the owning DRM
> > file-private object directly.
> >
> > Initial callers pass NULL and keep the current dummy allocation marker
> > behavior. A later patch in this series passes the DRM file-private
> > object for DRM PASIDs.
> >
> > This prepares for using:
> >
> >     PASID -> fpriv -> VM
> >
> > instead of:
> >
> >     PASID -> VM
> >
> > Also clear any stored owner from amdgpu_pasid_free_delayed() before
> > waiting for fences, so PASID lookups cannot observe a stale fpriv
> > while the PASID number itself is still pending delayed release.
> >
> > v4: (per Christian)
> > - Add fpriv as an optional parameter to amdgpu_pasid_alloc().
> > - Drop separate amdgpu_pasid_set_fpriv()/clear_fpriv() helpers.
> > - Clear PASID owner from amdgpu_pasid_free_delayed().
> >
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Suggested-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 82
> > +++++++++++++++++++++++--  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |
> > 6 +-  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 +-
> > drivers/gpu/drm/amd/amdgpu/mes_v12_1.c  |  2 +-
> >  4 files changed, 85 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> > index 684f40fce73f..669d0fff8cbc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> > @@ -52,6 +52,7 @@ struct amdgpu_pasid_cb {
> >  /**
> >   * amdgpu_pasid_alloc - Allocate a PASID
> >   * @bits: Maximum width of the PASID in bits, must be at least 1
> > + * @fpriv: optional DRM file-private owner
> >   *
> >   * Uses kernel's IDR cyclic allocator (same as PID allocation).
> >   * Allocates sequentially with automatic wrap-around.
> > @@ -60,17 +61,19 @@ struct amdgpu_pasid_cb {
> >   * Returns %-ENOSPC if no PASID was available. Returns %-ENOMEM on
> >   * memory allocation failure.
> >   */
> > -int amdgpu_pasid_alloc(unsigned int bits)
> > +int amdgpu_pasid_alloc(unsigned int bits, struct amdgpu_fpriv *fpriv)
> >  {
> >     u32 pasid;
> >     int r;
> > +   void *entry;
> >
> >     if (bits == 0)
> >             return -EINVAL;
> >
> > -   r = xa_alloc_cyclic_irq(&amdgpu_pasid_xa, &pasid, xa_mk_value(0),
> > -                       XA_LIMIT(1, (1U << bits) - 1),
> > -                       &amdgpu_pasid_xa_next, GFP_KERNEL);
> > +   entry = fpriv ? fpriv : xa_mk_value(0);
>
> That's problematic I think. The xa_mk_value(0) value is not NULL and needs to be
> filtered out when somebody looks the fpriv up using the array.
>
> But xa_insert() can handle NULL entries, but I'm not sure if xa_alloc_cyclic_irq()
> can do that as well.

Thanks Christian.

I checked the XArray implementation. xa_alloc_cyclic_irq() goes through
__xa_alloc_cyclic(), which calls __xa_alloc(). __xa_alloc() converts a
NULL entry to XA_ZERO_ENTRY before storing it, and xa_load() converts
XA_ZERO_ENTRY back to NULL through xa_zero_to_null().

So NULL should work here as well. I'll update the patch to pass fpriv
directly to xa_alloc_cyclic_irq(), remove xa_mk_value(0), and simplify
the fpriv lookup path.

int __xa_alloc(struct xarray *xa, u32 *id, void *entry,
               struct xa_limit limit, gfp_t gfp)
{
        ...

        if (!entry)
                entry = XA_ZERO_ENTRY;

        ...
        xas_store(&xas, entry);
        ...
}

static inline void *xa_zero_to_null(void *entry)
{
        return xa_is_zero(entry) ? NULL : entry;
}

void *xa_load(struct xarray *xa, unsigned long index)
{
        ...
        entry = xa_zero_to_null(xas_load(&xas));
        ...
        return entry;
}

Best regards,
Srini

Re: [PATCH v4 3/3] drm/amdgpu: Drop vm_manager PASID to VM mapping

[Christian König]

On 7/3/26 12:36, SHANMUGAM, SRINIVASAN wrote:
> AMD General
> 
> Hi Christian,
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, July 3, 2026 1:14 PM
>> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>;
>> Deucher, Alexander <Alexander.Deucher@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH v4 3/3] drm/amdgpu: Drop vm_manager PASID to VM
>> mapping
>>
>> On 7/3/26 08:18, Srinivasan Shanmugam wrote:
>>> VM lookup users now resolve DRM PASIDs through the global PASID xarray:
>>>
>>>     PASID -> fpriv -> VM
>>>
>>> The per-device vm_manager.pasids xarray is no longer needed.
>>>
>>> Remove PASID registration and teardown from VM init/fini paths, drop
>>> vm_manager PASID initialization/cleanup, and remove the xarray from
>>> struct amdgpu_vm_manager.
>>>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 25 ++-----------------------
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 ----
>>>  2 files changed, 2 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 9092ff227a55..74836240edbb 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2647,14 +2647,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>>>     if (r)
>>>             dev_dbg(adev->dev, "Failed to create task info for VM\n");
>>>
>>> -   /* Store new PASID in XArray (if non-zero) */
>>> -   if (pasid != 0) {
>>> -           r = xa_err(xa_store_irq(&adev->vm_manager.pasids, pasid, vm,
>> GFP_KERNEL));
>>> -           if (r < 0)
>>> -                   goto error_free_root;
>>> -
>>> -           vm->pasid = pasid;
>>> -   }
>>> +   vm->pasid = pasid;
>>
>> What do we actually still need the pasid in the VM for?
> 
> I checked the remaining vm->pasid users. It looks like vm->pasid is still needed for existing hardware programming paths (TLB flushes, PASID mapping packets, tracepoints, etc.), while this series only removes the separate vm_manager.pasids lookup table.

Ah, yes. Especially the TLB flushes are a good point.

> 
> My understanding is therefore that:
> 
> vm->pasid
>     remains as per-VM state
> 
> vm_manager.pasids
>     can be removed because lookups now go
>     PASID -> fpriv -> VM
> 
> May I kno pls, does that match your expectation, or were you thinking of removing vm->pasid as well?

Please go ahead with the current plan of removing vm_manager.pasids but keeping vm->pasid.

Thanks,
Christian.

> 
> Regards,
> Srini
> 
>>
>> Regards,
>> Christian.