[PATCH] drm/amdgpu: Optimize recursion in amdgpu_vm_update_level

[Felix Kuehling]

On 17-07-12 03:50 AM, Christian König wrote:
>  
> That optimization is unnecessary.
>
> I have a patch in the pipeline that makes the VM BO permanently CPU
> mapped and also fixes a couple of bugs in those code path.

You mean just for the CPU-update path? I think deferring the SDMA IB
allocation still makes sense. In fact that's the code path I tested.

>
> Give me a moment to clean that up and send it out.

Sure. BTW, I found that the most expensive operation turned out to be
amdgpu_vm_move_pt_bos_in_lru. It has to recurse through all PT BOs and
does lots of redundant list pointer manipulations for two lists (LRU and
swap), and kref get/put.

For KFD VMs I'm removing the call to it. I think it's useless for us.

amdgpu_vm_validate_pt_bos is also expensive, but not as bad because it's
only done if there was an eviction.

I was thinking about ways to optimize this. It should be possible to
allocate multiple PTBs and PDBs in a single BO in amdgpu_vm_alloc_pts.
That would potentially save lots of time in vm_move_pt_bos and
vm_validate. What do you think?

Regards,
  Felix

>
> Regards,
> Christian.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 109
>> +++++++++++++++++----------------
>>   1 file changed, 55 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index ff5de3a..23b899b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1025,7 +1025,7 @@ static int amdgpu_vm_update_level(struct
>> amdgpu_device *adev,
>>   {
>>       struct amdgpu_bo *shadow;
>>       struct amdgpu_ring *ring = NULL;
>> -    uint64_t pd_addr, shadow_addr = 0;
>> +    uint64_t pd_addr = 0, shadow_addr = 0;
>>       uint32_t incr = amdgpu_vm_bo_size(adev, level + 1);
>>       uint64_t last_pde = ~0, last_pt = ~0, last_shadow = ~0;
>>       unsigned count = 0, pt_idx, ndw = 0;
>> @@ -1044,48 +1044,19 @@ static int amdgpu_vm_update_level(struct
>> amdgpu_device *adev,
>>         WARN_ON(vm->use_cpu_for_update && shadow);
>>       if (vm->use_cpu_for_update && !shadow) {
>> -        r = amdgpu_bo_kmap(parent->bo, (void **)&pd_addr);
>> -        if (r)
>> -            return r;
>> -        r = amdgpu_vm_bo_wait(adev, parent->bo);
>> -        if (unlikely(r)) {
>> -            amdgpu_bo_kunmap(parent->bo);
>> -            return r;
>> -        }
>> +        /* Defer kmapping until it's actually needed. Some
>> +         * PDBs may need no update at all
>> +         */
>>           params.func = amdgpu_vm_cpu_set_ptes;
>> +        params.ib = (void *)(long)-1;
>>       } else {
>> -        if (shadow) {
>> -            r = amdgpu_ttm_bind(&shadow->tbo, &shadow->tbo.mem);
>> -            if (r)
>> -                return r;
>> -        }
>> -        ring = container_of(vm->entity.sched, struct amdgpu_ring,
>> -                    sched);
>> -
>> -        /* padding, etc. */
>> -        ndw = 64;
>> -
>> -        /* assume the worst case */
>> -        ndw += parent->last_entry_used * 6;
>> -
>> -        pd_addr = amdgpu_bo_gpu_offset(parent->bo);
>> -
>> -        if (shadow) {
>> -            shadow_addr = amdgpu_bo_gpu_offset(shadow);
>> -            ndw *= 2;
>> -        } else {
>> -            shadow_addr = 0;
>> -        }
>> -
>> -        r = amdgpu_job_alloc_with_ib(adev, ndw * 4, &job);
>> -        if (r)
>> -            return r;
>> -
>> -        params.ib = &job->ibs[0];
>> +        /* Defer IB allocation until it's actually
>> +         * needed. Some PDBs may need no update at all
>> +         */
>> +        params.ib = NULL;
>>           params.func = amdgpu_vm_do_set_ptes;
>>       }
>>   -
>>       /* walk over the address space and update the directory */
>>       for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
>>           struct amdgpu_bo *bo = parent->entries[pt_idx].bo;
>> @@ -1094,22 +1065,53 @@ static int amdgpu_vm_update_level(struct
>> amdgpu_device *adev,
>>           if (bo == NULL)
>>               continue;
>>   -        if (bo->shadow) {
>> -            struct amdgpu_bo *pt_shadow = bo->shadow;
>> -
>> -            r = amdgpu_ttm_bind(&pt_shadow->tbo,
>> -                        &pt_shadow->tbo.mem);
>> -            if (r)
>> -                return r;
>> -        }
>> -
>> -        pt = amdgpu_bo_gpu_offset(bo);
>> -        pt = amdgpu_gart_get_vm_pde(adev, pt);
>> +        pt = amdgpu_gart_get_vm_pde(adev, bo->tbo.offset);
>>           if (parent->entries[pt_idx].addr == pt)
>>               continue;
>>             parent->entries[pt_idx].addr = pt;
>>   +        if (!params.ib) {
>> +            if (shadow) {
>> +                r = amdgpu_ttm_bind(&shadow->tbo,
>> +                            &shadow->tbo.mem);
>> +                if (r)
>> +                    return r;
>> +            }
>> +
>> +            ring = container_of(vm->entity.sched,
>> +                        struct amdgpu_ring, sched);
>> +
>> +            /* padding, etc. */
>> +            ndw = 64;
>> +
>> +            /* assume the worst case */
>> +            ndw += (parent->last_entry_used - pt_idx) * 6;
>> +
>> +            pd_addr = parent->bo->tbo.offset;
>> +
>> +            if (shadow) {
>> +                shadow_addr = shadow->tbo.offset;
>> +                ndw *= 2;
>> +            } else {
>> +                shadow_addr = 0;
>> +            }
>> +            r = amdgpu_job_alloc_with_ib(adev, ndw * 4, &job);
>> +            if (r)
>> +                return r;
>> +
>> +            params.ib = &job->ibs[0];
>> +        } else if (!pd_addr) {
>> +            r = amdgpu_bo_kmap(parent->bo, (void **)&pd_addr);
>> +            if (r)
>> +                return r;
>> +            r = amdgpu_vm_bo_wait(adev, parent->bo);
>> +            if (unlikely(r)) {
>> +                amdgpu_bo_kunmap(parent->bo);
>> +                return r;
>> +            }
>> +        }
>> +
>>           pde = pd_addr + pt_idx * 8;
>>           if (((last_pde + 8 * count) != pde) ||
>>               ((last_pt + incr * count) != pt) ||
>> @@ -1148,9 +1150,9 @@ static int amdgpu_vm_update_level(struct
>> amdgpu_device *adev,
>>         if (params.func == amdgpu_vm_cpu_set_ptes)
>>           amdgpu_bo_kunmap(parent->bo);
>> -    else if (params.ib->length_dw == 0) {
>> +    else if (params.ib && params.ib->length_dw == 0) {
>>           amdgpu_job_free(job);
>> -    } else {
>> +    } else if (params.ib) {
>>           amdgpu_ring_pad_ib(ring, params.ib);
>>           amdgpu_sync_resv(adev, &job->sync, parent->bo->tbo.resv,
>>                    AMDGPU_FENCE_OWNER_VM);
>> @@ -1166,8 +1168,7 @@ static int amdgpu_vm_update_level(struct
>> amdgpu_device *adev,
>>             amdgpu_bo_fence(parent->bo, fence, true);
>>           dma_fence_put(vm->last_dir_update);
>> -        vm->last_dir_update = dma_fence_get(fence);
>> -        dma_fence_put(fence);
>> +        vm->last_dir_update = fence;
>>       }
>>       /*
>>        * Recurse into the subdirectories. This recursion is harmless
>> because
>> @@ -1176,7 +1177,7 @@ static int amdgpu_vm_update_level(struct
>> amdgpu_device *adev,
>>       for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
>>           struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
>>   -        if (!entry->bo)
>> +        if (!entry->bo || !entry->entries)
>>               continue;
>>             r = amdgpu_vm_update_level(adev, vm, entry, level + 1);
>
>