[igt-dev] [PATCH i-g-t 4/4] lib/i915/perf: don't forget last timeline element

[Umesh Nerlige Ramappa]

On Thu, Jan 07, 2021 at 12:12:27PM +0200, Lionel Landwerlin wrote:
>On 07/01/2021 03:07, Umesh Nerlige Ramappa wrote:
>>On Mon, Dec 28, 2020 at 05:19:40AM +0200, Lionel Landwerlin wrote:
>>>We're currently dropping the last element of the timeline.
>>
>>Inside the for-loop we append timeline events on context switches 
>>indicating what context ran and for how long, but that's not the 
>>case for the append_timeline_event outside the for-loop.
>>
>>Does the one outside indicate how long last_ctx_id ran before 
>>capture ended (or we ran out of records)? Is it possible for the 
>>user to distinguish the two? Just thinking out loud.
>
>
>Let's say you have the following reports
>
>
>report0 ctx=1
>
>report1 ctx=2
>
>report2 ctx=1
>
>
>That generates 2 timeline events (report1 - report0 for ctx1), 
>(report2 - report1 for ctx2).
>
>Those 2 are generated within the for loop because it notices a change 
>in ctx value twice.
>
>
>Now this :
>
>
>report0 ctx=1
>
>report1 ctx=2
>
>report2 ctx=1
>
>report3 ctx=1
>
>
>This also generates 2 timeline events within the for loop.
>
>But because the last report3 has the same ctx as report2, there is no 
>additional timeline event generated for ctx1 within the for loop.
>
>And we just quit the loop because we ran out of reports.
>
>So we need to add one more item.
>
>
>The gpu_ts_start & gpu_ts_end being generated before the continue; in 
>the for loop, make the last append_timeline_event() add the right 
>timestamps after the for loop for that last timeline.

Makes sense, thanks.
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>

-Umesh
>
>
>-Lionel
>
>
>>
>>Otherwise:
>>
>>Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>>
>>Thanks,
>>Umesh
>>>
>>>Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>Fixes: 43116ee368585d ("lib/i915-perf: add i915 perf data reader")
>>>---
>>>lib/i915/perf_data_reader.c | 27 ++++++++++++++++++---------
>>>1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>>diff --git a/lib/i915/perf_data_reader.c b/lib/i915/perf_data_reader.c
>>>index 4b68fb502..065fe6066 100644
>>>--- a/lib/i915/perf_data_reader.c
>>>+++ b/lib/i915/perf_data_reader.c
>>>@@ -298,17 +298,23 @@ static void
>>>generate_cpu_events(struct intel_perf_data_reader *reader)
>>>{
>>>    uint32_t last_header_idx = 0;
>>>-    const struct drm_i915_perf_record_header *last_header = 
>>>reader->records[0];
>>>+    const struct drm_i915_perf_record_header *last_header = 
>>>reader->records[0],
>>>+        *current_header = reader->records[0];
>>>+    const uint8_t *start_report, *end_report;
>>>+    uint32_t last_ctx_id, current_ctx_id;
>>>+    uint64_t gpu_ts_start, gpu_ts_end;
>>>
>>>    for (uint32_t i = 1; i < reader->n_records; i++) {
>>>-        const struct drm_i915_perf_record_header *current_header =
>>>-            reader->records[i];
>>>-        const uint8_t *start_report = (const uint8_t *) 
>>>(last_header + 1),
>>>-            *end_report = (const uint8_t *) (current_header + 1);
>>>-        uint32_t last_ctx_id = oa_report_ctx_id(&reader->devinfo, 
>>>start_report),
>>>-            current_ctx_id = oa_report_ctx_id(&reader->devinfo, 
>>>end_report);
>>>-        uint64_t gpu_ts_start = oa_report_timestamp(start_report),
>>>-            gpu_ts_end = oa_report_timestamp(end_report);
>>>+        current_header = reader->records[i];
>>>+
>>>+        start_report = (const uint8_t *) (last_header + 1);
>>>+        end_report = (const uint8_t *) (current_header + 1);
>>>+
>>>+        last_ctx_id = oa_report_ctx_id(&reader->devinfo, start_report);
>>>+        current_ctx_id = oa_report_ctx_id(&reader->devinfo, 
>>>end_report);
>>>+
>>>+        gpu_ts_start = oa_report_timestamp(start_report);
>>>+        gpu_ts_end = oa_report_timestamp(end_report);
>>>
>>>        if (last_ctx_id == current_ctx_id)
>>>            continue;
>>>@@ -318,6 +324,9 @@ generate_cpu_events(struct 
>>>intel_perf_data_reader *reader)
>>>        last_header = current_header;
>>>        last_header_idx = i;
>>>    }
>>>+
>>>+    if (last_header != current_header)
>>>+        append_timeline_event(reader, gpu_ts_start, gpu_ts_end, 
>>>last_header_idx, reader->n_records - 1, last_ctx_id);
>>>}
>>>
>>>static void
>>>-- 
>>>2.30.0.rc2
>>>
>