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

[Christian König]

Am 19.08.20 um 13:52 schrieb Shashank Sharma:
> On 13/08/20 1:28 pm, Christian König wrote:
>> Am 13.08.20 um 05:04 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
>>>
>>> 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 | 37 +++++++++++++++++++++++
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    |  9 ++++--
>>>    2 files changed, 44 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..df12cf8466c2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>>> @@ -321,6 +321,43 @@ 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),
>>> +	TP_ARGS(p, start, end, nptes, dst, incr, flags),
>>> +	TP_STRUCT__entry(
>>> +			 __field(u64, start)
>>> +			 __field(u64, end)
>>> +			 __field(u64, flags)
>>> +			 __field(unsigned int, nptes)
>>> +			 __field(u64, incr)
>>> +			 __dynamic_array(u64, dst, nptes)
>> As discussed with the trace subsystem maintainer we need to add the pid
>> and probably the VM context ID we use here to identify the updated VM.
>>
>> Christian.
> I printed both vm->task_info.pid Vs current->pid for testing, and I can see different values coming out of .
>
> gnome-shell-2114  [011]    41.812894: amdgpu_vm_update_ptes: start:0x0800102e80 end:0x0800102e82, flags:0x80, incr:4096, pid=2128 vmid=0 cpid=2114
>
> pid is vm->task_info.pid=2128 whereas cpid=2114 is current.pid.
>
> Which is the one we want to send with the event ?

That is vm->task_info.pid, since this is the PID which is using the VM 
for command submission.

> Trace event by default seems to be adding the process name and id at the header of the event (gnome-shell-2114), which is same as current.pid
>
>
> Also, is it ok to extract vmid from job->vmid ?

Only in trace_amdgpu_vm_grab_id(), in all other cases it's probably not 
assigned yet.

Christian.

>
>
> Regards
>
> Shashank
>
>>> +	),
>>> +
>>> +	TP_fast_assign(
>>> +			unsigned int i;
>>> +
>>> +			__entry->start = start;
>>> +			__entry->end = end;
>>> +			__entry->flags = flags;
>>> +			__entry->incr = incr;
>>> +			__entry->nptes = nptes;
>>> +			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,"
>>> +		  " dst:\n%s", __entry->start, __entry->end, __entry->flags,
>>> +		  __entry->incr, __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 71e005cf2952..b5dbb5e8bc61 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1513,17 +1513,22 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>>>    		do {
>>>    			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);
>>>    			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) {