On Sat, Aug 25, 2018 at 10:35:23AM +0100, Chris Wilson wrote:
Quoting Lucas De Marchi (2018-08-25 00:56:46)
diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h index 4a34b7be..8a0e3e76 100644 --- a/intel/intel_chipset.h +++ b/intel/intel_chipset.h @@ -568,6 +568,26 @@
#define IS_GEN11(devid) (IS_ICELAKE_11(devid))
+/* New platforms use kernel pci ids */ +#include "i915_pciids.h"
+struct pci_device_id {
Don't call it pci_device_id, depending on caller that name may already be taken by libpciaccess.
uint32_t unused0, device;uint32_t unused1, unused2;uint32_t unused3, unused4;These are all uint16_t.
I can make the first 2 uint16_t, but not the rest due to the way they are declared in INTEL_VGA_DEVICE: (~0 has int type by default), unused3 and unused4 are clearly not uint16_t
unsigned long unused5;Simply make the unused disappear from the macro.
that would mean defining macro hackery to get hid of them from INTEL_VGA_DEVICE() by using macro recursion, but not worth IMO. If we want to go this route, then I think we should at least use X Macro to define the ids rather than the list we currently have. Something along the lines (in i915_pciids.h):
#define _INTEL_ICL_IDS \ X(0x8A50) \ X(0x8A51) \ X(0x8A5C) \ X(0x8A5D) \ X(0x8A52) \ X(0x8A5A) \ X(0x8A5B) \ X(0x8A71) \ X(0x8A70)
...
#define X(id, info) INTEL_VGA_DEVICE(id, info), #define INTEL_ICL_IDS(info) _INTEL_ICL_IDS #undef X
Then here we would just define another X to transform the list into an array:
#include "i915_pciids.h" ... #define X(id, gen) { id, gen }
struct pci_device { uint16_t id; uint16_t gen; } devices = { _INTEL_ICL_IDS }
We would be screwed if X is defined to something else, but we could change the name.
Lucas De Marchi
+};
+#define IS_GENX(x, devid) ({ \
struct pci_device_id __ids[] = { INTEL_ ## x ## _IDS(0) }; \While that's a neat trick it's instantiating the array for each caller, and it does appear that we repeat a few of the macros.
The best I can offer to keep the change non-invasive (other than just switching to a platform bitmask and filling (devid, gen, platform) from the pci match data on initialisation) is a two pass approach.
static inline int __find_in_pciid(uint16_t devid, const struct pci_device_id *ids, size_t count) { size_t i = 0; /* we should rethink this if we think there are more than 4B of them! */ for (i = 0; i < count; i++) { if (ids[i].device == devid) return 1; } return 0; }
#define __is_genx(x) \ static inline int __is_gen##x(uint16_t devid) \ { \ static const struct pci_device_id __ids[] = { INTEL_ ## x ## _IDS(0) }; \ return __find_in_pciid(devid, __ids, sizeof(__ids)/sizeof(__ids[0]); \ } __is_genx(3); __is_genx(4); __is_genx(5); __is_genx(6); __is_genx(7); __is_genx(8); __is_genx(9); __is_genx(10); __is_genx(11);
#define IS_GENX(x, devid) __is_gen##x(devid)
That should help cut down the object size expansion. But longer term I'd prefer if we moved to towards finding the match data once. Also we need to pull into the canonical header the friendly names for mesa. -Chris