Re: [RFC 6/7] drm/i915: Introduce subplatform concept

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

On Tue, 13 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 13/11/2018 11:40, Jani Nikula wrote:
>> On Mon, 12 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@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.

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


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx