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

[Andres Rodriguez]

On Feb 25, 2017 4:40 AM, "Christian König" <deathsimple at 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 at 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->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 at 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)
>   );
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170225/c2e0785f/attachment.html>