Re: [Intel-gfx] [PATCH 16/19] drm/i915/perf: Apply Wa_18013179988

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

On Tue, 23 Aug 2022 13:41:52 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> OA reports in the OA buffer contain an OA timestamp field that helps
> user calculate delta between 2 OA reports. The calculation relies on the
> CS timestamp frequency to convert the timestamp value to nanoseconds.
> The CS timestamp frequency is a function of the CTC_SHIFT value in
> RPM_CONFIG0.
>
> In DG2, OA unit assumes that the CTC_SHIFT is 3, instead of using the
> actual value from RPM_CONFIG0. At the user level, this results in an
> error in calculating delta between 2 OA reports since the OA timestamp
> is not shifted in the same manner as CS timestamp.
>
> To resolve this, return actual OA timestamp frequency to the user in
> i915_getparam_ioctl.

Rather than exposing actual OA timestamp frequency to userspace (with the
corresponding uapi change, specially if it's only DG2 and not all future
products) questions about a couple of other options:

Option 1. Can we set CTC_SHIFT in RPM_CONFIG0 to 3, so change GT freq to be the
          same as OA freq :-)

   The HSD seems to mention this:
   Is setting CTC SHIFT to 0b11 on driver init an acceptable W/A?
   Note: Changing the shift setting on live driver may break apps that are
   currently running (including desktop manager).

Option 2. Is it possible to correct the timestamps in OA report headers to
          compensate for the difference between OA and GT frequencies (say when
          copying OA data to userspace)?

	  Though not sure if this is preferable to having userspace do this.

A couple of minor optional nits on that patch below too.

> +u32 i915_perf_oa_timestamp_frequency(struct drm_i915_private *i915)
> +{
> +	/* Wa_18013179988:dg2 */
> +	if (IS_DG2(i915)) {
> +		intel_wakeref_t wakeref;
> +		u32 reg, shift;
> +
> +		with_intel_runtime_pm(to_gt(i915)->uncore->rpm, wakeref)
> +			reg = intel_uncore_read(to_gt(i915)->uncore, RPM_CONFIG0);
> +
> +		shift = (reg & GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK) >>
> +			 GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_SHIFT;

This can be:
		shift = REG_FIELD_GET(GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);

>  static u64 oa_exponent_to_ns(struct i915_perf *perf, int exponent)
>  {
> -	return intel_gt_clock_interval_to_ns(to_gt(perf->i915),
> -					     2ULL << exponent);
> +	u64 nom = (2ULL << exponent) * NSEC_PER_SEC;
> +	u32 den = i915_perf_oa_timestamp_frequency(perf->i915);
> +
> +	return div_u64(nom + den - 1, den);

div_u64_roundup?

Thanks.
--
Ashutosh