Associate dma_fence and amdpgu tracepoints

Associate dma_fence and amdpgu tracepoints

[Andres Rodriguez]

This patch series is an meant to supersede:
drm/amdgpu: trace amdgpu_job fence details

It uses a different approach of tracking through the scheduler fence
instead of the amdgpu job fence.

The difference between using these two fences account for a maximum of
1us of latency as measured by ftrace. E.g.:
<idle>-0     [002] d.h.    69.073015: dma_fence_signaled: driver=amdgpu timeline=gfx context=1 seqno=4616
<idle>-0     [002] d.h.    69.073016: dma_fence_signaled: driver=amd_sched timeline=gfx context=591 seqno=3558

In most cases the difference is 0us. I don't think this issue will be
a concern on our current time scales.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/2] drm/amdgpu: make trace format uniform csv name=value

[Andres Rodriguez]

Most of the traces have uniform format except for two of them. Having
all the traces match makes it simple to run awk on the ftrace output.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index a18ae1e..01623d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -142,7 +142,7 @@ TRACE_EVENT(amdgpu_sched_run_job,
 			   __entry->ring_name = job->ring->name;
 			   __entry->num_ibs = job->num_ibs;
 			   ),
-	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, ring name:%s, num_ibs:%u",
+	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, ring name=%s, num_ibs=%u",
 		      __entry->adev, __entry->sched_job, __entry->ib,
 		      __entry->fence, __entry->ring_name, __entry->num_ibs)
 );
@@ -359,7 +359,7 @@ TRACE_EVENT(amdgpu_ttm_bo_move,
 			__entry->new_placement = new_placement;
 			__entry->old_placement = old_placement;
 			),
-	    TP_printk("bo=%p from:%d to %d with size = %Ld",
+	    TP_printk("bo=%p, from=%d, to=%d, size=%Ld",
 			__entry->bo, __entry->old_placement,
 			__entry->new_placement, __entry->bo_size)
 );
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Andres Rodriguez]

This information is intended to provide the required data to associate
amdgpu tracepoints with their corresponding dma_fence_* events.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 01623d1..cc9a31d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_job,
 			     __field(struct amd_sched_job *, sched_job)
 			     __field(struct amdgpu_ib *, ib)
 			     __field(struct dma_fence *, fence)
+			     __string(timeline, job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
+			     __field(unsigned int, context)
+			     __field(unsigned int, seqno)
 			     __field(char *, ring_name)
 			     __field(u32, num_ibs)
 			     ),
@@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_job,
 			   __entry->sched_job = &job->base;
 			   __entry->ib = job->ibs;
 			   __entry->fence = &job->base.s_fence->finished;
+			   __assign_str(timeline, job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
+			   __entry->context = job->base.s_fence->finished.context;
+			   __entry->seqno = job->base.s_fence->finished.seqno;
 			   __entry->ring_name = job->ring->name;
 			   __entry->num_ibs = job->num_ibs;
 			   ),
-	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, ring name=%s, num_ibs=%u",
+	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, timeline=%s, context=%u, seqno=%u, ring name=%s, num_ibs=%u",
 		      __entry->adev, __entry->sched_job, __entry->ib,
-		      __entry->fence, __entry->ring_name, __entry->num_ibs)
+		      __entry->fence, __get_str(timeline), __entry->context, __entry->seqno,
+		      __entry->ring_name, __entry->num_ibs)
 );
 
 
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: make trace format uniform csv name=value

[Christian König]

Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:
> Most of the traces have uniform format except for two of them. Having
> all the traces match makes it simple to run awk on the ftrace output.
>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>

