Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 29/04/2024 12:02, Christian König wrote:
> Am 26.04.24 um 18:43 schrieb Tvrtko Ursulin:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> Help code readability by replacing a bunch of:
>>
>> bo->tbo.base.resv == vm->root.bo->tbo.base.resv
>>
>> With:
>>
>> amdgpu_bo_is_vm_bo(bo, vm)
>>
>> No functional changes.
> 
> Ah,yes that was on my TODO list as well.
> 
> But I would have rather added this to the VM instead. In other words 
> move it to amdgpu_vm.h and call it amdgpu_vm_is_bo_always_valid() or 
> something like that.

I am happy to move it around as long as you are sure amdgpu_vm.h is the 
location.

For instance main API there it seems to be amdgpu_vm_bo's. At least all 
the amdgpu_bo usages do not needing anything more that the struct 
forward declared.

So if I move the helper there I either need to make it include another 
header, or move the helper out of line to amdgpu_vm.c.

Thoughts?

Regards,

Tvrtko

>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 14 ++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 31 +++++++++-------------
>>   3 files changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 67c234bcf89f..32e4a9c6e805 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_bo_is_vm_bo(abo, vm))
>>           return -EPERM;
>>       r = amdgpu_bo_reserve(abo, false);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index be679c42b0b8..f2bb6965cc77 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -307,6 +307,20 @@ static inline struct amdgpu_bo 
>> *amdgpu_bo_shadowed(struct amdgpu_bo *bo)
>>       return NULL;
>>   }
>> +/**
>> + * amdgpu_bo_is_vm_bo - check if the BO is VM always valid
>> + *
>> + * @abo: BO to be tested.
>> + * @vm: VM to test against.
>> + *
>> + * Returns true if the BO is VM always valid.
>> + */
>> +static inline bool amdgpu_bo_is_vm_bo(struct amdgpu_bo *bo,
>> +                      struct amdgpu_vm *vm)
>> +{
>> +    return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
>> +}
>> +
>>   bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo);
>>   void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 
>> domain);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8af3f0fd3073..6d6f0e325172 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_bo_is_vm_bo(bo, vm))
>>           return;
>>       dma_resv_assert_held(vm->root.bo->tbo.base.resv);
>> @@ -1101,13 +1101,12 @@ 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 &&
>> -        !dma_resv_trylock(bo->tbo.base.resv))
>> +    if (!amdgpu_bo_is_vm_bo(bo, vm) && 
>> !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_bo_is_vm_bo(bo, vm))
>> +        dma_resv_unlock(bo->tbo.base.resv);
>>   }
>>   void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
>> @@ -1203,8 +1202,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_bo_is_vm_bo(bo, vm))
>>           last_update = &vm->last_update;
>>       else
>>           last_update = &bo_va->last_pt_update;
>> @@ -1246,7 +1244,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_bo_is_vm_bo(bo, vm)) {
>>           uint32_t mem_type = bo->tbo.resource->mem_type;
>>           if (!(bo->preferred_domains &
>> @@ -1640,10 +1638,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_bo_is_vm_bo(bo, vm) && !bo_va->base.moved)
>>           amdgpu_vm_bo_moved(&bo_va->base);
>> -    }
>> +
>>       trace_amdgpu_vm_bo_map(bo_va, mapping);
>>   }
>> @@ -1922,8 +1919,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 &&
>> -            !before->bo_va->base.moved)
>> +        if (amdgpu_bo_is_vm_bo(bo, vm) && !before->bo_va->base.moved)
>>               amdgpu_vm_bo_moved(&before->bo_va->base);
>>       } else {
>>           kfree(before);
>> @@ -1937,8 +1933,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 &&
>> -            !after->bo_va->base.moved)
>> +        if (amdgpu_bo_is_vm_bo(bo, vm) && !after->bo_va->base.moved)
>>               amdgpu_vm_bo_moved(&after->bo_va->base);
>>       } else {
>>           kfree(after);
>> @@ -2017,7 +2012,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_bo_is_vm_bo(bo, vm))
>>               ttm_bo_set_bulk_move(&bo->tbo, NULL);
>>           for (base = &bo_va->base.bo->vm_bo; *base;
>> @@ -2111,7 +2106,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_bo_is_vm_bo(bo, vm)) {
>>               amdgpu_vm_bo_evicted(bo_base);
>>               continue;
>>           }
>> @@ -2122,7 +2117,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_bo_is_vm_bo(bo, vm))
>>               amdgpu_vm_bo_moved(bo_base);
>>           else
>>               amdgpu_vm_bo_invalidated(bo_base);
>