[PATCH i-g-t 0/4] IGT xe_oa fixes, debug and new test

[PATCH i-g-t 0/4] IGT xe_oa fixes, debug and new test

[Umesh Nerlige Ramappa]

Random stuff gathered from various debugs:

1) Sanity check dumps 4 times the report size, fix that
2) Waiting for mmaped reports need to increment correctly in the loop
3) Add some ftrace capability for debugs
4) Add a test for tail address wrap

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Umesh Nerlige Ramappa (4):
  tests/intel/xe_oa: Fix report dumps in sanity check
  tests/intel/xe_oa: Fix waiting for mmaped reports
  tests/intel/xe_oa: Add a test for tail address wrap
  tests/intel/xe_oa: Add an option to capture ftrace

 tests/intel/xe_oa.c | 141 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 135 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH i-g-t 1/4] tests/intel/xe_oa: Fix report dumps in sanity check

[Umesh Nerlige Ramappa]

Fix report dumps in sanity check to dump the right amount of dwords.

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 tests/intel/xe_oa.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
index 63b59028fc5d..35d25150ec7d 100644
--- a/tests/intel/xe_oa.c
+++ b/tests/intel/xe_oa.c
@@ -1052,8 +1052,8 @@ static void pec_sanity_check_reports(const u32 *report0, const u32 *report1,
 		return;
 	}
 
-	dump_report(report0, set->perf_raw_size, "pec_report0");
-	dump_report(report1, set->perf_raw_size, "pec_report1");
+	dump_report(report0, set->perf_raw_size / 4, "pec_report0");
+	dump_report(report1, set->perf_raw_size / 4, "pec_report1");
 
 	pec_sanity_check(report0, report1, set);
 }
@@ -1471,8 +1471,6 @@ read_2_oa_reports(int format_id,
 				break;
 			}
 
-			dump_report(report, format_size / 4, "oa-formats");
-
 			igt_debug("read report: reason = %x, timestamp = %"PRIx64", exponent mask=%x\n",
 				  report[0], oa_timestamp(report, format_id), exponent_mask);
 
-- 
2.43.0

Re: [PATCH i-g-t 1/4] tests/intel/xe_oa: Fix report dumps in sanity check

[Dixit, Ashutosh]

On Fri, 22 Aug 2025 17:34:03 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> Fix report dumps in sanity check to dump the right amount of dwords.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  tests/intel/xe_oa.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 63b59028fc5d..35d25150ec7d 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -1052,8 +1052,8 @@ static void pec_sanity_check_reports(const u32 *report0, const u32 *report1,
>		return;
>	}
>
> -	dump_report(report0, set->perf_raw_size, "pec_report0");
> -	dump_report(report1, set->perf_raw_size, "pec_report1");
> +	dump_report(report0, set->perf_raw_size / 4, "pec_report0");
> +	dump_report(report1, set->perf_raw_size / 4, "pec_report1");

This is correct.

>
>	pec_sanity_check(report0, report1, set);
>  }
> @@ -1471,8 +1471,6 @@ read_2_oa_reports(int format_id,
>				break;
>			}
>
> -			dump_report(report, format_size / 4, "oa-formats");
> -

Not sure about this. Could you move this to a different patch with
justification, if it's needed? I think this is there to dump Xe1 reports,
which pec_sanity_check_reports() will not dump, though maybe for Xe2 it
double dumps? I'd rather have more data there for debug, rather than data
missing.

>			igt_debug("read report: reason = %x, timestamp = %"PRIx64", exponent mask=%x\n",
>				  report[0], oa_timestamp(report, format_id), exponent_mask);
>
> --
> 2.43.0
>

Thanks.
--
Ashutosh

Re: [PATCH i-g-t 1/4] tests/intel/xe_oa: Fix report dumps in sanity check

[Umesh Nerlige Ramappa]

On Sat, Aug 23, 2025 at 11:25:07AM -0700, Dixit, Ashutosh wrote:
>On Fri, 22 Aug 2025 17:34:03 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> Fix report dumps in sanity check to dump the right amount of dwords.
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  tests/intel/xe_oa.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> index 63b59028fc5d..35d25150ec7d 100644
>> --- a/tests/intel/xe_oa.c
>> +++ b/tests/intel/xe_oa.c
>> @@ -1052,8 +1052,8 @@ static void pec_sanity_check_reports(const u32 *report0, const u32 *report1,
>>		return;
>>	}
>>
>> -	dump_report(report0, set->perf_raw_size, "pec_report0");
>> -	dump_report(report1, set->perf_raw_size, "pec_report1");
>> +	dump_report(report0, set->perf_raw_size / 4, "pec_report0");
>> +	dump_report(report1, set->perf_raw_size / 4, "pec_report1");
>
>This is correct.
>
>>
>>	pec_sanity_check(report0, report1, set);
>>  }
>> @@ -1471,8 +1471,6 @@ read_2_oa_reports(int format_id,
>>				break;
>>			}
>>
>> -			dump_report(report, format_size / 4, "oa-formats");
>> -
>
>Not sure about this. Could you move this to a different patch with
>justification, if it's needed? I think this is there to dump Xe1 reports,
>which pec_sanity_check_reports() will not dump, though maybe for Xe2 it
>double dumps? I'd rather have more data there for debug, rather than data
>missing.

oh, I removed it since I saw double dumps. Let me recheck this one.

Thanks,
Umesh
>
>>			igt_debug("read report: reason = %x, timestamp = %"PRIx64", exponent mask=%x\n",
>>				  report[0], oa_timestamp(report, format_id), exponent_mask);
>>
>> --
>> 2.43.0
>>
>
>Thanks.
>--
>Ashutosh

[PATCH i-g-t 2/4] tests/intel/xe_oa: Fix waiting for mmaped reports

[Umesh Nerlige Ramappa]

