[PATCH v2] drm/amdkfd: simplify vm_validate_pt_pd_bos

[PATCH v2] 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 need to map the root PD again.
So just move root PD mapping to amdgpu_vm_make_compute.

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           |  5 +++++
 2 files changed, 5 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 6a3bd8b9a08f..3805eef9ab69 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -425,22 +425,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..08fda57f5aa2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2225,6 +2225,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	} else {
 		vm->update_funcs = &amdgpu_vm_sdma_funcs;
 	}
+
+	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 v2] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Christian König]

Am 13.06.22 um 09:59 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.

I'm not sure if that's correct. Traditionally we have handled the root 
PD differently to the rest in the VM.

It doesn't make much sense any more today, but I need to double check if 
that isn't still the case.

Christian.

>
> The special case is when turning a GFX VM to a compute VM,
> if vm_update_mode changed, we need to map the root PD again.
> So just move root PD mapping to amdgpu_vm_make_compute.
>
> 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           |  5 +++++
>   2 files changed, 5 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 6a3bd8b9a08f..3805eef9ab69 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -425,22 +425,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..08fda57f5aa2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2225,6 +2225,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	} else {
>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>   	}
> +
> +	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 v2] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Lang Yu]

On 06/13/ , Christian König wrote:
> Am 13.06.22 um 09:59 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.
> 
> I'm not sure if that's correct. Traditionally we have handled the root PD
> differently to the rest in the VM.
> 
> It doesn't make much sense any more today, but I need to double check if
> that isn't still the case.

From my observations, if root PD is evicted. amdgpu_vm_validate_pt_bos
will validate and map it.

And amdgpu_cs_list_validate always validates root PD after
amdgpu_vm_validate_pt_bos has done that, it is actually unnecessary. 
Do you think it's worth skiping root PD validation in
amdgpu_cs_list_validate? Thanks!

Regards,
Lang


> Christian.
> 
> > 
> > The special case is when turning a GFX VM to a compute VM,
> > if vm_update_mode changed, we need to map the root PD again.
> > So just move root PD mapping to amdgpu_vm_make_compute.
> > 
> > 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           |  5 +++++
> >   2 files changed, 5 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 6a3bd8b9a08f..3805eef9ab69 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -425,22 +425,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..08fda57f5aa2 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -2225,6 +2225,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> >   	} else {
> >   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
> >   	}
> > +
> > +	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 v2] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Christian König]

Am 13.06.22 um 10:26 schrieb Lang Yu:
> On 06/13/ , Christian König wrote:
>> Am 13.06.22 um 09:59 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.
>> I'm not sure if that's correct. Traditionally we have handled the root PD
>> differently to the rest in the VM.
>>
>> It doesn't make much sense any more today, but I need to double check if
>> that isn't still the case.
>  From my observations, if root PD is evicted. amdgpu_vm_validate_pt_bos
> will validate and map it.
>
> And amdgpu_cs_list_validate always validates root PD after
> amdgpu_vm_validate_pt_bos has done that, it is actually unnecessary.
> Do you think it's worth skiping root PD validation in
> amdgpu_cs_list_validate? Thanks!

No, it's just your change is completely irrelevant and just complicates 
things.

Validating a BO twice should have basically no overhead at all.

And especially adding the manual call to map_table() in 
amdgpu_vm_make_compute() is a no-go. We don't want such specific 
handling for compute contexts.

Regards,
Christian.

>
> Regards,
> Lang
>
>
>> Christian.
>>
>>> The special case is when turning a GFX VM to a compute VM,
>>> if vm_update_mode changed, we need to map the root PD again.
>>> So just move root PD mapping to amdgpu_vm_make_compute.
>>>
>>> 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           |  5 +++++
>>>    2 files changed, 5 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 6a3bd8b9a08f..3805eef9ab69 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -425,22 +425,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..08fda57f5aa2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -2225,6 +2225,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>>>    	} else {
>>>    		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>>>    	}
>>> +
>>> +	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 v2] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Lang Yu]

