[PATCH 1/3] drm/amdkfd: don't validate pinned BOs

[PATCH 1/3] drm/amdkfd: don't validate pinned BOs

[Lang Yu]

If a BO is pinned to VRAM and you try to validate it into GTT,
you will get an error.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index abf2fc421323..81bcffb510f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -346,6 +346,9 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
 	struct ttm_operation_ctx ctx = { false, false };
 	int ret;
 
+	if (bo->tbo.pin_count)
+		return 0;
+
 	if (WARN(amdgpu_ttm_tt_get_usermm(bo->tbo.ttm),
 		 "Called with userptr BO"))
 		return -EINVAL;
-- 
2.25.1

[PATCH 2/3] drm/amdkfd: simplify PD and PT BOs validation

[Lang Yu]

1, Move root BO kmapping to amdgpu_vm_make_compute.
2, Don't validate and kmap root BO intentional, it would be
validated and mapped by amdgpu_vm_validate_pt_bos if necessary.
3, Rename and expose vm_validate_pt_pd_bos, so that it
could be used by SVM.

Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  1 +
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 26 +++++--------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  5 ++++
 3 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index f8b9f27adcf5..463c1a3fa587 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -288,6 +288,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
 					    struct dma_fence **ef);
 int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
 					      struct kfd_vm_fault_info *info);
+int amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(struct amdgpu_vm *vm);
 int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 				      struct dma_buf *dmabuf,
 				      uint64_t va, void *drm_priv,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 81bcffb510f4..9f6531597d1f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -370,14 +370,14 @@ static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)
 	return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
 }
 
-/* vm_validate_pt_pd_bos - Validate page table and directory BOs
+/* amdgpu_amdkfd_gpuvm_validate_pt_pd_bos - Validate page table and directory BOs
  *
  * Page directories are not updated here because huge page handling
  * during page table updates can invalidate page directory entries
  * again. Page directories are only updated after updating page
  * tables.
  */
-static int vm_validate_pt_pd_bos(struct amdgpu_vm *vm)
+int amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(struct amdgpu_vm *vm)
 {
 	struct amdgpu_bo *pd = vm->root.bo;
 	struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
@@ -389,22 +389,8 @@ static int vm_validate_pt_pd_bos(struct amdgpu_vm *vm)
 		return ret;
 	}
 
-	ret = amdgpu_amdkfd_validate_vm_bo(NULL, pd);
-	if (ret) {
-		pr_err("failed to validate PD\n");
-		return ret;
-	}
-
 	vm->pd_phys_addr = amdgpu_gmc_pd_addr(vm->root.bo);
 
-	if (vm->use_cpu_for_update) {
-		ret = amdgpu_bo_kmap(pd, NULL);
-		if (ret) {
-			pr_err("failed to kmap PD, ret=%d\n", ret);
-			return ret;
-		}
-	}
-
 	return 0;
 }
 
@@ -1175,7 +1161,7 @@ static int process_validate_vms(struct amdkfd_process_info *process_info)
 
 	list_for_each_entry(peer_vm, &process_info->vm_list_head,
 			    vm_list_node) {
-		ret = vm_validate_pt_pd_bos(peer_vm);
+		ret = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(peer_vm);
 		if (ret)
 			return ret;
 	}
@@ -1261,7 +1247,7 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
 	ret = amdgpu_bo_reserve(vm->root.bo, true);
 	if (ret)
 		goto reserve_pd_fail;
-	ret = vm_validate_pt_pd_bos(vm);
+	ret = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(vm);
 	if (ret) {
 		pr_err("validate_pt_pd_bos() failed\n");
 		goto validate_pd_fail;
@@ -1809,7 +1795,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 	    bo->tbo.resource->mem_type == TTM_PL_SYSTEM)
 		is_invalid_userptr = true;
 
-	ret = vm_validate_pt_pd_bos(avm);
+	ret = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(avm);
 	if (unlikely(ret))
 		goto out_unreserve;
 
@@ -1889,7 +1875,7 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
 		goto unreserve_out;
 	}
 
-	ret = vm_validate_pt_pd_bos(avm);
+	ret = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(avm);
 	if (unlikely(ret))
 		goto unreserve_out;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1ea204218903..697ce6825c18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2217,6 +2217,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 			goto unreserve_bo;
 
 		vm->update_funcs = &amdgpu_vm_cpu_funcs;
