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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 


On 2017å¹´12æ??12æ?¥ 17:23, Christian König wrote:
> Am 11.12.2017 um 13:08 schrieb Christian König:
>> 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 at 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.
>
> Ping? Any more objections to this patch or can I commit it?
>
> Wanted to commit those up till patch #7, 
feel free to add my RB on this one and patch#7.

Regards,
david Zhou
> then add your work to reverse the level and then put patch #8 on top.
>
> Christian.
>
>>
>> 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))
>>>
>>
>
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux