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

[Lucas De Marchi <lucas.demarchi@intel.com>]

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