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

[Koenig, Christian]

Am 12.10.2018 um 09:31 schrieb 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?

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 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