Re: [PATCH i-g-t 10/10] lib: switch i915_pciids_local.h to xe driver style PCI ID macros

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, May 22, 2024 at 08:13:24PM +0300, Jani Nikula wrote:
> On Wed, 22 May 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Wed, May 22, 2024 at 01:35:23PM +0300, Jani Nikula wrote:
> >> Follow-up the kernel i915_pciids.h switching to xe driver style PCI ID
> >> macros, and do the same for i915_pciids_local.h. This is a clear
> >> improvement in the perf code, for example.
> >
> > I'm confused now on why do we need this i915_pciids_local.h o.O
> > but anyway, the change is good and right:
> 
> We'd need to split the MTL macros up kernel side to match IGT perf.c
> needs, even if we don't need that kernel side. But might as well do it
> for Single Point of Truth.

hmmm I see...  yeap, we should probably do this.

> 
> Ditto for PVC, we should use XE_PVC_IDS from xe_pciids.h, but there's a
> diff between those and what's in i915_pciids_local.h. Needs some
> analysis why.

This case is different... PVC IDs are not fully merged upstream.
They are part of a drm-tip's topic branch.

> 
> Anyway, all this can come as follow-up.

Agreed!

> 
> Thanks for the review,
> Jani.
> 
> 
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  lib/i915/perf.c         | 12 +++--------
> >>  lib/i915_pciids_local.h | 44 ++++++++++++++++++++---------------------
> >>  lib/intel_device_info.c |  2 +-
> >>  3 files changed, 26 insertions(+), 32 deletions(-)
> >> 
> >> diff --git a/lib/i915/perf.c b/lib/i915/perf.c
> >> index 4b00ba5de9d4..ee950b3c03e4 100644
> >> --- a/lib/i915/perf.c
> >> +++ b/lib/i915/perf.c
> >> @@ -204,13 +204,10 @@ is_acm_gt3(const struct intel_perf_devinfo *devinfo)
> >>  static bool
> >>  is_mtl_gt2(const struct intel_perf_devinfo *devinfo)
> >>  {
> >> -#undef INTEL_VGA_DEVICE
> >> -#define INTEL_VGA_DEVICE(_id, _info) _id
> >>  	static const uint32_t devids[] = {
> >> -		INTEL_MTL_M_IDS(NULL),
> >> -		INTEL_MTL_P_GT2_IDS(NULL),
> >> +		INTEL_MTL_M_IDS(ID),
> >> +		INTEL_MTL_P_GT2_IDS(ID),
> >>  	};
> >> -#undef INTEL_VGA_DEVICE
> >>  	for (uint32_t i = 0; i < ARRAY_SIZE(devids); i++) {
> >>  		if (devids[i] == devinfo->devid)
> >>  			return true;
> >> @@ -222,12 +219,9 @@ is_mtl_gt2(const struct intel_perf_devinfo *devinfo)
> >>  static bool
> >>  is_mtl_gt3(const struct intel_perf_devinfo *devinfo)
> >>  {
> >> -#undef INTEL_VGA_DEVICE
> >> -#define INTEL_VGA_DEVICE(_id, _info) _id
> >>  	static const uint32_t devids[] = {
> >> -		INTEL_MTL_P_GT3_IDS(NULL),
> >> +		INTEL_MTL_P_GT3_IDS(ID),
> >>  	};
> >> -#undef INTEL_VGA_DEVICE
> >>  	for (uint32_t i = 0; i < ARRAY_SIZE(devids); i++) {
> >>  		if (devids[i] == devinfo->devid)
> >>  			return true;
> >> diff --git a/lib/i915_pciids_local.h b/lib/i915_pciids_local.h
> >> index 0043b0cd9b34..92879704aa8e 100644
> >> --- a/lib/i915_pciids_local.h
> >> +++ b/lib/i915_pciids_local.h
> >> @@ -9,41 +9,41 @@
> >>  
> >>  /* MTL perf */
> >>  #ifndef INTEL_MTL_M_IDS
> >> -#define INTEL_MTL_M_IDS(info) \
> >> -	INTEL_VGA_DEVICE(0x7D60, info), \
> >> -	INTEL_VGA_DEVICE(0x7D67, info)
> >> +#define INTEL_MTL_M_IDS(MACRO__, ...) \
> >> +	MACRO__(0x7D60, ## __VA_ARGS__), \
> >> +	MACRO__(0x7D67, ## __VA_ARGS__)
> >>  #endif
> >>  
> >>  #ifndef INTEL_MTL_P_GT2_IDS
> >> -#define INTEL_MTL_P_GT2_IDS(info) \
> >> -	INTEL_VGA_DEVICE(0x7D45, info)
> >> +#define INTEL_MTL_P_GT2_IDS(MACRO__, ...) \
> >> +	MACRO__(0x7D45, ## __VA_ARGS__)
> >>  #endif
> >>  
> >>  #ifndef INTEL_MTL_P_GT3_IDS
> >> -#define INTEL_MTL_P_GT3_IDS(info) \
> >> -	INTEL_VGA_DEVICE(0x7D55, info), \
> >> -	INTEL_VGA_DEVICE(0x7DD5, info)
> >> +#define INTEL_MTL_P_GT3_IDS(MACRO__, ...) \
> >> +	MACRO__(0x7D55, ## __VA_ARGS__), \
> >> +	MACRO__(0x7DD5, ## __VA_ARGS__)
> >>  #endif
> >>  
> >>  #ifndef INTEL_MTL_P_IDS
> >> -#define INTEL_MTL_P_IDS(info) \
> >> -	INTEL_MTL_P_GT2_IDS(info), \
> >> -	INTEL_MTL_P_GT3_IDS(info)
> >> +#define INTEL_MTL_P_IDS(MACRO__, ...) \
> >> +	INTEL_MTL_P_GT2_IDS(MACRO__, ## __VA_ARGS__), \
> >> +	INTEL_MTL_P_GT3_IDS(MACRO__, ## __VA_ARGS__)
> >>  #endif
> >>  
> >>  /* PVC */
> >>  #ifndef INTEL_PVC_IDS
> >> -#define INTEL_PVC_IDS(info) \
> >> -	INTEL_VGA_DEVICE(0x0BD0, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD1, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD2, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD5, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD6, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD7, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD8, info), \
> >> -	INTEL_VGA_DEVICE(0x0BD9, info), \
> >> -	INTEL_VGA_DEVICE(0x0BDA, info), \
> >> -	INTEL_VGA_DEVICE(0x0BDB, info)
> >> +#define INTEL_PVC_IDS(MACRO__, ...) \
> >> +	MACRO__(0x0BD0, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD1, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD2, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD5, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD6, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD7, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD8, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BD9, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BDA, ## __VA_ARGS__), \
> >> +	MACRO__(0x0BDB, ## __VA_ARGS__)
> >>  #endif
> >>  
> >>  #endif /* _I915_PCIIDS_LOCAL_H */
> >> diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c
> >> index 30aca2abd7be..e80ea54707de 100644
> >> --- a/lib/intel_device_info.c
> >> +++ b/lib/intel_device_info.c
> >> @@ -617,7 +617,7 @@ static const struct pci_id_match intel_device_match[] = {
> >>  
> >>  	INTEL_MTL_IDS(INTEL_VGA_DEVICE, &intel_meteorlake_info),
> >>  
> >> -	INTEL_PVC_IDS(&intel_pontevecchio_info),
> >> +	INTEL_PVC_IDS(INTEL_VGA_DEVICE, &intel_pontevecchio_info),
> >>  
> >>  	XE_LNL_IDS(INTEL_VGA_DEVICE, &intel_lunarlake_info),
> >>  
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel