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