* [PATCH v4] drm/amdgpu: add new trace event for page table update v3
@ 2020-08-13 3:04 Shashank Sharma
2020-08-13 7:58 ` Christian König
0 siblings, 1 reply; 15+ messages in thread
From: Shashank Sharma @ 2020-08-13 3:04 UTC (permalink / raw)
To: amd-gfx; +Cc: Alex Deucher, Christian König, 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@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Shashank Sharma <shashank.sharma@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)
+ ),
+
+ 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) {
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-13 3:04 [PATCH v4] drm/amdgpu: add new trace event for page table update v3 Shashank Sharma
@ 2020-08-13 7:58 ` Christian König
2020-08-19 11:52 ` Shashank Sharma
0 siblings, 1 reply; 15+ messages in thread
From: Christian König @ 2020-08-13 7:58 UTC (permalink / raw)
To: Shashank Sharma, amd-gfx; +Cc: Alex Deucher
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@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@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.
> + ),
> +
> + 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) {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-13 7:58 ` Christian König
@ 2020-08-19 11:52 ` Shashank Sharma
2020-08-19 12:08 ` Christian König
0 siblings, 1 reply; 15+ messages in thread
From: Shashank Sharma @ 2020-08-19 11:52 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@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 ?
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 ?
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) {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 11:52 ` Shashank Sharma
@ 2020-08-19 12:08 ` Christian König
2020-08-19 12:19 ` Shashank Sharma
2020-08-19 12:45 ` Nirmoy
0 siblings, 2 replies; 15+ messages in thread
From: Christian König @ 2020-08-19 12:08 UTC (permalink / raw)
To: Shashank Sharma, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@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) {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:08 ` Christian König
@ 2020-08-19 12:19 ` Shashank Sharma
2020-08-19 12:33 ` Christian König
2020-08-19 12:45 ` Nirmoy
1 sibling, 1 reply; 15+ messages in thread
From: Shashank Sharma @ 2020-08-19 12:19 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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 ?
- Shashank
> 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) {
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:19 ` Shashank Sharma
@ 2020-08-19 12:33 ` Christian König
2020-08-19 12:37 ` Shashank Sharma
0 siblings, 1 reply; 15+ messages in thread
From: Christian König @ 2020-08-19 12:33 UTC (permalink / raw)
To: Shashank Sharma, Christian König, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
Christian.
>
> - Shashank
>
>> 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) {
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:33 ` Christian König
@ 2020-08-19 12:37 ` Shashank Sharma
2020-08-19 13:04 ` Christian König
0 siblings, 1 reply; 15+ messages in thread
From: Shashank Sharma @ 2020-08-19 12:37 UTC (permalink / raw)
To: christian.koenig, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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 ?
Regards
Shashank
>
> Christian.
>
>> - Shashank
>>
>>> 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) {
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cshashank.sharma%40amd.com%7C294c0cf0460847953e8308d8443c163c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334372147156674&sdata=3sWZcuLcWVTcspxhRq6gT1SM%2BvHQCrJqmJijKgREks0%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:08 ` Christian König
2020-08-19 12:19 ` Shashank Sharma
@ 2020-08-19 12:45 ` Nirmoy
2020-08-19 12:51 ` Shashank Sharma
1 sibling, 1 reply; 15+ messages in thread
From: Nirmoy @ 2020-08-19 12:45 UTC (permalink / raw)
To: Christian König, Shashank Sharma, amd-gfx; +Cc: Alex Deucher
Hi Christian,
On 8/19/20 2:08 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@amd.com>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
Noob question:
Why these two pids are different ? Is it like that, the cpid-2114
process created a page/memory-area and now pid-2128 using that
page/memory-area to submit a command ?
Regards,
Nirmoy
>
>> 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) {
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cnirmoy.das%40amd.com%7Ccf56b7d79fe847f9e0ae08d84438a89e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334357456981915&sdata=pix8Q%2FIHwTA4CGoYRVdKyDEzQHmx4ASKgAbcgwUKWBQ%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:45 ` Nirmoy
@ 2020-08-19 12:51 ` Shashank Sharma
2020-08-19 12:58 ` Christian König
0 siblings, 1 reply; 15+ messages in thread
From: Shashank Sharma @ 2020-08-19 12:51 UTC (permalink / raw)
To: Nirmoy, Christian König, amd-gfx; +Cc: Alex Deucher
On 19/08/20 6:15 pm, Nirmoy wrote:
> Hi Christian,
>
> On 8/19/20 2:08 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@amd.com>
>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
>
> Noob question:
>
> Why these two pids are different ? Is it like that, the cpid-2114
> process created a page/memory-area and now pid-2128 using that
> page/memory-area to submit a command ?
My understanding is that this is due to a server-client arrangement, where a client process can create a memory map and fill it for the submission, but it doesnt have privilege to do that, and that should be done by the server process (like X/wayland etc)
- Shashank
>
>
> Regards,
>
> Nirmoy
>
>
>
>>> 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) {
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cnirmoy.das%40amd.com%7Ccf56b7d79fe847f9e0ae08d84438a89e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334357456981915&sdata=pix8Q%2FIHwTA4CGoYRVdKyDEzQHmx4ASKgAbcgwUKWBQ%3D&reserved=0
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:51 ` Shashank Sharma
@ 2020-08-19 12:58 ` Christian König
2020-08-19 13:13 ` Nirmoy
0 siblings, 1 reply; 15+ messages in thread
From: Christian König @ 2020-08-19 12:58 UTC (permalink / raw)
To: Shashank Sharma, Nirmoy, Christian König, amd-gfx; +Cc: Alex Deucher
Am 19.08.20 um 14:51 schrieb Shashank Sharma:
> On 19/08/20 6:15 pm, Nirmoy wrote:
>> Hi Christian,
>>
>> On 8/19/20 2:08 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@amd.com>
>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
>> Noob question:
>>
>> Why these two pids are different ? Is it like that, the cpid-2114
>> process created a page/memory-area and now pid-2128 using that
>> page/memory-area to submit a command ?
> My understanding is that this is due to a server-client arrangement, where a client process can create a memory map and fill it for the submission, but it doesnt have privilege to do that, and that should be done by the server process (like X/wayland etc)
That's close but not quite correct.
One use case is what Shashank describes here where the server allocates
something on behalves of the client and because of delayed updates we
end up updating the page tables in a different process then where
something is really used.
But the more command use case is eviction where process A is kicking out
BOs of process B and because of this the page table of process B are
updated in the context of A.
Regards,
Christian.
>
> - Shashank
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:37 ` Shashank Sharma
@ 2020-08-19 13:04 ` Christian König
2020-08-19 13:08 ` Shashank Sharma
0 siblings, 1 reply; 15+ messages in thread
From: Christian König @ 2020-08-19 13:04 UTC (permalink / raw)
To: Shashank Sharma, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
Regards,
Christian.
>
>
> Regards
>
> Shashank
>
>> Christian.
>>
>>> - Shashank
>>>
>>>> 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) {
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cshashank.sharma%40amd.com%7C294c0cf0460847953e8308d8443c163c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334372147156674&sdata=3sWZcuLcWVTcspxhRq6gT1SM%2BvHQCrJqmJijKgREks0%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 13:04 ` Christian König
@ 2020-08-19 13:08 ` Shashank Sharma
2020-08-19 13:30 ` Christian König
0 siblings, 1 reply; 15+ messages in thread
From: Shashank Sharma @ 2020-08-19 13:08 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
- Shashank
> Regards,
> Christian.
>
>>
>> Regards
>>
>> Shashank
>>
>>> Christian.
>>>
>>>> - Shashank
>>>>
>>>>> 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) {
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cshashank.sharma%40amd.com%7C294c0cf0460847953e8308d8443c163c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334372147156674&sdata=3sWZcuLcWVTcspxhRq6gT1SM%2BvHQCrJqmJijKgREks0%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 12:58 ` Christian König
@ 2020-08-19 13:13 ` Nirmoy
0 siblings, 0 replies; 15+ messages in thread
From: Nirmoy @ 2020-08-19 13:13 UTC (permalink / raw)
To: christian.koenig, Shashank Sharma, amd-gfx; +Cc: Alex Deucher
On 8/19/20 2:58 PM, Christian König wrote:
> Am 19.08.20 um 14:51 schrieb Shashank Sharma:
>> On 19/08/20 6:15 pm, Nirmoy wrote:
>>> Hi Christian,
>>>
>>> On 8/19/20 2:08 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@amd.com>
>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
>>> Noob question:
>>>
>>> Why these two pids are different ? Is it like that, the cpid-2114
>>> process created a page/memory-area and now pid-2128 using that
>>> page/memory-area to submit a command ?
>> My understanding is that this is due to a server-client arrangement,
>> where a client process can create a memory map and fill it for the
>> submission, but it doesnt have privilege to do that, and that should
>> be done by the server process (like X/wayland etc)
>
> That's close but not quite correct.
>
> One use case is what Shashank describes here where the server
> allocates something on behalves of the client and because of delayed
> updates we end up updating the page tables in a different process then
> where something is really used.
>
> But the more command use case is eviction where process A is kicking
> out BOs of process B and because of this the page table of process B
> are updated in the context of A.
>
Understood, thanks Christain and Shashank for the explanation!
Nirmoy.
> Regards,
> Christian.
>
>
>>
>> - Shashank
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 13:08 ` Shashank Sharma
@ 2020-08-19 13:30 ` Christian König
2020-08-19 14:17 ` Shashank Sharma
0 siblings, 1 reply; 15+ messages in thread
From: Christian König @ 2020-08-19 13:30 UTC (permalink / raw)
To: Shashank Sharma, Christian König, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] drm/amdgpu: add new trace event for page table update v3
2020-08-19 13:30 ` Christian König
@ 2020-08-19 14:17 ` Shashank Sharma
0 siblings, 0 replies; 15+ messages in thread
From: Shashank Sharma @ 2020-08-19 14:17 UTC (permalink / raw)
To: christian.koenig, amd-gfx; +Cc: Alex Deucher
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@amd.com>
>>>>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-08-19 14:17 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-13 3:04 [PATCH v4] drm/amdgpu: add new trace event for page table update v3 Shashank Sharma
2020-08-13 7:58 ` Christian König
2020-08-19 11:52 ` Shashank Sharma
2020-08-19 12:08 ` Christian König
2020-08-19 12:19 ` Shashank Sharma
2020-08-19 12:33 ` Christian König
2020-08-19 12:37 ` Shashank Sharma
2020-08-19 13:04 ` Christian König
2020-08-19 13:08 ` Shashank Sharma
2020-08-19 13:30 ` Christian König
2020-08-19 14:17 ` Shashank Sharma
2020-08-19 12:45 ` Nirmoy
2020-08-19 12:51 ` Shashank Sharma
2020-08-19 12:58 ` Christian König
2020-08-19 13:13 ` Nirmoy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.