[PATCH 4/8] drm/amdgpu: remove last_entry_used from the VM code
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Dec 12 09:23:37 UTC 2017
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, 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