[Intel-gfx] [PATCH libdrm v2 1/5] intel: add generic functions to check PCI ID

[Chris Wilson]

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.

> +};
> +
> +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?

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

> 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

>  #define IS_9XX(dev)            (IS_GEN3(dev) || \
>                                  IS_GEN4(dev) || \
>                                  IS_GEN5(dev) || \