[PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
Christian König
christian.koenig at amd.com
Fri May 17 11:02:18 UTC 2024
Am 16.05.24 um 14:21 schrieb Tvrtko Ursulin:
>
> Hi Christian,
>
> On 08/05/2024 09:26, Tvrtko Ursulin wrote:
>> On 08/05/2024 06:42, Christian König wrote:
>>> Am 06.05.24 um 18:26 schrieb Tvrtko Ursulin:
>>>>
>>>> On 03/05/2024 10:14, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>>
>>>>> Help code readability by replacing a bunch of:
>>>>>
>>>>> bo->tbo.base.resv == vm->root.bo->tbo.base.resv
>>>>>
>>>>> With:
>>>>>
>>>>> amdgpu_vm_is_bo_always_valid(vm, bo)
>>>>>
>>>>> No functional changes.
>>>>>
>>>>> v2:
>>>>> * Rename helper and move to amdgpu_vm. (Christian)
>>>>>
>>>>> v3:
>>>>> * Use Christian's kerneldoc.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>> Cc: Christian König <christian.koenig at amd.com>
>>>>> Reviewed-by: Christian König <christian.koenig at amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41
>>>>> ++++++++++++++++---------
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
>>>>> 3 files changed, 29 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 67c234bcf89f..e698d65e9508 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -174,7 +174,7 @@ static int amdgpu_gem_object_open(struct
>>>>> drm_gem_object *obj,
>>>>> return -EPERM;
>>>>> if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID &&
>>>>> - abo->tbo.base.resv != vm->root.bo->tbo.base.resv)
>>>>> + !amdgpu_vm_is_bo_always_valid(vm, abo))
>>>>> return -EPERM;
>>>>> r = amdgpu_bo_reserve(abo, false);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 4e2391c83d7c..d451ff9d5af4 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -333,7 +333,7 @@ void amdgpu_vm_bo_base_init(struct
>>>>> amdgpu_vm_bo_base *base,
>>>>> base->next = bo->vm_bo;
>>>>> bo->vm_bo = base;
>>>>> - if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv)
>>>>> + if (!amdgpu_vm_is_bo_always_valid(vm, bo))
>>>>> return;
>>>>> dma_resv_assert_held(vm->root.bo->tbo.base.resv);
>>>>> @@ -1101,13 +1101,13 @@ static void amdgpu_vm_bo_get_memory(struct
>>>>> amdgpu_bo_va *bo_va,
>>>>> * For now ignore BOs which are currently locked and
>>>>> potentially
>>>>> * changing their location.
>>>>> */
>>>>> - if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv &&
>>>>> + if (!amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>>>> !dma_resv_trylock(bo->tbo.base.resv))
>>>>> return;
>>>>> amdgpu_bo_get_memory(bo, stats);
>>>>> - if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv)
>>>>> - dma_resv_unlock(bo->tbo.base.resv);
>>>>> + if (amdgpu_vm_is_bo_always_valid(vm, bo))
>>>>
>>>> Beware the logic inversion here - luckily no one merged it yet! I
>>>> will send a respin..
>>>
>>> Let me know when that is done, I was about to merge it.
>>
>> First three patches in the series
>> https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/
>> if you can pull it from there or I should resend standalone?
>>
>> Please double check it though. Sorry about that I am so used to
>> pre-merge CI we have on the i915 preventing silly mistakes such as
>> that one.
>
> Small reminder on the above since you have been out - are you okay to
> pick up the first three patches from
> https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/,
> if they still look okay to you? Or I should re-send them standalone?
Ah, thanks for the reminder. Totally forgotten that I wanted to push
this. Just done so.
Regards,
Christian.
>
> Regards,
>
> Tvrtko
>
>>>>> + dma_resv_unlock(bo->tbo.base.resv);
>>>>> }
>>>>> void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
>>>>> @@ -1203,8 +1203,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>>> *adev, struct amdgpu_bo_va *bo_va,
>>>>> uncached = false;
>>>>> }
>>>>> - if (clear || (bo && bo->tbo.base.resv ==
>>>>> - vm->root.bo->tbo.base.resv))
>>>>> + if (clear || amdgpu_vm_is_bo_always_valid(vm, bo))
>>>>> last_update = &vm->last_update;
>>>>> else
>>>>> last_update = &bo_va->last_pt_update;
>>>>> @@ -1246,7 +1245,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>>> *adev, struct amdgpu_bo_va *bo_va,
>>>>> * the evicted list so that it gets validated again on the
>>>>> * next command submission.
>>>>> */
>>>>> - if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv) {
>>>>> + if (amdgpu_vm_is_bo_always_valid(vm, bo)) {
>>>>> uint32_t mem_type = bo->tbo.resource->mem_type;
>>>>> if (!(bo->preferred_domains &
>>>>> @@ -1640,10 +1639,9 @@ static void amdgpu_vm_bo_insert_map(struct
>>>>> amdgpu_device *adev,
>>>>> if (mapping->flags & AMDGPU_PTE_PRT)
>>>>> amdgpu_vm_prt_get(adev);
>>>>> - if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv &&
>>>>> - !bo_va->base.moved) {
>>>>> + if (amdgpu_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved)
>>>>> amdgpu_vm_bo_moved(&bo_va->base);
>>>>> - }
>>>>> +
>>>>> trace_amdgpu_vm_bo_map(bo_va, mapping);
>>>>> }
>>>>> @@ -1942,7 +1940,7 @@ int amdgpu_vm_bo_clear_mappings(struct
>>>>> amdgpu_device *adev,
>>>>> if (before->flags & AMDGPU_PTE_PRT)
>>>>> amdgpu_vm_prt_get(adev);
>>>>> - if (bo && bo->tbo.base.resv ==
>>>>> vm->root.bo->tbo.base.resv &&
>>>>> + if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>>>> !before->bo_va->base.moved)
>>>>> amdgpu_vm_bo_moved(&before->bo_va->base);
>>>>> } else {
>>>>> @@ -1957,7 +1955,7 @@ int amdgpu_vm_bo_clear_mappings(struct
>>>>> amdgpu_device *adev,
>>>>> if (after->flags & AMDGPU_PTE_PRT)
>>>>> amdgpu_vm_prt_get(adev);
>>>>> - if (bo && bo->tbo.base.resv ==
>>>>> vm->root.bo->tbo.base.resv &&
>>>>> + if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>>>> !after->bo_va->base.moved)
>>>>> amdgpu_vm_bo_moved(&after->bo_va->base);
>>>>> } else {
>>>>> @@ -2037,7 +2035,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device
>>>>> *adev,
>>>>> if (bo) {
>>>>> dma_resv_assert_held(bo->tbo.base.resv);
>>>>> - if (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)
>>>>> + if (amdgpu_vm_is_bo_always_valid(vm, bo))
>>>>> ttm_bo_set_bulk_move(&bo->tbo, NULL);
>>>>> for (base = &bo_va->base.bo->vm_bo; *base;
>>>>> @@ -2131,7 +2129,7 @@ void amdgpu_vm_bo_invalidate(struct
>>>>> amdgpu_device *adev,
>>>>> for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>>> struct amdgpu_vm *vm = bo_base->vm;
>>>>> - if (evicted && bo->tbo.base.resv ==
>>>>> vm->root.bo->tbo.base.resv) {
>>>>> + if (evicted && amdgpu_vm_is_bo_always_valid(vm, bo)) {
>>>>> amdgpu_vm_bo_evicted(bo_base);
>>>>> continue;
>>>>> }
>>>>> @@ -2142,7 +2140,7 @@ void amdgpu_vm_bo_invalidate(struct
>>>>> amdgpu_device *adev,
>>>>> if (bo->tbo.type == ttm_bo_type_kernel)
>>>>> amdgpu_vm_bo_relocated(bo_base);
>>>>> - else if (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)
>>>>> + else if (amdgpu_vm_is_bo_always_valid(vm, bo))
>>>>> amdgpu_vm_bo_moved(bo_base);
>>>>> else
>>>>> amdgpu_vm_bo_invalidated(bo_base);
>>>>> @@ -3006,3 +3004,16 @@ void amdgpu_vm_update_fault_cache(struct
>>>>> amdgpu_device *adev,
>>>>> xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>> }
>>>>> +/**
>>>>> + * amdgpu_vm_is_bo_always_valid - check if the BO is VM always valid
>>>>> + *
>>>>> + * @vm: VM to test against.
>>>>> + * @abo: BO to be tested.
>>>>> + *
>>>>> + * Returns true if the BO shares the dma_resv object with the
>>>>> root PD and is
>>>>> + * always guaranteed to be valid inside the VM.
>>>>> + */
>>>>> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct
>>>>> amdgpu_bo *bo)
>>>>> +{
>>>>> + return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
>>>>> +}
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index 54d7da396de0..ec688a47dec1 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -561,6 +561,8 @@ void amdgpu_debugfs_vm_bo_info(struct
>>>>> amdgpu_vm *vm, struct seq_file *m);
>>>>> int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct
>>>>> amdgpu_vm *vm);
>>>>> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct
>>>>> amdgpu_bo *bo);
>>>>> +
>>>>> /**
>>>>> * amdgpu_vm_tlb_seq - return tlb flush sequence number
>>>>> * @vm: the amdgpu_vm structure to query
>>>
>>>
More information about the amd-gfx
mailing list