[PATCH 1/5] drm/amdgpu: rework moved handling in the VM v2

[Christian König]

> I asked you how you keep the access of base.moved is safely in last 
> thread,
Sorry, I've missed that question somehow.

> I check it just now, it depends on the shared resv lock.
Actually that's not 100% correct. The moved member is protected by the 
BOs resv lock.

That can be the shared one (in the case of PDs/PTs/local BOs), but can 
also be a separate lock.

> I find most of all vm lists are protected by the shared resv lock, 
> whether the status lock can be removed as well? seems everything of vm 
> is protected by that big resv lock. 
Nope, that won't work. When amdgpu_vm_bo_invalidate() is called only the 
BO itself is locked, but not something from the VM.

So we need a separate lock to protect this list.

Thanks for the review,
Christian.

Am 29.08.2017 um 04:13 schrieb zhoucm1:
> Hi Christian,
>
> I asked you how you keep the access of base.moved is safely in last 
> thread, I check it just now, it depends on the shared resv lock.
>
> For patch itself, Reviewed-by: Chunming Zhou <david1.zhou at amd.com>
>
> another question comes up to me, I find most of all vm lists are 
> protected by the shared resv lock, whether the status lock can be 
> removed as well? seems everything of vm is protected by that big resv 
> lock.
>
> Regards,
> David Zhou
>
> On 2017年08月29日 02:50, Christian König wrote:
>> From: Christian König <christian.koenig at amd.com>
>>
>> Instead of using the vm_state use a separate flag to note
>> that the BO was moved.
>>
>> v2: reorder patches to avoid temporary lockless access
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  3 +++
>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f621dba..ee53293 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1787,10 +1787,16 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>> *adev,
>>       else
>>           flags = 0x0;
>>   -    spin_lock(&vm->status_lock);
>> -    if (!list_empty(&bo_va->base.vm_status))
>> +    if (!clear && bo_va->base.moved) {
>> +        bo_va->base.moved = false;
>>           list_splice_init(&bo_va->valids, &bo_va->invalids);
>> -    spin_unlock(&vm->status_lock);
>> +
>> +    } else {
>> +        spin_lock(&vm->status_lock);
>> +        if (!list_empty(&bo_va->base.vm_status))
>> +            list_splice_init(&bo_va->valids, &bo_va->invalids);
>> +        spin_unlock(&vm->status_lock);
>> +    }
>>         list_for_each_entry(mapping, &bo_va->invalids, list) {
>>           r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, 
>> vm,
>> @@ -2418,6 +2424,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>       struct amdgpu_vm_bo_base *bo_base;
>>         list_for_each_entry(bo_base, &bo->va, bo_list) {
>> +        bo_base->moved = true;
>>           spin_lock(&bo_base->vm->status_lock);
>>           if (list_empty(&bo_base->vm_status))
>>               list_add(&bo_base->vm_status,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 9347d28..1b478e6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -105,6 +105,9 @@ struct amdgpu_vm_bo_base {
>>         /* protected by spinlock */
>>       struct list_head        vm_status;
>> +
>> +    /* protected by the BO being reserved */
>> +    bool                moved;
>>   };
>>     struct amdgpu_vm_pt {
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx