On Fri, Aug 31, 2018 at 09:16:23AM +0100, Chris Wilson wrote: > Quoting Lucas De Marchi (2018-08-29 01:35:28) > > +static const struct pci_device { > > + uint16_t device; > > + uint16_t gen; > > +} pciids[] = { > > Add a comment here as well for the ordering requirement. > > /* Keep ids sorted by gen; latest gen first */ > > We're unlikely to notice a comment in the function later trying to > impose its restriction. ok > > > +}; > > + > > +bool intel_is_genx(unsigned int devid, int gen) > > +{ > > + const struct pci_device *p, > > + *pend = pciids + sizeof(pciids) / sizeof(pciids[0]); > > + > > + for (p = pciids; p < pend; p++) { > > + /* PCI IDs are sorted */ > > + if (p->gen < gen) > > + break; > > If we have lots of gen with lots of subids, a binary search for gen > would be sensible. However, do we need this function? Do we not just > convert everyone over to a lookup of pci-id on entry? in some places we need the single IS_GEN9(). The advantage of using this function rather than intel_get_genx() is that it can be faster due to stopping here, or doing a binary search as you pointed out. With intel_get_genx we don't have this. IS_GEN9() is may be called in non-initialization code paths, so IMO its worth. What we *can* do here instead is: guarantee all codepaths will occur after the call to drm_intel_bufmgr_gem_init() then remove all macros and just implement a single function that checks the "cached value". > > > + > > + if (p->device != devid) > > + continue; > > + > > + if (gen == p->gen) > > + return true; > > + > > + break; > > + } > > + > > + return false; > > +} > > + > > +bool intel_get_genx(unsigned int devid, int *gen) > > +{ > > + const struct pci_device *p, > > + *pend = pciids + sizeof(pciids) / sizeof(pciids[0]); > > + > > + for (p = pciids; p < pend; p++) { > > + if (p->device != devid) > > + continue; > > + > > + if (gen) > > + *gen = p->gen; > > + > > + return true; > > + } > > + > > + return false; > > +} > > Idle thought > #ifdef SELFTEST > int main(void) > { > /* check pci-ids are ordered by gen */ > } > #endif $ git grep SELFTEST $ you do know this is a patch for libdrm, right? > > > diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h > > index 4a34b7be..0e14c58f 100644 > > --- a/intel/intel_chipset.h > > +++ b/intel/intel_chipset.h > > @@ -568,6 +568,13 @@ > > > > #define IS_GEN11(devid) (IS_ICELAKE_11(devid)) > > > > +/* New platforms use kernel pci ids */ > > +#include <stdbool.h> > > + > > +bool intel_is_genx(unsigned int devid, int gen); > > +bool intel_get_genx(unsigned int devid, int *gen); > > + > > +/* all platforms */ > > Quite clearly not all platforms :-p by some definition of "all".... the " New platforms use kernel pci ids " + the ones that don't ;) I'm ok with just removing the comment Lucas De Marchi > > > #define IS_9XX(dev) (IS_GEN3(dev) || \ > > IS_GEN4(dev) || \ > > IS_GEN5(dev) || \ _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx