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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Thu, Jul 04, 2024 at 12:12:20PM -0700, Dixit, Ashutosh wrote:
>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?

Wasn't sure about that since BSpec and IGT code has a mismatch on 
gt1/gt2 and gt2/gt3 for MTL, so ended up creating a new set of helpers.

>
>> +
>> +#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>

Thanks,
Umesh