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

[Shashank Sharma]

On 19/08/20 7:00 pm, Christian König wrote:
> Am 19.08.20 um 15:08 schrieb Shashank Sharma:
>> On 19/08/20 6:34 pm, Christian König wrote:
>>> Am 19.08.20 um 14:37 schrieb Shashank Sharma:
>>>> On 19/08/20 6:03 pm, Christian König wrote:
>>>>> Am 19.08.20 um 14:19 schrieb Shashank Sharma:
>>>>>> On 19/08/20 5:38 pm, Christian König wrote:
>>>>>>> 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.
>>>>>> got it.
>>>>>>>> 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.
>>>>>> Ok, let me check how can we get vmid from this context we are sending the event from. Or maybe I we should  keep V5 with pid only, and later send a separate patch to add vmid ?
>>>>> I'm not sure how you want to get a VMID in the first place. We
>>>>> dynamically assign VMIDs during command submission.
>>>>>
>>>>> See the amdgpu_vm_grab_id trace point.
>>>> Actually I was adding vmid to address this last comment on V4:
>>>>> and probably the VM context ID we use here to identify the updated VM.
>>>> I assumed you meant the vmid by this, is that not so ?
>>> Ah! No that's something completely different :)
>>>
>>> The VMID is a 4bit hardware identifier used for TLB optimization.
>>>
>>> The VM context ID is an unique 64bit number, we usually use
>>> vm->immediate.fence_context for this.
>> Damn, why is it never what you hope it to be :). Thanks for this information, I will check this out first.
> Multiple reasons :)
>
> One is that I'm not a native speaker of English and only had very 
> limited formal education in the language :)
>
> Another one is that the hardware and driver is rather complicated.

Nah, I think you were pretty clear in the communication,

I will be a good software engineer, and go ahead and blame it on the hardware :D.

Thanks again, I will check to add the vm context here.

- Shashank

>
> Regards,
> Christian.