On 20/08/20 2:28 pm, Christian König wrote:
> Am 20.08.20 um 07:27 schrieb Shashank Sharma:
>> This patch adds a new trace event to track the PTE update
>> events. This specific event will provide information like:
>> - start and end of virtual memory mapping
>> - HW engine flags for the map
>> - physical address for mapping
>>
>> This will be particularly useful for memory profiling tools
>> (like RMV) which are monitoring the page table update events.
>>
>> V2: Added physical address lookup logic in trace point
>> V3: switch to use __dynamic_array
>>      added nptes int the TPprint arguments list
>>      added page size in the arg list
>> V4: Addressed Christian's review comments
>>      add start/end instead of seg
>>      use incr instead of page_sz to be accurate
>> V5: Addressed Christian's review comments:
>>      add pid and vm context information in the event
>>
>> Cc: Christian König <[email protected]>
>> Cc: Alex Deucher <[email protected]>
>> Signed-off-by: Christian König <[email protected]>
>> Signed-off-by: Shashank Sharma <[email protected]>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 43 +++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    | 11 ++++--
>>   2 files changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> index 63e734a125fb..02406fc6db0f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> @@ -321,6 +321,49 @@ DEFINE_EVENT(amdgpu_vm_mapping, amdgpu_vm_bo_cs,
>>          TP_ARGS(mapping)
>>   );
>>   
>> +TRACE_EVENT(amdgpu_vm_update_ptes,
>> +        TP_PROTO(struct amdgpu_vm_update_params *p,
>> +                 uint64_t start, uint64_t end,
>> +                 unsigned int nptes, uint64_t dst,
>> +                 uint64_t incr, uint64_t flags,
>> +                 uint64_t pid, uint64_t vm_ctx),
>> +    TP_ARGS(p, start, end, nptes, dst, incr, flags, pid, vm_ctx),
>> +    TP_STRUCT__entry(
>> +                     __field(u64, start)
>> +                     __field(u64, end)
>> +                     __field(u64, flags)
>> +                     __field(unsigned int, nptes)
>> +                     __field(u64, incr)
>> +                     __field(u64, pid)
> Please use pid_t for the pid here.
got it,
>> +                     __field(u64, vm_ctx)
>> +                     __dynamic_array(u64, dst, nptes)
>> +    ),
>> +
>> +    TP_fast_assign(
>> +                    unsigned int i;
>> +
>> +                    __entry->start = start;
>> +                    __entry->end = end;
>> +                    __entry->flags = flags;
>> +                    __entry->incr = incr;
>> +                    __entry->nptes = nptes;
>> +                    __entry->pid = pid;
>> +                    __entry->vm_ctx = vm_ctx;
>> +                    for (i = 0; i < nptes; ++i) {
>> +                            u64 addr = p->pages_addr ? amdgpu_vm_map_gart(
>> +                                    p->pages_addr, dst) : dst;
>> +
>> +                            ((u64 *)__get_dynamic_array(dst))[i] = addr;
>> +                            dst += incr;
>> +                    }
>> +    ),
>> +    TP_printk("start:0x%010llx end:0x%010llx, flags:0x%llx, incr:%llu,"
>> +              " pid:%lld vm_ctx:0x%llx dst:\n%s", __entry->start,
> I would reorder this to "pid, vm_ctx, start, end, flags, incr, dst". And 
> better use %u for the pid.
>
> Apart from that this looks good to me.
>
Thanks, will re-arrange this.

- Shashank

> Christian.
>
>> +              __entry->end, __entry->flags, __entry->incr, __entry->pid,
>> +              __entry->vm_ctx, __print_array(__get_dynamic_array(dst),
>> +              __entry->nptes, 8))
>> +);
>> +
>>   TRACE_EVENT(amdgpu_vm_set_ptes,
>>          TP_PROTO(uint64_t pe, uint64_t addr, unsigned count,
>>                   uint32_t incr, uint64_t flags, bool direct),
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8bc2253939be..3d25739b4158 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1511,19 +1511,26 @@ static int amdgpu_vm_update_ptes(struct 
>> amdgpu_vm_update_params *params,
>>              entry_end = min(entry_end, end);
>>   
>>              do {
>> +                    struct amdgpu_vm *vm = params->vm;
>>                      uint64_t upd_end = min(entry_end, frag_end);
>>                      unsigned nptes = (upd_end - frag_start) >> shift;
>> +                    uint64_t upd_flags = flags | AMDGPU_PTE_FRAG(frag);
>>   
>>                      /* This can happen when we set higher level PDs to
>>                       * silent to stop fault floods.
>>                       */
>>                      nptes = max(nptes, 1u);
>> +
>> +                    trace_amdgpu_vm_update_ptes(params, frag_start, upd_end,
>> +                                                nptes, dst, incr, upd_flags,
>> +                                                vm->task_info.pid,
>> +                                                
>> vm->immediate.fence_context);
>>                      amdgpu_vm_update_flags(params, pt, cursor.level,
>>                                             pe_start, dst, nptes, incr,
>> -                                           flags | AMDGPU_PTE_FRAG(frag));
>> +                                           upd_flags);
>>   
>>                      pe_start += nptes * 8;
>> -                    dst += (uint64_t)nptes * AMDGPU_GPU_PAGE_SIZE << shift;
>> +                    dst += nptes * incr;
>>   
>>                      frag_start = upd_end;
>>                      if (frag_start >= frag_end) {
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to