Re: [PATCH i-g-t] lib/i915/perf: Add ARL support in IGT perf library

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

On Wed, 03 Jul 2024 15:44:05 -0700, Umesh Nerlige Ramappa wrote:
>
> Add ARL PCI ids for ARL in IGT perf library.
>
> Resolves: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11356
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  lib/i915/perf.c         | 32 ++++++++++++++++++++++++++++++++
>  lib/i915_pciids_local.h | 18 ++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/lib/i915/perf.c b/lib/i915/perf.c
> index ee950b3c0..ef2f74be8 100644
> --- a/lib/i915/perf.c
> +++ b/lib/i915/perf.c
> @@ -230,6 +230,34 @@ is_mtl_gt3(const struct intel_perf_devinfo *devinfo)
>	return false;
>  }
>
> +static bool
> +is_arl_gt1(const struct intel_perf_devinfo *devinfo)
> +{
> +	static const uint32_t devids[] = {
> +		INTEL_ARL_GT1_IDS(ID),
> +	};
> +	for (uint32_t i = 0; i < ARRAY_SIZE(devids); i++) {
> +		if (devids[i] == devinfo->devid)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool
> +is_arl_gt2(const struct intel_perf_devinfo *devinfo)
> +{
> +	static const uint32_t devids[] = {
> +		INTEL_ARL_GT2_IDS(ID),
> +	};
> +	for (uint32_t i = 0; i < ARRAY_SIZE(devids); i++) {
> +		if (devids[i] == devinfo->devid)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  struct intel_perf *
>  intel_perf_for_devinfo(uint32_t device_id,
>		       uint32_t revision,
> @@ -432,6 +460,10 @@ intel_perf_for_devinfo(uint32_t device_id,
>			intel_perf_load_metrics_mtlgt2(perf);
>		else if (is_mtl_gt3(&perf->devinfo))
>			intel_perf_load_metrics_mtlgt3(perf);
> +		else if (is_arl_gt1(&perf->devinfo))
> +			intel_perf_load_metrics_mtlgt2(perf);
> +		else if (is_arl_gt2(&perf->devinfo))
> +			intel_perf_load_metrics_mtlgt3(perf);

Bspec 55420 says MTL has gt1 and gt2 instead of gt2 and gt3 here?


>		else
>			return unsupported_i915_perf_platform(perf);
>	} else {
> diff --git a/lib/i915_pciids_local.h b/lib/i915_pciids_local.h
> index 92879704a..c404a51af 100644
> --- a/lib/i915_pciids_local.h
> +++ b/lib/i915_pciids_local.h
> @@ -31,6 +31,24 @@
>	INTEL_MTL_P_GT3_IDS(MACRO__, ## __VA_ARGS__)
>  #endif
>
> +#ifndef INTEL_ARL_GT1_IDS
> +#define INTEL_ARL_GT1_IDS(MACRO__, ...) \
> +	MACRO__(0x7D41, ## __VA_ARGS__), \
> +	MACRO__(0x7D67, ## __VA_ARGS__)
> +#endif
> +
> +#ifndef INTEL_ARL_GT2_IDS
> +#define INTEL_ARL_GT2_IDS(MACRO__, ...) \
> +	MACRO__(0x7D51, ## __VA_ARGS__), \
> +	MACRO__(0x7DD1, ## __VA_ARGS__)
> +#endif

OK, these match Bspec 55420 for ARL GT1 and GT2.

Since ARL is not distinguished from MTL in the kernel, another idea would
be add ARL ID's to MTL ID's directly, but maybe maintaining separate ID's
is better?

> +
> +#ifndef INTEL_ARL_IDS
> +#define INTEL_ARL_IDS(MACRO__, ...) \
> +	INTEL_ARL_GT1_IDS(MACRO__, ## __VA_ARGS__), \
> +	INTEL_ARL_GT2_IDS(MACRO__, ## __VA_ARGS__)
> +#endif
> +
>  /* PVC */
>  #ifndef INTEL_PVC_IDS
>  #define INTEL_PVC_IDS(MACRO__, ...) \
> --
> 2.38.1
>

In any case, LGTM:

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