+		r = amdgpu_bo_kmap(vm->root.bo, NULL);
+		if (r) {
+			dev_err(adev->dev, "failed to kmap PD, r = %d\n", r);
+			return r;
+		}
 	} else {
 		vm->update_funcs = &amdgpu_vm_sdma_funcs;
 	}
-- 
2.25.1

[PATCH 3/3] drm/amdkfd: use existing VM helper for PD and PT validation in SVM

[Lang Yu]

This will remove some redundant codes.

Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index d6fc00d51c8c..03e07d1d1d1a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -625,15 +625,6 @@ svm_range_get_pdd_by_adev(struct svm_range *prange, struct amdgpu_device *adev)
 	return kfd_process_device_from_gpuidx(p, gpu_idx);
 }
 
-static int svm_range_bo_validate(void *param, struct amdgpu_bo *bo)
-{
-	struct ttm_operation_ctx ctx = { false, false };
-
-	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM);
-
-	return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
-}
-
 static int
 svm_range_check_attr(struct kfd_process *p,
 		     uint32_t nattr, struct kfd_ioctl_svm_attribute *attrs)
@@ -1428,9 +1419,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx)
 			goto unreserve_out;
 		}
 
-		r = amdgpu_vm_validate_pt_bos(pdd->dev->adev,
-					      drm_priv_to_vm(pdd->drm_priv),
-					      svm_range_bo_validate, NULL);
+		r = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(drm_priv_to_vm(pdd->drm_priv));
 		if (r) {
 			pr_debug("failed %d validate pt bos\n", r);
 			goto unreserve_out;
-- 
2.25.1

Re: [PATCH 3/3] drm/amdkfd: use existing VM helper for PD and PT validation in SVM

[Felix Kuehling]

Am 2022-06-07 um 05:59 schrieb Lang Yu:
> This will remove some redundant codes.
>
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>

The redundancy is quite small, and 
amdgpu_amdkfd_gpuvm_validate_pt_pd_bos and amdgpu_amdkfd_bo_validate are 
quite a bit more complex and handle more different cases. Someone 
changing those functions in the future may not realize the effect that 
may have on the SVM code.

I'd prefer to keep the svm_range_bo_validate function in kfd_svm.c to 
make the code easier to understand and maintain. If anything, I'd move 
it closer to where its used, because it's only used in one place.

Regards,
   Felix


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 +------------
>   1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index d6fc00d51c8c..03e07d1d1d1a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -625,15 +625,6 @@ svm_range_get_pdd_by_adev(struct svm_range *prange, struct amdgpu_device *adev)
>   	return kfd_process_device_from_gpuidx(p, gpu_idx);
>   }
>   
> -static int svm_range_bo_validate(void *param, struct amdgpu_bo *bo)
> -{
> -	struct ttm_operation_ctx ctx = { false, false };
> -
> -	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM);
> -
> -	return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> -}
> -
>   static int
>   svm_range_check_attr(struct kfd_process *p,
>   		     uint32_t nattr, struct kfd_ioctl_svm_attribute *attrs)
> @@ -1428,9 +1419,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx)
>   			goto unreserve_out;
>   		}
>   
> -		r = amdgpu_vm_validate_pt_bos(pdd->dev->adev,
> -					      drm_priv_to_vm(pdd->drm_priv),
> -					      svm_range_bo_validate, NULL);
> +		r = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(drm_priv_to_vm(pdd->drm_priv));
>   		if (r) {
>   			pr_debug("failed %d validate pt bos\n", r);
>   			goto unreserve_out;

Re: [PATCH 3/3] drm/amdkfd: use existing VM helper for PD and PT validation in SVM

[Lang Yu]

On 06/07/ , Felix Kuehling wrote:
> Am 2022-06-07 um 05:59 schrieb Lang Yu:
> > This will remove some redundant codes.
> > 
> > Signed-off-by: Lang Yu <Lang.Yu@amd.com>
> 
> The redundancy is quite small, and amdgpu_amdkfd_gpuvm_validate_pt_pd_bos
> and amdgpu_amdkfd_bo_validate are quite a bit more complex and handle more
> different cases. Someone changing those functions in the future may not
> realize the effect that may have on the SVM code.
> 
> I'd prefer to keep the svm_range_bo_validate function in kfd_svm.c to make
> the code easier to understand and maintain. If anything, I'd move it closer
> to where its used, because it's only used in one place.

Thanks for your comments. I got it. By the way,
is it necessary to update vm->pd_phys_addr here?
I noticed that vm->pd_phys_addr is updated in
vm_validate_pt_pd_bos()? Thanks!

Regards,
Lang

> Regards,
>   Felix
> 
> 
> > ---
> >   drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 +------------
> >   1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > index d6fc00d51c8c..03e07d1d1d1a 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > @@ -625,15 +625,6 @@ svm_range_get_pdd_by_adev(struct svm_range *prange, struct amdgpu_device *adev)
> >   	return kfd_process_device_from_gpuidx(p, gpu_idx);
> >   }
> > -static int svm_range_bo_validate(void *param, struct amdgpu_bo *bo)
> > -{
> > -	struct ttm_operation_ctx ctx = { false, false };
> > -
> > -	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM);
> > -
> > -	return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> > -}
> > -
> >   static int
> >   svm_range_check_attr(struct kfd_process *p,
> >   		     uint32_t nattr, struct kfd_ioctl_svm_attribute *attrs)
> > @@ -1428,9 +1419,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx)
> >   			goto unreserve_out;
> >   		}
> > -		r = amdgpu_vm_validate_pt_bos(pdd->dev->adev,
> > -					      drm_priv_to_vm(pdd->drm_priv),
> > -					      svm_range_bo_validate, NULL);
> > +		r = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(drm_priv_to_vm(pdd->drm_priv));
> >   		if (r) {
> >   			pr_debug("failed %d validate pt bos\n", r);
> >   			goto unreserve_out;

Re: [PATCH 3/3] drm/amdkfd: use existing VM helper for PD and PT validation in SVM

[Felix Kuehling]

Am 2022-06-07 um 22:21 schrieb Lang Yu:
> On 06/07/ , Felix Kuehling wrote:
>> Am 2022-06-07 um 05:59 schrieb Lang Yu:
>>> This will remove some redundant codes.
>>>
>>> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
>> The redundancy is quite small, and amdgpu_amdkfd_gpuvm_validate_pt_pd_bos
>> and amdgpu_amdkfd_bo_validate are quite a bit more complex and handle more
>> different cases. Someone changing those functions in the future may not
>> realize the effect that may have on the SVM code.
>>
>> I'd prefer to keep the svm_range_bo_validate function in kfd_svm.c to make
>> the code easier to understand and maintain. If anything, I'd move it closer
>> to where its used, because it's only used in one place.
> Thanks for your comments. I got it. By the way,
> is it necessary to update vm->pd_phys_addr here?
> I noticed that vm->pd_phys_addr is updated in
> vm_validate_pt_pd_bos()? Thanks!

It's not needed here. The only time we need to update this is, when we 
re-enable queues after a PD eviction. This is handled by the PD 
validation in amdgpu_amdkfd_gpuvm_restore_process_bos -> 
process_validate_vms -> vm_validate_pt_pd_bos.

Regards,
   Felix


>
> Regards,
> Lang
>
>> Regards,
>>    Felix
>>
>>
>>> ---
>>>    drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 +------------
>>>    1 file changed, 1 insertion(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index d6fc00d51c8c..03e07d1d1d1a 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -625,15 +625,6 @@ svm_range_get_pdd_by_adev(struct svm_range *prange, struct amdgpu_device *adev)
>>>    	return kfd_process_device_from_gpuidx(p, gpu_idx);
>>>    }
>>> -static int svm_range_bo_validate(void *param, struct amdgpu_bo *bo)
>>> -{
>>> -	struct ttm_operation_ctx ctx = { false, false };
>>> -
>>> -	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM);
>>> -
>>> -	return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>>> -}
>>> -
>>>    static int
>>>    svm_range_check_attr(struct kfd_process *p,
>>>    		     uint32_t nattr, struct kfd_ioctl_svm_attribute *attrs)
>>> @@ -1428,9 +1419,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx)
>>>    			goto unreserve_out;
>>>    		}
>>> -		r = amdgpu_vm_validate_pt_bos(pdd->dev->adev,
>>> -					      drm_priv_to_vm(pdd->drm_priv),
>>> -					      svm_range_bo_validate, NULL);
>>> +		r = amdgpu_amdkfd_gpuvm_validate_pt_pd_bos(drm_priv_to_vm(pdd->drm_priv));
>>>    		if (r) {
>>>    			pr_debug("failed %d validate pt bos\n", r);
>>>    			goto unreserve_out;