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))
>>
>
