Am 05.07.2018 um 04:09 schrieb zhoucm1: > > > On 2018å¹´07æ??05æ?¥ 03:49, Andrey Grodzovsky wrote: >> Problem: When PD/PT update made by CPU root PD was not yet mapped >> causing >> page fault. >> >> Fix: Move amdgpu_bo_kmap into amdgpu_vm_bo_base_init to cover >> all cases and avoid code duplication with amdgpu_vm_alloc_levels. >> >> Link: https://bugs.freedesktop.org/show_bug.cgi?id=107065 >> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com> >> --- >> Â drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 47 >> ++++++++++++++++++++++------------ >> Â 1 file changed, 31 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> index 845f73a..f546afa 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> @@ -143,25 +143,36 @@ struct amdgpu_prt_cb { >> Â Â * Initialize a bo_va_base structure and add it to the appropriate >> lists >> Â Â * >> Â Â */ >> -static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base, >> +static int amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base, >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_vm *vm, >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_bo *bo) >> Â { >> +Â Â Â int r = 0; >> Â Â Â Â Â base->vm = vm; >> Â Â Â Â Â base->bo = bo; >> Â Â Â Â Â INIT_LIST_HEAD(&base->bo_list); >> Â Â Â Â Â INIT_LIST_HEAD(&base->vm_status); >> Â Â Â Â Â Â if (!bo) >> -Â Â Â Â Â Â Â return; >> +Â Â Â Â Â Â Â return r; >> + >> Â Â Â Â Â list_add_tail(&base->bo_list, &bo->va); >> Â +Â Â Â if (vm->use_cpu_for_update && bo->tbo.type == >> ttm_bo_type_kernel) { >> +Â Â Â Â Â Â Â r = amdgpu_bo_kmap(bo, NULL); >> +Â Â Â Â Â Â Â if (r) { >> +Â Â Â Â Â Â Â Â Â Â Â amdgpu_bo_unref(&bo->shadow); >> +Â Â Â Â Â Â Â Â Â Â Â amdgpu_bo_unref(&bo); > I feel these two lines should move out of helper function, ref/unref > should appear in where used with pair. Yeah agree, but there is an approach which is even cleaner I think. > > Regards, > David Zhou >> +Â Â Â Â Â Â Â Â Â Â Â return r; >> +Â Â Â Â Â Â Â } >> +Â Â Â } >> + >> Â Â Â Â Â if (bo->tbo.resv != vm->root.base.bo->tbo.resv) >> -Â Â Â Â Â Â Â return; >> +Â Â Â Â Â Â Â return r; >> Â Â Â Â Â Â if (bo->preferred_domains & >> Â Â Â Â Â Â Â Â Â amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type)) >> -Â Â Â Â Â Â Â return; >> +Â Â Â Â Â Â Â return r; >> Â Â Â Â Â Â /* >> Â Â Â Â Â Â * we checked all the prerequisites, but it looks like this per >> vm bo >> @@ -169,6 +180,8 @@ static void amdgpu_vm_bo_base_init(struct >> amdgpu_vm_bo_base *base, >> Â Â Â Â Â Â * is validated on next vm use to avoid fault. >> Â Â Â Â Â Â * */ >> Â Â Â Â Â list_move_tail(&base->vm_status, &vm->evicted); >> + >> +Â Â Â return r; Here we move it into the evicted status when the placement isn't ok. Now for PDs/PTs the caller moves it immediately into the relocated status to make sure that the parent entries are updated: > amdgpu_vm_bo_base_init(&entry->base, vm, pt); > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â list_move(&entry->base.vm_status, &vm->relocated); And amdgpu_vm_update_directories() maps all relocated BOs anyway before it begins: > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â list_for_each_entry(bo_base, &vm->relocated, vm_status) { > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â r = amdgpu_bo_kmap(bo_base->bo, NULL); > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (unlikely(r)) > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return r; > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } So all we need to do is to make sure that we add the root PD to the relocated list as well. Cleanest approach would probably be to move the "list_move(&entry->base.vm_status, &vm->relocated);" into amdgpu_vm_bo_base_init(). Regards, Christian. >> Â } >> Â Â /** >> @@ -525,21 +538,15 @@ static int amdgpu_vm_alloc_levels(struct >> amdgpu_device *adev, >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return r; >> Â Â Â Â Â Â Â Â Â Â Â Â Â } >> Â -Â Â Â Â Â Â Â Â Â Â Â if (vm->use_cpu_for_update) { >> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â r = amdgpu_bo_kmap(pt, NULL); >> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (r) { >> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â amdgpu_bo_unref(&pt->shadow); >> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â amdgpu_bo_unref(&pt); >> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return r; >> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } >> -Â Â Â Â Â Â Â Â Â Â Â } >> - >> Â Â Â Â Â Â Â Â Â Â Â Â Â /* Keep a reference to the root directory to avoid >> Â Â Â Â Â Â Â Â Â Â Â Â Â * freeing them up in the wrong order. >> Â Â Â Â Â Â Â Â Â Â Â Â Â */ >> Â Â Â Â Â Â Â Â Â Â Â Â Â pt->parent = amdgpu_bo_ref(parent->base.bo); >> Â -Â Â Â Â Â Â Â Â Â Â Â amdgpu_vm_bo_base_init(&entry->base, vm, pt); >> +Â Â Â Â Â Â Â Â Â Â Â r = amdgpu_vm_bo_base_init(&entry->base, vm, pt); >> +Â Â Â Â Â Â Â Â Â Â Â if (r) >> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return r; >> + >> Â Â Â Â Â Â Â Â Â Â Â Â Â list_move(&entry->base.vm_status, &vm->relocated); >> Â Â Â Â Â Â Â Â Â } >> Â @@ -1992,12 +1999,17 @@ struct amdgpu_bo_va >> *amdgpu_vm_bo_add(struct amdgpu_device *adev, >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_bo *bo) >> Â { >> Â Â Â Â Â struct amdgpu_bo_va *bo_va; >> +Â Â Â int r; >> Â Â Â Â Â Â bo_va = kzalloc(sizeof(struct amdgpu_bo_va), GFP_KERNEL); >> Â Â Â Â Â if (bo_va == NULL) { >> Â Â Â Â Â Â Â Â Â return NULL; >> Â Â Â Â Â } >> -Â Â Â amdgpu_vm_bo_base_init(&bo_va->base, vm, bo); >> + >> +Â Â Â if ((r = amdgpu_vm_bo_base_init(&bo_va->base, vm, bo))) { >> +Â Â Â Â Â Â Â WARN_ONCE(1,"r = %d\n", r); >> +Â Â Â Â Â Â Â return NULL; >> +Â Â Â } >> Â Â Â Â Â Â bo_va->ref_count = 1; >> Â Â Â Â Â INIT_LIST_HEAD(&bo_va->valids); >> @@ -2613,7 +2625,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, >> struct amdgpu_vm *vm, >> Â Â Â Â Â if (r) >> Â Â Â Â Â Â Â Â Â goto error_unreserve; >> Â -Â Â Â amdgpu_vm_bo_base_init(&vm->root.base, vm, root); >> +Â Â Â r = amdgpu_vm_bo_base_init(&vm->root.base, vm, root); >> +Â Â Â if (r) >> +Â Â Â Â Â Â Â Â Â Â Â goto error_unreserve; >> + >> Â Â Â Â Â amdgpu_bo_unreserve(vm->root.base.bo); >> Â Â Â Â Â Â if (pasid) { >
