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

[Christian König]

Am 25.02.2017 um 18:28 schrieb Andres Rodriguez:
>
>
> On Feb 25, 2017 4:40 AM, "Christian König" <deathsimple at vodafone.de 
> <mailto: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
>         <mailto: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->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 at amd.com
>     <mailto: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/20170226/5b0c1d72/attachment-0001.html>