When iterating over reports using a uint32_t pointer, ensure that the
report size is expressed in 32 bits.

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 tests/intel/xe_oa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
index 35d25150ec7d..745f364e52e9 100644
--- a/tests/intel/xe_oa.c
+++ b/tests/intel/xe_oa.c
@@ -4448,6 +4448,7 @@ static void mmap_wait_for_periodic_reports(void *oa_vaddr, uint32_t n,
 	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
 	uint64_t fmt = test_set->perf_oa_format;
 	uint32_t num_periodic_reports = 0;
+	uint32_t report_words = get_oa_format(fmt).size >> 2;
 	uint32_t *reports;
 
 	while (num_periodic_reports < n) {
@@ -4455,7 +4456,7 @@ static void mmap_wait_for_periodic_reports(void *oa_vaddr, uint32_t n,
 		num_periodic_reports = 0;
 		for (reports = (uint32_t *)oa_vaddr;
 		     reports[0] && oa_timestamp(reports, fmt) && oa_report_is_periodic(reports);
-		     reports += get_oa_format(fmt).size) {
+		     reports += report_words) {
 			num_periodic_reports++;
 		}
 	}
-- 
2.43.0

Re: [PATCH i-g-t 2/4] tests/intel/xe_oa: Fix waiting for mmaped reports

[Dixit, Ashutosh]

On Fri, 22 Aug 2025 17:34:04 -0700, Umesh Nerlige Ramappa wrote:
>
> When iterating over reports using a uint32_t pointer, ensure that the
> report size is expressed in 32 bits.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

> ---
>  tests/intel/xe_oa.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 35d25150ec7d..745f364e52e9 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -4448,6 +4448,7 @@ static void mmap_wait_for_periodic_reports(void *oa_vaddr, uint32_t n,
>	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
>	uint64_t fmt = test_set->perf_oa_format;
>	uint32_t num_periodic_reports = 0;
> +	uint32_t report_words = get_oa_format(fmt).size >> 2;
>	uint32_t *reports;
>
>	while (num_periodic_reports < n) {
> @@ -4455,7 +4456,7 @@ static void mmap_wait_for_periodic_reports(void *oa_vaddr, uint32_t n,
>		num_periodic_reports = 0;
>		for (reports = (uint32_t *)oa_vaddr;
>		     reports[0] && oa_timestamp(reports, fmt) && oa_report_is_periodic(reports);
> -		     reports += get_oa_format(fmt).size) {
> +		     reports += report_words) {
>			num_periodic_reports++;
>		}
>	}
> --
> 2.43.0
>

[PATCH i-g-t 3/4] tests/intel/xe_oa: Add a test for tail address wrap

[Umesh Nerlige Ramappa]

Add a test to verify that the HW wraps to start of the OA buffer when a
report does not fit in the remaining space in the buffer.

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 tests/intel/xe_oa.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
index 745f364e52e9..7381e7b9bd44 100644
--- a/tests/intel/xe_oa.c
+++ b/tests/intel/xe_oa.c
@@ -4535,6 +4535,76 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta
 	try_invalid_access(vaddr);
 }
 
+/**
+ * SUBTEST: tail-address-wrap
+ * Description: Test tail address wrap on odd format sizes. Ensure that the
+ * format size is not a power of 2. This means that the last report will not be
+ * broken down across the OA buffer end. Instead it will be written to the
+ * beginning of the OA buffer. We will check the end of the buffer to ensure it
+ * has zeroes in it.
+ */
+static void
+test_tail_address_wrap(const struct drm_xe_engine_class_instance *hwe)
+{
+#define TIMER_PERIOD_US 20
+	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
+	uint64_t exponent = max_oa_exponent_for_period_lte(TIMER_PERIOD_US * 1000);
+	uint64_t fmt = test_set->perf_oa_format;
+	uint64_t properties[] = {
+		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
+		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
+		DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set,
+		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(fmt),
+		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, exponent,
+		DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE, hwe->engine_instance,
+		DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE, buffer_fill_size,
+	};
+	struct intel_xe_oa_open_prop param = {
+		.num_properties = ARRAY_SIZE(properties) / 2,
+		.properties_ptr = to_user_pointer(properties),
+	};
+	uint32_t fmt_size = get_oa_format(fmt).size;
+	uint32_t fit_reports = buffer_fill_size / fmt_size;
+	uint32_t wrap_offset = (fit_reports * fmt_size) >> 2;
+	uint32_t end_offset = buffer_fill_size >> 2;
+	uint32_t *report, *zero_area, *buffer_end;
+	void *buffer_start;
+	int i, tolerance = 10;
+
+	igt_require(oau->capabilities & DRM_XE_OA_CAPS_OA_BUFFER_SIZE);
+
+	/* Ensure report does not fit */
+	igt_require(wrap_offset < end_offset);
+
+	/* mmap the buffer */
+	stream_fd = __perf_open(drm_fd, &param, false);
+	buffer_start = mmap(0, buffer_fill_size, PROT_READ, MAP_PRIVATE, stream_fd, 0);
+	igt_assert(buffer_start);
+
+	/* Wait for reports to fill the buffer with some tolerance */
+	report = buffer_start;
+	for (i = 0; fit_reports && i < fit_reports + tolerance; i++) {
+		usleep(TIMER_PERIOD_US);
+		if (*report && oa_timestamp(report, fmt)) {
+			fit_reports--;
+			report += (fmt_size >> 2);
+		}
+	}
+
+	/* Let some reports land in the start of the buffer */
+	usleep(tolerance * TIMER_PERIOD_US);
+
+	zero_area = (uint32_t *)buffer_start + wrap_offset;
+	buffer_end = (uint32_t *)buffer_start + end_offset;
+
+	/* Ensure HW did not write to this area */
+	while (zero_area < buffer_end)
+		igt_assert_eq(*zero_area++, 0);
+
+	munmap(buffer_start, buffer_fill_size);
+	__perf_close(stream_fd);
+}
+
 /**
  * SUBTEST: map-oa-buffer
  * Description: Verify mapping of oa buffer
@@ -5091,6 +5161,10 @@ igt_main
 		igt_subtest_with_dynamic("closed-fd-and-unmapped-access")
 			__for_one_hwe_in_oag(hwe)
 				closed_fd_and_unmapped_access(hwe);
+
+		igt_subtest_with_dynamic("tail-address-wrap")
+			__for_one_hwe_in_oag(hwe)
+				test_tail_address_wrap(hwe);
 	}
 
 	igt_subtest_group {
-- 
2.43.0

Re: [PATCH i-g-t 3/4] tests/intel/xe_oa: Add a test for tail address wrap

[Dixit, Ashutosh]

On Fri, 22 Aug 2025 17:34:05 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

OK good to add such as test, previously tail address wrap was only tested
indirectly, e.g. by report headers being incorrect. But with multiple
problems such as report sizes being wrong for different formats, a specific
test for tail address wrap is good to have.

> Add a test to verify that the HW wraps to start of the OA buffer when a
> report does not fit in the remaining space in the buffer.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  tests/intel/xe_oa.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 745f364e52e9..7381e7b9bd44 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -4535,6 +4535,76 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta
>	try_invalid_access(vaddr);
>  }
>
> +/**
> + * SUBTEST: tail-address-wrap
> + * Description: Test tail address wrap on odd format sizes. Ensure that the
> + * format size is not a power of 2. This means that the last report will not be
> + * broken down across the OA buffer end. Instead it will be written to the
> + * beginning of the OA buffer. We will check the end of the buffer to ensure it
> + * has zeroes in it.
> + */
> +static void
> +test_tail_address_wrap(const struct drm_xe_engine_class_instance *hwe)
> +{
> +#define TIMER_PERIOD_US 20

Let's not call this "TIMER_PERIOD", something like ADDR_WRAP_US etc.

> +	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
> +	uint64_t exponent = max_oa_exponent_for_period_lte(TIMER_PERIOD_US * 1000);
> +	uint64_t fmt = test_set->perf_oa_format;
> +	uint64_t properties[] = {
> +		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
> +		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
> +		DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set,
> +		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(fmt),
> +		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, exponent,
> +		DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE, hwe->engine_instance,
> +		DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE, buffer_fill_size,
> +	};
> +	struct intel_xe_oa_open_prop param = {
> +		.num_properties = ARRAY_SIZE(properties) / 2,
> +		.properties_ptr = to_user_pointer(properties),
> +	};
> +	uint32_t fmt_size = get_oa_format(fmt).size;
> +	uint32_t fit_reports = buffer_fill_size / fmt_size;
> +	uint32_t wrap_offset = (fit_reports * fmt_size) >> 2;
> +	uint32_t end_offset = buffer_fill_size >> 2;
> +	uint32_t *report, *zero_area, *buffer_end;
> +	void *buffer_start;
> +	int i, tolerance = 10;
> +
> +	igt_require(oau->capabilities & DRM_XE_OA_CAPS_OA_BUFFER_SIZE);
> +
> +	/* Ensure report does not fit */
> +	igt_require(wrap_offset < end_offset);
> +
> +	/* mmap the buffer */
> +	stream_fd = __perf_open(drm_fd, &param, false);
> +	buffer_start = mmap(0, buffer_fill_size, PROT_READ, MAP_PRIVATE, stream_fd, 0);
> +	igt_assert(buffer_start);
> +
> +	/* Wait for reports to fill the buffer with some tolerance */
> +	report = buffer_start;
> +	for (i = 0; fit_reports && i < fit_reports + tolerance; i++) {

Since fit_reports is decreasing below and i is increasing, looks like this
loop will exit when OA buffer is half full? Maybe that is why the test is
always passing? Tolerance is not making much sense either since it will
make the report pointer go over the OA buffer? So we need to simplify the
loop.

Maybe we can add an argument to mmap_wait_for_periodic_reports() to also
count all reports, so make the function work for both periodic and
non-periodic reports and use it here?

So wait for the buffer to first fill once and once that happens wait for it
to completely fill again (maybe even more than once). Something like
that. But maybe will need to zero out the buffer for
mmap_wait_for_periodic_reports() which probably is not allowed by the
kernel. So anyway, need some way to figure out if the buffer is filled
again, like read the tail pointer. Or go back to verifying OA packet
headers...

> +		usleep(TIMER_PERIOD_US);
> +		if (*report && oa_timestamp(report, fmt)) {
> +			fit_reports--;
> +			report += (fmt_size >> 2);
> +		}
> +	}
> +
> +	/* Let some reports land in the start of the buffer */
> +	usleep(tolerance * TIMER_PERIOD_US);

My preference is to not use direct usleep's since it breaks in different
environments (sim etc.), see comments above. We've eliminated several such
things in Xe OA IGT's previously iirc.

Maybe separate out this patch so that the other patches can get merged
first?

Thanks.
--
Ashutosh

> +
> +	zero_area = (uint32_t *)buffer_start + wrap_offset;
> +	buffer_end = (uint32_t *)buffer_start + end_offset;
> +
> +	/* Ensure HW did not write to this area */
> +	while (zero_area < buffer_end)
> +		igt_assert_eq(*zero_area++, 0);
> +
> +	munmap(buffer_start, buffer_fill_size);
> +	__perf_close(stream_fd);
> +}
> +
>  /**
>   * SUBTEST: map-oa-buffer
>   * Description: Verify mapping of oa buffer
> @@ -5091,6 +5161,10 @@ igt_main
>		igt_subtest_with_dynamic("closed-fd-and-unmapped-access")
>			__for_one_hwe_in_oag(hwe)
>				closed_fd_and_unmapped_access(hwe);
> +
> +		igt_subtest_with_dynamic("tail-address-wrap")
> +			__for_one_hwe_in_oag(hwe)
> +				test_tail_address_wrap(hwe);
>	}
>
>	igt_subtest_group {
> --
> 2.43.0
>

Re: [PATCH i-g-t 3/4] tests/intel/xe_oa: Add a test for tail address wrap

[Umesh Nerlige Ramappa]

On Mon, Aug 25, 2025 at 11:03:37AM -0700, Dixit, Ashutosh wrote:
>On Fri, 22 Aug 2025 17:34:05 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>OK good to add such as test, previously tail address wrap was only tested
>indirectly, e.g. by report headers being incorrect. But with multiple
>problems such as report sizes being wrong for different formats, a specific
>test for tail address wrap is good to have.
>
>> Add a test to verify that the HW wraps to start of the OA buffer when a
>> report does not fit in the remaining space in the buffer.
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  tests/intel/xe_oa.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>
>> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> index 745f364e52e9..7381e7b9bd44 100644
>> --- a/tests/intel/xe_oa.c
>> +++ b/tests/intel/xe_oa.c
>> @@ -4535,6 +4535,76 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta
>>	try_invalid_access(vaddr);
>>  }
>>
>> +/**
>> + * SUBTEST: tail-address-wrap
>> + * Description: Test tail address wrap on odd format sizes. Ensure that the
>> + * format size is not a power of 2. This means that the last report will not be
>> + * broken down across the OA buffer end. Instead it will be written to the
>> + * beginning of the OA buffer. We will check the end of the buffer to ensure it
>> + * has zeroes in it.
>> + */
>> +static void
>> +test_tail_address_wrap(const struct drm_xe_engine_class_instance *hwe)
>> +{
>> +#define TIMER_PERIOD_US 20
>
>Let's not call this "TIMER_PERIOD", something like ADDR_WRAP_US etc.
>

It's actually OA EXPONENT PERIOD, so I called it timer period. Reports 
are being generated at 20us intervals.

>> +	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
>> +	uint64_t exponent = max_oa_exponent_for_period_lte(TIMER_PERIOD_US * 1000);
>> +	uint64_t fmt = test_set->perf_oa_format;
>> +	uint64_t properties[] = {
>> +		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
>> +		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
>> +		DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set,
>> +		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(fmt),
>> +		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, exponent,
>> +		DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE, hwe->engine_instance,
>> +		DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE, buffer_fill_size,
>> +	};
>> +	struct intel_xe_oa_open_prop param = {
>> +		.num_properties = ARRAY_SIZE(properties) / 2,
>> +		.properties_ptr = to_user_pointer(properties),
>> +	};
>> +	uint32_t fmt_size = get_oa_format(fmt).size;
>> +	uint32_t fit_reports = buffer_fill_size / fmt_size;
>> +	uint32_t wrap_offset = (fit_reports * fmt_size) >> 2;
>> +	uint32_t end_offset = buffer_fill_size >> 2;
>> +	uint32_t *report, *zero_area, *buffer_end;
>> +	void *buffer_start;
>> +	int i, tolerance = 10;
>> +
>> +	igt_require(oau->capabilities & DRM_XE_OA_CAPS_OA_BUFFER_SIZE);
>> +
>> +	/* Ensure report does not fit */
>> +	igt_require(wrap_offset < end_offset);
>> +
>> +	/* mmap the buffer */
>> +	stream_fd = __perf_open(drm_fd, &param, false);
>> +	buffer_start = mmap(0, buffer_fill_size, PROT_READ, MAP_PRIVATE, stream_fd, 0);
>> +	igt_assert(buffer_start);
>> +
>> +	/* Wait for reports to fill the buffer with some tolerance */
>> +	report = buffer_start;
>> +	for (i = 0; fit_reports && i < fit_reports + tolerance; i++) {
>
>Since fit_reports is decreasing below and i is increasing, looks like this
>loop will exit when OA buffer is half full? Maybe that is why the test is
>always passing? Tolerance is not making much sense either since it will
>make the report pointer go over the OA buffer? So we need to simplify the
>loop.

Yikes, bad programming... Cannot use fit_reports that is decrementing in 
the loop check.

Tolerance is intentional. I want the reports to go over the fit_report 
value so that I can ensure that the HW did not write to the region in 
the end of the buffer where the report does not fit. Technically, a 
tolerance of 1 is good enough.

>
>Maybe we can add an argument to mmap_wait_for_periodic_reports() to also
>count all reports, so make the function work for both periodic and
>non-periodic reports and use it here?

Yeah, a possibility.

>
>So wait for the buffer to first fill once and once that happens wait for it
>to completely fill again (maybe even more than once). Something like
>that. But maybe will need to zero out the buffer for
>mmap_wait_for_periodic_reports() which probably is not allowed by the
>kernel.

No, I am just wanting to fill the buffer once and then wait for at least 
one additional report to land in the buffer. However, like you said, 
mmap mode will not clear the buffer header, so we don't know if a new 
report landed. So I am just using a 10 * TIMER_PERIOD_US delay to let 
that happen.

>So anyway, need some way to figure out if the buffer is filled
>again, like read the tail pointer. Or go back to verifying OA packet
>headers...


Maybe this:

We could stop the OA capture, read out the entire buffer + a tolreance 
amount of reports. Ensure all reports are valid, Then we check this 
region to make sure it's all zeroes.

>
>> +		usleep(TIMER_PERIOD_US);
>> +		if (*report && oa_timestamp(report, fmt)) {
>> +			fit_reports--;
>> +			report += (fmt_size >> 2);
>> +		}
>> +	}
>> +
>> +	/* Let some reports land in the start of the buffer */
>> +	usleep(tolerance * TIMER_PERIOD_US);
>
>My preference is to not use direct usleep's since it breaks in different
>environments (sim etc.), see comments above. We've eliminated several such
>things in Xe OA IGT's previously iirc.
>
>Maybe separate out this patch so that the other patches can get merged
>first?

Will post others first, but anyways wanted to get your feedback on this 
and looks like it has a glaring bug.

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh
>
>> +
>> +	zero_area = (uint32_t *)buffer_start + wrap_offset;
>> +	buffer_end = (uint32_t *)buffer_start + end_offset;
>> +
>> +	/* Ensure HW did not write to this area */
>> +	while (zero_area < buffer_end)
>> +		igt_assert_eq(*zero_area++, 0);
>> +
>> +	munmap(buffer_start, buffer_fill_size);
>> +	__perf_close(stream_fd);
>> +}
>> +
>>  /**
>>   * SUBTEST: map-oa-buffer
>>   * Description: Verify mapping of oa buffer
>> @@ -5091,6 +5161,10 @@ igt_main
>>		igt_subtest_with_dynamic("closed-fd-and-unmapped-access")
>>			__for_one_hwe_in_oag(hwe)
>>				closed_fd_and_unmapped_access(hwe);
>> +
>> +		igt_subtest_with_dynamic("tail-address-wrap")
>> +			__for_one_hwe_in_oag(hwe)
>> +				test_tail_address_wrap(hwe);
>>	}
>>
>>	igt_subtest_group {
>> --
>> 2.43.0
>>

Re: [PATCH i-g-t 3/4] tests/intel/xe_oa: Add a test for tail address wrap

[Dixit, Ashutosh]

On Mon, 25 Aug 2025 17:58:07 -0700, Umesh Nerlige Ramappa wrote:
>
> On Mon, Aug 25, 2025 at 11:03:37AM -0700, Dixit, Ashutosh wrote:
> > On Fri, 22 Aug 2025 17:34:05 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >
> > Hi Umesh,
> >
> > OK good to add such as test, previously tail address wrap was only tested
> > indirectly, e.g. by report headers being incorrect. But with multiple
> > problems such as report sizes being wrong for different formats, a specific
> > test for tail address wrap is good to have.
> >
> >> Add a test to verify that the HW wraps to start of the OA buffer when a
> >> report does not fit in the remaining space in the buffer.
> >>
> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> ---
> >>  tests/intel/xe_oa.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 74 insertions(+)
> >>
> >> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> >> index 745f364e52e9..7381e7b9bd44 100644
> >> --- a/tests/intel/xe_oa.c
> >> +++ b/tests/intel/xe_oa.c
> >> @@ -4535,6 +4535,76 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta
> >>	try_invalid_access(vaddr);
> >>  }
> >>
> >> +/**
> >> + * SUBTEST: tail-address-wrap
> >> + * Description: Test tail address wrap on odd format sizes. Ensure that the
> >> + * format size is not a power of 2. This means that the last report will not be
> >> + * broken down across the OA buffer end. Instead it will be written to the
> >> + * beginning of the OA buffer. We will check the end of the buffer to ensure it
> >> + * has zeroes in it.
> >> + */
> >> +static void
> >> +test_tail_address_wrap(const struct drm_xe_engine_class_instance *hwe)
> >> +{
> >> +#define TIMER_PERIOD_US 20
> >
> > Let's not call this "TIMER_PERIOD", something like ADDR_WRAP_US etc.
> >
>
> It's actually OA EXPONENT PERIOD, so I called it timer period. Reports are
> being generated at 20us intervals.
>
> >> +	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
> >> +	uint64_t exponent = max_oa_exponent_for_period_lte(TIMER_PERIOD_US * 1000);
> >> +	uint64_t fmt = test_set->perf_oa_format;
> >> +	uint64_t properties[] = {
> >> +		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
> >> +		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
> >> +		DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set,
> >> +		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(fmt),
> >> +		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, exponent,
> >> +		DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE, hwe->engine_instance,
> >> +		DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE, buffer_fill_size,
> >> +	};
> >> +	struct intel_xe_oa_open_prop param = {
> >> +		.num_properties = ARRAY_SIZE(properties) / 2,
> >> +		.properties_ptr = to_user_pointer(properties),
> >> +	};
> >> +	uint32_t fmt_size = get_oa_format(fmt).size;
> >> +	uint32_t fit_reports = buffer_fill_size / fmt_size;
> >> +	uint32_t wrap_offset = (fit_reports * fmt_size) >> 2;
> >> +	uint32_t end_offset = buffer_fill_size >> 2;
> >> +	uint32_t *report, *zero_area, *buffer_end;
> >> +	void *buffer_start;
> >> +	int i, tolerance = 10;
> >> +
> >> +	igt_require(oau->capabilities & DRM_XE_OA_CAPS_OA_BUFFER_SIZE);
> >> +
> >> +	/* Ensure report does not fit */
> >> +	igt_require(wrap_offset < end_offset);
> >> +
> >> +	/* mmap the buffer */
> >> +	stream_fd = __perf_open(drm_fd, &param, false);
> >> +	buffer_start = mmap(0, buffer_fill_size, PROT_READ, MAP_PRIVATE, stream_fd, 0);
> >> +	igt_assert(buffer_start);
> >> +
> >> +	/* Wait for reports to fill the buffer with some tolerance */
> >> +	report = buffer_start;
> >> +	for (i = 0; fit_reports && i < fit_reports + tolerance; i++) {
> >
> > Since fit_reports is decreasing below and i is increasing, looks like this
> > loop will exit when OA buffer is half full? Maybe that is why the test is
> > always passing? Tolerance is not making much sense either since it will
> > make the report pointer go over the OA buffer? So we need to simplify the
> > loop.
>
> Yikes, bad programming... Cannot use fit_reports that is decrementing in
> the loop check.
>
> Tolerance is intentional. I want the reports to go over the fit_report
> value so that I can ensure that the HW did not write to the region in the
> end of the buffer where the report does not fit. Technically, a tolerance
> of 1 is good enough.
>
> >
> > Maybe we can add an argument to mmap_wait_for_periodic_reports() to also
> > count all reports, so make the function work for both periodic and
> > non-periodic reports and use it here?
>
> Yeah, a possibility.
>
> >
> > So wait for the buffer to first fill once and once that happens wait for it
> > to completely fill again (maybe even more than once). Something like
> > that. But maybe will need to zero out the buffer for
> > mmap_wait_for_periodic_reports() which probably is not allowed by the
> > kernel.
>
> No, I am just wanting to fill the buffer once and then wait for at least
> one additional report to land in the buffer. However, like you said, mmap
> mode will not clear the buffer header, so we don't know if a new report
> landed. So I am just using a 10 * TIMER_PERIOD_US delay to let that happen.
>
> > So anyway, need some way to figure out if the buffer is filled
> > again, like read the tail pointer. Or go back to verifying OA packet
> > headers...
>
>
> Maybe this:
>
> We could stop the OA capture, read out the entire buffer + a tolreance
> amount of reports. Ensure all reports are valid, Then we check this region
> to make sure it's all zeroes.

Yeah good idea. Just read() more than an OA buffer worth of reports and
then mmap the buffer and make sure it contains 0's at the end. Something
like that or variations will work. Thanks!

>
> >
> >> +		usleep(TIMER_PERIOD_US);
> >> +		if (*report && oa_timestamp(report, fmt)) {
> >> +			fit_reports--;
> >> +			report += (fmt_size >> 2);
> >> +		}
> >> +	}
> >> +
> >> +	/* Let some reports land in the start of the buffer */
> >> +	usleep(tolerance * TIMER_PERIOD_US);
> >
> > My preference is to not use direct usleep's since it breaks in different
> > environments (sim etc.), see comments above. We've eliminated several such
> > things in Xe OA IGT's previously iirc.
> >
> > Maybe separate out this patch so that the other patches can get merged
> > first?
>
> Will post others first, but anyways wanted to get your feedback on this and
> looks like it has a glaring bug.
>
> Thanks,
> Umesh
>
> >
> > Thanks.
> > --
> > Ashutosh
> >
> >> +
> >> +	zero_area = (uint32_t *)buffer_start + wrap_offset;
> >> +	buffer_end = (uint32_t *)buffer_start + end_offset;
> >> +
> >> +	/* Ensure HW did not write to this area */
> >> +	while (zero_area < buffer_end)
> >> +		igt_assert_eq(*zero_area++, 0);
> >> +
> >> +	munmap(buffer_start, buffer_fill_size);
> >> +	__perf_close(stream_fd);
> >> +}
> >> +
> >>  /**
> >>   * SUBTEST: map-oa-buffer
> >>   * Description: Verify mapping of oa buffer
> >> @@ -5091,6 +5161,10 @@ igt_main
> >>		igt_subtest_with_dynamic("closed-fd-and-unmapped-access")
> >>			__for_one_hwe_in_oag(hwe)
> >>				closed_fd_and_unmapped_access(hwe);
> >> +
> >> +		igt_subtest_with_dynamic("tail-address-wrap")
> >> +			__for_one_hwe_in_oag(hwe)
> >> +				test_tail_address_wrap(hwe);
> >>	}
> >>
> >>	igt_subtest_group {
> >> --
> >> 2.43.0
> >>

[PATCH i-g-t 4/4] tests/intel/xe_oa: Add an option to capture ftrace

[Umesh Nerlige Ramappa]

Capture ftrace with the --trace option when needed. This is not intended
to run as default, but more of a debug functionality when manually
running individual tests.

In addition configure the ftrace buffer using the --trace_buf_size_mb

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 tests/intel/xe_oa.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
index 7381e7b9bd44..e5473a5ff70f 100644
--- a/tests/intel/xe_oa.c
+++ b/tests/intel/xe_oa.c
@@ -307,6 +307,8 @@ static struct oa_format lnl_oa_formats[XE_OA_FORMAT_MAX] = {
 		.bc_report = 0 },
 };
 
+static bool oa_trace = false;
+static uint32_t oa_trace_buf_mb = 20;
 static int drm_fd = -1;
 static int sysfs = -1;
 static int pm_fd = -1;
@@ -389,6 +391,31 @@ static u32 get_stream_status(int fd)
 	return status.oa_status;
 }
 
+static void enable_trace_log(void)
+{
+	char cmd[64] = {0};
+
+	if (!oa_trace || oa_trace_buf_mb > 20)
+		return;
+
+	snprintf(cmd, sizeof(cmd) - 1, "echo %d > /sys/kernel/debug/tracing/buffer_size_kb", oa_trace_buf_mb * 1000);
+	system(cmd);
+	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
+	system("echo > /sys/kernel/debug/tracing/trace");
+	system("echo 1 > /sys/kernel/debug/tracing/events/xe/enable");
+	system("echo 1 > /sys/kernel/debug/tracing/events/xe/xe_reg_rw/enable");
+	system("echo 1 > /sys/kernel/debug/tracing/tracing_on");
+}
+
+static void disable_trace_log(void)
+{
+	if (!oa_trace || oa_trace_buf_mb > 20)
+		return;
+
+	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
+	system("cat /sys/kernel/debug/tracing/trace");
+}
+
 static void
 dump_report(const uint32_t *report, uint32_t size, const char *message) {
 	uint32_t i;
@@ -4937,7 +4964,34 @@ static const char *xe_engine_class_name(uint32_t engine_class)
 	igt_require_f(hwe, "no render engine\n"); \
 	igt_dynamic_f("rcs-%d", hwe->engine_instance)
 
-igt_main
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	switch (opt) {
+	case 'b':
+		oa_trace_buf_mb = strtoul(optarg, NULL, 0);
+		igt_debug("Trace buffer %d Mb\n", oa_trace_buf_mb);
+		break;
+	case 't':
+		oa_trace = true;
+		igt_debug("Trace enabled\n");
+		break;
+	default:
+		return IGT_OPT_HANDLER_ERROR;
+	}
+
+	return IGT_OPT_HANDLER_SUCCESS;
+}
+
+static const char *help_str =  "  --trace | -t\t\tEnable ftrace\n"
+			       "  --trace_buf_size_mb | -b\t\tSet ftrace buffer size in Mb (default = 0, max = 20)\n";
+
+static struct option long_options[] = {
+	{"trace", 0, 0, 't'},
+	{"trace_buf_size_mb", 0, 0, 'b'},
+	{ NULL, 0, 0, 0 }
+};
+
+igt_main_args("b:t", long_options, help_str, opt_handler, NULL)
 {
 	const struct sync_section {
 		const char *name;
@@ -4981,6 +5035,7 @@ igt_main
 		 */
 		igt_assert_eq(drm_fd, -1);
 
+		enable_trace_log();
 		drm_fd = drm_open_driver(DRIVER_XE);
 		xe_dev = xe_device_get(drm_fd);
 
@@ -5210,5 +5265,6 @@ igt_main
 			intel_xe_perf_free(intel_xe_perf);
 
 		drm_close_driver(drm_fd);
+		disable_trace_log();
 	}
 }
-- 
2.43.0

Re: [PATCH i-g-t 4/4] tests/intel/xe_oa: Add an option to capture ftrace

[Dixit, Ashutosh]

On Fri, 22 Aug 2025 17:34:06 -0700, Umesh Nerlige Ramappa wrote:
>
> Capture ftrace with the --trace option when needed. This is not intended
> to run as default, but more of a debug functionality when manually
> running individual tests.

Maybe mention here or in the commit title that we are capturing register
read/writes in ftrace.

>
> In addition configure the ftrace buffer using the --trace_buf_size_mb
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  tests/intel/xe_oa.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 7381e7b9bd44..e5473a5ff70f 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -307,6 +307,8 @@ static struct oa_format lnl_oa_formats[XE_OA_FORMAT_MAX] = {
>		.bc_report = 0 },
>  };
>
> +static bool oa_trace = false;
> +static uint32_t oa_trace_buf_mb = 20;
>  static int drm_fd = -1;
>  static int sysfs = -1;
>  static int pm_fd = -1;
> @@ -389,6 +391,31 @@ static u32 get_stream_status(int fd)
>	return status.oa_status;
>  }
>
> +static void enable_trace_log(void)
> +{
> +	char cmd[64] = {0};
> +
> +	if (!oa_trace || oa_trace_buf_mb > 20)
> +		return;
> +
> +	snprintf(cmd, sizeof(cmd) - 1, "echo %d > /sys/kernel/debug/tracing/buffer_size_kb", oa_trace_buf_mb * 1000);
> +	system(cmd);
> +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
> +	system("echo > /sys/kernel/debug/tracing/trace");
> +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/enable");
> +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/xe_reg_rw/enable");
> +	system("echo 1 > /sys/kernel/debug/tracing/tracing_on");
> +}
> +
> +static void disable_trace_log(void)
> +{
> +	if (!oa_trace || oa_trace_buf_mb > 20)
> +		return;
> +
> +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
> +	system("cat /sys/kernel/debug/tracing/trace");
> +}
> +
>  static void
>  dump_report(const uint32_t *report, uint32_t size, const char *message) {
>	uint32_t i;
> @@ -4937,7 +4964,34 @@ static const char *xe_engine_class_name(uint32_t engine_class)
>	igt_require_f(hwe, "no render engine\n"); \
>	igt_dynamic_f("rcs-%d", hwe->engine_instance)
>
> -igt_main
> +static int opt_handler(int opt, int opt_index, void *data)
> +{
> +	switch (opt) {
> +	case 'b':
> +		oa_trace_buf_mb = strtoul(optarg, NULL, 0);
> +		igt_debug("Trace buffer %d Mb\n", oa_trace_buf_mb);
> +		break;
> +	case 't':
> +		oa_trace = true;
> +		igt_debug("Trace enabled\n");
> +		break;
> +	default:
> +		return IGT_OPT_HANDLER_ERROR;
> +	}
> +
> +	return IGT_OPT_HANDLER_SUCCESS;
> +}
> +
> +static const char *help_str =  "  --trace | -t\t\tEnable ftrace\n"
> +			       "  --trace_buf_size_mb | -b\t\tSet ftrace buffer size in Mb (default = 0, max = 20)\n";
> +
> +static struct option long_options[] = {
> +	{"trace", 0, 0, 't'},
> +	{"trace_buf_size_mb", 0, 0, 'b'},
> +	{ NULL, 0, 0, 0 }
> +};
> +
> +igt_main_args("b:t", long_options, help_str, opt_handler, NULL)
>  {
>	const struct sync_section {
>		const char *name;
> @@ -4981,6 +5035,7 @@ igt_main
>		 */
>		igt_assert_eq(drm_fd, -1);
>
> +		enable_trace_log();
>		drm_fd = drm_open_driver(DRIVER_XE);
>		xe_dev = xe_device_get(drm_fd);
>
> @@ -5210,5 +5265,6 @@ igt_main
>			intel_xe_perf_free(intel_xe_perf);
>
>		drm_close_driver(drm_fd);
> +		disable_trace_log();

Does this get disabled even if the test crashes? Anyway since it is for
debug and not enabled by default, this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

>	}
>  }
> --
> 2.43.0
>

Re: [PATCH i-g-t 4/4] tests/intel/xe_oa: Add an option to capture ftrace

[Dixit, Ashutosh]

On Sat, 23 Aug 2025 12:01:08 -0700, Dixit, Ashutosh wrote:
>
> On Fri, 22 Aug 2025 17:34:06 -0700, Umesh Nerlige Ramappa wrote:
> >
> > Capture ftrace with the --trace option when needed. This is not intended
> > to run as default, but more of a debug functionality when manually
> > running individual tests.
>
> Maybe mention here or in the commit title that we are capturing register
> read/writes in ftrace.
>
> >
> > In addition configure the ftrace buffer using the --trace_buf_size_mb
> >
> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > ---
> >  tests/intel/xe_oa.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> > index 7381e7b9bd44..e5473a5ff70f 100644
> > --- a/tests/intel/xe_oa.c
> > +++ b/tests/intel/xe_oa.c
> > @@ -307,6 +307,8 @@ static struct oa_format lnl_oa_formats[XE_OA_FORMAT_MAX] = {
> >		.bc_report = 0 },
> >  };
> >
> > +static bool oa_trace = false;
> > +static uint32_t oa_trace_buf_mb = 20;
> >  static int drm_fd = -1;
> >  static int sysfs = -1;
> >  static int pm_fd = -1;
> > @@ -389,6 +391,31 @@ static u32 get_stream_status(int fd)
> >	return status.oa_status;
> >  }
> >
> > +static void enable_trace_log(void)
> > +{
> > +	char cmd[64] = {0};
> > +
> > +	if (!oa_trace || oa_trace_buf_mb > 20)

Actually, why is this oa_trace_buf_mb condition there in enable/disable? If
we don't want the trace buf to be > 20 MB, maybe return an error in
opt_handler?

Also, do we really need this oa_trace_buf_mb option? Maybe just select a
size and hardcode it? Or we need the option because we don't know the
hardcoded value?

Since we are only tracing xe_reg_rw, maybe even the default size is ok? How
many reg r/w's are there for an OA IGT? Maybe not that many? Since the reg
r/w's done by batches won't be captured here anyway, just the direct mmio
r/w's?

When we generate lots of ftrace, I have previously found the default ftrace
buffer size is not sufficient and printf's get dropped. Previously I used
512 KB (the largest size which worked in those old kernels) and that seemed
sufficient. But anyway even larger values are ok.

Anyway, ok to leave as is too. Just some ideas.

> > +		return;
> > +
> > +	snprintf(cmd, sizeof(cmd) - 1, "echo %d > /sys/kernel/debug/tracing/buffer_size_kb", oa_trace_buf_mb * 1000);

Maybe * 1024 instead?

> > +	system(cmd);
> > +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
> > +	system("echo > /sys/kernel/debug/tracing/trace");
> > +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/enable");
> > +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/xe_reg_rw/enable");
> > +	system("echo 1 > /sys/kernel/debug/tracing/tracing_on");
> > +}
> > +
> > +static void disable_trace_log(void)
> > +{
> > +	if (!oa_trace || oa_trace_buf_mb > 20)
> > +		return;
> > +
> > +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
> > +	system("cat /sys/kernel/debug/tracing/trace");
> > +}
> > +
> >  static void
> >  dump_report(const uint32_t *report, uint32_t size, const char *message) {
> >	uint32_t i;
> > @@ -4937,7 +4964,34 @@ static const char *xe_engine_class_name(uint32_t engine_class)
> >	igt_require_f(hwe, "no render engine\n"); \
> >	igt_dynamic_f("rcs-%d", hwe->engine_instance)
> >
> > -igt_main
> > +static int opt_handler(int opt, int opt_index, void *data)
> > +{
> > +	switch (opt) {
> > +	case 'b':
> > +		oa_trace_buf_mb = strtoul(optarg, NULL, 0);
> > +		igt_debug("Trace buffer %d Mb\n", oa_trace_buf_mb);
> > +		break;
> > +	case 't':
> > +		oa_trace = true;
> > +		igt_debug("Trace enabled\n");
> > +		break;
> > +	default:
> > +		return IGT_OPT_HANDLER_ERROR;
> > +	}
> > +
> > +	return IGT_OPT_HANDLER_SUCCESS;
> > +}
> > +
> > +static const char *help_str =  "  --trace | -t\t\tEnable ftrace\n"
> > +			       "  --trace_buf_size_mb | -b\t\tSet ftrace buffer size in Mb (default = 0, max = 20)\n";

s/Mb/MB/ ?

Thanks.
--
Ashutosh

> > +
> > +static struct option long_options[] = {
> > +	{"trace", 0, 0, 't'},
> > +	{"trace_buf_size_mb", 0, 0, 'b'},
> > +	{ NULL, 0, 0, 0 }
> > +};
> > +
> > +igt_main_args("b:t", long_options, help_str, opt_handler, NULL)
> >  {
> >	const struct sync_section {
> >		const char *name;
> > @@ -4981,6 +5035,7 @@ igt_main
> >		 */
> >		igt_assert_eq(drm_fd, -1);
> >
> > +		enable_trace_log();
> >		drm_fd = drm_open_driver(DRIVER_XE);
> >		xe_dev = xe_device_get(drm_fd);
> >
> > @@ -5210,5 +5265,6 @@ igt_main
> >			intel_xe_perf_free(intel_xe_perf);
> >
> >		drm_close_driver(drm_fd);
> > +		disable_trace_log();
>
> Does this get disabled even if the test crashes? Anyway since it is for
> debug and not enabled by default, this is:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> >	}
> >  }
> > --
> > 2.43.0
> >

Re: [PATCH i-g-t 4/4] tests/intel/xe_oa: Add an option to capture ftrace

[Umesh Nerlige Ramappa]

On Mon, Aug 25, 2025 at 10:18:12AM -0700, Dixit, Ashutosh wrote:
>On Sat, 23 Aug 2025 12:01:08 -0700, Dixit, Ashutosh wrote:
>>
>> On Fri, 22 Aug 2025 17:34:06 -0700, Umesh Nerlige Ramappa wrote:
>> >
>> > Capture ftrace with the --trace option when needed. This is not intended
>> > to run as default, but more of a debug functionality when manually
>> > running individual tests.
>>
>> Maybe mention here or in the commit title that we are capturing register
>> read/writes in ftrace.
>>
>> >
>> > In addition configure the ftrace buffer using the --trace_buf_size_mb
>> >
>> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> > ---
>> >  tests/intel/xe_oa.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 57 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> > index 7381e7b9bd44..e5473a5ff70f 100644
>> > --- a/tests/intel/xe_oa.c
>> > +++ b/tests/intel/xe_oa.c
>> > @@ -307,6 +307,8 @@ static struct oa_format lnl_oa_formats[XE_OA_FORMAT_MAX] = {
>> >		.bc_report = 0 },
>> >  };
>> >
>> > +static bool oa_trace = false;
>> > +static uint32_t oa_trace_buf_mb = 20;
>> >  static int drm_fd = -1;
>> >  static int sysfs = -1;
>> >  static int pm_fd = -1;
>> > @@ -389,6 +391,31 @@ static u32 get_stream_status(int fd)
>> >	return status.oa_status;
>> >  }
>> >
>> > +static void enable_trace_log(void)
>> > +{
>> > +	char cmd[64] = {0};
>> > +
>> > +	if (!oa_trace || oa_trace_buf_mb > 20)
>
>Actually, why is this oa_trace_buf_mb condition there in enable/disable? If
>we don't want the trace buf to be > 20 MB, maybe return an error in
>opt_handler?

Ah, right. That's easier to do. will change that. I would still use a 
default buffer size if the user passed a bad value.

>
>Also, do we really need this oa_trace_buf_mb option? Maybe just select a
>size and hardcode it? Or we need the option because we don't know the
>hardcoded value?
>
>Since we are only tracing xe_reg_rw, maybe even the default size is ok? How
>many reg r/w's are there for an OA IGT? Maybe not that many? Since the reg
>r/w's done by batches won't be captured here anyway, just the direct mmio
>r/w's?
>
>When we generate lots of ftrace, I have previously found the default ftrace
>buffer size is not sufficient and printf's get dropped. Previously I used
>512 KB (the largest size which worked in those old kernels) and that seemed
>sufficient. But anyway even larger values are ok.
>
Didn't think much about it though. I have a default of 20MB in my script 
somewhere, so copied it over. Sometimes I am dumping OA report contents 
to ftrace and it fills up quickly, so I find it helpful if I can pass it 
in the command line.

Thanks,
Umesh

>Anyway, ok to leave as is too. Just some ideas.
>
>> > +		return;
>> > +
>> > +	snprintf(cmd, sizeof(cmd) - 1, "echo %d > /sys/kernel/debug/tracing/buffer_size_kb", oa_trace_buf_mb * 1000);
>
>Maybe * 1024 instead?
>
>> > +	system(cmd);
>> > +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
>> > +	system("echo > /sys/kernel/debug/tracing/trace");
>> > +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/enable");
>> > +	system("echo 1 > /sys/kernel/debug/tracing/events/xe/xe_reg_rw/enable");
>> > +	system("echo 1 > /sys/kernel/debug/tracing/tracing_on");
>> > +}
>> > +
>> > +static void disable_trace_log(void)
>> > +{
>> > +	if (!oa_trace || oa_trace_buf_mb > 20)
>> > +		return;
>> > +
>> > +	system("echo 0 > /sys/kernel/debug/tracing/tracing_on");
>> > +	system("cat /sys/kernel/debug/tracing/trace");
>> > +}
>> > +
>> >  static void
>> >  dump_report(const uint32_t *report, uint32_t size, const char *message) {
>> >	uint32_t i;
>> > @@ -4937,7 +4964,34 @@ static const char *xe_engine_class_name(uint32_t engine_class)
>> >	igt_require_f(hwe, "no render engine\n"); \
>> >	igt_dynamic_f("rcs-%d", hwe->engine_instance)
>> >
>> > -igt_main
>> > +static int opt_handler(int opt, int opt_index, void *data)
>> > +{
>> > +	switch (opt) {
>> > +	case 'b':
>> > +		oa_trace_buf_mb = strtoul(optarg, NULL, 0);
>> > +		igt_debug("Trace buffer %d Mb\n", oa_trace_buf_mb);
>> > +		break;
>> > +	case 't':
>> > +		oa_trace = true;
>> > +		igt_debug("Trace enabled\n");
>> > +		break;
>> > +	default:
>> > +		return IGT_OPT_HANDLER_ERROR;
>> > +	}
>> > +
>> > +	return IGT_OPT_HANDLER_SUCCESS;
>> > +}
>> > +
>> > +static const char *help_str =  "  --trace | -t\t\tEnable ftrace\n"
>> > +			       "  --trace_buf_size_mb | -b\t\tSet ftrace buffer size in Mb (default = 0, max = 20)\n";
>
>s/Mb/MB/ ?
>
>Thanks.
>--
>Ashutosh
>
>> > +
>> > +static struct option long_options[] = {
>> > +	{"trace", 0, 0, 't'},
>> > +	{"trace_buf_size_mb", 0, 0, 'b'},
>> > +	{ NULL, 0, 0, 0 }
>> > +};
>> > +
>> > +igt_main_args("b:t", long_options, help_str, opt_handler, NULL)
>> >  {
>> >	const struct sync_section {
>> >		const char *name;
>> > @@ -4981,6 +5035,7 @@ igt_main
>> >		 */
>> >		igt_assert_eq(drm_fd, -1);
>> >
>> > +		enable_trace_log();
>> >		drm_fd = drm_open_driver(DRIVER_XE);
>> >		xe_dev = xe_device_get(drm_fd);
>> >
>> > @@ -5210,5 +5265,6 @@ igt_main
>> >			intel_xe_perf_free(intel_xe_perf);
>> >
>> >		drm_close_driver(drm_fd);
>> > +		disable_trace_log();
>>
>> Does this get disabled even if the test crashes? Anyway since it is for
>> debug and not enabled by default, this is:
>>
>> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>
>> >	}
>> >  }
>> > --
>> > 2.43.0
>> >