| Message ID | 1422540820-10954-1-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 29, 2015 at 02:13:38PM +0000, Damien Lespiau wrote: > We need to have a separate GT3 struct intel_device_info to declare they > have a second VCS. Let's start by splitting the PCI ids per-GT. > Would it be a good idea to do more programmatic population of these fields, rather than creating an entire new instance of the struct just to alter one field? This relates to our other conversation about the memory consumed by the 30+ device infos and the concern when adding new fields. -Jeff
On Thu, 29 Jan 2015, Jeff McGee <jeff.mcgee@intel.com> wrote: > On Thu, Jan 29, 2015 at 02:13:38PM +0000, Damien Lespiau wrote: >> We need to have a separate GT3 struct intel_device_info to declare they >> have a second VCS. Let's start by splitting the PCI ids per-GT. >> > Would it be a good idea to do more programmatic population of > these fields, rather than creating an entire new instance of the > struct just to alter one field? This relates to our other > conversation about the memory consumed by the 30+ device infos > and the concern when adding new fields. From a debugging perspective, I do like the way it is. You can look at or search the info structs and you know which platforms have what, no thinking involved. On a related note, I'm contemplating sending a patch to obliterate the _INTEL_BDW_M and _INTEL_BDW_D macros from i915_pciids.h because it hides the IDs from a simple grep. See how I try to optimize space and time resources - of my brain! BR, Jani.
On Fri, Jan 30, 2015 at 09:30:07AM +0200, Jani Nikula wrote: > On Thu, 29 Jan 2015, Jeff McGee <jeff.mcgee@intel.com> wrote: > > On Thu, Jan 29, 2015 at 02:13:38PM +0000, Damien Lespiau wrote: > >> We need to have a separate GT3 struct intel_device_info to declare they > >> have a second VCS. Let's start by splitting the PCI ids per-GT. > >> > > Would it be a good idea to do more programmatic population of > > these fields, rather than creating an entire new instance of the > > struct just to alter one field? This relates to our other > > conversation about the memory consumed by the 30+ device infos > > and the concern when adding new fields. > > From a debugging perspective, I do like the way it is. You can look at > or search the info structs and you know which platforms have what, no > thinking involved. > > On a related note, I'm contemplating sending a patch to obliterate the > _INTEL_BDW_M and _INTEL_BDW_D macros from i915_pciids.h because it hides > the IDs from a simple grep. > > See how I try to optimize space and time resources - of my brain! I think I agree after considering this more. At least in this case. -Jeff
On Thu, Jan 29, 2015 at 02:11:13PM -0600, Jeff McGee wrote: > On Thu, Jan 29, 2015 at 02:13:38PM +0000, Damien Lespiau wrote: > > We need to have a separate GT3 struct intel_device_info to declare they > > have a second VCS. Let's start by splitting the PCI ids per-GT. > > > Would it be a good idea to do more programmatic population of > these fields, rather than creating an entire new instance of the > struct just to alter one field? This relates to our other > conversation about the memory consumed by the 30+ device infos > and the concern when adding new fields. Broken record maintainer blurb: Please provide hard data when justifying memory savings in the kmd. I see a lot of people throwing around ideas and it seems a general concern, but like with everything else we need to strike a cost/benefit balance and opt for the most efficient way to save these bytes. Which usually means shrinking down one of the runtime structs we allocate by the thousands. -Daniel
On Fri, Jan 30, 2015 at 09:30:07AM +0200, Jani Nikula wrote: > On a related note, I'm contemplating sending a patch to obliterate the > _INTEL_BDW_M and _INTEL_BDW_D macros from i915_pciids.h because it hides > the IDs from a simple grep. Yes, please!
On Thu, Jan 29, 2015 at 6:13 AM, Damien Lespiau <damien.lespiau@intel.com> wrote: > We need to have a separate GT3 struct intel_device_info to declare they > have a second VCS. Let's start by splitting the PCI ids per-GT. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > include/drm/i915_pciids.h | 28 +++++++++++++++++++--------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > index 180ad0e..38a7c80 100644 > --- a/include/drm/i915_pciids.h > +++ b/include/drm/i915_pciids.h > @@ -259,21 +259,31 @@ > INTEL_VGA_DEVICE(0x22b2, info), \ > INTEL_VGA_DEVICE(0x22b3, info) > > -#define INTEL_SKL_IDS(info) \ > - INTEL_VGA_DEVICE(0x1916, info), /* ULT GT2 */ \ > +#define INTEL_SKL_GT1_IDS(info) \ > INTEL_VGA_DEVICE(0x1906, info), /* ULT GT1 */ \ > - INTEL_VGA_DEVICE(0x1926, info), /* ULT GT3 */ \ > - INTEL_VGA_DEVICE(0x1921, info), /* ULT GT2F */ \ > INTEL_VGA_DEVICE(0x190E, info), /* ULX GT1 */ \ > + INTEL_VGA_DEVICE(0x1902, info), /* DT GT1 */ \ spec shows this id as GT2 DT > + INTEL_VGA_DEVICE(0x190B, info), /* Halo GT1 */ \ > + INTEL_VGA_DEVICE(0x190A, info) /* SRV GT1 */ couldn't find those 2 on spec > + > +#define INTEL_SKL_GT2_IDS(info) \ > + INTEL_VGA_DEVICE(0x1916, info), /* ULT GT2 */ \ > + INTEL_VGA_DEVICE(0x1921, info), /* ULT GT2F */ \ > INTEL_VGA_DEVICE(0x191E, info), /* ULX GT2 */ \ > INTEL_VGA_DEVICE(0x1912, info), /* DT GT2 */ \ > - INTEL_VGA_DEVICE(0x1902, info), /* DT GT1 */ \ > INTEL_VGA_DEVICE(0x191B, info), /* Halo GT2 */ \ > - INTEL_VGA_DEVICE(0x192B, info), /* Halo GT3 */ \ > - INTEL_VGA_DEVICE(0x190B, info), /* Halo GT1 */ \ > INTEL_VGA_DEVICE(0x191A, info), /* SRV GT2 */ \ couldn't find this on spec > - INTEL_VGA_DEVICE(0x192A, info), /* SRV GT3 */ \ > - INTEL_VGA_DEVICE(0x190A, info), /* SRV GT1 */ \ > INTEL_VGA_DEVICE(0x191D, info) /* WKS GT2 */ > > +#define INTEL_SKL_GT3_IDS(info) \ > + INTEL_VGA_DEVICE(0x1926, info), /* ULT GT3 */ \ > + INTEL_VGA_DEVICE(0x192B, info), /* Halo GT3 */ \ > + INTEL_VGA_DEVICE(0x192A, info) /* SRV GT3 */ \ couldn't find this on spec > + > +#define INTEL_SKL_IDS(info) \ > + INTEL_SKL_GT1_IDS(info), \ > + INTEL_SKL_GT2_IDS(info), \ > + INTEL_SKL_GT3_IDS(info) > + > + > #endif /* _I915_PCIIDS_H */ > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx Also I've seem some ids there that aren't here... I know this patch doesn't introduce the those IDs I couldn't fine so with 0x1902 fixed on v2 or on follow-up or explained consider this one here: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
On Tue, Feb 03, 2015 at 05:51:29PM -0800, Rodrigo Vivi wrote: > On Thu, Jan 29, 2015 at 6:13 AM, Damien Lespiau > <damien.lespiau@intel.com> wrote: > > We need to have a separate GT3 struct intel_device_info to declare they > > have a second VCS. Let's start by splitting the PCI ids per-GT. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> FWIW, I'd rather not mix mechanical changes with the ones actually changing something, so will do semantic changes in separate patches.
On Tue, Feb 03, 2015 at 05:51:29PM -0800, Rodrigo Vivi wrote: > On Thu, Jan 29, 2015 at 6:13 AM, Damien Lespiau > <damien.lespiau@intel.com> wrote: > > We need to have a separate GT3 struct intel_device_info to declare they > > have a second VCS. Let's start by splitting the PCI ids per-GT. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > include/drm/i915_pciids.h | 28 +++++++++++++++++++--------- > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > > index 180ad0e..38a7c80 100644 > > --- a/include/drm/i915_pciids.h > > +++ b/include/drm/i915_pciids.h > > @@ -259,21 +259,31 @@ > > INTEL_VGA_DEVICE(0x22b2, info), \ > > INTEL_VGA_DEVICE(0x22b3, info) > > > > -#define INTEL_SKL_IDS(info) \ > > - INTEL_VGA_DEVICE(0x1916, info), /* ULT GT2 */ \ > > +#define INTEL_SKL_GT1_IDS(info) \ > > INTEL_VGA_DEVICE(0x1906, info), /* ULT GT1 */ \ > > - INTEL_VGA_DEVICE(0x1926, info), /* ULT GT3 */ \ > > - INTEL_VGA_DEVICE(0x1921, info), /* ULT GT2F */ \ > > INTEL_VGA_DEVICE(0x190E, info), /* ULX GT1 */ \ > > + INTEL_VGA_DEVICE(0x1902, info), /* DT GT1 */ \ > > spec shows this id as GT2 DT That is weird, for the other ids, 0 << 4 means GT1, while GT2 are 1 << 4. Those ids have gone through review once, so 0x1902 was clearly marked as GT1 then. Could be an error in BSpec. will ask. > > > + INTEL_VGA_DEVICE(0x190B, info), /* Halo GT1 */ \ > > + INTEL_VGA_DEVICE(0x190A, info) /* SRV GT1 */ > > couldn't find those 2 on spec For these and the rest of those, I'd rather keep them in tree as they may stil be pre-production/early-adopters parts. > Also I've seem some ids there that aren't here... This is a known thing and on "purpose". > I know this patch doesn't introduce the those IDs I couldn't fine > so with 0x1902 fixed on v2 or on follow-up or explained consider this one here: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Considering the above I think we should go ahead with this patch.
On Wed, Feb 4, 2015 at 5:10 AM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Tue, Feb 03, 2015 at 05:51:29PM -0800, Rodrigo Vivi wrote: >> On Thu, Jan 29, 2015 at 6:13 AM, Damien Lespiau >> <damien.lespiau@intel.com> wrote: >> > We need to have a separate GT3 struct intel_device_info to declare they >> > have a second VCS. Let's start by splitting the PCI ids per-GT. >> > >> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >> > --- >> > include/drm/i915_pciids.h | 28 +++++++++++++++++++--------- >> > 1 file changed, 19 insertions(+), 9 deletions(-) >> > >> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h >> > index 180ad0e..38a7c80 100644 >> > --- a/include/drm/i915_pciids.h >> > +++ b/include/drm/i915_pciids.h >> > @@ -259,21 +259,31 @@ >> > INTEL_VGA_DEVICE(0x22b2, info), \ >> > INTEL_VGA_DEVICE(0x22b3, info) >> > >> > -#define INTEL_SKL_IDS(info) \ >> > - INTEL_VGA_DEVICE(0x1916, info), /* ULT GT2 */ \ >> > +#define INTEL_SKL_GT1_IDS(info) \ >> > INTEL_VGA_DEVICE(0x1906, info), /* ULT GT1 */ \ >> > - INTEL_VGA_DEVICE(0x1926, info), /* ULT GT3 */ \ >> > - INTEL_VGA_DEVICE(0x1921, info), /* ULT GT2F */ \ >> > INTEL_VGA_DEVICE(0x190E, info), /* ULX GT1 */ \ >> > + INTEL_VGA_DEVICE(0x1902, info), /* DT GT1 */ \ >> >> spec shows this id as GT2 DT > > That is weird, for the other ids, 0 << 4 means GT1, while GT2 are 1 << 4. > > Those ids have gone through review once, so 0x1902 was clearly marked as > GT1 then. Could be an error in BSpec. will ask. > >> >> > + INTEL_VGA_DEVICE(0x190B, info), /* Halo GT1 */ \ >> > + INTEL_VGA_DEVICE(0x190A, info) /* SRV GT1 */ >> >> couldn't find those 2 on spec > > For these and the rest of those, I'd rather keep them in tree as they > may stil be pre-production/early-adopters parts. > >> Also I've seem some ids there that aren't here... > > This is a known thing and on "purpose". > >> I know this patch doesn't introduce the those IDs I couldn't fine >> so with 0x1902 fixed on v2 or on follow-up or explained consider this one here: >> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Considering the above I think we should go ahead with this patch. Agree! > > -- > Damien
diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h index 180ad0e..38a7c80 100644 --- a/include/drm/i915_pciids.h +++ b/include/drm/i915_pciids.h @@ -259,21 +259,31 @@ INTEL_VGA_DEVICE(0x22b2, info), \ INTEL_VGA_DEVICE(0x22b3, info) -#define INTEL_SKL_IDS(info) \ - INTEL_VGA_DEVICE(0x1916, info), /* ULT GT2 */ \ +#define INTEL_SKL_GT1_IDS(info) \ INTEL_VGA_DEVICE(0x1906, info), /* ULT GT1 */ \ - INTEL_VGA_DEVICE(0x1926, info), /* ULT GT3 */ \ - INTEL_VGA_DEVICE(0x1921, info), /* ULT GT2F */ \ INTEL_VGA_DEVICE(0x190E, info), /* ULX GT1 */ \ + INTEL_VGA_DEVICE(0x1902, info), /* DT GT1 */ \ + INTEL_VGA_DEVICE(0x190B, info), /* Halo GT1 */ \ + INTEL_VGA_DEVICE(0x190A, info) /* SRV GT1 */ + +#define INTEL_SKL_GT2_IDS(info) \ + INTEL_VGA_DEVICE(0x1916, info), /* ULT GT2 */ \ + INTEL_VGA_DEVICE(0x1921, info), /* ULT GT2F */ \ INTEL_VGA_DEVICE(0x191E, info), /* ULX GT2 */ \ INTEL_VGA_DEVICE(0x1912, info), /* DT GT2 */ \ - INTEL_VGA_DEVICE(0x1902, info), /* DT GT1 */ \ INTEL_VGA_DEVICE(0x191B, info), /* Halo GT2 */ \ - INTEL_VGA_DEVICE(0x192B, info), /* Halo GT3 */ \ - INTEL_VGA_DEVICE(0x190B, info), /* Halo GT1 */ \ INTEL_VGA_DEVICE(0x191A, info), /* SRV GT2 */ \ - INTEL_VGA_DEVICE(0x192A, info), /* SRV GT3 */ \ - INTEL_VGA_DEVICE(0x190A, info), /* SRV GT1 */ \ INTEL_VGA_DEVICE(0x191D, info) /* WKS GT2 */ +#define INTEL_SKL_GT3_IDS(info) \ + INTEL_VGA_DEVICE(0x1926, info), /* ULT GT3 */ \ + INTEL_VGA_DEVICE(0x192B, info), /* Halo GT3 */ \ + INTEL_VGA_DEVICE(0x192A, info) /* SRV GT3 */ \ + +#define INTEL_SKL_IDS(info) \ + INTEL_SKL_GT1_IDS(info), \ + INTEL_SKL_GT2_IDS(info), \ + INTEL_SKL_GT3_IDS(info) + + #endif /* _I915_PCIIDS_H */
We need to have a separate GT3 struct intel_device_info to declare they have a second VCS. Let's start by splitting the PCI ids per-GT. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- include/drm/i915_pciids.h | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)