Re: [PATCH] lib: sync intel PCI ID macros with kernel

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 29 Jan 2025, Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:
> Hi Jani,
> On 2025-01-29 at 11:33:25 +0200, Jani Nikula wrote:
>> On Wed, 29 Jan 2025, "Chauhan, Shekhar" <shekhar.chauhan@intel.com> wrote:
>> > On 1/29/2025 6:01, Matt Atwood wrote:
>> >> lib: sync PCI ID macros with kernel
>> >>
>> >> Sync with kernel commit:
>> >> 16016ade13f6 ("drm/xe/ptl: Update the PTL pci id table")
>> >
>> > Can we have a simpler commit description, something like: "Sync PCI IDs 
>> > of various platforms with the Xe-KMD."
>> >
>> > Reason being: The structural modifications below aren't really a part of 
>> > the kernel commit mentioned.
>> >
>> >>
>> >> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
>> >> ---
>> >>   lib/pciids.h | 73 ++++++++++++++++++++++++++++++++++++++--------------
>> >>   1 file changed, 53 insertions(+), 20 deletions(-)
>> >>
>> >> diff --git a/lib/pciids.h b/lib/pciids.h
>> >> index 7883384ac..4736ea525 100644
>> >> --- a/lib/pciids.h
>> >> +++ b/lib/pciids.h
>> >> @@ -717,37 +717,66 @@
>> >>   	MACRO__(0xA7AB, ## __VA_ARGS__)
>> >>   
>> >>   /* DG2 */
>> >> -#define INTEL_DG2_G10_IDS(MACRO__, ...) \
>> >> -	MACRO__(0x5690, ## __VA_ARGS__), \
>> >> -	MACRO__(0x5691, ## __VA_ARGS__), \
>> >> -	MACRO__(0x5692, ## __VA_ARGS__), \
>> >> +#define INTEL_DG2_G10_D_IDS(MACRO__, ...) \
>> >>   	MACRO__(0x56A0, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56A1, ## __VA_ARGS__), \
>> >> -	MACRO__(0x56A2, ## __VA_ARGS__), \
>> >> +	MACRO__(0x56A2, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G10_E_IDS(MACRO__, ...) \
>> >>   	MACRO__(0x56BE, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56BF, ## __VA_ARGS__)
>> >>   
>> >> -#define INTEL_DG2_G11_IDS(MACRO__, ...) \
>> >> -	MACRO__(0x5693, ## __VA_ARGS__), \
>> >> -	MACRO__(0x5694, ## __VA_ARGS__), \
>> >> -	MACRO__(0x5695, ## __VA_ARGS__), \
>> >> +#define INTEL_DG2_G10_M_IDS(MACRO__, ...) \
>> >> +	MACRO__(0x5690, ## __VA_ARGS__), \
>> >> +	MACRO__(0x5691, ## __VA_ARGS__), \
>> >> +	MACRO__(0x5692, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G10_IDS(MACRO__, ...) \
>> >> +	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G10_E_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G10_M_IDS(MACRO__, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G11_D_IDS(MACRO__, ...) \
>> >>   	MACRO__(0x56A5, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56A6, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56B0, ## __VA_ARGS__), \
>> >> -	MACRO__(0x56B1, ## __VA_ARGS__), \
>> >> +	MACRO__(0x56B1, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G11_E_IDS(MACRO__, ...) \
>> >>   	MACRO__(0x56BA, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56BB, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56BC, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56BD, ## __VA_ARGS__)
>> >>   
>> >> -#define INTEL_DG2_G12_IDS(MACRO__, ...) \
>> >> -	MACRO__(0x5696, ## __VA_ARGS__), \
>> >> -	MACRO__(0x5697, ## __VA_ARGS__), \
>> >> +#define INTEL_DG2_G11_M_IDS(MACRO__, ...) \
>> >> +	MACRO__(0x5693, ## __VA_ARGS__), \
>> >> +	MACRO__(0x5694, ## __VA_ARGS__), \
>> >> +	MACRO__(0x5695, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G11_IDS(MACRO__, ...) \
>> >> +	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G11_E_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G11_M_IDS(MACRO__, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G12_D_IDS(MACRO__, ...) \
>> >>   	MACRO__(0x56A3, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56A4, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56B2, ## __VA_ARGS__), \
>> >>   	MACRO__(0x56B3, ## __VA_ARGS__)
>> >>   
>> >> +#define INTEL_DG2_G12_M_IDS(MACRO__, ...) \
>> >> +	MACRO__(0x5696, ## __VA_ARGS__), \
>> >> +	MACRO__(0x5697, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_G12_IDS(MACRO__, ...) \
>> >> +	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G12_M_IDS(MACRO__, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_DG2_D_IDS(MACRO__, ...) \
>> >> +	INTEL_DG2_G10_D_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G11_D_IDS(MACRO__, ## __VA_ARGS__), \
>> >> +	INTEL_DG2_G12_D_IDS(MACRO__, ## __VA_ARGS__)
>> >> +
>> >>   #define INTEL_DG2_IDS(MACRO__, ...) \
>> >>   	INTEL_DG2_G10_IDS(MACRO__, ## __VA_ARGS__), \
>> >>   	INTEL_DG2_G11_IDS(MACRO__, ## __VA_ARGS__), \
>> > Although the DG2 IDs look clean to me, but mind explaining me why are we 
>> > creating these sub-defines for DG2_D / DG2_E / DG2_M
>> >> @@ -782,9 +811,12 @@
>> >>   	INTEL_ARL_S_IDS(MACRO__, ## __VA_ARGS__)
>> >>   
>> >>   /* MTL */
>> >> -#define INTEL_MTL_IDS(MACRO__, ...) \
>> >> +#define INTEL_MTL_U_IDS(MACRO__, ...) \
>> >>   	MACRO__(0x7D40, ## __VA_ARGS__), \
>> >> -	MACRO__(0x7D45, ## __VA_ARGS__), \
>> >> +	MACRO__(0x7D45, ## __VA_ARGS__)
>> >> +
>> >> +#define INTEL_MTL_IDS(MACRO__, ...) \
>> >> +	INTEL_MTL_U_IDS(MACRO__, ## __VA_ARGS__), \
>> >>   	MACRO__(0x7D55, ## __VA_ARGS__), \
>> >>   	MACRO__(0x7D60, ## __VA_ARGS__), \
>> >>   	MACRO__(0x7DD5, ## __VA_ARGS__)
>> >
>> > Following up on my last comment, if we are creating sub-defines, the 
>> > design isn't followed here in MTL. MTL_IDS extends to MTL_U_IDS and a 
>> > bunch of singletons. Or, if there's a reason behind doing so, please 
>> > help me understand.
>> 
>> This is verbatim copy of the PCI ID file from kernel to IGT. There isn't
>> all that much point in debating the changes here. It's all been done
>> when the changes were made in kernel.
>> 
>> BR,
>> Jani.
>> 
>
> Changes should match given commit SHA1, in this patch they don't
> or am I missing something?

There have been kernel changes to that file in two different upstream
branches. The branches will have different content until they get merged
together in the future. I presume the file was picked up from drm-tip,
in which case there will be no permanent commit id that will match the
contents of the file.

But that's really not what I was commenting on previously.

BR,
Jani.





>
> Regards,
> Kamil
>
>> 
>> >
>> >> @@ -817,19 +849,20 @@
>> >>   	MACRO__(0xE20B, ## __VA_ARGS__), \
>> >>   	MACRO__(0xE20C, ## __VA_ARGS__), \
>> >>   	MACRO__(0xE20D, ## __VA_ARGS__), \
>> >> -	MACRO__(0xE212, ## __VA_ARGS__)
>> >> +	MACRO__(0xE210, ## __VA_ARGS__), \
>> >> +	MACRO__(0xE212, ## __VA_ARGS__), \
>> >> +	MACRO__(0xE215, ## __VA_ARGS__), \
>> >> +	MACRO__(0xE216, ## __VA_ARGS__)
>> >>   
>> >>   /* PTL */
>> >>   #define INTEL_PTL_IDS(MACRO__, ...) \
>> >>   	MACRO__(0xB080, ## __VA_ARGS__), \
>> >>   	MACRO__(0xB081, ## __VA_ARGS__), \
>> >>   	MACRO__(0xB082, ## __VA_ARGS__), \
>> >> +	MACRO__(0xB083, ## __VA_ARGS__), \
>> >> +	MACRO__(0xB08F, ## __VA_ARGS__), \
>> >>   	MACRO__(0xB090, ## __VA_ARGS__), \
>> >> -	MACRO__(0xB091, ## __VA_ARGS__), \
>> >> -	MACRO__(0xB092, ## __VA_ARGS__), \
>> >>   	MACRO__(0xB0A0, ## __VA_ARGS__), \
>> >> -	MACRO__(0xB0A1, ## __VA_ARGS__), \
>> >> -	MACRO__(0xB0A2, ## __VA_ARGS__), \
>> >>   	MACRO__(0xB0B0, ## __VA_ARGS__)
>> >>   
>> >>   #endif /* __PCIIDS_H__ */
>> 
>> -- 
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel