Re: [Intel-gfx] [PATCH libdrm v2 1/5] intel: add generic functions to check PCI ID
Quoting Lucas De Marchi (2018-08-31 17:06:01)
> On Fri, Aug 31, 2018 at 09:16:23AM +0100, Chris Wilson wrote:
> > Quoting Lucas De Marchi (2018-08-29 01:35:28)
> > > +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".
That would be similar to how we handle elsewhere. But there's no need to
jump there in one series.
> > 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?
Wouldn't be that hard to add a check_PROGRAMS target with a -DSELFTEST.
It was just an idle thought if you cared to improve the standard of a
stagnant library. It might as well retire with grace ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH libdrm v2 1/5] intel: add generic functions to check PCI ID
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
> > +
> > +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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH libdrm v2 1/5] intel: add generic functions to check PCI ID
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
> +
> +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) || \
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH libdrm v2 1/5] intel: add generic functions to check PCI ID
This will allow platforms to reuse kernel IDs instead of manually
keeping them in sync. In most of the cases we only need to extend
IS_9XX(). Current platforms that fit this requirement can be ported
over to use this macro. Right now it's a nop since it doesn't have any
PCI ID added.
The i915_pciids.h header is in sync with kernel tree on
drm-tip 2018y-08m-20d-21h-41m-11s.
v2: - move to a separate .c so we can have the array in a single
compilation unit
- use a single array for all gens
- add real functions to get or check gen by pciid
- define our own pci device struct rather than inherit the one
kernel uses: we can throw away most of the fields
Cc: Chris Wilson
Signed-off-by: Lucas De Marchi
---
intel/Makefile.sources | 1 +
intel/i915_pciids.h| 461 +
intel/intel_chipset.c | 77 +++
intel/intel_chipset.h | 10 +-
intel/meson.build | 2 +-
5 files changed, 549 insertions(+), 2 deletions(-)
create mode 100644 intel/i915_pciids.h
create mode 100644 intel/intel_chipset.c
diff --git a/intel/Makefile.sources b/intel/Makefile.sources
index 6947ab74..61f43aeb 100644
--- a/intel/Makefile.sources
+++ b/intel/Makefile.sources
@@ -5,6 +5,7 @@ LIBDRM_INTEL_FILES := \
intel_bufmgr_gem.c \
intel_decode.c \
intel_chipset.h \
+ intel_chipset.c \
mm.c \
mm.h \
uthash.h
diff --git a/intel/i915_pciids.h b/intel/i915_pciids.h
new file mode 100644
index ..fd965ffb
--- /dev/null
+++ b/intel/i915_pciids.h
@@ -0,0 +1,461 @@
+/*
+ * Copyright 2013 Intel Corporation
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+#ifndef _I915_PCIIDS_H
+#define _I915_PCIIDS_H
+
+/*
+ * A pci_device_id struct {
+ * __u32 vendor, device;
+ * __u32 subvendor, subdevice;
+ * __u32 class, class_mask;
+ * kernel_ulong_t driver_data;
+ * };
+ * Don't use C99 here because "class" is reserved and we want to
+ * give userspace flexibility.
+ */
+#define INTEL_VGA_DEVICE(id, info) { \
+ 0x8086, id, \
+ ~0, ~0, \
+ 0x03, 0xff, \
+ (unsigned long) info }
+
+#define INTEL_QUANTA_VGA_DEVICE(info) {\
+ 0x8086, 0x16a, \
+ 0x152d, 0x8990, \
+ 0x03, 0xff, \
+ (unsigned long) info }
+
+#define INTEL_I810_IDS(info) \
+ INTEL_VGA_DEVICE(0x7121, info), /* I810 */ \
+ INTEL_VGA_DEVICE(0x7123, info), /* I810_DC100 */\
+ INTEL_VGA_DEVICE(0x7125, info) /* I810_E */
+
+#define INTEL_I815_IDS(info) \
+ INTEL_VGA_DEVICE(0x1132, info) /* I815*/
+
+#define INTEL_I830_IDS(info) \
+ INTEL_VGA_DEVICE(0x3577, info)
+
+#define INTEL_I845G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2562, info)
+
+#define INTEL_I85X_IDS(info) \
+ INTEL_VGA_DEVICE(0x3582, info), /* I855_GM */ \
+ INTEL_VGA_DEVICE(0x358e, info)
+
+#define INTEL_I865G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2572, info) /* I865_G */
+
+#define INTEL_I915G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2582, info), /* I915_G */ \
+ INTEL_VGA_DEVICE(0x258a, info) /* E7221_G */
+
+#define INTEL_I915GM_IDS(info) \
+ INTEL_VGA_DEVICE(0x2592, info) /* I915_GM */
+
+#define INTEL_I945G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2772, info) /* I945_G */
+
+#define INTEL_I945GM_IDS(info) \
+ INTEL_VGA_DEVICE(0x27a2, info), /* I945_GM */ \
+ INTEL_VGA_DEVICE(0x27ae, info) /* I945_GME */
+
