Am 26.04.2018 um 05:01 schrieb Zhang, Jerry (Junwei):
> On 04/24/2018 03:35 PM, Chunming Zhou wrote:
>> Change-Id: Ia3e57dbacff05f32b6c02e29aeadabd36f08028e
>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
> Reviewed-by: Junwei Zhang <Jerry.Zhang at amd.com>
>
> some trivial comments.
Both a nice cleanup as well as the right comments.
With Jerry's comments addressed Reviewed-by: Christian König
<christian.koenig at amd.com> as well.
>
>> ---
>> Â drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75
>> ++++++++++++++++++----------------
>> Â 1 file changed, 40 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8c34060e130f..c75b96433ee7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -94,6 +94,9 @@ struct amdgpu_prt_cb {
>> Â Â Â Â Â struct dma_fence_cb cb;
>> Â };
>>
>> +static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_vm *vm,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_bo *bo);
>
> Move the function upper, we could avoid such declaration.
>
>> Â /**
>> Â Â * amdgpu_vm_level_shift - return the addr shift for each level
>> Â Â *
>> @@ -446,11 +449,9 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â */
>> Â Â Â Â Â Â Â Â Â Â Â Â Â pt->parent = amdgpu_bo_ref(parent->base.bo);
>>
>> -Â Â Â Â Â Â Â Â Â Â Â entry->base.vm = vm;
>> -Â Â Â Â Â Â Â Â Â Â Â entry->base.bo = pt;
>> -Â Â Â Â Â Â Â Â Â Â Â list_add_tail(&entry->base.bo_list, &pt->va);
>> +Â Â Â Â Â Â Â Â Â Â Â amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â spin_lock(&vm->status_lock);
>> -Â Â Â Â Â Â Â Â Â Â Â list_add(&entry->base.vm_status, &vm->relocated);
>> +Â Â Â Â Â Â Â Â Â Â Â list_move(&entry->base.vm_status, &vm->relocated);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock(&vm->status_lock);
>> Â Â Â Â Â Â Â Â Â }
>>
>> @@ -1821,6 +1822,35 @@ int amdgpu_vm_handle_moved(struct
>> amdgpu_device *adev,
>> Â Â Â Â Â return r;
>> Â }
>>
>> +static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_vm *vm,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amdgpu_bo *bo)
>> +{
>> +Â Â Â base->vm = vm;
>> +Â Â Â base->bo = bo;
>> +Â Â Â INIT_LIST_HEAD(&base->bo_list);
>> +Â Â Â INIT_LIST_HEAD(&base->vm_status);
>> +
>> +Â Â Â if (!bo)
>> +Â Â Â Â Â Â Â return;
>> +Â Â Â list_add_tail(&base->bo_list, &bo->va);
>> +
>> +Â Â Â if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
>> +Â Â Â Â Â Â Â return;
>> +
>> +Â Â Â if (bo->preferred_domains &
>> +Â Â Â Â Â Â Â amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
>> +Â Â Â Â Â Â Â return;
>> +
>> +Â Â Â /*
>> +Â Â Â Â * we checked all the prerequisites, but it looks like this per
>> vm bo
>> +Â Â Â Â * is currently evicted. add the bo to the evicted list to make
>> sure it
>> +Â Â Â Â * is validated on next vm use to avoid fault.
>> +Â Â Â Â * */
>
> /*
> Â *
> Â */Â is expected
>
>> +Â Â Â spin_lock(&vm->status_lock);
>> +Â Â Â list_move_tail(&base->vm_status, &vm->evicted);
>> +Â Â Â spin_unlock(&vm->status_lock);
>> +}
>> Â /**
>> Â Â * amdgpu_vm_bo_add - add a bo to a specific vm
>> Â Â *
>> @@ -1844,36 +1874,12 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct
>> amdgpu_device *adev,
>> Â Â Â Â Â if (bo_va == NULL) {
>> Â Â Â Â Â Â Â Â Â return NULL;
>> Â Â Â Â Â }
>> -Â Â Â bo_va->base.vm = vm;
>> -Â Â Â bo_va->base.bo = bo;
>> -Â Â Â INIT_LIST_HEAD(&bo_va->base.bo_list);
>> -Â Â Â INIT_LIST_HEAD(&bo_va->base.vm_status);
>> +Â Â Â amdgpu_vm_bo_base_init(&bo_va->base, vm, bo);
>>
>> Â Â Â Â Â bo_va->ref_count = 1;
>> Â Â Â Â Â INIT_LIST_HEAD(&bo_va->valids);
>> Â Â Â Â Â INIT_LIST_HEAD(&bo_va->invalids);
>>
>> -Â Â Â if (!bo)
>> -Â Â Â Â Â Â Â return bo_va;
>> -
>> -Â Â Â list_add_tail(&bo_va->base.bo_list, &bo->va);
>> -
>> -Â Â Â if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
>> -Â Â Â Â Â Â Â return bo_va;
>> -
>> -Â Â Â if (bo->preferred_domains &
>> -Â Â Â Â Â Â Â amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
>> -Â Â Â Â Â Â Â return bo_va;
>> -
>> -Â Â Â /*
>> -Â Â Â Â * We checked all the prerequisites, but it looks like this per
>> VM BO
>> -Â Â Â Â * is currently evicted. add the BO to the evicted list to make
>> sure it
>> -Â Â Â Â * is validated on next VM use to avoid fault.
>> -Â Â Â Â * */
>> -Â Â Â spin_lock(&vm->status_lock);
>> -Â Â Â list_move_tail(&bo_va->base.vm_status, &vm->evicted);
>> -Â Â Â spin_unlock(&vm->status_lock);
>> -
>> Â Â Â Â Â return bo_va;
>> Â }
>>
>> @@ -2373,6 +2379,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>> Â Â Â Â Â Â Â Â Â Â Â Â int vm_context, unsigned int pasid)
>> Â {
>> Â Â Â Â Â struct amdgpu_bo_param bp;
>> +Â Â Â struct amdgpu_bo *root;
>> Â Â Â Â Â const unsigned align = min(AMDGPU_VM_PTB_ALIGN_SIZE,
>> Â Â Â Â Â Â Â Â Â AMDGPU_VM_PTE_COUNT(adev) * 8);
>> Â Â Â Â Â unsigned ring_instance;
>> @@ -2434,23 +2441,21 @@ int amdgpu_vm_init(struct amdgpu_device
>> *adev, struct amdgpu_vm *vm,
>> Â Â Â Â Â bp.flags = flags;
>> Â Â Â Â Â bp.type = ttm_bo_type_kernel;
>> Â Â Â Â Â bp.resv = NULL;
>> -Â Â Â r = amdgpu_bo_create(adev, &bp, &vm->root.base.bo);
>> +Â Â Â r = amdgpu_bo_create(adev, &bp, &root);
>> Â Â Â Â Â if (r)
>> Â Â Â Â Â Â Â Â Â goto error_free_sched_entity;
>>
>> -Â Â Â r = amdgpu_bo_reserve(vm->root.base.bo, true);
>> +Â Â Â r = amdgpu_bo_reserve(root, true);
>> Â Â Â Â Â if (r)
>> Â Â Â Â Â Â Â Â Â goto error_free_root;
>>
>> -Â Â Â r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
>> +Â Â Â r = amdgpu_vm_clear_bo(adev, vm, root,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â adev->vm_manager.root_level,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â vm->pte_support_ats);
>> Â Â Â Â Â if (r)
>> Â Â Â Â Â Â Â Â Â goto error_unreserve;
>>
>> -Â Â Â vm->root.base.vm = vm;
>> -Â Â Â list_add_tail(&vm->root.base.bo_list, &vm->root.base.bo->va);
>> -Â Â Â list_add_tail(&vm->root.base.vm_status, &vm->evicted);
>> +Â Â Â amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
>> Â Â Â Â Â amdgpu_bo_unreserve(vm->root.base.bo);
>>
>> Â Â Â Â Â if (pasid) {
>>
