[PATCH 2/2] drm/amdgpu: link shadow bo as well

[zhoucm1]

On 2018年05月03日 18:14, Christian König wrote:
> Am 24.04.2018 um 09:35 schrieb Chunming Zhou:
>> Shadow BO is located on GTT and its parent (PT and PD) BO could 
>> located on VRAM.
>> In some case, the BO on GTT could be evicted but the parent did not. 
>> This may
>> cause the shadow BO not be put in the evict list and could not be 
>> invalidate
>> correctly.
>>
>> Change-Id: Iad10d9a3031fa2b243879b9e58ee4d8c527eb433
>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>> Reported-by: Shaoyun Liu <Shaoyun.Liu at amd.com>
>
> Way to much memory usage and to complicated.
>
> What we need to do is just to handle the real BO as evicted when the 
> shadow BO is evicted.
>
> Something like the following at the start of amdgpu_vm_bo_invalidate() 
> should be sufficient:
>
> if (bo->parent && bo->parent->shadow == bo)
>     bo = bo->parent;
clever, will send a patch soon.

Regards,
David Zhou
>
> Regards,
> Christian.
>
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     |  5 -----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 11 +++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  1 +
>>   4 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index e1756b68a17b..9c9f6fc5c994 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -462,11 +462,6 @@ static int amdgpu_cs_validate(void *param, 
>> struct amdgpu_bo *bo)
>>       do {
>>           r = amdgpu_cs_bo_validate(p, bo);
>>       } while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
>> -    if (r)
>> -        return r;
>> -
>> -    if (bo->shadow)
>> -        r = amdgpu_cs_bo_validate(p, bo->shadow);
>>         return r;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index 540e03fa159f..8078da36aec7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -58,6 +58,7 @@ struct amdgpu_bo_va_mapping {
>>   /* User space allocated BO in a VM */
>>   struct amdgpu_bo_va {
>>       struct amdgpu_vm_bo_base    base;
>> +    struct amdgpu_vm_bo_base    shadow_base;
>>         /* protected by bo being reserved */
>>       unsigned            ref_count;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index c75b96433ee7..026147cc5104 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -450,8 +450,13 @@ static int amdgpu_vm_alloc_levels(struct 
>> amdgpu_device *adev,
>>               pt->parent = amdgpu_bo_ref(parent->base.bo);
>>                 amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> +            amdgpu_vm_bo_base_init(&entry->shadow_base, vm,
>> +                           pt->shadow);
>>               spin_lock(&vm->status_lock);
>>               list_move(&entry->base.vm_status, &vm->relocated);
>> +            if (pt->shadow)
>> +                list_move(&entry->shadow_base.vm_status,
>> +                      &vm->relocated);
>>               spin_unlock(&vm->status_lock);
>>           }
>>   @@ -1875,6 +1880,7 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct 
>> amdgpu_device *adev,
>>           return NULL;
>>       }
>>       amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
>> +    amdgpu_vm_bo_base_init(&bo_va->shadow_base, vm, bo ? bo->shadow 
>> : NULL);
>>         bo_va->ref_count = 1;
>>       INIT_LIST_HEAD(&bo_va->valids);
>> @@ -2220,9 +2226,11 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>>       struct amdgpu_vm *vm = bo_va->base.vm;
>>         list_del(&bo_va->base.bo_list);
>> +    list_del(&bo_va->shadow_base.bo_list);
>>         spin_lock(&vm->status_lock);
>>       list_del(&bo_va->base.vm_status);
>> +    list_del(&bo_va->shadow_base.vm_status);
>>       spin_unlock(&vm->status_lock);
>>         list_for_each_entry_safe(mapping, next, &bo_va->valids, list) {
>> @@ -2456,6 +2464,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm,
>>           goto error_unreserve;
>>         amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
>> +    amdgpu_vm_bo_base_init(&vm->root.shadow_base, vm, root->shadow);
>>       amdgpu_bo_unreserve(vm->root.base.bo);
>>         if (pasid) {
>> @@ -2575,6 +2584,8 @@ static void amdgpu_vm_free_levels(struct 
>> amdgpu_device *adev,
>>       if (parent->base.bo) {
>>           list_del(&parent->base.bo_list);
>>           list_del(&parent->base.vm_status);
>> +        list_del(&parent->shadow_base.bo_list);
>> +        list_del(&parent->shadow_base.vm_status);
>>           amdgpu_bo_unref(&parent->base.bo->shadow);
>>           amdgpu_bo_unref(&parent->base.bo);
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 30f080364c97..f4ae6c6b28b8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -153,6 +153,7 @@ struct amdgpu_vm_bo_base {
>>     struct amdgpu_vm_pt {
>>       struct amdgpu_vm_bo_base    base;
>> +    struct amdgpu_vm_bo_base    shadow_base;
>>       bool                huge;
>>         /* array of page tables, one for each directory entry */
>