On 06/13/ , Christian König wrote:
> Am 13.06.22 um 10:26 schrieb Lang Yu:
> > On 06/13/ , Christian König wrote:
> > > Am 13.06.22 um 09:59 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.
> > > I'm not sure if that's correct. Traditionally we have handled the root PD
> > > differently to the rest in the VM.
> > > 
> > > It doesn't make much sense any more today, but I need to double check if
> > > that isn't still the case.
> >  From my observations, if root PD is evicted. amdgpu_vm_validate_pt_bos
> > will validate and map it.
> > 
> > And amdgpu_cs_list_validate always validates root PD after
> > amdgpu_vm_validate_pt_bos has done that, it is actually unnecessary.
> > Do you think it's worth skiping root PD validation in
> > amdgpu_cs_list_validate? Thanks!
> 
> No, it's just your change is completely irrelevant and just complicates
> things.
> 
> Validating a BO twice should have basically no overhead at all.
> 
> And especially adding the manual call to map_table() in
> amdgpu_vm_make_compute() is a no-go. We don't want such specific handling
> for compute contexts.

That's because when turning a GFX VM to a compute VM, if vm_update_mode changed,
we need to map the root PD again.

If we always validate and map it in vm_validate_pt_pd_bos, that's so confused.

Regards,
Lang

> Regards,
> Christian.
> 
> > 
> > Regards,
> > Lang
> > 
> > 
> > > Christian.
> > > 
> > > > The special case is when turning a GFX VM to a compute VM,
> > > > if vm_update_mode changed, we need to map the root PD again.
> > > > So just move root PD mapping to amdgpu_vm_make_compute.
> > > > 
> > > > 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           |  5 +++++
> > > >    2 files changed, 5 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 6a3bd8b9a08f..3805eef9ab69 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > > @@ -425,22 +425,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..08fda57f5aa2 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > > @@ -2225,6 +2225,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> > > >    	} else {
> > > >    		vm->update_funcs = &amdgpu_vm_sdma_funcs;
> > > >    	}
> > > > +
> > > > +	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 v2] drm/amdkfd: simplify vm_validate_pt_pd_bos

[Christian König]

Am 13.06.22 um 10:41 schrieb Lang Yu:
> On 06/13/ , Christian König wrote:
>> Am 13.06.22 um 10:26 schrieb Lang Yu:
>>> On 06/13/ , Christian König wrote:
>>>> Am 13.06.22 um 09:59 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.
>>>> I'm not sure if that's correct. Traditionally we have handled the root PD
>>>> differently to the rest in the VM.
>>>>
>>>> It doesn't make much sense any more today, but I need to double check if
>>>> that isn't still the case.
>>>   From my observations, if root PD is evicted. amdgpu_vm_validate_pt_bos
>>> will validate and map it.
>>>
>>> And amdgpu_cs_list_validate always validates root PD after
>>> amdgpu_vm_validate_pt_bos has done that, it is actually unnecessary.
>>> Do you think it's worth skiping root PD validation in
>>> amdgpu_cs_list_validate? Thanks!
>> No, it's just your change is completely irrelevant and just complicates
>> things.
>>
>> Validating a BO twice should have basically no overhead at all.
>>
>> And especially adding the manual call to map_table() in
>> amdgpu_vm_make_compute() is a no-go. We don't want such specific handling
>> for compute contexts.
> That's because when turning a GFX VM to a compute VM, if vm_update_mode changed,
> we need to map the root PD again.

Ah, ok that sounds valid. But please add a comment explaining this.

> If we always validate and map it in vm_validate_pt_pd_bos, that's so confused.

It's indeed not really clean, we shouldn't leak requirements like this 
outside of the VM code.

Regards,
Christian.

>
> Regards,
> Lang
>
>> Regards,
>> Christian.
>>
>>> Regards,
>>> Lang
>>>
>>>
>>>> Christian.
>>>>
>>>>> The special case is when turning a GFX VM to a compute VM,
>>>>> if vm_update_mode changed, we need to map the root PD again.
>>>>> So just move root PD mapping to amdgpu_vm_make_compute.
>>>>>
>>>>> 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           |  5 +++++
>>>>>     2 files changed, 5 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 6a3bd8b9a08f..3805eef9ab69 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> @@ -425,22 +425,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..08fda57f5aa2 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -2225,6 +2225,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>>>>>     	} else {
>>>>>     		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>>>>>     	}
>>>>> +
>>>>> +	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;