Re: [PATCH v3 2/2] drm/amdgpu: unref the bo after job submit

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

 



Am 13.03.20 um 15:07 schrieb xinhui pan:
Otherwise we might free BOs before job has completed.
We add the fence to BO after commit, so free BOs after that.

I'm not sure if this is necessary, but probably better save than sorry.

Some comments below.


Cc: Christian König <christian.koenig@xxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 38 +++++++++++++++++++-------
  1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 809ca6e8f40f..605a1bb40280 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -942,13 +942,17 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
   *
   * @entry: PDE to free
   */
-static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
+static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry,
+					struct list_head *head)
  {
  	if (entry->base.bo) {
  		entry->base.bo->vm_bo = NULL;
  		list_del(&entry->base.vm_status);
-		amdgpu_bo_unref(&entry->base.bo->shadow);
-		amdgpu_bo_unref(&entry->base.bo);
+		if (!head) {
+			amdgpu_bo_unref(&entry->base.bo->shadow);
+			amdgpu_bo_unref(&entry->base.bo);
+		} else
+			list_add(&entry->base.vm_status, head);

Instead of adding a parameter make this a new state in the VM. Something like vm->zombies or something similar.

  	}
  	kvfree(entry->entries);
  	entry->entries = NULL;
@@ -965,7 +969,8 @@ static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
   */
  static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
  			       struct amdgpu_vm *vm,
-			       struct amdgpu_vm_pt_cursor *start)
+			       struct amdgpu_vm_pt_cursor *start,
+				   struct list_head *head)
  {
  	struct amdgpu_vm_pt_cursor cursor;
  	struct amdgpu_vm_pt *entry;
@@ -973,10 +978,10 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
  	vm->bulk_moveable = false;
for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
-		amdgpu_vm_free_table(entry);
+		amdgpu_vm_free_table(entry, head);
if (start)
-		amdgpu_vm_free_table(start->entry);
+		amdgpu_vm_free_table(start->entry, head);
  }
/**
@@ -1428,7 +1433,8 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
   */
  static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
  				 uint64_t start, uint64_t end,
-				 uint64_t dst, uint64_t flags)
+				 uint64_t dst, uint64_t flags,
+				 struct list_head *head)
  {
  	struct amdgpu_device *adev = params->adev;
  	struct amdgpu_vm_pt_cursor cursor;
@@ -1539,7 +1545,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
  			 * completely covered by the range and so potentially still in use.
  			 */
  			while (cursor.pfn < frag_start) {
-				amdgpu_vm_free_pts(adev, params->vm, &cursor);
+				amdgpu_vm_free_pts(adev, params->vm, &cursor, head);
  				amdgpu_vm_pt_next(adev, &cursor);
  			}
@@ -1583,6 +1589,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
  	struct amdgpu_vm_update_params params;
  	enum amdgpu_sync_mode sync_mode;
  	int r;
+	struct list_head head;
memset(&params, 0, sizeof(params));
  	params.adev = adev;
@@ -1590,6 +1597,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
  	params.direct = direct;
  	params.pages_addr = pages_addr;
+ INIT_LIST_HEAD(&head);
+
  	/* Implicitly sync to command submissions in the same VM before
  	 * unmapping. Sync to moving fences before mapping.
  	 */
@@ -1617,13 +1626,22 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
  	if (r)
  		goto error_unlock;
- r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
+	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags, &head);
  	if (r)
  		goto error_unlock;
r = vm->update_funcs->commit(&params, fence); error_unlock:
+	while (!list_empty(&head)) {
+		struct amdgpu_vm_pt *entry = list_first_entry(&head,
+				struct amdgpu_vm_pt, base.vm_status);
+		list_del(&entry->base.vm_status);
+
+		amdgpu_bo_unref(&entry->base.bo->shadow);
+		amdgpu_bo_unref(&entry->base.bo);
+	}

Maybe put that into a separate function.

+
  	amdgpu_vm_eviction_unlock(vm);
  	return r;
  }
@@ -3118,7 +3136,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
  		amdgpu_vm_free_mapping(adev, vm, mapping, NULL);
  	}
- amdgpu_vm_free_pts(adev, vm, NULL);
+	amdgpu_vm_free_pts(adev, vm, NULL, NULL);

And then also call it here.

Regards,
Christian.

  	amdgpu_bo_unreserve(root);
  	amdgpu_bo_unref(&root);
  	WARN_ON(vm->root.base.bo);

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux