[PATCH 4/8] drm/amdgpu: remove last_entry_used from the VM code
Chunming Zhou
zhoucm1 at amd.com
Tue Dec 12 09:32:38 UTC 2017
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))
>>>
>>
>
More information about the amd-gfx
mailing list