[PATCH v3 1/1] drm/amdgpu: fix amdgpu_vm_pt_free warning
Felix Kuehling
felix.kuehling at amd.com
Tue Sep 13 20:10:58 UTC 2022
Am 2022-09-13 um 09:19 schrieb Philip Yang:
> Free page table BO from vm resv unlocked context generate below
> warnings.
>
> Add a pt_free_work in vm to free page table BO from vm->pt_freed list.
> pass vm resv unlock status from page table update caller, and add vm_bo
> entry to vm->pt_freed list and schedule the pt_free_work if calling with
> vm resv unlocked.
>
> WARNING: CPU: 12 PID: 3238 at
> drivers/gpu/drm/ttm/ttm_bo.c:106 ttm_bo_set_bulk_move+0xa1/0xc0
> Call Trace:
> amdgpu_vm_pt_free+0x42/0xd0 [amdgpu]
> amdgpu_vm_pt_free_dfs+0xb3/0xf0 [amdgpu]
> amdgpu_vm_ptes_update+0x52d/0x850 [amdgpu]
> amdgpu_vm_update_range+0x2a6/0x640 [amdgpu]
> svm_range_unmap_from_gpus+0x110/0x300 [amdgpu]
> svm_range_cpu_invalidate_pagetables+0x535/0x600 [amdgpu]
> __mmu_notifier_invalidate_range_start+0x1cd/0x230
> unmap_vmas+0x9d/0x140
> unmap_region+0xa8/0x110
>
> WARNING: CPU: 0 PID: 1475 at
> drivers/dma-buf/dma-resv.c:483 dma_resv_iter_next
> Call Trace:
> dma_resv_iter_first+0x43/0xa0
> amdgpu_vm_sdma_update+0x69/0x2d0 [amdgpu]
> amdgpu_vm_ptes_update+0x29c/0x870 [amdgpu]
> amdgpu_vm_update_range+0x2f6/0x6c0 [amdgpu]
> svm_range_unmap_from_gpus+0x115/0x300 [amdgpu]
> svm_range_cpu_invalidate_pagetables+0x510/0x5e0 [amdgpu]
> __mmu_notifier_invalidate_range_start+0x1d3/0x230
> unmap_vmas+0x140/0x150
> unmap_region+0xa8/0x110
>
> Signed-off-by: Philip Yang <Philip.Yang at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 47 ++++++++++++++++++++---
> 3 files changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 59cac347baa3..27e6155053b6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2022,6 +2022,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> spin_lock_init(&vm->invalidated_lock);
> INIT_LIST_HEAD(&vm->freed);
> INIT_LIST_HEAD(&vm->done);
> + INIT_LIST_HEAD(&vm->pt_freed);
> + INIT_WORK(&vm->pt_free_work, amdgpu_vm_pt_free_work);
> + spin_lock_init(&vm->pt_free_lock);
>
> /* create scheduler entities for page table updates */
> r = drm_sched_entity_init(&vm->immediate, DRM_SCHED_PRIORITY_NORMAL,
> @@ -2244,6 +2247,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> amdgpu_vm_free_mapping(adev, vm, mapping, NULL);
> }
>
> + cancel_work_sync(&vm->pt_free_work);
> amdgpu_vm_pt_free_root(adev, vm);
> amdgpu_bo_unreserve(root);
> amdgpu_bo_unref(&root);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 9ecb7f663e19..b77fe838c327 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -276,6 +276,11 @@ struct amdgpu_vm {
> /* BOs which are invalidated, has been updated in the PTs */
> struct list_head done;
>
> + /* PT BOs scheduled to free and fill with zero if vm_resv is not hold */
> + struct list_head pt_freed;
> + struct work_struct pt_free_work;
> + spinlock_t pt_free_lock;
> +
> /* contains the page directory */
> struct amdgpu_vm_bo_base root;
> struct dma_fence *last_update;
> @@ -471,6 +476,7 @@ int amdgpu_vm_pde_update(struct amdgpu_vm_update_params *params,
> int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
> uint64_t start, uint64_t end,
> uint64_t dst, uint64_t flags);
> +void amdgpu_vm_pt_free_work(struct work_struct *work);
>
> #if defined(CONFIG_DEBUG_FS)
> void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm *vm, struct seq_file *m);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index 88de9f0d4728..c6f91731ecfb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -624,12 +624,22 @@ static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev,
> *
> * @entry: PDE to free
> */
> -static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry)
> +static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry, bool unlocked)
> {
> struct amdgpu_bo *shadow;
>
> if (!entry->bo)
> return;
> +
> + if (unlocked) {
> + spin_lock(&entry->vm->pt_free_lock);
> + list_move(&entry->vm_status, &entry->vm->pt_freed);
> + spin_unlock(&entry->vm->pt_free_lock);
> +
> + schedule_work(&entry->vm->pt_free_work);
> + return;
> + }
> +
> shadow = amdgpu_bo_shadowed(entry->bo);
> if (shadow) {
> ttm_bo_set_bulk_move(&shadow->tbo, NULL);
> @@ -641,6 +651,29 @@ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry)
> amdgpu_bo_unref(&entry->bo);
> }
>
> +void amdgpu_vm_pt_free_work(struct work_struct *work)
> +{
> + struct amdgpu_vm_bo_base *entry, *next;
> + struct amdgpu_vm *vm;
> + struct amdgpu_bo *root;
> + LIST_HEAD(pt_freed);
> +
> + vm = container_of(work, struct amdgpu_vm, pt_free_work);
> +
> + spin_lock(&vm->pt_free_lock);
> + list_splice_init(&vm->pt_freed, &pt_freed);
> + spin_unlock(&vm->pt_free_lock);
> +
> + root = amdgpu_bo_ref(vm->root.bo);
I'm not sure why you need this extra reference. If your concern is, that
the root bo could disappear while the worker is running, it could
disappear even before the worker started running. In that case, you
should take the reference before scheduling the work. But I think
cancel_work_sync in amdgpu_vm_fini protects you from that. Therefore I
believe there is no need to take an extra reference here.
Other than that, the patch looks good to me.
Regards,
Felix
> + amdgpu_bo_reserve(root, true);
> +
> + list_for_each_entry_safe(entry, next, &pt_freed, vm_status)
> + amdgpu_vm_pt_free(entry, false);
> +
> + amdgpu_bo_unreserve(root);
> + amdgpu_bo_unref(&root);
> +}
> +
> /**
> * amdgpu_vm_pt_free_dfs - free PD/PT levels
> *
> @@ -652,16 +685,17 @@ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry)
> */
> static void amdgpu_vm_pt_free_dfs(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> - struct amdgpu_vm_pt_cursor *start)
> + struct amdgpu_vm_pt_cursor *start,
> + bool unlocked)
> {
> struct amdgpu_vm_pt_cursor cursor;
> struct amdgpu_vm_bo_base *entry;
>
> for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
> - amdgpu_vm_pt_free(entry);
> + amdgpu_vm_pt_free(entry, unlocked);
>
> if (start)
> - amdgpu_vm_pt_free(start->entry);
> + amdgpu_vm_pt_free(start->entry, unlocked);
> }
>
> /**
> @@ -673,7 +707,7 @@ static void amdgpu_vm_pt_free_dfs(struct amdgpu_device *adev,
> */
> void amdgpu_vm_pt_free_root(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> {
> - amdgpu_vm_pt_free_dfs(adev, vm, NULL);
> + amdgpu_vm_pt_free_dfs(adev, vm, NULL, false);
> }
>
> /**
> @@ -966,7 +1000,8 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
> if (cursor.entry->bo) {
> params->table_freed = true;
> amdgpu_vm_pt_free_dfs(adev, params->vm,
> - &cursor);
> + &cursor,
> + params->unlocked);
> }
> amdgpu_vm_pt_next(adev, &cursor);
> }
More information about the amd-gfx
mailing list