[PATCH v5] drm/amdgpu: add new trace event for page table update

[Christian König]

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 <christian.koenig at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> Signed-off-by: Shashank Sharma <shashank.sharma at amd.com>
> ---
>   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.

> +			 __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.

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) {