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@xxxxxxx>
> > > > ---
> > > > 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;
>
