Re: [PATCH 4/8] drm/amdgpu: remove last_entry_used from the VM code

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 11.12.2017 um 06:52 schrieb Chunming Zhou:
>
>
> On 2017年12月09日 00:41, Christian König wrote:
>> Not needed any more.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 52 
>> +++++++++++++++++++---------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  1 -
>>   2 files changed, 29 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 400a00fababd..ae5451bf5873 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -329,9 +329,6 @@ static int amdgpu_vm_alloc_levels(struct 
>> amdgpu_device *adev,
>>           to >= amdgpu_vm_num_entries(adev, level))
>>           return -EINVAL;
>>   -    if (to > parent->last_entry_used)
>> -        parent->last_entry_used = to;
>> -
>>       ++level;
>>       saddr = saddr & ((1 << shift) - 1);
>>       eaddr = eaddr & ((1 << shift) - 1);
>> @@ -1187,16 +1184,19 @@ static int amdgpu_vm_update_pde(struct 
>> amdgpu_device *adev,
>>    *
>>    * Mark all PD level as invalid after an error.
>>    */
>> -static void amdgpu_vm_invalidate_level(struct amdgpu_vm *vm,
>> -                       struct amdgpu_vm_pt *parent)
>> +static void amdgpu_vm_invalidate_level(struct amdgpu_device *adev,
>> +                       struct amdgpu_vm *vm,
>> +                       struct amdgpu_vm_pt *parent,
>> +                       unsigned level)
> can we move level to struct amdgpu_vm_pt?

I considered this as well, but then abandoned the approach and moved to 
using it as parameter again.

The general problem is that amdgpu_vm_pt is already *WAY* to big, we use 
60 bytes to manage 4K in the worst case.

Working on getting this down to something sane again, but adding the 
level here just to save passing it as parameter during the destruction 
would make it worse.

Christian.

> otherwise, it looks ok to me.
>
> Regards,
> David Zhou
>>   {
>> -    unsigned pt_idx;
>> +    unsigned pt_idx, num_entries;
>>         /*
>>        * Recurse into the subdirectories. This recursion is harmless 
>> because
>>        * we only have a maximum of 5 layers.
>>        */
>> -    for (pt_idx = 0; pt_idx <= parent->last_entry_used; ++pt_idx) {
>> +    num_entries = amdgpu_vm_num_entries(adev, level);
>> +    for (pt_idx = 0; pt_idx < num_entries; ++pt_idx) {
>>           struct amdgpu_vm_pt *entry = &parent->entries[pt_idx];
>>             if (!entry->base.bo)
>> @@ -1207,7 +1207,7 @@ static void amdgpu_vm_invalidate_level(struct 
>> amdgpu_vm *vm,
>>           if (list_empty(&entry->base.vm_status))
>>               list_add(&entry->base.vm_status, &vm->relocated);
>>           spin_unlock(&vm->status_lock);
>> -        amdgpu_vm_invalidate_level(vm, entry);
>> +        amdgpu_vm_invalidate_level(adev, vm, entry, level + 1);
>>       }
>>   }
>>   @@ -1249,7 +1249,8 @@ int amdgpu_vm_update_directories(struct 
>> amdgpu_device *adev,
>>                 r = amdgpu_vm_update_pde(adev, vm, pt, entry);
>>               if (r) {
>> -                amdgpu_vm_invalidate_level(vm, &vm->root);
>> +                amdgpu_vm_invalidate_level(adev, vm,
>> +                               &vm->root, 0);
>>                   return r;
>>               }
>>               spin_lock(&vm->status_lock);
>> @@ -1652,7 +1653,7 @@ static int amdgpu_vm_bo_update_mapping(struct 
>> amdgpu_device *adev,
>>     error_free:
>>       amdgpu_job_free(job);
>> -    amdgpu_vm_invalidate_level(vm, &vm->root);
>> +    amdgpu_vm_invalidate_level(adev, vm, &vm->root, 0);
>>       return r;
>>   }
>>   @@ -2716,26 +2717,31 @@ int amdgpu_vm_init(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm,
>>   /**
>>    * amdgpu_vm_free_levels - free PD/PT levels
>>    *
>> - * @level: PD/PT starting level to free
>> + * @adev: amdgpu device structure
>> + * @parent: PD/PT starting level to free
>> + * @level: level of parent structure
>>    *
>>    * Free the page directory or page table level and all sub levels.
>>    */
>> -static void amdgpu_vm_free_levels(struct amdgpu_vm_pt *level)
>> +static void amdgpu_vm_free_levels(struct amdgpu_device *adev,
>> +                  struct amdgpu_vm_pt *parent,
>> +                  unsigned level)
>>   {
>> -    unsigned i;
>> +    unsigned i, num_entries = amdgpu_vm_num_entries(adev, level);
>>   -    if (level->base.bo) {
>> -        list_del(&level->base.bo_list);
>> -        list_del(&level->base.vm_status);
>> -        amdgpu_bo_unref(&level->base.bo->shadow);
>> -        amdgpu_bo_unref(&level->base.bo);
>> +    if (parent->base.bo) {
>> +        list_del(&parent->base.bo_list);
>> +        list_del(&parent->base.vm_status);
>> +        amdgpu_bo_unref(&parent->base.bo->shadow);
>> +        amdgpu_bo_unref(&parent->base.bo);
>>       }
>>   -    if (level->entries)
>> -        for (i = 0; i <= level->last_entry_used; i++)
>> -            amdgpu_vm_free_levels(&level->entries[i]);
>> +    if (parent->entries)
>> +        for (i = 0; i < num_entries; i++)
>> +            amdgpu_vm_free_levels(adev, &parent->entries[i],
>> +                          level + 1);
>>   -    kvfree(level->entries);
>> +    kvfree(parent->entries);
>>   }
>>     /**
>> @@ -2793,7 +2799,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm)
>>       if (r) {
>>           dev_err(adev->dev, "Leaking page tables because BO 
>> reservation failed\n");
>>       } else {
>> -        amdgpu_vm_free_levels(&vm->root);
>> +        amdgpu_vm_free_levels(adev, &vm->root, 0);
>>           amdgpu_bo_unreserve(root);
>>       }
>>       amdgpu_bo_unref(&root);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 43ea131dd411..7a308a1ea048 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -141,7 +141,6 @@ struct amdgpu_vm_pt {
>>         /* array of page tables, one for each directory entry */
>>       struct amdgpu_vm_pt        *entries;
>> -    unsigned            last_entry_used;
>>   };
>>     #define AMDGPU_VM_FAULT(pasid, addr) (((u64)(pasid) << 48) | (addr))
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx