Re: [PATCH i-g-t v2] tools/xe-perf-recorder: Wait for mmio-trigger-report

[Shekhar Chauhan <shekhar.chauhan@intel.com>]

On 9/21/2026 12:10, Shekhar Chauhan wrote:
>
> On 9/5/2026 6:50, Ashutosh Dixit wrote:
>> After mmio trigger is issued, we should wait till we receive a report 
>> with
>> the context id issued in the mmio trigger.
>>
>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> ---
>>   tools/xe-perf/xe_perf_recorder.c | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)

Plus (other than my last comment, read below), I tested this patch and 
it works fine:

$ sudo ./build/tools/xe-perf/xe-perf-recorder -m "RenderBasic" -o 
/tmp/v2.record.renderbasic
Device name=pantherlake gen=30 id=0xb081 oa_unit=0 gt=0
Writing recoding to /tmp/v2.record.renderbasic
Using correlation clock: monotonic
Opening perf stream with metric_id=1 oa_exponent=14 oa_format=11
Received trigger report with value 0xc0ffee01
^CReceived trigger report with value 0xc0ffee02
Exiting...

So, we're not breaking anything with this change.

-shekhar

>>
>> diff --git a/tools/xe-perf/xe_perf_recorder.c 
>> b/tools/xe-perf/xe_perf_recorder.c
>> index e13eef5190..cb7ca67bdb 100644
>> --- a/tools/xe-perf/xe_perf_recorder.c
>> +++ b/tools/xe-perf/xe_perf_recorder.c
>> @@ -370,6 +370,7 @@ struct recording_context {
>>       uint32_t vm;
>>       uint32_t exec_queue;
>>       struct intel_bb *ibb;
>> +    uint32_t mmio_trg_val;
>>   };
>>     static uint32_t oa_unit_mmio_trigger_reg(struct recording_context 
>> *ctx)
>> @@ -642,13 +643,15 @@ check_mmio_trigger_report(struct 
>> recording_context *ctx, const void *report)
>>       const uint32_t *report_32 = report;
>>       uint64_t value;
>>   -    if (report_reason(report_32))
>> +    if (!ctx->mmio_trg_val || report_reason(report_32))
>>           return;
>>         value = (fmt->header == HDR_64_BIT) ? ((const uint64_t 
>> *)report)[2] : report_32[2];
>>   -    if (value == 0xc0ffee01 || value == 0xc0ffee02)
>> +    if (value == ctx->mmio_trg_val) {
>>           fprintf(stdout, "Received trigger report with value 0x%" 
>> PRIx64 "\n", value);
>> +        ctx->mmio_trg_val = 0;
>> +    }
>>   }
>>     static bool write_stream_data(struct recording_context *ctx,
>> @@ -712,6 +715,18 @@ static bool write_perf_data(FILE *output, struct 
>> recording_context *ctx)
>>       return false;
>>   }
>>   +static void write_mmio_trigger_data(struct recording_context *ctx)
>> +{
>> +    struct pollfd pollfd = { .fd = ctx->perf_fd, .events = POLLIN };
>> +
>> +    while (ctx->mmio_trg_val) {
>> +        poll(&pollfd, 1, -1);
>> +        assert(pollfd.revents & POLLIN);
>> +
>> +        write_perf_data(ctx->output_stream, ctx);
>> +    }
>> +}
>
> On a write failure, write_stream_data() bails out in the middle, and 
> some reports are dropped without being checked for the trigger value.  
> If the trigger is one of them, this while loop spins forever.  I think 
> we should use a return value (maybe a bool value) and bail out when 
> write_perf_data() fails so the error reaches main() instead of looping 
> forever.  Something like v1.
>
> Rest looks fine to me.
>
> -shekhar
>
>> +
>>   static clock_t correlation_clock_id = CLOCK_MONOTONIC;
>>     static const char *
>> @@ -797,6 +812,9 @@ static void emit_oa_trigger(struct 
>> recording_context *ctx, uint32_t value)
>>       intel_bb_flush_render(ctx->ibb);
>>       intel_bb_sync(ctx->ibb);
>>       intel_bb_reset(ctx->ibb, false);
>> +
>> +    ctx->mmio_trg_val = value;
>> +    write_mmio_trigger_data(ctx);
>>   }
>>     static void
>> @@ -815,7 +833,6 @@ read_command_file(struct recording_context *ctx)
>>           FILE *file;
>>             emit_oa_trigger(ctx, 0xc0ffee02);
>> -        write_perf_data(ctx->output_stream, ctx);
>>             while (offset < len &&
>>                  ((ret = read(ctx->command_fifo_fd,
>
-- 
Shekhar Chauhan
Linux Graphics Software Engineer
Intel Corporation