Am 12.10.2018 um 09:31 schrieb Zhu, Rex: > >> -----Original Message----- >> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of >> Koenig, Christian >> Sent: Friday, October 12, 2018 3:25 PM >> To: Huang, Ray <Ray.Huang@xxxxxxx>; Kuehling, Felix >> <Felix.Kuehling@xxxxxxx>; Min, Frank <Frank.Min@xxxxxxx>; amd- >> gfx@xxxxxxxxxxxxxxxxxxxxx >> Subject: Re: [PATCH 2/2] drm/amdgpu: skip vm entries checking while in sriov >> vf >> >> Am 12.10.2018 um 06:05 schrieb Huang, Ray: >>>> -----Original Message----- >>>> From: amd-gfx [mailto:amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On >>>> Behalf Of Felix Kuehling >>>> Sent: Friday, October 12, 2018 3:46 AM >>>> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Min, Frank >>>> <Frank.Min@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx >>>> Subject: Re: [PATCH 2/2] drm/amdgpu: skip vm entries checking while >>>> in sriov vf >>>> >>>> On 2018-10-11 08:32 AM, Christian König wrote: >>>>> Am 11.10.2018 um 14:09 schrieb Frank.Min: >>>>>> vm page table would be allocated while in amdgpu_vm_init by >>>>>> amdgpu_allocate_static_csa for sriov, so the checking here would be >>>>>> skipped. >>>>> NAK, that checking is actually correct and points out that this >>>>> doesn't work correctly. >>>>> >>>>> Fix the function to be able to handle the case when there are >>>>> already mappings in the VM. >>>> KFD and the Thunk assume that they own the entire address space. If >>>> something is already mapped, KFD and the Thunk would not be aware of >> it. >>>> amdgpu_allocate_static_csa runs during driver initialization, before >>>> there are any VMs. Looks like amdgpu_map_static_csa is the more >>>> important function here, which currently runs in >> amdgpu_driver_open_kms. >>>> Could amdgpu_map_static_csa be delayed to the first time user mode >>>> maps memory into the VM through GEM APIs? That way it would allow >>>> turning a VM into a compute VM that hasn't been used for any GEM >>>> memory mappings. >>>> >>> Yes. Frank and I found, while we do acquire_process_vm(), the vm->va RB >> trees are always not empty under SRIOV, because amdgpu_map_static_csa() >> will be called while vm inits under SRIOV, that will actually map the CSA BO to >> the VM and insert the node into vm->va before it acquires process VM in KFD. >> >> Yeah and exactly that will clash with the KFD. >> >> Could be that it still works initially because the CSA is not mapped at the >> beginning of the address space IIRC, but we need to clean that up in the long >> term. >> >> We should either delay adding the CSA BO to the VM (probably best done >> during context creation). > Yes, we planed to create sdma csa buffer when context creation. > >> Or we start to make nails with heads and move the CSA BO into the context > So we can add the sdma_csa bo/static_csa in the struct amdgpu_ctx? Yes, exactly that's my idea as well. Regards, Christian. > > Best Regards > Rex > >> altogether. We are going to need this anyway for MCBP, so that would be a >> good first step. >> Ray/Frank can anybody of you take care of this? >> >> Thanks, >> Christian. >> >>> Thanks, >>> Ray >>> >>>> Regards, >>>> Felix >>>> >>>>> Christian. >>>>> >>>>>> Change-Id: Id30b86ad15ae509aeed9ed8ab60c259c88af3df5 >>>>>> Signed-off-by: Frank.Min <Frank.Min@xxxxxxx> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++---- >>>>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>>>> index 6904d79..6066f87 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>>>>> @@ -3100,11 +3100,12 @@ int amdgpu_vm_make_compute(struct >>>>>> amdgpu_device *adev, struct amdgpu_vm *vm, uns >>>>>> return r; >>>>>> /* Sanity checks */ >>>>>> - if (!RB_EMPTY_ROOT(&vm->va.rb_root) || vm->root.entries) { >>>>>> - r = -EINVAL; >>>>>> - goto unreserve_bo; >>>>>> + if (!amdgpu_sriov_vf(adev)) { >>>>>> + if (!RB_EMPTY_ROOT(&vm->va.rb_root) || vm->root.entries) { >>>>>> + r = -EINVAL; >>>>>> + goto unreserve_bo; >>>>>> + } >>>>>> } >>>>>> - >>>>>> if (pasid) { >>>>>> unsigned long flags; >>>>>> >>>>> _______________________________________________ >>>>> amd-gfx mailing list >>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
