Actually that lockless access is removed again in the next patch.
I will reorder the patches, so that it never even occur.
Christian.
Am 28.08.2017 um 06:12 schrieb zhoucm1:
> I'm not sure this one, since you remove status lock for vm_status
> list, I think we need to test this carefully.
>
> Regards,
>
> David Zhou
>
>
> On 2017å¹´08æ??25æ?¥ 17:38, Christian König wrote:
>> From: Christian König <christian.koenig at amd.com>
>>
>> We changed this to use an extra list a while back, but for the next
>> series I need a separate flag again.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 +++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37
>> ++++++++++++++----------------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 ---
>> 3 files changed, 20 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index a288fa6..e613ba4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -55,6 +55,9 @@ struct amdgpu_bo_va {
>> /* mappings for this bo_va */
>> struct list_head invalids;
>> struct list_head valids;
>> +
>> + /* If the mappings are cleared or filled */
>> + bool cleared;
>> };
>> struct amdgpu_bo {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f621dba..16148ef 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1787,10 +1787,13 @@ 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))
>> + /* We access vm_status without the status lock here, but that is ok
>> + * because when we don't clear the BO is locked and so the
>> status can't
>> + * change
>> + */
>> + if ((!clear && !list_empty(&bo_va->base.vm_status)) ||
>> + bo_va->cleared != clear)
>> 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,
>> @@ -1800,25 +1803,22 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>> *adev,
>> return r;
>> }
>> - if (trace_amdgpu_vm_bo_mapping_enabled()) {
>> - list_for_each_entry(mapping, &bo_va->valids, list)
>> - trace_amdgpu_vm_bo_mapping(mapping);
>> -
>> - list_for_each_entry(mapping, &bo_va->invalids, list)
>> - trace_amdgpu_vm_bo_mapping(mapping);
>> + if (vm->use_cpu_for_update) {
>> + /* Flush HDP */
>> + mb();
>> + amdgpu_gart_flush_gpu_tlb(adev, 0);
>> }
>> spin_lock(&vm->status_lock);
>> - list_splice_init(&bo_va->invalids, &bo_va->valids);
>> list_del_init(&bo_va->base.vm_status);
>> - if (clear)
>> - list_add(&bo_va->base.vm_status, &vm->cleared);
>> spin_unlock(&vm->status_lock);
>> - if (vm->use_cpu_for_update) {
>> - /* Flush HDP */
>> - mb();
>> - amdgpu_gart_flush_gpu_tlb(adev, 0);
>> + list_splice_init(&bo_va->invalids, &bo_va->valids);
>> + bo_va->cleared = clear;
>> +
>> + if (trace_amdgpu_vm_bo_mapping_enabled()) {
>> + list_for_each_entry(mapping, &bo_va->valids, list)
>> + trace_amdgpu_vm_bo_mapping(mapping);
>> }
>> return 0;
>> @@ -2419,9 +2419,7 @@ void amdgpu_vm_bo_invalidate(struct
>> amdgpu_device *adev,
>> list_for_each_entry(bo_base, &bo->va, bo_list) {
>> spin_lock(&bo_base->vm->status_lock);
>> - if (list_empty(&bo_base->vm_status))
>> - list_add(&bo_base->vm_status,
>> - &bo_base->vm->moved);
>> + list_move(&bo_base->vm_status, &bo_base->vm->moved);
>> spin_unlock(&bo_base->vm->status_lock);
>> }
>> }
>> @@ -2508,7 +2506,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>> vm->reserved_vmid[i] = NULL;
>> spin_lock_init(&vm->status_lock);
>> INIT_LIST_HEAD(&vm->moved);
>> - INIT_LIST_HEAD(&vm->cleared);
>> INIT_LIST_HEAD(&vm->freed);
>> /* create scheduler entity for page table updates */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 9347d28..e705f0f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -126,9 +126,6 @@ struct amdgpu_vm {
>> /* BOs moved, but not yet updated in the PT */
>> struct list_head moved;
>> - /* BOs cleared in the PT because of a move */
>> - struct list_head cleared;
>> -
>> /* BO mappings freed, but not yet updated in the PT */
>> struct list_head freed;
>
