Re: [PATCH 1/2] amdgpu: add the amdgpu_vm ptr in the vm_bo_map/unmap events

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 03.06.24 um 13:52 schrieb Pierre-Eric Pelloux-Prayer:
> Hi Christia,
>
> Le 03/06/2024 à 11:58, Christian König a écrit :
>> Am 03.06.24 um 10:46 schrieb Pierre-Eric Pelloux-Prayer:
>>> These 2 traces events are tied to a specific VM so in order for them
>>> to be useful for a tool we need to trace the amdgpu_vm as well.
>>
>> The bo_va already contains the VM pointer the map/unmap operation 
>> belongs to.
>>
>
> Indeed, I've missed that. I'll fix that in v2.
>
>>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 20 ++++++++++++--------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    |  8 ++++----
>>>   2 files changed, 16 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> index f539b1d00234..c84050d318d6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> @@ -243,10 +243,11 @@ TRACE_EVENT(amdgpu_vm_grab_id,
>>>   );
>>>   TRACE_EVENT(amdgpu_vm_bo_map,
>>> -        TP_PROTO(struct amdgpu_bo_va *bo_va,
>>> +        TP_PROTO(struct amdgpu_vm *vm, struct amdgpu_bo_va *bo_va,
>>>                struct amdgpu_bo_va_mapping *mapping),
>>> -        TP_ARGS(bo_va, mapping),
>>> +        TP_ARGS(vm, bo_va, mapping),
>>>           TP_STRUCT__entry(
>>> +                 __field(struct amdgpu_vm *, vm)
>>>                    __field(struct amdgpu_bo *, bo)
>>>                    __field(long, start)
>>>                    __field(long, last)
>>> @@ -255,22 +256,24 @@ TRACE_EVENT(amdgpu_vm_bo_map,
>>>                    ),
>>>           TP_fast_assign(
>>> +               __entry->vm = vm;
>>>                  __entry->bo = bo_va ? bo_va->base.bo : NULL;
>>>                  __entry->start = mapping->start;
>>>                  __entry->last = mapping->last;
>>>                  __entry->offset = mapping->offset;
>>>                  __entry->flags = mapping->flags;
>>>                  ),
>>> -        TP_printk("bo=%p, start=%lx, last=%lx, offset=%010llx, 
>>> flags=%llx",
>>> -              __entry->bo, __entry->start, __entry->last,
>>> +        TP_printk("vm=%p bo=%p, start=%lx, last=%lx, 
>>> offset=%010llx, flags=%llx",
>>> +              __entry->vm, __entry->bo, __entry->start, __entry->last,
>>>                 __entry->offset, __entry->flags)
>>>   );
>>>   TRACE_EVENT(amdgpu_vm_bo_unmap,
>>> -        TP_PROTO(struct amdgpu_bo_va *bo_va,
>>> +        TP_PROTO(struct amdgpu_vm *vm, struct amdgpu_bo_va *bo_va,
>>>                struct amdgpu_bo_va_mapping *mapping),
>>> -        TP_ARGS(bo_va, mapping),
>>> +        TP_ARGS(vm, bo_va, mapping),
>>>           TP_STRUCT__entry(
>>> +                 __field(struct amdgpu_vm *, vm)
>>>                    __field(struct amdgpu_bo *, bo)
>>>                    __field(long, start)
>>>                    __field(long, last)
>>> @@ -279,14 +282,15 @@ TRACE_EVENT(amdgpu_vm_bo_unmap,
>>>                    ),
>>>           TP_fast_assign(
>>> +               __entry->vm = vm;
>>>                  __entry->bo = bo_va ? bo_va->base.bo : NULL;
>>>                  __entry->start = mapping->start;
>>>                  __entry->last = mapping->last;
>>>                  __entry->offset = mapping->offset;
>>>                  __entry->flags = mapping->flags;
>>>                  ),
>>> -        TP_printk("bo=%p, start=%lx, last=%lx, offset=%010llx, 
>>> flags=%llx",
>>> -              __entry->bo, __entry->start, __entry->last,
>>> +        TP_printk("vm=%p bo=%p, start=%lx, last=%lx, 
>>> offset=%010llx, flags=%llx",
>>> +              __entry->vm, __entry->bo, __entry->start, __entry->last,
>>>                 __entry->offset, __entry->flags)
>>>   );
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 3abfa66d72a2..e04928d2e26a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1642,7 +1642,7 @@ static void amdgpu_vm_bo_insert_map(struct 
>>> amdgpu_device *adev,
>>>       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);
>>> +    trace_amdgpu_vm_bo_map(vm, bo_va, mapping);
>>>   }
>>>   /* Validate operation parameters to prevent potential abuse */
>>> @@ -1834,7 +1834,7 @@ int amdgpu_vm_bo_unmap(struct amdgpu_device 
>>> *adev,
>>>       list_del(&mapping->list);
>>>       amdgpu_vm_it_remove(mapping, &vm->va);
>>>       mapping->bo_va = NULL;
>>> -    trace_amdgpu_vm_bo_unmap(bo_va, mapping);
>>> +    trace_amdgpu_vm_bo_unmap(vm, bo_va, mapping);
>>>       if (valid)
>>>           list_add(&mapping->list, &vm->freed);
>>> @@ -1929,7 +1929,7 @@ int amdgpu_vm_bo_clear_mappings(struct 
>>> amdgpu_device *adev,
>>>           tmp->bo_va = NULL;
>>>           list_add(&tmp->list, &vm->freed);
>>> -        trace_amdgpu_vm_bo_unmap(NULL, tmp);
>>> +        trace_amdgpu_vm_bo_unmap(vm, NULL, tmp);
>>
>> That bo_va is NULL here is probably a bug and should be fixed.
>
> Would something like this work?
>
>     trace_amdgpu_vm_bo_unmap(tmp->bo_va, tmp);
>     tmp->bo_va = NULL;
>     list_add(&tmp->list, &vm->freed);

It's not 100% accurate because only parts of the mapping is unmapped, 
but yes I think that should work.

Regards,
Christian.

>
> Thanks,
> Pierre-Eric
>
>
>>
>> Regards,
>> Christian.
>>
>>>       }
>>>       /* Insert partial mapping before the range */
>>> @@ -2056,7 +2056,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
>>>           list_del(&mapping->list);
>>>           amdgpu_vm_it_remove(mapping, &vm->va);
>>>           mapping->bo_va = NULL;
>>> -        trace_amdgpu_vm_bo_unmap(bo_va, mapping);
>>> +        trace_amdgpu_vm_bo_unmap(vm, bo_va, mapping);
>>>           list_add(&mapping->list, &vm->freed);
>>>       }
>>>       list_for_each_entry_safe(mapping, next, &bo_va->invalids, list) {