[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 at vodafone.de> wrote:
> Am 25.02.2017 um 18:28 schrieb 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->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 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)
>>   );
>>
>
>
>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>