<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">Am 25.02.2017 um 18:28 schrieb Andres
      Rodriguez:<br>
    </div>
    <blockquote
cite="mid:CAFQ_0eET-ydfJyFthnGOR-3-q9ZD8dev=C0eM1dfZHC8vpyXWQ@mail.gmail.com"
      type="cite">
      <div dir="auto">
        <div><br>
          <div class="gmail_extra"><br>
            <div class="gmail_quote">On Feb 25, 2017 4:40 AM, "Christian
              König" <<a moz-do-not-send="true"
                href="mailto:deathsimple@vodafone.de">deathsimple@vodafone.de</a>>
              wrote:<br type="attribution">
              <blockquote class="quote" style="margin:0 0 0
                .8ex;border-left:1px #ccc solid;padding-left:1ex">
                <div class="elided-text">Am 24.02.2017 um 19:20 schrieb
                  Andres Rodriguez:<br>
                  <blockquote class="gmail_quote" style="margin:0 0 0
                    .8ex;border-left:1px #ccc solid;padding-left:1ex">
                    This information is intended to provide the required
                    data to associate<br>
                    amdgpu tracepoints with their corresponding
                    dma_fence_* events.<br>
                    <br>
                    Signed-off-by: Andres Rodriguez <<a
                      moz-do-not-send="true"
                      href="mailto:andresx7@gmail.com" target="_blank">andresx7@gmail.com</a>><br>
                    ---<br>
                      drivers/gpu/drm/amd/amdgpu/amd<wbr>gpu_trace.h |
                    11 +++++++++--<br>
                      1 file changed, 9 insertions(+), 2 deletions(-)<br>
                    <br>
                    diff --git a/drivers/gpu/drm/amd/amdgpu/a<wbr>mdgpu_trace.h
                    b/drivers/gpu/drm/amd/amdgpu/a<wbr>mdgpu_trace.h<br>
                    index 01623d1..cc9a31d 100644<br>
                    --- a/drivers/gpu/drm/amd/amdgpu/a<wbr>mdgpu_trace.h<br>
                    +++ b/drivers/gpu/drm/amd/amdgpu/a<wbr>mdgpu_trace.h<br>
                    @@ -130,6 +130,9 @@ TRACE_EVENT(amdgpu_sched_run_j<wbr>ob,<br>
                                                 __field(struct
                    amd_sched_job *, sched_job)<br>
                                                 __field(struct
                    amdgpu_ib *, ib)<br>
                                                 __field(struct
                    dma_fence *, fence)<br>
                    +                            __string(timeline,
                    job->base.s_fence->finished.op<wbr>s->get_timeline_name(&job->bas<wbr>e.s_fence->finished))<br>
                    +                            __field(unsigned int,
                    context)<br>
                    +                            __field(unsigned int,
                    seqno)<br>
                                                 __field(char *,
                    ring_name)<br>
                                                 __field(u32, num_ibs)<br>
                                                 ),<br>
                    @@ -139,12 +142,16 @@ TRACE_EVENT(amdgpu_sched_run_j<wbr>ob,<br>
                                               __entry->sched_job =
                    &job->base;<br>
                                               __entry->ib =
                    job->ibs;<br>
                                               __entry->fence =
                    &job->base.s_fence->finished;<br>
                    +                          __assign_str(timeline,
                    job->base.s_fence->finished.op<wbr>s->get_timeline_name(&job->bas<wbr>e.s_fence->finished))<br>
                  </blockquote>
                  <br>
                </div>
                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.</blockquote>
            </div>
          </div>
        </div>
        <div dir="auto"><br>
        </div>
        <div dir="auto">I'll give this a try. </div>
        <div dir="auto">
          <div class="gmail_extra">
            <div class="gmail_quote">
              <blockquote class="quote" style="margin:0 0 0
                .8ex;border-left:1px #ccc solid;padding-left:1ex">
                <div class="quoted-text"><br>
                  <br>
                  <blockquote class="gmail_quote" style="margin:0 0 0
                    .8ex;border-left:1px #ccc solid;padding-left:1ex">
                    +                          __entry->context =
                    job->base.s_fence-><a moz-do-not-send="true"
                      href="http://finished.co">finished.co</a><wbr>ntext;<br>
                    +                          __entry->seqno =
                    job->base.s_fence-><a moz-do-not-send="true"
                      href="http://finished.se">finished.se</a><wbr>qno;<br>
                                               __entry->ring_name =
                    job->ring->name;<br>
                                               __entry->num_ibs =
                    job->num_ibs;<br>
                                               ),<br>
                    -           TP_printk("adev=%p, sched_job=%p, first
                    ib=%p, sched fence=%p, ring name=%s, num_ibs=%u",<br>
                    +           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",<br>
                  </blockquote>
                  <br>
                </div>
                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.<br>
              </blockquote>
            </div>
          </div>
        </div>
        <div dir="auto"><br>
        </div>
        <div dir="auto">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. <br>
        </div>
      </div>
    </blockquote>
    <br>
    Yeah, that's why I try to avoid them in the trace files.<br>
    <br>
    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.<br>
    <br>
    <blockquote
cite="mid:CAFQ_0eET-ydfJyFthnGOR-3-q9ZD8dev=C0eM1dfZHC8vpyXWQ@mail.gmail.com"
      type="cite">
      <div dir="auto">
        <div dir="auto"><br>
        </div>
        <div dir="auto">How do you feel about if we replaced the job
          pointer with a job id, and replaced the fence pointers with
          the fence data? <br>
        </div>
      </div>
    </blockquote>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    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.<br>
    <br>
    Regards,<br>
    Christian.<br>
    <br>
    <blockquote
cite="mid:CAFQ_0eET-ydfJyFthnGOR-3-q9ZD8dev=C0eM1dfZHC8vpyXWQ@mail.gmail.com"
      type="cite">
      <div dir="auto">
        <div dir="auto"> </div>
        <div dir="auto">
          <div class="gmail_extra">
            <div class="gmail_quote">
              <blockquote class="quote" style="margin:0 0 0
                .8ex;border-left:1px #ccc solid;padding-left:1ex">
                <br>
                Anyway that should be done in an extra patch, so this
                one is Reviewed-by: Christian König <<a
                  moz-do-not-send="true"
                  href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>>.<br>
                <br>
                Regards,<br>
                Christian.
                <div class="elided-text"><br>
                  <br>
                  <blockquote class="gmail_quote" style="margin:0 0 0
                    .8ex;border-left:1px #ccc solid;padding-left:1ex">
                                          __entry->adev,
                    __entry->sched_job, __entry->ib,<br>
                    -                     __entry->fence,
                    __entry->ring_name, __entry->num_ibs)<br>
                    +                     __entry->fence,
                    __get_str(timeline), __entry->context,
                    __entry->seqno,<br>
                    +                     __entry->ring_name,
                    __entry->num_ibs)<br>
                      );<br>
                        <br>
                  </blockquote>
                  <br>
                  <br>
                </div>
              </blockquote>
            </div>
            <br>
          </div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
  </body>
</html>