Reviewed-by: Christian König <christian.koenig@amd.com>.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index a18ae1e..01623d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -142,7 +142,7 @@ TRACE_EVENT(amdgpu_sched_run_job,
>   			   __entry->ring_name = job->ring->name;
>   			   __entry->num_ibs = job->num_ibs;
>   			   ),
> -	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, ring name:%s, num_ibs:%u",
> +	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, ring name=%s, num_ibs=%u",
>   		      __entry->adev, __entry->sched_job, __entry->ib,
>   		      __entry->fence, __entry->ring_name, __entry->num_ibs)
>   );
> @@ -359,7 +359,7 @@ TRACE_EVENT(amdgpu_ttm_bo_move,
>   			__entry->new_placement = new_placement;
>   			__entry->old_placement = old_placement;
>   			),
> -	    TP_printk("bo=%p from:%d to %d with size = %Ld",
> +	    TP_printk("bo=%p, from=%d, to=%d, size=%Ld",
>   			__entry->bo, __entry->old_placement,
>   			__entry->new_placement, __entry->bo_size)
>   );


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Christian König]

Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:
> This information is intended to provide the required data to associate
> amdgpu tracepoints with their corresponding dma_fence_* events.
>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index 01623d1..cc9a31d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_job,
>   			     __field(struct amd_sched_job *, sched_job)
>   			     __field(struct amdgpu_ib *, ib)
>   			     __field(struct dma_fence *, fence)
> +			     __string(timeline, job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
> +			     __field(unsigned int, context)
> +			     __field(unsigned int, seqno)
>   			     __field(char *, ring_name)
>   			     __field(u32, num_ibs)
>   			     ),
> @@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_job,
>   			   __entry->sched_job = &job->base;
>   			   __entry->ib = job->ibs;
>   			   __entry->fence = &job->base.s_fence->finished;
> +			   __assign_str(timeline, job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))

Not 100% sure, but we might be able to use a char * field here instead 
of the extra overhead of embedding a string, the timeline names are 
never freed IIRC.

> +			   __entry->context = job->base.s_fence->finished.context;
> +			   __entry->seqno = job->base.s_fence->finished.seqno;
>   			   __entry->ring_name = job->ring->name;
>   			   __entry->num_ibs = job->num_ibs;
>   			   ),
> -	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, ring name=%s, num_ibs=%u",
> +	    TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p, timeline=%s, context=%u, seqno=%u, ring name=%s, num_ibs=%u",

If you have time for another patch please drop the pinters here as well. 
Scheduler job, IBs and fence are all heavily reallocated (sometimes even 
with a slab allocator). So the pointers are completely meaningless. The 
adev pointer is superseded by the timeline name and context numbers.

Anyway that should be done in an extra patch, so this one is 
Reviewed-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

>   		      __entry->adev, __entry->sched_job, __entry->ib,
> -		      __entry->fence, __entry->ring_name, __entry->num_ibs)
> +		      __entry->fence, __get_str(timeline), __entry->context, __entry->seqno,
> +		      __entry->ring_name, __entry->num_ibs)
>   );
>   
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Andres Rodriguez]

[-- Attachment #1.1: Type: text/plain, Size: 3742 bytes --]

On Feb 25, 2017 4:40 AM, "Christian König" <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> wrote:

Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:

> This information is intended to provide the required data to associate
> amdgpu tracepoints with their corresponding dma_fence_* events.
>
> Signed-off-by: Andres Rodriguez <andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index 01623d1..cc9a31d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_job,
>                              __field(struct amd_sched_job *, sched_job)
>                              __field(struct amdgpu_ib *, ib)
>                              __field(struct dma_fence *, fence)
> +                            __string(timeline,
> job->base.s_fence->finished.ops->get_timeline_name(&job->bas
> e.s_fence->finished))
> +                            __field(unsigned int, context)
> +                            __field(unsigned int, seqno)
>                              __field(char *, ring_name)
>                              __field(u32, num_ibs)
>                              ),
> @@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_job,
>                            __entry->sched_job = &job->base;
>                            __entry->ib = job->ibs;
>                            __entry->fence = &job->base.s_fence->finished;
> +                          __assign_str(timeline,
> job->base.s_fence->finished.ops->get_timeline_name(&job->bas
> e.s_fence->finished))
>

Not 100% sure, but we might be able to use a char * field here instead of
the extra overhead of embedding a string, the timeline names are never
freed IIRC.


I'll give this a try.



+                          __entry->context = job->base.s_fence->finished.co
> ntext;
> +                          __entry->seqno = job->base.s_fence->finished.se
> qno;
>                            __entry->ring_name = job->ring->name;
>                            __entry->num_ibs = job->num_ibs;
>                            ),
> -           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
> ring name=%s, num_ibs=%u",
> +           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
> timeline=%s, context=%u, seqno=%u, ring name=%s, num_ibs=%u",
>

