[RFC PATCH 1/4] drm/amdkfd: Improve amdgpu_vm_handle_moved

[Felix Kuehling]

Am 2022-03-17 um 04:21 schrieb Christian König:
> Am 17.03.22 um 01:20 schrieb Felix Kuehling:
>> Let amdgpu_vm_handle_moved update all BO VA mappings of BOs reserved by
>> the caller. This will be useful for handling extra BO VA mappings in
>> KFD VMs that are managed through the render node API.
>
> Yes, that change is on my TODO list for quite a while as well.
>
>> TODO: This may also allow simplification of amdgpu_cs_vm_handling. See
>> the TODO comment in the code.
>
> No, that won't work just yet.
>
> We need to change the TLB flush detection for that, but I'm already 
> working on those as well.

Your TLB flushing patch series looks good to me.

There is one other issue, though. amdgpu_vm_handle_moved doesn't update 
the sync object, so I couldn't figure out I can wait for all the page 
table updates to finish.

Regards,
   Felix


>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>
> Please update the TODO, with that done: Reviewed-by: Christian König 
> <christian.koenig at amd.com>
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  6 +++++-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 18 +++++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  3 ++-
>>   4 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index d162243d8e78..10941f0d8dde 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -826,6 +826,10 @@ static int amdgpu_cs_vm_handling(struct 
>> amdgpu_cs_parser *p)
>>               return r;
>>       }
>>   +    /* TODO: Is this loop still needed, or could this be handled by
>> +     * amdgpu_vm_handle_moved, now that it can handle all BOs that are
>> +     * reserved under p->ticket?
>> +     */
>>       amdgpu_bo_list_for_each_entry(e, p->bo_list) {
>>           /* ignore duplicates */
>>           bo = ttm_to_amdgpu_bo(e->tv.bo);
>> @@ -845,7 +849,7 @@ static int amdgpu_cs_vm_handling(struct 
>> amdgpu_cs_parser *p)
>>               return r;
>>       }
>>   -    r = amdgpu_vm_handle_moved(adev, vm);
>> +    r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
>>       if (r)
>>           return r;
>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> index 579adfafe4d0..50805613c38c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> @@ -414,7 +414,7 @@ amdgpu_dma_buf_move_notify(struct 
>> dma_buf_attachment *attach)
>>             r = amdgpu_vm_clear_freed(adev, vm, NULL);
>>           if (!r)
>> -            r = amdgpu_vm_handle_moved(adev, vm);
>> +            r = amdgpu_vm_handle_moved(adev, vm, ticket);
>>             if (r && r != -EBUSY)
>>               DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index fc4563cf2828..726b42c6d606 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2190,11 +2190,12 @@ int amdgpu_vm_clear_freed(struct 
>> amdgpu_device *adev,
>>    * PTs have to be reserved!
>>    */
>>   int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>> -               struct amdgpu_vm *vm)
>> +               struct amdgpu_vm *vm,
>> +               struct ww_acquire_ctx *ticket)
>>   {
>>       struct amdgpu_bo_va *bo_va, *tmp;
>>       struct dma_resv *resv;
>> -    bool clear;
>> +    bool clear, unlock;
>>       int r;
>>         list_for_each_entry_safe(bo_va, tmp, &vm->moved, 
>> base.vm_status) {
>> @@ -2212,17 +2213,24 @@ int amdgpu_vm_handle_moved(struct 
>> amdgpu_device *adev,
>>           spin_unlock(&vm->invalidated_lock);
>>             /* Try to reserve the BO to avoid clearing its ptes */
>> -        if (!amdgpu_vm_debug && dma_resv_trylock(resv))
>> +        if (!amdgpu_vm_debug && dma_resv_trylock(resv)) {
>>               clear = false;
>> +            unlock = true;
>> +        /* The caller is already holding the reservation lock */
>> +        } else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
>> +            clear = false;
>> +            unlock = false;
>>           /* Somebody else is using the BO right now */
>> -        else
>> +        } else {
>>               clear = true;
>> +            unlock = false;
>> +        }
>>             r = amdgpu_vm_bo_update(adev, bo_va, clear, NULL);
>>           if (r)
>>               return r;
>>   -        if (!clear)
>> +        if (unlock)
>>               dma_resv_unlock(resv);
>>           spin_lock(&vm->invalidated_lock);
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index a40a6a993bb0..120a76aaae75 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -396,7 +396,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device 
>> *adev,
>>                 struct amdgpu_vm *vm,
>>                 struct dma_fence **fence);
>>   int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>> -               struct amdgpu_vm *vm);
>> +               struct amdgpu_vm *vm,
>> +               struct ww_acquire_ctx *ticket);
>>   int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>                   struct amdgpu_device *bo_adev,
>>                   struct amdgpu_vm *vm, bool immediate,
>