[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 at 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;