[PATCH 2/2] drm/amdgpu: skip vm entries checking while in sriov vf

[Zhu, Rex]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Koenig, Christian
> Sent: Friday, October 12, 2018 3:25 PM
> To: Huang, Ray <Ray.Huang at amd.com>; Kuehling, Felix
> <Felix.Kuehling at amd.com>; Min, Frank <Frank.Min at amd.com>; amd-
> gfx at lists.freedesktop.org
> 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 at lists.freedesktop.org] On
> >> Behalf Of Felix Kuehling
> >> Sent: Friday, October 12, 2018 3:46 AM
> >> To: Koenig, Christian <Christian.Koenig at amd.com>; Min, Frank
> >> <Frank.Min at amd.com>; amd-gfx at lists.freedesktop.org
> >> 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?

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 at amd.com>
> >>>> ---
> >>>>    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 at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx