Re: [PATCH i-g-t 12/28] tests/intel/xe_oa: Add "oa-formats" subtest

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 20 Jun 2024 16:15:56 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Thu, Jun 20, 2024 at 01:00:37PM -0700, Ashutosh Dixit wrote:
> > Add "oa-formats" subtest.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Some nits to clean up, but otherwise looks good.
>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks!

>
> > ---
> > tests/intel/xe_oa.c | 550 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 550 insertions(+)
> >
> > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> > index 97bbd13674..b7de6f7bd2 100644
> > --- a/tests/intel/xe_oa.c
> > +++ b/tests/intel/xe_oa.c
> > @@ -290,6 +290,9 @@ static struct oa_format lnl_oa_formats[XE_OA_FORMAT_MAX] = {
> >		.bc_report = 0 },
> > };
> >
> > +/* No A counters currently reserved/undefined for gen8+ so far */
> > +static bool undefined_a_counters[45];
>
> As the comment says, we can drop undefined_a_counters.

Done.

> > +/* Debug function, only useful when reports don't make sense. */
> > +#if 0
>
> Let's get rid of this #if 0 block as well.

Done.

>
> > +static void
> > +print_report(uint32_t *report, int fmt)
> > +{
> > +	struct oa_format format = get_oa_format(fmt);
> > +
> > +	igt_debug("TIMESTAMP: %"PRIu64"\n", oa_timestamp(report, fmt));
> > +
> > +	{
> > +		uint64_t clock = read_report_ticks(report, fmt);
> > +
> > +		igt_debug("CLOCK: %"PRIu64"\n", clock);
> > +	}
> > +
> > +	{
> > +		uint32_t slice_freq, unslice_freq;
> > +		const char *reason = read_report_reason(report);
> > +
> > +		read_report_clock_ratios(report, &slice_freq, &unslice_freq);
> > +
> > +		igt_debug("SLICE CLK: %umhz\n", slice_freq);
> > +		igt_debug("UNSLICE CLK: %umhz\n", unslice_freq);
> > +		igt_debug("REASON: \"%s\"\n", reason);
> > +		igt_debug("CTX ID: %"PRIu32"/%"PRIx32"\n", report[2], report[2]);
> > +	}
> > +
> > +	/* Gen8+ has some 40bit A counters... */
> > +	for (int j = 0; j < format.n_a40; j++) {
> > +		uint64_t value = read_40bit_a_counter(report, fmt, j);
> > +
> > +		if (undefined_a_counters[j])
> > +			continue;
> > +
> > +		igt_debug("A%d: %"PRIu64"\n", j, value);
> > +	}
> > +
> > +	for (int j = 0; j < format.n_a; j++) {
> > +		uint32_t *a = (uint32_t *)(((uint8_t *)report) +
> > +					   format.a_off);
> > +		int a_id = format.first_a + j;
> > +
> > +		if (undefined_a_counters[a_id])
> > +			continue;
> > +
> > +		igt_debug("A%d: %"PRIu32"\n", a_id, a[j]);
> > +	}
> > +
> > +	for (int j = 0; j < format.n_b; j++) {
> > +		uint32_t *b = (uint32_t *)(((uint8_t *)report) +
> > +					   format.b_off);
> > +
> > +		igt_debug("B%d: %"PRIu32"\n", j, b[j]);
> > +	}
> > +
> > +	for (int j = 0; j < format.n_c; j++) {
> > +		uint32_t *c = (uint32_t *)(((uint8_t *)report) +
> > +					   format.c_off);
> > +
> > +		igt_debug("C%d: %"PRIu32"\n", j, c[j]);
> > +	}
> > +}
> > +#endif

Thanks.
--
Ashutosh