Re: [igt-dev] [PATCH i-g-t 08/31] i915/perf: Treat ticks as 64 bit

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

On Tue, 14 Feb 2023 16:46:25 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> To support 64 bit OA report headers, treat all ticks as 64 bits.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  tests/i915/perf.c | 79 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 25 deletions(-)
>
> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
> index bd3cdb6a..4c7b4e49 100644
> --- a/tests/i915/perf.c
> +++ b/tests/i915/perf.c
> @@ -125,6 +125,7 @@ struct oa_format {
>	int c_off;
>	int n_c;
>	int oa_type;
> +	bool report_hdr_64bit;
>  };
>
>  static struct oa_format hsw_oa_formats[I915_OA_FORMAT_MAX] = {
> @@ -244,7 +245,7 @@ static bool *undefined_a_counters;
>  static uint64_t oa_exp_1_millisec;
>
>  static igt_render_copyfunc_t render_copy = NULL;
> -static uint32_t (*read_report_ticks)(const uint32_t *report,
> +static uint64_t (*read_report_ticks)(const uint32_t *report,
>				     enum drm_i915_oa_format format);
>  static void (*sanity_check_reports)(const uint32_t *oa_report0,
>				    const uint32_t *oa_report1,
> @@ -438,7 +439,7 @@ sysfs_read(enum i915_attr_id id)
>   * C2 corresponds to a clock counter for the Haswell render basic metric set
>   * but it's not included in all of the formats.
>   */
> -static uint32_t
> +static uint64_t
>  hsw_read_report_ticks(const uint32_t *report, enum drm_i915_oa_format format)
>  {
>	uint32_t *c = (uint32_t *)(((uint8_t *)report) + get_oa_format(format).c_off);
> @@ -448,10 +449,41 @@ hsw_read_report_ticks(const uint32_t *report, enum drm_i915_oa_format format)
>	return c[2];
>  }
>
> -static uint32_t
> +static uint64_t
>  gen8_read_report_ticks(const uint32_t *report, enum drm_i915_oa_format format)
>  {
> -	return report[3];
> +
> +	struct oa_format fmt = get_oa_format(format);
> +
> +	return fmt.report_hdr_64bit ? *(uint64_t *)&report[6] : report[3];
> +}
> +
> +/*
> + * t0 is a value sampled before t1. width is number of bits used to represent
> + * t0/t1. Normally t1 is greater than t0. In cases where t1 < t0 use this
> + * helper. Since the size of t1/t0 is already 64 bits, no special handling is
> + * needed for width = 64.
> + */
> +static uint64_t
> +elapsed_delta(uint64_t t1, uint64_t t0, uint32_t width)
> +{
> +	uint32_t max_bits = sizeof(t1) * 8;
> +
> +	igt_assert(width <= max_bits);
> +
> +	if (t1 < t0 && width != max_bits)
> +		return ((1ULL << width) - t0) + t1;
> +
> +	return t1 - t0;

OK, this looks fine due to 2's complement arithmentic.

> +}
> +
> +static uint64_t
> +oa_tick_delta(const uint32_t *report1,
> +	      const uint32_t *report0,
> +	      enum drm_i915_oa_format format)
> +{
> +	return elapsed_delta(read_report_ticks(report1, format),
> +			     read_report_ticks(report0, format), 32);

Why are ticks always considered 32 bits? For 64 bit report headers aren't
ticks 64 bits?

Also, sorry my ignorance, what is the difference between timestamps and
ticks (both are present in the report headers)?

Thanks.
--
Ashutosh