[PATCH v3] drm/amdkfd: simplify vm_validate_pt_pd_bos

[PATCH v3] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Lang Yu]

We don't need to validate and map root PD specially here,
it would be validated and mapped by amdgpu_vm_validate_pt_bos
if it is evicted.

The special case is when turning a GFX VM to a compute VM,
if vm_update_mode changed, we should make sure root PD gets
mapped. So just map root PD after updating vm->update_funcs
in amdgpu_vm_make_compute whether the vm_update_mode changed
or not.

v3:
 - Add some comments suggested by Christian.

v2:
 - Don't rename vm_validate_pt_pd_bos and make it public.

Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 14 --------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           |  8 ++++++++
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 50bb590c3306..c9ef242177e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -439,22 +439,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;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 703552f9a6d7..3a6b827e540c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2225,6 +2225,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	} else {
 		vm->update_funcs = &amdgpu_vm_sdma_funcs;
 	}
+	/*
+	 * Make sure root PD gets mapped. As vm_update_mode could be changed
+	 * when turning a GFX VM into a compute VM.
+	 */
+	r = vm->update_funcs->map_table(to_amdgpu_bo_vm(vm->root.bo));
+	if (r)
+		goto unreserve_bo;
+
 	dma_fence_put(vm->last_update);
 	vm->last_update = NULL;
 	vm->is_compute_context = true;
-- 
2.25.1

Re: [PATCH v3] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Christian König]

Am 14.06.22 um 11:03 schrieb Lang Yu:
> We don't need to validate and map root PD specially here,
> it would be validated and mapped by amdgpu_vm_validate_pt_bos
> if it is evicted.
>
> The special case is when turning a GFX VM to a compute VM,
> if vm_update_mode changed, we should make sure root PD gets
> mapped. So just map root PD after updating vm->update_funcs
> in amdgpu_vm_make_compute whether the vm_update_mode changed
> or not.
>
> v3:
>   - Add some comments suggested by Christian.
>
> v2:
>   - Don't rename vm_validate_pt_pd_bos and make it public.
>
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>

I can't judge the kfd part, but the VM stuff looks good to me now.

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 14 --------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           |  8 ++++++++
>   2 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 50bb590c3306..c9ef242177e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -439,22 +439,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;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 703552f9a6d7..3a6b827e540c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2225,6 +2225,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	} else {
>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>   	}
> +	/*
> +	 * Make sure root PD gets mapped. As vm_update_mode could be changed
> +	 * when turning a GFX VM into a compute VM.
> +	 */
> +	r = vm->update_funcs->map_table(to_amdgpu_bo_vm(vm->root.bo));
> +	if (r)
> +		goto unreserve_bo;
> +
>   	dma_fence_put(vm->last_update);
>   	vm->last_update = NULL;
>   	vm->is_compute_context = true;

RE: [PATCH v3] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Yu, Lang]

[AMD Official Use Only - General]

Ping!

Hi Felix, what do you think? Thanks!

Regards,
Lang

>-----Original Message-----
>From: Koenig, Christian <Christian.Koenig@amd.com>
>Sent: Tuesday, June 14, 2022 5:08 PM
>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Kuehling, Felix <Felix.Kuehling@amd.com>; Deucher, Alexander
><Alexander.Deucher@amd.com>; Huang, Ray <Ray.Huang@amd.com>
>Subject: Re: [PATCH v3] drm/amdkfd: simplify vm_validate_pt_pd_bos
>
>Am 14.06.22 um 11:03 schrieb Lang Yu:
>> We don't need to validate and map root PD specially here, it would be
>> validated and mapped by amdgpu_vm_validate_pt_bos if it is evicted.
>>
>> The special case is when turning a GFX VM to a compute VM, if
>> vm_update_mode changed, we should make sure root PD gets mapped. So
>> just map root PD after updating vm->update_funcs in
>> amdgpu_vm_make_compute whether the vm_update_mode changed or
>not.
>>
>> v3:
>>   - Add some comments suggested by Christian.
>>
>> v2:
>>   - Don't rename vm_validate_pt_pd_bos and make it public.
>>
>> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
>
>I can't judge the kfd part, but the VM stuff looks good to me now.
>
>Acked-by: Christian König <christian.koenig@amd.com>
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 14 -------------
>-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           |  8 ++++++++
>>   2 files changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 50bb590c3306..c9ef242177e2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -439,22 +439,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;
>>   }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 703552f9a6d7..3a6b827e540c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2225,6 +2225,14 @@ int amdgpu_vm_make_compute(struct
>amdgpu_device *adev, struct amdgpu_vm *vm)
>>   	} else {
>>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>>   	}
>> +	/*
>> +	 * Make sure root PD gets mapped. As vm_update_mode could be
>changed
>> +	 * when turning a GFX VM into a compute VM.
>> +	 */
>> +	r = vm->update_funcs->map_table(to_amdgpu_bo_vm(vm-
>>root.bo));
>> +	if (r)
>> +		goto unreserve_bo;
>> +
>>   	dma_fence_put(vm->last_update);
>>   	vm->last_update = NULL;
>>   	vm->is_compute_context = true;

Re: [PATCH v3] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Felix Kuehling]

On 2022-06-14 05:03, Lang Yu wrote:
> We don't need to validate and map root PD specially here,
> it would be validated and mapped by amdgpu_vm_validate_pt_bos
> if it is evicted.
>
> The special case is when turning a GFX VM to a compute VM,
> if vm_update_mode changed, we should make sure root PD gets
> mapped. So just map root PD after updating vm->update_funcs
> in amdgpu_vm_make_compute whether the vm_update_mode changed
> or not.
>
> v3:
>   - Add some comments suggested by Christian.
>
> v2:
>   - Don't rename vm_validate_pt_pd_bos and make it public.
>
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 14 --------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           |  8 ++++++++
>   2 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 50bb590c3306..c9ef242177e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -439,22 +439,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;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 703552f9a6d7..3a6b827e540c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2225,6 +2225,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	} else {
>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>   	}
> +	/*
> +	 * Make sure root PD gets mapped. As vm_update_mode could be changed
> +	 * when turning a GFX VM into a compute VM.
> +	 */
> +	r = vm->update_funcs->map_table(to_amdgpu_bo_vm(vm->root.bo));
> +	if (r)
> +		goto unreserve_bo;
> +
>   	dma_fence_put(vm->last_update);
>   	vm->last_update = NULL;
>   	vm->is_compute_context = true;