| Message ID | 20210311153415.3024607-3-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate display version numbering and add XE_LPD (version 13) | expand |
On Thu, 11 Mar 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > Although we've long referred to platforms by a single "GEN" number, the > hardware teams have recommended that we stop doing this since the > various component IP blocks are going to start using independent number > schemes with varying cadence. To support this, hardware platforms a bit > down the road are going to start providing MMIO registers that the > driver can read to obtain the "graphics version," "media version," and > "display version" without needing to do a PCI ID -> platform -> version > translation. > > Although our current platforms don't yet expose these registers (and the > next couple we release probably won't have them yet either), the > hardware teams would still like to see us move to this independent > numbering scheme now in preparation. For i915 that means we should try > to eliminate all usage of INTEL_GEN() throughout our code and instead > replace it with separate GRAPHICS_VER(), MEDIA_VER(), and DISPLAY_VER() > constructs in the code. For old platforms, these will all usually give > the same value for each IP block (aside from a few special cases like > GLK which we can no more accurately represent as graphics=9 + > display=10), but future platforms will have more flexibility to bump IP > version numbers independently. > > The next hardware platform we'll be upstreaming (very soon!) will have a > display version of 13 and a graphics version of 12, so let's just the > first step of breaking out DISPLAY_VER(), but leaving the rest of > INTEL_GEN() untouched for now. For now we'll automatically > derive the display version from the platform's INTEL_GEN() value except > in cases where an alternative display version is explicitly provided in > the device info structure. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_pci.c | 2 +- > drivers/gpu/drm/i915/intel_device_info.h | 3 +++ > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4fe90a9782e8..5ec0524d3418 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1247,6 +1247,8 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev) > #define INTEL_GEN(dev_priv) (INTEL_INFO(dev_priv)->gen) > #define INTEL_DEVID(dev_priv) (RUNTIME_INFO(dev_priv)->device_id) > > +#define DISPLAY_VER(i915) INTEL_INFO(i915)->display_ver The value needs to be wrapped in parenthesis. Maybe this should be ->display.version? > + > #define REVID_FOREVER 0xff > #define INTEL_REVID(dev_priv) (to_pci_dev((dev_priv)->drm.dev)->revision) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index a9f24f2bda33..3543611cf0fc 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -36,7 +36,7 @@ > #include "i915_selftest.h" > > #define PLATFORM(x) .platform = (x) > -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), .display_ver = x x needs parenthesis. > > #define I845_PIPE_OFFSETS \ > .pipe_offsets = { \ > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index d44f64b57b7a..3c7db9c690f4 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -162,6 +162,9 @@ struct intel_device_info { > > u8 gen; > u8 gt; /* GT number, 0 if undefined */ > + > + u8 display_ver; > + > intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */ > > enum intel_platform platform;
On Wed, Mar 17, 2021 at 07:45:00PM +0200, Jani Nikula wrote: > On Thu, 11 Mar 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > > Although we've long referred to platforms by a single "GEN" number, the > > hardware teams have recommended that we stop doing this since the > > various component IP blocks are going to start using independent number > > schemes with varying cadence. To support this, hardware platforms a bit > > down the road are going to start providing MMIO registers that the > > driver can read to obtain the "graphics version," "media version," and > > "display version" without needing to do a PCI ID -> platform -> version > > translation. > > > > Although our current platforms don't yet expose these registers (and the > > next couple we release probably won't have them yet either), the > > hardware teams would still like to see us move to this independent > > numbering scheme now in preparation. For i915 that means we should try > > to eliminate all usage of INTEL_GEN() throughout our code and instead > > replace it with separate GRAPHICS_VER(), MEDIA_VER(), and DISPLAY_VER() > > constructs in the code. For old platforms, these will all usually give > > the same value for each IP block (aside from a few special cases like > > GLK which we can no more accurately represent as graphics=9 + > > display=10), but future platforms will have more flexibility to bump IP > > version numbers independently. > > > > The next hardware platform we'll be upstreaming (very soon!) will have a > > display version of 13 and a graphics version of 12, so let's just the > > first step of breaking out DISPLAY_VER(), but leaving the rest of > > INTEL_GEN() untouched for now. For now we'll automatically > > derive the display version from the platform's INTEL_GEN() value except > > in cases where an alternative display version is explicitly provided in > > the device info structure. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > drivers/gpu/drm/i915/i915_pci.c | 2 +- > > drivers/gpu/drm/i915/intel_device_info.h | 3 +++ > > 3 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 4fe90a9782e8..5ec0524d3418 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1247,6 +1247,8 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev) > > #define INTEL_GEN(dev_priv) (INTEL_INFO(dev_priv)->gen) > > #define INTEL_DEVID(dev_priv) (RUNTIME_INFO(dev_priv)->device_id) > > > > +#define DISPLAY_VER(i915) INTEL_INFO(i915)->display_ver > > The value needs to be wrapped in parenthesis. It's not obvious to me what the extra parenthesis on a bunch of these are for. I don't see any obvious order-of-operations problems that they're saving us from? > > Maybe this should be ->display.version? Yeah, I was torn on this. Eventually we're going to have .graphics_ver and .media_ver too so it seems strange to have .display_ver as the special case off in its own sub-structure. But since we haven't added the other two yet, I'll move it down for now and we can figure out whether we want to keep it there or not when the other two version numbers show up in the future. > > > + > > #define REVID_FOREVER 0xff > > #define INTEL_REVID(dev_priv) (to_pci_dev((dev_priv)->drm.dev)->revision) > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > index a9f24f2bda33..3543611cf0fc 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -36,7 +36,7 @@ > > #include "i915_selftest.h" > > > > #define PLATFORM(x) .platform = (x) > > -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), .display_ver = x > > x needs parenthesis. Same question as above...what do the extra parentheses give us (other than consistency with the surrounding code)? Especially since any 'x' passed to this macro is always a simple integer literal. Matt > > > > > #define I845_PIPE_OFFSETS \ > > .pipe_offsets = { \ > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > index d44f64b57b7a..3c7db9c690f4 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -162,6 +162,9 @@ struct intel_device_info { > > > > u8 gen; > > u8 gt; /* GT number, 0 if undefined */ > > + > > + u8 display_ver; > > + > > intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */ > > > > enum intel_platform platform; > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4fe90a9782e8..5ec0524d3418 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1247,6 +1247,8 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev) #define INTEL_GEN(dev_priv) (INTEL_INFO(dev_priv)->gen) #define INTEL_DEVID(dev_priv) (RUNTIME_INFO(dev_priv)->device_id) +#define DISPLAY_VER(i915) INTEL_INFO(i915)->display_ver + #define REVID_FOREVER 0xff #define INTEL_REVID(dev_priv) (to_pci_dev((dev_priv)->drm.dev)->revision) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index a9f24f2bda33..3543611cf0fc 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -36,7 +36,7 @@ #include "i915_selftest.h" #define PLATFORM(x) .platform = (x) -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), .display_ver = x #define I845_PIPE_OFFSETS \ .pipe_offsets = { \ diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index d44f64b57b7a..3c7db9c690f4 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -162,6 +162,9 @@ struct intel_device_info { u8 gen; u8 gt; /* GT number, 0 if undefined */ + + u8 display_ver; + intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */ enum intel_platform platform;
Although we've long referred to platforms by a single "GEN" number, the hardware teams have recommended that we stop doing this since the various component IP blocks are going to start using independent number schemes with varying cadence. To support this, hardware platforms a bit down the road are going to start providing MMIO registers that the driver can read to obtain the "graphics version," "media version," and "display version" without needing to do a PCI ID -> platform -> version translation. Although our current platforms don't yet expose these registers (and the next couple we release probably won't have them yet either), the hardware teams would still like to see us move to this independent numbering scheme now in preparation. For i915 that means we should try to eliminate all usage of INTEL_GEN() throughout our code and instead replace it with separate GRAPHICS_VER(), MEDIA_VER(), and DISPLAY_VER() constructs in the code. For old platforms, these will all usually give the same value for each IP block (aside from a few special cases like GLK which we can no more accurately represent as graphics=9 + display=10), but future platforms will have more flexibility to bump IP version numbers independently. The next hardware platform we'll be upstreaming (very soon!) will have a display version of 13 and a graphics version of 12, so let's just the first step of breaking out DISPLAY_VER(), but leaving the rest of INTEL_GEN() untouched for now. For now we'll automatically derive the display version from the platform's INTEL_GEN() value except in cases where an alternative display version is explicitly provided in the device info structure. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_pci.c | 2 +- drivers/gpu/drm/i915/intel_device_info.h | 3 +++ 3 files changed, 6 insertions(+), 1 deletion(-)