If you have time for another patch please drop the pinters here as well.
Scheduler job, IBs and fence are all heavily reallocated (sometimes even
with a slab allocator). So the pointers are completely meaningless. The
adev pointer is superseded by the timeline name and context numbers.


The pointers do provide some useful data. But you need to make sure you
read it in between an allocation event and a fence signal event. It is
extremely terrible for reading from a trace text file.

How do you feel about if we replaced the job pointer with a job id, and
replaced the fence pointers with the fence data?



Anyway that should be done in an extra patch, so this one is Reviewed-by:
Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>.

Regards,
Christian.


                      __entry->adev, __entry->sched_job, __entry->ib,
> -                     __entry->fence, __entry->ring_name, __entry->num_ibs)
> +                     __entry->fence, __get_str(timeline),
> __entry->context, __entry->seqno,
> +                     __entry->ring_name, __entry->num_ibs)
>   );
>
>

[-- Attachment #1.2: Type: text/html, Size: 5956 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 5259 bytes --]

Am 25.02.2017 um 18:28 schrieb Andres Rodriguez:
>
>
> On Feb 25, 2017 4:40 AM, "Christian König" <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org 
> <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>
>     Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:
>
>         This information is intended to provide the required data to
>         associate
>         amdgpu tracepoints with their corresponding dma_fence_* events.
>
>         Signed-off-by: Andres Rodriguez <andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
>         <mailto:andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>>
>         ---
>           drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 11 +++++++++--
>           1 file changed, 9 insertions(+), 2 deletions(-)
>
>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>         b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>         index 01623d1..cc9a31d 100644
>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>         @@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_job,
>                                      __field(struct amd_sched_job *,
>         sched_job)
>                                      __field(struct amdgpu_ib *, ib)
>                                      __field(struct dma_fence *, fence)
>         +                            __string(timeline,
>         job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
>         +                            __field(unsigned int, context)
>         +                            __field(unsigned int, seqno)
>                                      __field(char *, ring_name)
>                                      __field(u32, num_ibs)
>                                      ),
>         @@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_job,
>                                    __entry->sched_job = &job->base;
>                                    __entry->ib = job->ibs;
>                                    __entry->fence =
>         &job->base.s_fence->finished;
>         +                          __assign_str(timeline,
>         job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
>
>
>     Not 100% sure, but we might be able to use a char * field here
>     instead of the extra overhead of embedding a string, the timeline
>     names are never freed IIRC.
>
>
> I'll give this a try.
>
>
>
>         +                          __entry->context =
>         job->base.s_fence->finished.co <http://finished.co>ntext;
>         +                          __entry->seqno =
>         job->base.s_fence->finished.se <http://finished.se>qno;
>                                    __entry->ring_name = job->ring->name;
>                                    __entry->num_ibs = job->num_ibs;
>                                    ),
>         -           TP_printk("adev=%p, sched_job=%p, first ib=%p,
>         sched fence=%p, ring name=%s, num_ibs=%u",
>         +           TP_printk("adev=%p, sched_job=%p, first ib=%p,
>         sched fence=%p, timeline=%s, context=%u, seqno=%u, ring
>         name=%s, num_ibs=%u",
>
>
>     If you have time for another patch please drop the pinters here as
>     well. Scheduler job, IBs and fence are all heavily reallocated
>     (sometimes even with a slab allocator). So the pointers are
>     completely meaningless. The adev pointer is superseded by the
>     timeline name and context numbers.
>
>
> The pointers do provide some useful data. But you need to make sure 
> you read it in between an allocation event and a fence signal event. 
> It is extremely terrible for reading from a trace text file.

Yeah, that's why I try to avoid them in the trace files.

Additional to that the adev pointer is 8 bytes and pretty much 
meaningless in the trace file, but the PCI bus number is only 4 bytes 
IIRC and really easy to related to other log messages.

>
> How do you feel about if we replaced the job pointer with a job id, 
> and replaced the fence pointers with the fence data?

Mostly sounds like a plan to me. I would just try to get away from 
trying to use the job to identify a command submission.

Instead use the scheduler fence for this. The difference is that we 
create the job structure early during command submission, but the 
scheduler fence only when the command submission is actually 
successfully and not interrupted by a signal.

So a job id would contain a bunch of numbers which are never submitted. 
Especially for the X server that is usually rather annoying in the logs.

Regards,
Christian.

>
>     Anyway that should be done in an extra patch, so this one is
>     Reviewed-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org
>     <mailto:christian.koenig-5C7GfCeVMHo@public.gmane.org>>.
>
>     Regards,
>     Christian.
>
>
>                               __entry->adev, __entry->sched_job,
>         __entry->ib,
>         -                     __entry->fence, __entry->ring_name,
>         __entry->num_ibs)
>         +                     __entry->fence, __get_str(timeline),
>         __entry->context, __entry->seqno,
>         +                     __entry->ring_name, __entry->num_ibs)
>           );
>
>
>
>


