<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - AMD Internal Distribution Only] <div> <div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Let's drop this patch: the amdgpu_vm_*_ptes events already contain all the info I need.</div> <div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thanks,</div> <div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Pierre-Eric</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Christian König <ckoenig.leichtzumerken@gmail.com> Sent: Monday, June 3, 2024 4:12 PM To: Pierre-Eric Pelloux-Prayer <pierre-eric@damsy.net>; Koenig, Christian <Christian.Koenig@amd.com>; Pelloux-Prayer, Pierre-Eric <Pierre-eric.Pelloux-prayer@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 1/2] amdgpu: add the amdgpu_vm ptr in the vm_bo_map/unmap events <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">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) { </div> </div> </div> </body> </html>