[Intel-gfx] [PATCH libdrm 1/4] intel: add IS_GENX() generic macro

[Lucas De Marchi]

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.

more on this:

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