[-- Attachment #1.2: Type: text/html, Size: 10364 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: make trace format uniform csv name=value

[Alex Deucher]

On Sat, Feb 25, 2017 at 4:33 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:
>>
>> Most of the traces have uniform format except for two of them. Having
>> all the traces match makes it simple to run awk on the ftrace output.
>>
>> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>.

Applied.  thanks!

Alex

>
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> index a18ae1e..01623d1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> @@ -142,7 +142,7 @@ TRACE_EVENT(amdgpu_sched_run_job,
>>                            __entry->ring_name = job->ring->name;
>>                            __entry->num_ibs = job->num_ibs;
>>                            ),
>> -           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
>> ring name:%s, num_ibs:%u",
>> +           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
>> ring name=%s, num_ibs=%u",
>>                       __entry->adev, __entry->sched_job, __entry->ib,
>>                       __entry->fence, __entry->ring_name,
>> __entry->num_ibs)
>>   );
>> @@ -359,7 +359,7 @@ TRACE_EVENT(amdgpu_ttm_bo_move,
>>                         __entry->new_placement = new_placement;
>>                         __entry->old_placement = old_placement;
>>                         ),
>> -           TP_printk("bo=%p from:%d to %d with size = %Ld",
>> +           TP_printk("bo=%p, from=%d, to=%d, size=%Ld",
>>                         __entry->bo, __entry->old_placement,
>>                         __entry->new_placement, __entry->bo_size)
>>   );
>
>
>
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Alex Deucher]

Is this patch ready to land or is there more work required?

Thanks,

Alex

On Sun, Feb 26, 2017 at 5:54 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 25.02.2017 um 18:28 schrieb Andres Rodriguez:
>
>
>
> On Feb 25, 2017 4:40 AM, "Christian König" <deathsimple@vodafone.de> wrote:
>
> Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:
>>
>> This information is intended to provide the required data to associate
>> amdgpu tracepoints with their corresponding dma_fence_* events.
>>
>> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> index 01623d1..cc9a31d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> @@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_job,
>>                              __field(struct amd_sched_job *, sched_job)
>>                              __field(struct amdgpu_ib *, ib)
>>                              __field(struct dma_fence *, fence)
>> +                            __string(timeline,
>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
>> +                            __field(unsigned int, context)
>> +                            __field(unsigned int, seqno)
>>                              __field(char *, ring_name)
>>                              __field(u32, num_ibs)
>>                              ),
>> @@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_job,
>>                            __entry->sched_job = &job->base;
>>                            __entry->ib = job->ibs;
>>                            __entry->fence = &job->base.s_fence->finished;
>> +                          __assign_str(timeline,
>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
>
>
> Not 100% sure, but we might be able to use a char * field here instead of
> the extra overhead of embedding a string, the timeline names are never freed
> IIRC.
>
>
> I'll give this a try.
>
>
>
>> +                          __entry->context =
>> job->base.s_fence->finished.context;
>> +                          __entry->seqno =
>> job->base.s_fence->finished.seqno;
>>                            __entry->ring_name = job->ring->name;
>>                            __entry->num_ibs = job->num_ibs;
>>                            ),
>> -           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
>> ring name=%s, num_ibs=%u",
>> +           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
>> timeline=%s, context=%u, seqno=%u, ring name=%s, num_ibs=%u",
>
>
> If you have time for another patch please drop the pinters here as well.
> Scheduler job, IBs and fence are all heavily reallocated (sometimes even
> with a slab allocator). So the pointers are completely meaningless. The adev
> pointer is superseded by the timeline name and context numbers.
>
>
> The pointers do provide some useful data. But you need to make sure you read
> it in between an allocation event and a fence signal event. It is extremely
> terrible for reading from a trace text file.
>
>
> Yeah, that's why I try to avoid them in the trace files.
>
> Additional to that the adev pointer is 8 bytes and pretty much meaningless
> in the trace file, but the PCI bus number is only 4 bytes IIRC and really
> easy to related to other log messages.
>
>
> How do you feel about if we replaced the job pointer with a job id, and
> replaced the fence pointers with the fence data?
>
>
> Mostly sounds like a plan to me. I would just try to get away from trying to
> use the job to identify a command submission.
>
> Instead use the scheduler fence for this. The difference is that we create
> the job structure early during command submission, but the scheduler fence
> only when the command submission is actually successfully and not
> interrupted by a signal.
>
> So a job id would contain a bunch of numbers which are never submitted.
> Especially for the X server that is usually rather annoying in the logs.
>
> Regards,
> Christian.
>
>
>
>
> Anyway that should be done in an extra patch, so this one is Reviewed-by:
> Christian König <christian.koenig@amd.com>.
>
> Regards,
> Christian.
>
>
>>                       __entry->adev, __entry->sched_job, __entry->ib,
>> -                     __entry->fence, __entry->ring_name,
>> __entry->num_ibs)
>> +                     __entry->fence, __get_str(timeline),
>> __entry->context, __entry->seqno,
>> +                     __entry->ring_name, __entry->num_ibs)
>>   );
>>
>
>
>
>
>
>
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Andres Rodriguez]

