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

ok. I can actually move it inside the function/macro and use an
autogenerated name.

> 
> > +       uint32_t unused0, device;
> > +       uint32_t unused1, unused2;
> > +       uint32_t unused3, unused4;
> These are all uint16_t.

kernel "document" them as uint32_t:

/*
 * A pci_device_id struct {
 *      __u32 vendor, device;
 *      __u32 subvendor, subdevice;
 *      __u32 class, class_mask;
 *      kernel_ulong_t driver_data;
 * };
 * Don't use C99 here because "class" is reserved and we want to
 * give userspace flexibility.


> 
> > +       unsigned long unused5;
> 
> Simply make the unused disappear from the macro.
> 
> > +};
> > +
> > +#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) \

here I would name this DEFINE_IS_GENX, since it's a macro to define
the functions, not to be used in other places.

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

i915_pciids.h is not consistent with how the macros are called. See that
in my patch. So here I'd have:

#define DEFINE_IS_GENX(x, pfx) \
static inline int __is_gen##x(uint16_t devid) \
{ \
	static const struct pci_device_id __ids[] = { INTEL_ ## pfx ## _IDS(0) }; \
	return __find_in_pciid(devid, __ids, sizeof(__ids)/sizeof(__ids[0]); \
}

#define IS_GEN10(devid) IS_GENX(10, CNL, devid)

For gen9 is more complicated as it needs several ids.


> 
> That should help cut down the object size expansion. But longer term I'd

I'm not opposed to turning it into inline function, but if the goal is
to reduce the object size expansion, just making the array static const
should suffice... we do call the macros several times, but most of the
size is for constructing the array, not to find the elements.


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

what do you mean by "friendly names for mesa".

The other option that is actually my preferred is to let the kernel tell
user space what it is.  What do you think about having an ioctl that returns
what is the gen + additional interesting info?  Then user space could
base its decisions on features and fallback to gen version when there
isn't a way to discover if a feature/bug is present.  This would allow
to simply remove the pci ids from user space projects which IMO would be
a good thing.

Lucas De Marchi

> -Chris