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

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

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.


>
>> @@ -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