[-- Attachment #1.1: Type: text/plain, Size: 165 bytes --]

I'm working on a follow-up that has Christian's extra requests to remove
the job/fence pointers. But that is coming as a separate commit.

Regards,
Andres
​

[-- Attachment #1.2: Type: text/html, Size: 256 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: trace fence details in amdgpu_sched_run_job

[Alex Deucher]

On Sat, Feb 25, 2017 at 4:39 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 24.02.2017 um 19:20 schrieb Andres Rodriguez:
>>
>> This information is intended to provide the required data to associate
>> amdgpu tracepoints with their corresponding dma_fence_* events.
>>
>> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> index 01623d1..cc9a31d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
>> @@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_job,
>>                              __field(struct amd_sched_job *, sched_job)
>>                              __field(struct amdgpu_ib *, ib)
>>                              __field(struct dma_fence *, fence)
>> +                            __string(timeline,
>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
>> +                            __field(unsigned int, context)
>> +                            __field(unsigned int, seqno)
>>                              __field(char *, ring_name)
>>                              __field(u32, num_ibs)
>>                              ),
>> @@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_job,
>>                            __entry->sched_job = &job->base;
>>                            __entry->ib = job->ibs;
>>                            __entry->fence = &job->base.s_fence->finished;
>> +                          __assign_str(timeline,
>> job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished))
>
>
> Not 100% sure, but we might be able to use a char * field here instead of
> the extra overhead of embedding a string, the timeline names are never freed
> IIRC.
>
>> +                          __entry->context =
>> job->base.s_fence->finished.context;
>> +                          __entry->seqno =
>> job->base.s_fence->finished.seqno;
>>                            __entry->ring_name = job->ring->name;
>>                            __entry->num_ibs = job->num_ibs;
>>                            ),
>> -           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
>> ring name=%s, num_ibs=%u",
>> +           TP_printk("adev=%p, sched_job=%p, first ib=%p, sched fence=%p,
>> timeline=%s, context=%u, seqno=%u, ring name=%s, num_ibs=%u",
>
>
> If you have time for another patch please drop the pinters here as well.
> Scheduler job, IBs and fence are all heavily reallocated (sometimes even
> with a slab allocator). So the pointers are completely meaningless. The adev
> pointer is superseded by the timeline name and context numbers.
>
> Anyway that should be done in an extra patch, so this one is Reviewed-by:
> Christian König <christian.koenig@amd.com>.

Applied.  thanks!

Alex


>
> Regards,
> Christian.
>
>>                       __entry->adev, __entry->sched_job, __entry->ib,
>> -                     __entry->fence, __entry->ring_name,
>> __entry->num_ibs)
>> +                     __entry->fence, __get_str(timeline),
>> __entry->context, __entry->seqno,
>> +                     __entry->ring_name, __entry->num_ibs)
>>   );
>>
>
>
>
> _______________________________________________
> 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