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

[Christian König]

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.

Regards,
Christian.