On 13/11/2018 22:28, Jani Nikula wrote:
On Tue, 13 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 13/11/2018 11:40, Jani Nikula wrote:
On Mon, 12 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
