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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
