[Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

[Tvrtko Ursulin]

On 13/11/2018 22:28, Jani Nikula wrote:
> On Tue, 13 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>> On 13/11/2018 11:40, Jani Nikula wrote:
>>> On Mon, 12 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Introduce subplatform mask to eliminate throughout the code devid checking
>>>> sprinkle, mostly courtesy of IS_*_UL[TX] macros.
>>>>
>>>> Subplatform mask initialization is done at runtime device info init.
>>>
>>> I kind of like the concept, and I like the centralization of devid
>>> checks in one function, but I've always wanted to take this to one step
>>> further: only specify device ids in i915_pciids.h, and *nowhere* else.
>>>
>>> It's perhaps too much duplication to create a device info for all these
>>> variants, but I think it would be possible to make the subplatform info
>>> table driven using macros defined in i915_pciids.h.
>>
>> It would be much nicer, but how would you do it? Perhaps my imagination
>> is just strong enough today.
> 
> So here's an idea.
> 
>>
>> Simply by splitting the id's into subplatform parts, for instance where
>> we have today:
>>
>> #define INTEL_BDW_GT1_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>>           INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
>>           INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>>           INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
>>
>> We'd split to:
>>
>> #define INTEL_BDW_GT1_ULT_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>>           INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>>
>> #define INTEL_BDW_GT1_ULX_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
> 
> So far so good.
> 
>>
>> #define INTEL_BDW_GT1_IDS(info)  \
>>           INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>>           INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>>           INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
> 
> Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
> to the above...
> 
>>
>> Then in i915_pci.c, instead of:
>>
>> 	...
>> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>> 	...
>>
>> We'd have:
>>
>> 	...
>> 	INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
>> 	INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
>> 	INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>> 	...
> 
> ...so you don't need to make this change at all. But that's a minor
> detail.

Indeed, makes the change less intrusive.

>> And a separate table to map the id's to subplatform values.
>>
>> Hmm, but we would probably need to extrac the id's from the
>> INTEL_BDW1_GT_IDS like macros so they can be used in this second site
>> without the info parameter. Something like the trick for device info
>> flags, but can it be made to generate a macro? I think not..
> 
> Are we shy of macro magic? Pfft.
> 
> #undef INTEL_VGA_DEVICE
> #define INTEL_VGA_DEVICE(id, info) (id)
> 
> static const u32 bdw_ult_ids[] = {
> 	INTEL_BDW_GT1_ULT_IDS(0),
> };
> 
> static const u32 bdw_ulx_ids[] = {
> 	INTEL_BDW_GT1_ULX_IDS(0),
> };
> 
> #undef INTEL_VGA_DEVICE
> 
> Now you can add another mapping on top with pointers to similar arrays
> as above and corresponding subplatform bits. Just need to order the code
> to not clobber the real INTEL_VGA_DEVICE needs.
> 
> We don't need to split the ult/ulx tables by platform either if we only
> care about the subplatform ult/ulx here, just need to remember add all
> ult/ulx in corresponding arrays.

Nice and simple, thank you!

I am marking this for when I get round updating the series.

Regards,

Tvrtko