Re: [igt-dev] [PATCH i-g-t 11/31] i915/perf: Move OA format array from stack to heap

[Kamil Konieczny <kamil.konieczny@linux.intel.com>]

Hi Umesh,

On 2023-02-14 at 16:46:28 -0800, Umesh Nerlige Ramappa wrote:
> The OA format size will change based on the specific format used in some
> of the tests. In preparation for that, allocated the space required for
> the format on heap.
> 
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  tests/i915/perf.c | 48 ++++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
> index e9613dc9..add41f7d 100644
> --- a/tests/i915/perf.c
> +++ b/tests/i915/perf.c
> @@ -1950,9 +1950,9 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
>  		uint8_t *buf = calloc(1, buf_size);
>  		int ret, n_timer_reports = 0;
>  		uint32_t matches = 0;
> -		struct {
> -			uint32_t report[64];
> -		} timer_reports[30];
> +#define NUM_TIMER_REPORTS 30
> +		uint32_t *reports = malloc(NUM_TIMER_REPORTS * format_size);

I assume that format_size is size in bytes.

> +		uint32_t *timer_reports = reports;
>  
>  		igt_debug("testing OA exponent %d,"
>  			  " expected ts delta = %"PRIu64" (%"PRIu64"ns/%.2fus/%.2fms)\n",
> @@ -1963,7 +1963,7 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
>  
>  		stream_fd = __perf_open(drm_fd, &param, true /* prevent_pm */);
>  
> -		while (n_timer_reports < ARRAY_SIZE(timer_reports)) {
> +		while (n_timer_reports < NUM_TIMER_REPORTS) {
>  			struct drm_i915_perf_record_header *header;
>  
>  			while ((ret = read(stream_fd, buf, buf_size)) < 0 &&
> @@ -1976,7 +1976,7 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
>  			igt_assert(ret > 0);
>  
>  			for (int offset = 0;
> -			     offset < ret && n_timer_reports < ARRAY_SIZE(timer_reports);
> +			     offset < ret && n_timer_reports < NUM_TIMER_REPORTS;
>  			     offset += header->size) {
>  				uint32_t *report;
>  
> @@ -1998,25 +1998,25 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
>  				if (!oa_report_is_periodic(exponent, report))
>  					continue;
>  
> -				memcpy(timer_reports[n_timer_reports].report, report,
> -				       sizeof(timer_reports[n_timer_reports].report));
> +				memcpy(timer_reports, report, format_size);
>  				n_timer_reports++;
> +				timer_reports += (format_size / 4);
--------------------------------------------------------------- ^
imho better: / sizeof(*timer_reports)

				timer_reports += (format_size / sizeof(*timer_reports));

With or without this,
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
 
>  			}
>  		}
>  
>  		__perf_close(stream_fd);
>  
>  		igt_debug("report%04i ts=%"PRIx64" hw_id=0x%08x\n", 0,
> -			  oa_timestamp(timer_reports[0].report, fmt),
> -			  oa_report_get_ctx_id(timer_reports[0].report));
> +			  oa_timestamp(&reports[0], fmt),
> +			  oa_report_get_ctx_id(&reports[0]));
>  		for (int i = 1; i < n_timer_reports; i++) {
> -			uint64_t delta = oa_timestamp_delta(timer_reports[i].report,
> -							    timer_reports[i - 1].report,
> +			uint64_t delta = oa_timestamp_delta(&reports[i],
> +							    &reports[i - 1],
>  							    fmt);
>  
>  			igt_debug("report%04i ts=%"PRIx64" hw_id=0x%08x delta=%"PRIu64" %s\n", i,
> -				  oa_timestamp(timer_reports[i].report, fmt),
> -				  oa_report_get_ctx_id(timer_reports[i].report),
> +				  oa_timestamp(&reports[i], fmt),
> +				  oa_report_get_ctx_id(&reports[i]),
>  				  delta, expected_report_timing_delta(delta,
>  								      expected_timestamp_delta) ? "" : "******");
>  
> @@ -2033,6 +2033,8 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
>  		 * etc...
>  		 */
>  		igt_assert_lte(n_timer_reports / 2, matches);
> +
> +		free(reports);
>  	}
>  
>  	load_helper_stop();
> @@ -2754,13 +2756,14 @@ test_buffer_fill(const struct intel_execution_engine2 *e)
>  		.properties_ptr = to_user_pointer(properties),
>  	};
>  	struct drm_i915_perf_record_header *header;
> -	int buf_size = 65536 * (256 + sizeof(struct drm_i915_perf_record_header));
> +	size_t report_size = get_oa_format(fmt).size;
> +	int buf_size = 65536 * (report_size + sizeof(struct drm_i915_perf_record_header));
>  	uint8_t *buf = malloc(buf_size);
>  	int len;
>  	size_t oa_buf_size = MAX_OA_BUF_SIZE;
> -	size_t report_size = get_oa_format(fmt).size;
>  	int n_full_oa_reports = oa_buf_size / report_size;
>  	uint64_t fill_duration = n_full_oa_reports * oa_period;
> +	uint32_t *last_periodic_report = malloc(report_size / 4);
>  
>  	igt_assert(fill_duration < 1000000000);
>  
> @@ -2770,7 +2773,6 @@ test_buffer_fill(const struct intel_execution_engine2 *e)
>  		bool overflow_seen;
>  		uint32_t n_periodic_reports;
>  		uint32_t first_timestamp = 0, last_timestamp = 0;
> -		uint32_t last_periodic_report[64];
>  
>  		do_ioctl(stream_fd, I915_PERF_IOCTL_ENABLE, 0);
>  
> @@ -2849,8 +2851,7 @@ test_buffer_fill(const struct intel_execution_engine2 *e)
>  						break;
>  
>  					if (oa_report_is_periodic(oa_exponent, report)) {
> -						memcpy(last_periodic_report, report,
> -						       sizeof(last_periodic_report));
> +						memcpy(last_periodic_report, report, report_size);
>  						n_periodic_reports++;
>  					}
>  					break;
> @@ -2876,6 +2877,7 @@ test_buffer_fill(const struct intel_execution_engine2 *e)
>  			   report_size * n_full_oa_reports * 0.55);
>  	}
>  
> +	free(last_periodic_report);
>  	free(buf);
>  
>  	__perf_close(stream_fd);
> @@ -2990,12 +2992,13 @@ test_enable_disable(const struct intel_execution_engine2 *e)
>  				  (ARRAY_SIZE(properties) / 2) - 2,
>  		.properties_ptr = to_user_pointer(properties),
>  	};
> -	int buf_size = 65536 * (256 + sizeof(struct drm_i915_perf_record_header));
> +	size_t report_size = get_oa_format(fmt).size;
> +	int buf_size = 65536 * (report_size + sizeof(struct drm_i915_perf_record_header));
>  	uint8_t *buf = malloc(buf_size);
>  	size_t oa_buf_size = MAX_OA_BUF_SIZE;
> -	size_t report_size = get_oa_format(fmt).size;
>  	int n_full_oa_reports = oa_buf_size / report_size;
>  	uint64_t fill_duration = n_full_oa_reports * oa_period;
> +	uint32_t *last_periodic_report = malloc(report_size / 4);
>  
>  	load_helper_init();
>  	load_helper_run(HIGH);
> @@ -3007,7 +3010,6 @@ test_enable_disable(const struct intel_execution_engine2 *e)
>  		uint32_t n_periodic_reports;
>  		struct drm_i915_perf_record_header *header;
>  		uint64_t first_timestamp = 0, last_timestamp = 0;
> -		uint32_t last_periodic_report[64];
>  
>  		/* Giving enough time for an overflow might help catch whether
>  		 * the OA unit has been enabled even if the driver might at
> @@ -3069,8 +3071,7 @@ test_enable_disable(const struct intel_execution_engine2 *e)
>  						break;
>  
>  					if (oa_report_is_periodic(oa_exponent, report)) {
> -						memcpy(last_periodic_report, report,
> -						       sizeof(last_periodic_report));
> +						memcpy(last_periodic_report, report, report_size);
>  
>  						/* We want to measure only the
>  						 * periodic reports, ctx-switch
> @@ -3113,6 +3114,7 @@ test_enable_disable(const struct intel_execution_engine2 *e)
>  		igt_assert_eq(errno, EIO);
>  	}
>  
> +	free(last_periodic_report);
>  	free(buf);
>  
>  	__perf_close(stream_fd);
> -- 
> 2.36.1
>