Re: [igt-dev] [PATCH i-g-t v2 3/3] tests/i915/perf: verify reason field in OA reports is never 0

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 26/11/2020 00:35, Umesh Nerlige Ramappa wrote:
> On Wed, Nov 25, 2020 at 01:25:40PM +0200, Lionel Landwerlin wrote:
>> We're about to remove the filtering in i915 on 0 reason fields because
>> we assume this was a possibility, but it turned out to be a corruption
>> in the tail pointer register that made us read cleared data.
>>
>> This test is here to verify that our assumption hold that the HW never
>> produces such reports.
>>
>> v2: Fix len checking (Umesh)
>>    Count report lost events (Umesh)
>>    Check report sanity (Umesh)
>>    Limit test to 3 times the OA buffer (Umesh)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>> tests/i915/perf.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 90 insertions(+)
>>
>> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
>> index 811067aad..2d442df96 100644
>> --- a/tests/i915/perf.c
>> +++ b/tests/i915/perf.c
>> @@ -2550,6 +2550,89 @@ test_buffer_fill(void)
>>     __perf_close(stream_fd);
>> }
>>
>> +static void
>> +test_non_zero_reason(void)
>> +{
>> +    /* ~10 micro second period */
>> +    int oa_exponent = max_oa_exponent_for_period_lte(10000);
>> +    uint64_t properties[] = {
>> +        /* Include OA reports in samples */
>> +        DRM_I915_PERF_PROP_SAMPLE_OA, true,
>> +
>> +        /* OA unit configuration */
>> +        DRM_I915_PERF_PROP_OA_METRICS_SET, 
>> test_set->perf_oa_metrics_set,
>> +        DRM_I915_PERF_PROP_OA_FORMAT, test_set->perf_oa_format,
>> +        DRM_I915_PERF_PROP_OA_EXPONENT, oa_exponent,
>> +    };
>> +    struct drm_i915_perf_open_param param = {
>> +        .flags = I915_PERF_FLAG_FD_CLOEXEC,
>> +        .num_properties = sizeof(properties) / 16,
>> +        .properties_ptr = to_user_pointer(properties),
>> +    };
>> +    struct drm_i915_perf_record_header *header;
>> +    uint32_t buf_size = 3 * 65536 * (256 + sizeof(struct 
>> drm_i915_perf_record_header));
>> +    uint8_t *buf = malloc(buf_size);
>> +    uint32_t total_len = 0, reports_lost;
>> +    const uint32_t *last_report;
>> +    int len;
>> +
>> +    igt_assert(buf);
>
> nit: Looks like sanity check will log the report0/report1 pointers. We 
> could log the buf pointer here so we get a sense of the report index 
> that failed.


Thanks, I did not meant to leave the pointer to the report in the 
previous change, I was just checking those same timestamp reports ;)


I just pushed the 2 previous commits, waiting for the Gen11 fix to go 
before I land this patch.


>
> Either ways, this is
>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>
> Thanks for adding this test,
> Umesh
>> +
>> +    igt_debug("Ready to read about %u bytes\n", buf_size);
>> +
>> +    load_helper_init();
>> +    load_helper_run(HIGH);
>> +
>> +    stream_fd = __perf_open(drm_fd, &param, true /* prevent_pm */);
>> +
>> +    while (total_len < (buf_size - sizeof(struct 
>> drm_i915_perf_record_header)) &&
>> +           ((len = read(stream_fd, &buf[total_len], buf_size - 
>> total_len)) > 0 ||
>> +        (len == -1 && errno == EINTR))) {
>> +        if (len > 0)
>> +            total_len += len;
>> +    }
>> +
>> +    __perf_close(stream_fd);
>> +
>> +    load_helper_stop();
>> +    load_helper_fini();
>> +
>> +    igt_debug("Got %u bytes\n", total_len);
>> +
>> +    last_report = NULL;
>> +    reports_lost = 0;
>> +    for (uint32_t offset = 0; offset < total_len; offset += 
>> header->size) {
>> +        header = (void *) (buf + offset);
>> +
>> +        switch (header->type) {
>> +        case DRM_I915_PERF_RECORD_OA_REPORT_LOST:
>> +            reports_lost++;
>> +            break;
>> +        case DRM_I915_PERF_RECORD_SAMPLE: {
>> +            const uint32_t *report = (void *) (header + 1);
>> +            uint32_t reason = (report[0] >> OAREPORT_REASON_SHIFT) &
>> +                OAREPORT_REASON_MASK;
>> +
>> +            igt_assert_neq(reason, 0);
>> +
>> +            if (last_report) {
>> +                sanity_check_reports(last_report, report,
>> +                             test_set->perf_oa_format);
>> +            }
>> +            last_report = report;
>> +            break;
>> +        }
>> +        case DRM_I915_PERF_RECORD_OA_BUFFER_LOST:
>> +            igt_assert(!"unexpected overflow");
>> +            break;
>> +        }
>> +    }
>> +
>> +    igt_debug("Got %u report lost events\n", reports_lost);
>> +
>> +    free(buf);
>> +}
>> +
>> static void
>> test_enable_disable(void)
>> {
>> @@ -4893,6 +4976,13 @@ igt_main
>>     igt_subtest("buffer-fill")
>>         test_buffer_fill();
>>
>> +    igt_describe("Test that reason field in OA reports is never 0 on 
>> Gen8+");
>> +    igt_subtest("non-zero-reason") {
>> +        /* Reason field is only available on Gen8+ */
>> +        igt_require(intel_gen(devid) >= 8);
>> +        test_non_zero_reason();
>> +    }
>> +
>>     igt_subtest("disabled-read-error")
>>         test_disabled_read_error();
>>     igt_subtest("non-sampling-read-error")
>> -- 
>> 2.29.2
>>

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev