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