| Message ID | 20190326074057.27833-5-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Device id consolidation | expand |
On Tue, 26 Mar 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Concept of a sub-platform already exist in our code (like ULX and ULT > platform variants and similar),implemented via the macros which check a > list of device ids to determine a match. > > With this patch we consolidate device ids checking into a single function > called during early driver load. > > A few low bits in the platform mask are reserved for sub-platform > identification and defined as a per-platform namespace. > > At the same time it future proofs the platform_mask handling by preparing > the code for easy extending, and tidies the very verbose WARN strings > generated when IS_PLATFORM macros are embedded into a WARN type > statements. > > v2: Fixed IS_SUBPLATFORM. Updated commit msg. > v3: Chris was right, there is an ordering problem. > > v4: > * Catch-up with new sub-platforms. > * Rebase for RUNTIME_INFO. > * Drop subplatform mask union tricks and convert platform_mask to an > array for extensibility. > > v5: > * Fix subplatform check. > * Protect against forgetting to expand subplatform bits. > * Remove platform enum tallying. > * Add subplatform to error state. (Chris) > * Drop macros and just use static inlines. > * Remove redundant IRONLAKE_M. (Ville) > > v6: > * Split out Ironlake change. > * Optimize subplatform check. > * Use __always_inline. (Lucas) > * Add platform_mask comment. (Paulo) > * Pass stored runtime info in error capture. (Chris) > > v7: > * Rebased for new AML ULX device id. > * Bump platform mask array size for EHL. > * Stop mentioning device ids in intel_device_subplatform_init by using > the trick of splitting macros i915_pciids.h. (Jani) > * AML seems to be either a subplatform of KBL or CFL so express it like > that. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Jose Souza <jose.souza@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v6 > --- > drivers/gpu/drm/i915/i915_drv.c | 8 +- > drivers/gpu/drm/i915/i915_drv.h | 124 +++++++++++++------ > drivers/gpu/drm/i915/i915_gpu_error.c | 3 + > drivers/gpu/drm/i915/i915_pci.c | 2 +- > drivers/gpu/drm/i915/intel_device_info.c | 145 +++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_device_info.h | 27 ++++- > 6 files changed, 267 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 5465b99b4392..74255374cc6b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -868,6 +868,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > if (i915_inject_load_failure()) > return -ENODEV; > > + intel_device_info_subplatform_init(dev_priv); > + > spin_lock_init(&dev_priv->irq_lock); > spin_lock_init(&dev_priv->gpu_error.lock); > mutex_init(&dev_priv->backlight_lock); > @@ -1718,10 +1720,12 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) > if (drm_debug & DRM_UT_DRIVER) { > struct drm_printer p = drm_debug_printer("i915 device info:"); > > - drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s gen=%i\n", > + drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (subplatform=0x%x) gen=%i\n", > INTEL_DEVID(dev_priv), > INTEL_REVID(dev_priv), > intel_platform_name(INTEL_INFO(dev_priv)->platform), > + intel_subplatform(RUNTIME_INFO(dev_priv), > + INTEL_INFO(dev_priv)->platform), > INTEL_GEN(dev_priv)); > > intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p); > @@ -1764,8 +1768,6 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent) > memcpy(device_info, match_info, sizeof(*device_info)); > RUNTIME_INFO(i915)->device_id = pdev->device; > > - BUILD_BUG_ON(INTEL_MAX_PLATFORMS > > - BITS_PER_TYPE(device_info->platform_mask)); > BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask)); > > return i915; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 48c3b139f36f..c29ebfd94065 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2298,7 +2298,68 @@ static inline unsigned int i915_sg_segment_size(void) > #define IS_REVID(p, since, until) \ > (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) > > -#define IS_PLATFORM(dev_priv, p) (INTEL_INFO(dev_priv)->platform_mask & BIT(p)) > +static __always_inline unsigned int > +__platform_mask_index(const struct intel_runtime_info *info, > + enum intel_platform p) > +{ > + const unsigned int pbits = > + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; > + > + /* Expand the platform_mask array if this fails. */ > + BUILD_BUG_ON(INTEL_MAX_PLATFORMS > > + pbits * ARRAY_SIZE(info->platform_mask)); > + > + return p / pbits; > +} > + > +static __always_inline unsigned int > +__platform_mask_bit(const struct intel_runtime_info *info, > + enum intel_platform p) > +{ > + const unsigned int pbits = > + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; > + > + return p % pbits + INTEL_SUBPLATFORM_BITS; > +} > + > +static inline u32 > +intel_subplatform(const struct intel_runtime_info *info, > + enum intel_platform p) > +{ > + const unsigned int pi = __platform_mask_index(info, p); > + > + return info->platform_mask[pi] & INTEL_SUBPLATFORM_BITS; > +} > + > +static __always_inline bool > +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p) > +{ > + const struct intel_runtime_info *info = RUNTIME_INFO(i915); > + const unsigned int pi = __platform_mask_index(info, p); > + const unsigned int pb = __platform_mask_bit(info, p); > + > + BUILD_BUG_ON(!__builtin_constant_p(p)); > + > + return info->platform_mask[pi] & BIT(pb); > +} > + > +static __always_inline bool > +IS_SUBPLATFORM(const struct drm_i915_private *i915, > + enum intel_platform p, unsigned int s) > +{ > + const struct intel_runtime_info *info = RUNTIME_INFO(i915); > + const unsigned int pi = __platform_mask_index(info, p); > + const unsigned int pb = __platform_mask_bit(info, p); > + const unsigned int msb = BITS_PER_TYPE(info->platform_mask[0]) - 1; > + const u32 mask = info->platform_mask[pi]; > + > + BUILD_BUG_ON(!__builtin_constant_p(p)); > + BUILD_BUG_ON(!__builtin_constant_p(s)); > + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); > + > + /* Shift and test on the MSB position so sign flag can be used. */ > + return ((mask << (msb - pb)) & (mask << (msb - s))) & BIT(msb); > +} > > #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile) > > @@ -2337,43 +2398,32 @@ static inline unsigned int i915_sg_segment_size(void) > #define IS_ELKHARTLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) > #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \ > (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00) > -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \ > - ((INTEL_DEVID(dev_priv) & 0xf) == 0x6 || \ > - (INTEL_DEVID(dev_priv) & 0xf) == 0xb || \ > - (INTEL_DEVID(dev_priv) & 0xf) == 0xe)) > -/* ULX machines are also considered ULT. */ > -#define IS_BDW_ULX(dev_priv) (IS_BROADWELL(dev_priv) && \ > - (INTEL_DEVID(dev_priv) & 0xf) == 0xe) > +#define IS_BDW_ULT(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULT) > +#define IS_BDW_ULX(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULX) > #define IS_BDW_GT3(dev_priv) (IS_BROADWELL(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 3) > -#define IS_HSW_ULT(dev_priv) (IS_HASWELL(dev_priv) && \ > - (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00) > +#define IS_HSW_ULT(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULT) > #define IS_HSW_GT3(dev_priv) (IS_HASWELL(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 3) > #define IS_HSW_GT1(dev_priv) (IS_HASWELL(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 1) > /* ULX machines are also considered ULT. */ > -#define IS_HSW_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x0A0E || \ > - INTEL_DEVID(dev_priv) == 0x0A1E) > -#define IS_SKL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x1906 || \ > - INTEL_DEVID(dev_priv) == 0x1913 || \ > - INTEL_DEVID(dev_priv) == 0x1916 || \ > - INTEL_DEVID(dev_priv) == 0x1921 || \ > - INTEL_DEVID(dev_priv) == 0x1926) > -#define IS_SKL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x190E || \ > - INTEL_DEVID(dev_priv) == 0x1915 || \ > - INTEL_DEVID(dev_priv) == 0x191E) > -#define IS_KBL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x5906 || \ > - INTEL_DEVID(dev_priv) == 0x5913 || \ > - INTEL_DEVID(dev_priv) == 0x5916 || \ > - INTEL_DEVID(dev_priv) == 0x5921 || \ > - INTEL_DEVID(dev_priv) == 0x5926) > -#define IS_KBL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x590E || \ > - INTEL_DEVID(dev_priv) == 0x5915 || \ > - INTEL_DEVID(dev_priv) == 0x591E) > -#define IS_AML_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x591C || \ > - INTEL_DEVID(dev_priv) == 0x87C0 || \ > - INTEL_DEVID(dev_priv) == 0x87CA) > +#define IS_HSW_ULX(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULX) > +#define IS_SKL_ULT(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULT) > +#define IS_SKL_ULX(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULX) > +#define IS_KBL_ULT(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULT) > +#define IS_KBL_ULX(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULX) > +#define IS_AML_ULX(dev_priv) \ > + (IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_AML) || \ > + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_AML)) > #define IS_SKL_GT2(dev_priv) (IS_SKYLAKE(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 2) > #define IS_SKL_GT3(dev_priv) (IS_SKYLAKE(dev_priv) && \ > @@ -2384,16 +2434,16 @@ static inline unsigned int i915_sg_segment_size(void) > INTEL_INFO(dev_priv)->gt == 2) > #define IS_KBL_GT3(dev_priv) (IS_KABYLAKE(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 3) > -#define IS_CFL_ULT(dev_priv) (IS_COFFEELAKE(dev_priv) && \ > - (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0) > +#define IS_CFL_ULT(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_ULT) > #define IS_CFL_GT2(dev_priv) (IS_COFFEELAKE(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 2) > #define IS_CFL_GT3(dev_priv) (IS_COFFEELAKE(dev_priv) && \ > INTEL_INFO(dev_priv)->gt == 3) > -#define IS_CNL_WITH_PORT_F(dev_priv) (IS_CANNONLAKE(dev_priv) && \ > - (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004) > -#define IS_ICL_WITH_PORT_F(dev_priv) (IS_ICELAKE(dev_priv) && \ > - INTEL_DEVID(dev_priv) != 0x8A51) > +#define IS_CNL_WITH_PORT_F(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, INTEL_SUBPLATFORM_PORTF) > +#define IS_ICL_WITH_PORT_F(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_ICELAKE, INTEL_SUBPLATFORM_PORTF) > > #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index a9557f92756f..0c980b899056 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -677,6 +677,9 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m, > err_printf(m, "Reset count: %u\n", error->reset_count); > err_printf(m, "Suspend count: %u\n", error->suspend_count); > err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform)); > + err_printf(m, "Subplatform: 0x%x\n", > + intel_subplatform(&error->runtime_info, > + error->device_info.platform)); > err_print_pciid(m, m->i915); > > err_printf(m, "IOMMU enabled?: %d\n", error->iommu); > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index 716f2f95c57d..39251586349a 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -32,7 +32,7 @@ > #include "i915_globals.h" > #include "i915_selftest.h" > > -#define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) > +#define PLATFORM(x) .platform = (x) > #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > > #define I845_PIPE_OFFSETS \ > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index e0ac908bb4e9..85da87e14c02 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -714,6 +714,151 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) > return 0; > } > > +#undef INTEL_VGA_DEVICE > +#define INTEL_VGA_DEVICE(id, info) (id) > + > +static const u16 hsw_ult_ids[] = { > + INTEL_HSW_ULT_GT1_IDS(0), > + INTEL_HSW_ULT_GT2_IDS(0), > + INTEL_HSW_ULT_GT3_IDS(0) > +}; > + > +static const u16 hsw_ulx_ids[] = { > + INTEL_HSW_ULX_GT1_IDS(0), > + INTEL_HSW_ULX_GT2_IDS(0) > +}; > + > +static const u16 bdw_ult_ids[] = { > + INTEL_BDW_ULT_GT1_IDS(0), > + INTEL_BDW_ULT_GT2_IDS(0), > + INTEL_BDW_ULT_GT3_IDS(0), > + INTEL_BDW_ULT_RSVD_IDS(0) > +}; > + > +static const u16 bdw_ulx_ids[] = { > + INTEL_BDW_ULX_GT1_IDS(0), > + INTEL_BDW_ULX_GT2_IDS(0), > + INTEL_BDW_ULX_GT3_IDS(0), > + INTEL_BDW_ULX_RSVD_IDS(0) > +}; > + > +static const u16 skl_ult_ids[] = { > + INTEL_SKL_ULT_GT1_IDS(0), > + INTEL_SKL_ULT_GT2_IDS(0), > + INTEL_SKL_ULT_GT3_IDS(0) > +}; > + > +static const u16 skl_ulx_ids[] = { > + INTEL_SKL_ULX_GT1_IDS(0), > + INTEL_SKL_ULX_GT2_IDS(0) > +}; > + > +static const u16 kbl_ult_ids[] = { > + INTEL_KBL_ULT_GT1_IDS(0), > + INTEL_KBL_ULT_GT2_IDS(0), > + INTEL_KBL_ULT_GT3_IDS(0) > +}; > + > +static const u16 kbl_ulx_ids[] = { > + INTEL_KBL_ULX_GT1_IDS(0), > + INTEL_KBL_ULX_GT2_IDS(0) > +}; > + > +static const u16 kbl_aml_ids[] = { > + INTEL_AML_KBL_GT2_IDS(0) > +}; > + > +static const u16 cfl_ult_ids[] = { > + INTEL_CFL_U_GT2_IDS(0), > + INTEL_CFL_U_GT3_IDS(0), > + INTEL_WHL_U_GT1_IDS(0), > + INTEL_WHL_U_GT2_IDS(0), > + INTEL_WHL_U_GT3_IDS(0) > +}; > + > +static const u16 cfl_aml_ids[] = { > + INTEL_AML_CFL_GT2_IDS(0) > +}; > + > +static const u16 cnl_portf_ids[] = { > + INTEL_CNL_PORT_F_IDS(0) > +}; > + > +static const u16 icl_portf_ids[] = { > + INTEL_ICL_PORT_F_IDS(0) > +}; What's the benefit of having platform split in the arrays and an if ladder in the function below? I think I'd go with just the feature arrays. BR, Jani. > + > +static bool find_devid(u16 id, const u16 *p, unsigned int num) > +{ > + for (; num; num--, p++) { > + if (*p == id) > + return true; > + } > + > + return false; > +} > + > +void intel_device_info_subplatform_init(struct drm_i915_private *i915) > +{ > + const struct intel_device_info *info = INTEL_INFO(i915); > + const struct intel_runtime_info *rinfo = RUNTIME_INFO(i915); > + const unsigned int pi = __platform_mask_index(rinfo, info->platform); > + const unsigned int pb = __platform_mask_bit(rinfo, info->platform); > + u16 devid = INTEL_DEVID(i915); > + u32 mask = 0; > + > + RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb); > + > + if (IS_HASWELL(i915)) { > + if (find_devid(devid, hsw_ult_ids, ARRAY_SIZE(hsw_ult_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULT); > + else if (find_devid(devid, hsw_ulx_ids, > + ARRAY_SIZE(hsw_ulx_ids))) > + /* ULX machines are also considered ULT. */ > + mask = BIT(INTEL_SUBPLATFORM_ULT) | > + BIT(INTEL_SUBPLATFORM_ULX); > + } else if (IS_BROADWELL(i915)) { > + if (find_devid(devid, bdw_ult_ids, ARRAY_SIZE(bdw_ult_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULT); > + else if (find_devid(devid, bdw_ulx_ids, > + ARRAY_SIZE(bdw_ulx_ids))) > + /* ULX machines are also considered ULT. */ > + mask = BIT(INTEL_SUBPLATFORM_ULT) | > + BIT(INTEL_SUBPLATFORM_ULX); > + } else if (IS_SKYLAKE(i915)) { > + if (find_devid(devid, skl_ult_ids, ARRAY_SIZE(skl_ult_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULT); > + else if (find_devid(devid, skl_ulx_ids, > + ARRAY_SIZE(skl_ulx_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULX); > + } else if (IS_KABYLAKE(i915)) { > + if (find_devid(devid, kbl_ult_ids, ARRAY_SIZE(kbl_ult_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULT); > + else if (find_devid(devid, kbl_ulx_ids, > + ARRAY_SIZE(kbl_ulx_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULX); > + else if (find_devid(devid, kbl_aml_ids, > + ARRAY_SIZE(kbl_aml_ids))) > + mask = BIT(INTEL_SUBPLATFORM_AML); > + } else if (IS_COFFEELAKE(i915)) { > + if (find_devid(devid, cfl_ult_ids, ARRAY_SIZE(cfl_ult_ids))) > + mask = BIT(INTEL_SUBPLATFORM_ULT); > + else if (find_devid(devid, cfl_aml_ids, > + ARRAY_SIZE(cfl_aml_ids))) > + mask = BIT(INTEL_SUBPLATFORM_AML); > + } else if (IS_CANNONLAKE(i915)) { > + if (find_devid(devid, cnl_portf_ids, ARRAY_SIZE(cnl_portf_ids))) > + mask = BIT(INTEL_SUBPLATFORM_PORTF); > + } else if (IS_ICELAKE(i915)) { > + if (find_devid(devid, icl_portf_ids, ARRAY_SIZE(icl_portf_ids))) > + mask = BIT(INTEL_SUBPLATFORM_PORTF); > + } > + > + GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS); > + > + RUNTIME_INFO(i915)->platform_mask[pi] |= mask; > +} > + > /** > * intel_device_info_runtime_init - initialize runtime info > * @dev_priv: the i915 device > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 98acefaacec9..da1152eef46b 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -77,6 +77,21 @@ enum intel_platform { > INTEL_MAX_PLATFORMS > }; > > +/* > + * Subplatform bits share the same namespace per parent platform. In other words > + * it is fine for the same bit to be used on multiple parent platforms. > + */ > + > +#define INTEL_SUBPLATFORM_BITS (3) > + > +/* HSW/BDW/SKL/KBL/CFL */ > +#define INTEL_SUBPLATFORM_ULT (0) > +#define INTEL_SUBPLATFORM_ULX (1) > +#define INTEL_SUBPLATFORM_AML (2) > + > +/* CNL/ICL */ > +#define INTEL_SUBPLATFORM_PORTF (0) > + > enum intel_ppgtt_type { > INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE, > INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING, > @@ -160,7 +175,6 @@ struct intel_device_info { > intel_engine_mask_t engine_mask; /* Engines supported by the HW */ > > enum intel_platform platform; > - u32 platform_mask; > > enum intel_ppgtt_type ppgtt_type; > unsigned int ppgtt_size; /* log2, e.g. 31/32/48 bits */ > @@ -197,6 +211,16 @@ struct intel_device_info { > }; > > struct intel_runtime_info { > + /* > + * Platform mask is used for optimizing or-ed IS_PLATFORM calls into > + * into single runtime conditionals, and also to provide groundwork > + * for future per platform, or per SKU build optimizations. > + * > + * Array can be extended when necessary if the corresponding > + * BUILD_BUG_ON is hit. > + */ > + u32 platform_mask[2]; > + > u16 device_id; > > u8 num_sprites[I915_MAX_PIPES]; > @@ -271,6 +295,7 @@ static inline void sseu_set_eus(struct sseu_dev_info *sseu, > > const char *intel_platform_name(enum intel_platform platform); > > +void intel_device_info_subplatform_init(struct drm_i915_private *dev_priv); > void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); > void intel_device_info_dump_flags(const struct intel_device_info *info, > struct drm_printer *p);
On Tue, 26 Mar 2019, Jani Nikula <jani.nikula@intel.com> wrote: > On Tue, 26 Mar 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Concept of a sub-platform already exist in our code (like ULX and ULT >> platform variants and similar),implemented via the macros which check a >> list of device ids to determine a match. >> >> With this patch we consolidate device ids checking into a single function >> called during early driver load. >> >> A few low bits in the platform mask are reserved for sub-platform >> identification and defined as a per-platform namespace. >> >> At the same time it future proofs the platform_mask handling by preparing >> the code for easy extending, and tidies the very verbose WARN strings >> generated when IS_PLATFORM macros are embedded into a WARN type >> statements. >> >> v2: Fixed IS_SUBPLATFORM. Updated commit msg. >> v3: Chris was right, there is an ordering problem. >> >> v4: >> * Catch-up with new sub-platforms. >> * Rebase for RUNTIME_INFO. >> * Drop subplatform mask union tricks and convert platform_mask to an >> array for extensibility. >> >> v5: >> * Fix subplatform check. >> * Protect against forgetting to expand subplatform bits. >> * Remove platform enum tallying. >> * Add subplatform to error state. (Chris) >> * Drop macros and just use static inlines. >> * Remove redundant IRONLAKE_M. (Ville) >> >> v6: >> * Split out Ironlake change. >> * Optimize subplatform check. >> * Use __always_inline. (Lucas) >> * Add platform_mask comment. (Paulo) >> * Pass stored runtime info in error capture. (Chris) >> >> v7: >> * Rebased for new AML ULX device id. >> * Bump platform mask array size for EHL. >> * Stop mentioning device ids in intel_device_subplatform_init by using >> the trick of splitting macros i915_pciids.h. (Jani) >> * AML seems to be either a subplatform of KBL or CFL so express it like >> that. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: Jose Souza <jose.souza@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v6 >> --- >> drivers/gpu/drm/i915/i915_drv.c | 8 +- >> drivers/gpu/drm/i915/i915_drv.h | 124 +++++++++++++------ >> drivers/gpu/drm/i915/i915_gpu_error.c | 3 + >> drivers/gpu/drm/i915/i915_pci.c | 2 +- >> drivers/gpu/drm/i915/intel_device_info.c | 145 +++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_device_info.h | 27 ++++- >> 6 files changed, 267 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 5465b99b4392..74255374cc6b 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -868,6 +868,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) >> if (i915_inject_load_failure()) >> return -ENODEV; >> >> + intel_device_info_subplatform_init(dev_priv); >> + >> spin_lock_init(&dev_priv->irq_lock); >> spin_lock_init(&dev_priv->gpu_error.lock); >> mutex_init(&dev_priv->backlight_lock); >> @@ -1718,10 +1720,12 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) >> if (drm_debug & DRM_UT_DRIVER) { >> struct drm_printer p = drm_debug_printer("i915 device info:"); >> >> - drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s gen=%i\n", >> + drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (subplatform=0x%x) gen=%i\n", >> INTEL_DEVID(dev_priv), >> INTEL_REVID(dev_priv), >> intel_platform_name(INTEL_INFO(dev_priv)->platform), >> + intel_subplatform(RUNTIME_INFO(dev_priv), >> + INTEL_INFO(dev_priv)->platform), >> INTEL_GEN(dev_priv)); >> >> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p); >> @@ -1764,8 +1768,6 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent) >> memcpy(device_info, match_info, sizeof(*device_info)); >> RUNTIME_INFO(i915)->device_id = pdev->device; >> >> - BUILD_BUG_ON(INTEL_MAX_PLATFORMS > >> - BITS_PER_TYPE(device_info->platform_mask)); >> BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask)); >> >> return i915; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 48c3b139f36f..c29ebfd94065 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2298,7 +2298,68 @@ static inline unsigned int i915_sg_segment_size(void) >> #define IS_REVID(p, since, until) \ >> (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) >> >> -#define IS_PLATFORM(dev_priv, p) (INTEL_INFO(dev_priv)->platform_mask & BIT(p)) >> +static __always_inline unsigned int >> +__platform_mask_index(const struct intel_runtime_info *info, >> + enum intel_platform p) >> +{ >> + const unsigned int pbits = >> + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; >> + >> + /* Expand the platform_mask array if this fails. */ >> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS > >> + pbits * ARRAY_SIZE(info->platform_mask)); >> + >> + return p / pbits; >> +} >> + >> +static __always_inline unsigned int >> +__platform_mask_bit(const struct intel_runtime_info *info, >> + enum intel_platform p) >> +{ >> + const unsigned int pbits = >> + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; >> + >> + return p % pbits + INTEL_SUBPLATFORM_BITS; >> +} >> + >> +static inline u32 >> +intel_subplatform(const struct intel_runtime_info *info, >> + enum intel_platform p) >> +{ >> + const unsigned int pi = __platform_mask_index(info, p); >> + >> + return info->platform_mask[pi] & INTEL_SUBPLATFORM_BITS; >> +} >> + >> +static __always_inline bool >> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p) >> +{ >> + const struct intel_runtime_info *info = RUNTIME_INFO(i915); >> + const unsigned int pi = __platform_mask_index(info, p); >> + const unsigned int pb = __platform_mask_bit(info, p); >> + >> + BUILD_BUG_ON(!__builtin_constant_p(p)); >> + >> + return info->platform_mask[pi] & BIT(pb); >> +} >> + >> +static __always_inline bool >> +IS_SUBPLATFORM(const struct drm_i915_private *i915, >> + enum intel_platform p, unsigned int s) >> +{ >> + const struct intel_runtime_info *info = RUNTIME_INFO(i915); >> + const unsigned int pi = __platform_mask_index(info, p); >> + const unsigned int pb = __platform_mask_bit(info, p); >> + const unsigned int msb = BITS_PER_TYPE(info->platform_mask[0]) - 1; >> + const u32 mask = info->platform_mask[pi]; >> + >> + BUILD_BUG_ON(!__builtin_constant_p(p)); >> + BUILD_BUG_ON(!__builtin_constant_p(s)); >> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); >> + >> + /* Shift and test on the MSB position so sign flag can be used. */ >> + return ((mask << (msb - pb)) & (mask << (msb - s))) & BIT(msb); >> +} >> >> #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile) >> >> @@ -2337,43 +2398,32 @@ static inline unsigned int i915_sg_segment_size(void) >> #define IS_ELKHARTLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) >> #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \ >> (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00) >> -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \ >> - ((INTEL_DEVID(dev_priv) & 0xf) == 0x6 || \ >> - (INTEL_DEVID(dev_priv) & 0xf) == 0xb || \ >> - (INTEL_DEVID(dev_priv) & 0xf) == 0xe)) >> -/* ULX machines are also considered ULT. */ >> -#define IS_BDW_ULX(dev_priv) (IS_BROADWELL(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0xf) == 0xe) >> +#define IS_BDW_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULT) >> +#define IS_BDW_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULX) >> #define IS_BDW_GT3(dev_priv) (IS_BROADWELL(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> -#define IS_HSW_ULT(dev_priv) (IS_HASWELL(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00) >> +#define IS_HSW_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULT) >> #define IS_HSW_GT3(dev_priv) (IS_HASWELL(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> #define IS_HSW_GT1(dev_priv) (IS_HASWELL(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 1) >> /* ULX machines are also considered ULT. */ >> -#define IS_HSW_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x0A0E || \ >> - INTEL_DEVID(dev_priv) == 0x0A1E) >> -#define IS_SKL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x1906 || \ >> - INTEL_DEVID(dev_priv) == 0x1913 || \ >> - INTEL_DEVID(dev_priv) == 0x1916 || \ >> - INTEL_DEVID(dev_priv) == 0x1921 || \ >> - INTEL_DEVID(dev_priv) == 0x1926) >> -#define IS_SKL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x190E || \ >> - INTEL_DEVID(dev_priv) == 0x1915 || \ >> - INTEL_DEVID(dev_priv) == 0x191E) >> -#define IS_KBL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x5906 || \ >> - INTEL_DEVID(dev_priv) == 0x5913 || \ >> - INTEL_DEVID(dev_priv) == 0x5916 || \ >> - INTEL_DEVID(dev_priv) == 0x5921 || \ >> - INTEL_DEVID(dev_priv) == 0x5926) >> -#define IS_KBL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x590E || \ >> - INTEL_DEVID(dev_priv) == 0x5915 || \ >> - INTEL_DEVID(dev_priv) == 0x591E) >> -#define IS_AML_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x591C || \ >> - INTEL_DEVID(dev_priv) == 0x87C0 || \ >> - INTEL_DEVID(dev_priv) == 0x87CA) >> +#define IS_HSW_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULX) >> +#define IS_SKL_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULT) >> +#define IS_SKL_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULX) >> +#define IS_KBL_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULT) >> +#define IS_KBL_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULX) >> +#define IS_AML_ULX(dev_priv) \ >> + (IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_AML) || \ >> + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_AML)) >> #define IS_SKL_GT2(dev_priv) (IS_SKYLAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 2) >> #define IS_SKL_GT3(dev_priv) (IS_SKYLAKE(dev_priv) && \ >> @@ -2384,16 +2434,16 @@ static inline unsigned int i915_sg_segment_size(void) >> INTEL_INFO(dev_priv)->gt == 2) >> #define IS_KBL_GT3(dev_priv) (IS_KABYLAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> -#define IS_CFL_ULT(dev_priv) (IS_COFFEELAKE(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0) >> +#define IS_CFL_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_ULT) >> #define IS_CFL_GT2(dev_priv) (IS_COFFEELAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 2) >> #define IS_CFL_GT3(dev_priv) (IS_COFFEELAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> -#define IS_CNL_WITH_PORT_F(dev_priv) (IS_CANNONLAKE(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004) >> -#define IS_ICL_WITH_PORT_F(dev_priv) (IS_ICELAKE(dev_priv) && \ >> - INTEL_DEVID(dev_priv) != 0x8A51) >> +#define IS_CNL_WITH_PORT_F(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, INTEL_SUBPLATFORM_PORTF) >> +#define IS_ICL_WITH_PORT_F(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_ICELAKE, INTEL_SUBPLATFORM_PORTF) >> >> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support) >> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c >> index a9557f92756f..0c980b899056 100644 >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >> @@ -677,6 +677,9 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m, >> err_printf(m, "Reset count: %u\n", error->reset_count); >> err_printf(m, "Suspend count: %u\n", error->suspend_count); >> err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform)); >> + err_printf(m, "Subplatform: 0x%x\n", >> + intel_subplatform(&error->runtime_info, >> + error->device_info.platform)); >> err_print_pciid(m, m->i915); >> >> err_printf(m, "IOMMU enabled?: %d\n", error->iommu); >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >> index 716f2f95c57d..39251586349a 100644 >> --- a/drivers/gpu/drm/i915/i915_pci.c >> +++ b/drivers/gpu/drm/i915/i915_pci.c >> @@ -32,7 +32,7 @@ >> #include "i915_globals.h" >> #include "i915_selftest.h" >> >> -#define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) >> +#define PLATFORM(x) .platform = (x) >> #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) >> >> #define I845_PIPE_OFFSETS \ >> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >> index e0ac908bb4e9..85da87e14c02 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -714,6 +714,151 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) >> return 0; >> } >> >> +#undef INTEL_VGA_DEVICE >> +#define INTEL_VGA_DEVICE(id, info) (id) >> + >> +static const u16 hsw_ult_ids[] = { >> + INTEL_HSW_ULT_GT1_IDS(0), >> + INTEL_HSW_ULT_GT2_IDS(0), >> + INTEL_HSW_ULT_GT3_IDS(0) >> +}; >> + >> +static const u16 hsw_ulx_ids[] = { >> + INTEL_HSW_ULX_GT1_IDS(0), >> + INTEL_HSW_ULX_GT2_IDS(0) >> +}; >> + >> +static const u16 bdw_ult_ids[] = { >> + INTEL_BDW_ULT_GT1_IDS(0), >> + INTEL_BDW_ULT_GT2_IDS(0), >> + INTEL_BDW_ULT_GT3_IDS(0), >> + INTEL_BDW_ULT_RSVD_IDS(0) >> +}; >> + >> +static const u16 bdw_ulx_ids[] = { >> + INTEL_BDW_ULX_GT1_IDS(0), >> + INTEL_BDW_ULX_GT2_IDS(0), >> + INTEL_BDW_ULX_GT3_IDS(0), >> + INTEL_BDW_ULX_RSVD_IDS(0) >> +}; >> + >> +static const u16 skl_ult_ids[] = { >> + INTEL_SKL_ULT_GT1_IDS(0), >> + INTEL_SKL_ULT_GT2_IDS(0), >> + INTEL_SKL_ULT_GT3_IDS(0) >> +}; >> + >> +static const u16 skl_ulx_ids[] = { >> + INTEL_SKL_ULX_GT1_IDS(0), >> + INTEL_SKL_ULX_GT2_IDS(0) >> +}; >> + >> +static const u16 kbl_ult_ids[] = { >> + INTEL_KBL_ULT_GT1_IDS(0), >> + INTEL_KBL_ULT_GT2_IDS(0), >> + INTEL_KBL_ULT_GT3_IDS(0) >> +}; >> + >> +static const u16 kbl_ulx_ids[] = { >> + INTEL_KBL_ULX_GT1_IDS(0), >> + INTEL_KBL_ULX_GT2_IDS(0) >> +}; >> + >> +static const u16 kbl_aml_ids[] = { >> + INTEL_AML_KBL_GT2_IDS(0) >> +}; >> + >> +static const u16 cfl_ult_ids[] = { >> + INTEL_CFL_U_GT2_IDS(0), >> + INTEL_CFL_U_GT3_IDS(0), >> + INTEL_WHL_U_GT1_IDS(0), >> + INTEL_WHL_U_GT2_IDS(0), >> + INTEL_WHL_U_GT3_IDS(0) >> +}; >> + >> +static const u16 cfl_aml_ids[] = { >> + INTEL_AML_CFL_GT2_IDS(0) >> +}; >> + >> +static const u16 cnl_portf_ids[] = { >> + INTEL_CNL_PORT_F_IDS(0) >> +}; >> + >> +static const u16 icl_portf_ids[] = { >> + INTEL_ICL_PORT_F_IDS(0) >> +}; > > What's the benefit of having platform split in the arrays and an if > ladder in the function below? > > I think I'd go with just the feature arrays. Not to block this series, but looking further outside the box... I've still got the constant vs. runtime device info split unfinished. We've got so many things that are mostly constant, but occasionally need changes. And we've got so many things that could be device info flags, but would lead to proliferation of plenty of almost identical device info structures. Like this ULX/ULT and GT number. So I guess I'm wondering if we're doing the right thing by assigning device info pointers to the struct pci_device_id driver_data member in pciidlist[] table. For one thing, that's a whole lot of bits that could be used directly for assigning platform and subplatform, or features. Of course, we'd then need another table besides pciidlist[] to map to the device info, but we're sort of doing some of that with the ULX/ULT parts. I just overall feel that there must be a better way to do all this, and we just haven't figured it out yet, and we're partially putting ourselves into a box we can't break out of. Thoughts? BR, Jani. > > BR, > Jani. > >> + >> +static bool find_devid(u16 id, const u16 *p, unsigned int num) >> +{ >> + for (; num; num--, p++) { >> + if (*p == id) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +void intel_device_info_subplatform_init(struct drm_i915_private *i915) >> +{ >> + const struct intel_device_info *info = INTEL_INFO(i915); >> + const struct intel_runtime_info *rinfo = RUNTIME_INFO(i915); >> + const unsigned int pi = __platform_mask_index(rinfo, info->platform); >> + const unsigned int pb = __platform_mask_bit(rinfo, info->platform); >> + u16 devid = INTEL_DEVID(i915); >> + u32 mask = 0; >> + >> + RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb); >> + >> + if (IS_HASWELL(i915)) { >> + if (find_devid(devid, hsw_ult_ids, ARRAY_SIZE(hsw_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, hsw_ulx_ids, >> + ARRAY_SIZE(hsw_ulx_ids))) >> + /* ULX machines are also considered ULT. */ >> + mask = BIT(INTEL_SUBPLATFORM_ULT) | >> + BIT(INTEL_SUBPLATFORM_ULX); >> + } else if (IS_BROADWELL(i915)) { >> + if (find_devid(devid, bdw_ult_ids, ARRAY_SIZE(bdw_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, bdw_ulx_ids, >> + ARRAY_SIZE(bdw_ulx_ids))) >> + /* ULX machines are also considered ULT. */ >> + mask = BIT(INTEL_SUBPLATFORM_ULT) | >> + BIT(INTEL_SUBPLATFORM_ULX); >> + } else if (IS_SKYLAKE(i915)) { >> + if (find_devid(devid, skl_ult_ids, ARRAY_SIZE(skl_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, skl_ulx_ids, >> + ARRAY_SIZE(skl_ulx_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULX); >> + } else if (IS_KABYLAKE(i915)) { >> + if (find_devid(devid, kbl_ult_ids, ARRAY_SIZE(kbl_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, kbl_ulx_ids, >> + ARRAY_SIZE(kbl_ulx_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULX); >> + else if (find_devid(devid, kbl_aml_ids, >> + ARRAY_SIZE(kbl_aml_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_AML); >> + } else if (IS_COFFEELAKE(i915)) { >> + if (find_devid(devid, cfl_ult_ids, ARRAY_SIZE(cfl_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, cfl_aml_ids, >> + ARRAY_SIZE(cfl_aml_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_AML); >> + } else if (IS_CANNONLAKE(i915)) { >> + if (find_devid(devid, cnl_portf_ids, ARRAY_SIZE(cnl_portf_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_PORTF); >> + } else if (IS_ICELAKE(i915)) { >> + if (find_devid(devid, icl_portf_ids, ARRAY_SIZE(icl_portf_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_PORTF); >> + } >> + >> + GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS); >> + >> + RUNTIME_INFO(i915)->platform_mask[pi] |= mask; >> +} >> + >> /** >> * intel_device_info_runtime_init - initialize runtime info >> * @dev_priv: the i915 device >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >> index 98acefaacec9..da1152eef46b 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.h >> +++ b/drivers/gpu/drm/i915/intel_device_info.h >> @@ -77,6 +77,21 @@ enum intel_platform { >> INTEL_MAX_PLATFORMS >> }; >> >> +/* >> + * Subplatform bits share the same namespace per parent platform. In other words >> + * it is fine for the same bit to be used on multiple parent platforms. >> + */ >> + >> +#define INTEL_SUBPLATFORM_BITS (3) >> + >> +/* HSW/BDW/SKL/KBL/CFL */ >> +#define INTEL_SUBPLATFORM_ULT (0) >> +#define INTEL_SUBPLATFORM_ULX (1) >> +#define INTEL_SUBPLATFORM_AML (2) >> + >> +/* CNL/ICL */ >> +#define INTEL_SUBPLATFORM_PORTF (0) >> + >> enum intel_ppgtt_type { >> INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE, >> INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING, >> @@ -160,7 +175,6 @@ struct intel_device_info { >> intel_engine_mask_t engine_mask; /* Engines supported by the HW */ >> >> enum intel_platform platform; >> - u32 platform_mask; >> >> enum intel_ppgtt_type ppgtt_type; >> unsigned int ppgtt_size; /* log2, e.g. 31/32/48 bits */ >> @@ -197,6 +211,16 @@ struct intel_device_info { >> }; >> >> struct intel_runtime_info { >> + /* >> + * Platform mask is used for optimizing or-ed IS_PLATFORM calls into >> + * into single runtime conditionals, and also to provide groundwork >> + * for future per platform, or per SKU build optimizations. >> + * >> + * Array can be extended when necessary if the corresponding >> + * BUILD_BUG_ON is hit. >> + */ >> + u32 platform_mask[2]; >> + >> u16 device_id; >> >> u8 num_sprites[I915_MAX_PIPES]; >> @@ -271,6 +295,7 @@ static inline void sseu_set_eus(struct sseu_dev_info *sseu, >> >> const char *intel_platform_name(enum intel_platform platform); >> >> +void intel_device_info_subplatform_init(struct drm_i915_private *dev_priv); >> void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); >> void intel_device_info_dump_flags(const struct intel_device_info *info, >> struct drm_printer *p);
Quoting Jani Nikula (2019-03-26 09:34:45) > Not to block this series, but looking further outside the box... > > I've still got the constant vs. runtime device info split > unfinished. We've got so many things that are mostly constant, but > occasionally need changes. And we've got so many things that could be > device info flags, but would lead to proliferation of plenty of almost > identical device info structures. Like this ULX/ULT and GT number. > > So I guess I'm wondering if we're doing the right thing by assigning > device info pointers to the struct pci_device_id driver_data member in > pciidlist[] table. > > For one thing, that's a whole lot of bits that could be used directly > for assigning platform and subplatform, or features. > > Of course, we'd then need another table besides pciidlist[] to map to > the device info, but we're sort of doing some of that with the ULX/ULT > parts. > > I just overall feel that there must be a better way to do all this, and > we just haven't figured it out yet, and we're partially putting > ourselves into a box we can't break out of. > > Thoughts? I think intel_device_info is still fundamentally useful. The disadvantage of having the feature discovery separate from use is outweighed by having consistent stanzas for those features - it makes comparing platforms, finding feature sets much easier. (The cost being that with the setting of the feature flag far away from the code using it, people updating the cost are more likely to forget the flag.) One end goal of this madness, is that we can recompile the kernel module to only support a single sku and dce the rest. But what are the diminishing returns here? Without measurement, I'd say a single platform. So that dictates what can be in the static intel_device_info, features that are constant across a whole platform. And as you point out, we don't need a pointer to the device_info itself, just a platform field which is an index into the device_info block, with plenty of room for subplatform flags. While that says how we hook up device_info, I don't think that reflects on the use of feature flags themselves, or our ability to statically determine a reduced feature set. So not a box, just a mere wet paper bag. -Chris
On 26/03/2019 09:53, Chris Wilson wrote: > Quoting Jani Nikula (2019-03-26 09:34:45) >> Not to block this series, but looking further outside the box... >> >> I've still got the constant vs. runtime device info split >> unfinished. We've got so many things that are mostly constant, but >> occasionally need changes. And we've got so many things that could be >> device info flags, but would lead to proliferation of plenty of almost >> identical device info structures. Like this ULX/ULT and GT number. >> >> So I guess I'm wondering if we're doing the right thing by assigning >> device info pointers to the struct pci_device_id driver_data member in >> pciidlist[] table. >> >> For one thing, that's a whole lot of bits that could be used directly >> for assigning platform and subplatform, or features. >> >> Of course, we'd then need another table besides pciidlist[] to map to >> the device info, but we're sort of doing some of that with the ULX/ULT >> parts. >> >> I just overall feel that there must be a better way to do all this, and >> we just haven't figured it out yet, and we're partially putting >> ourselves into a box we can't break out of. >> >> Thoughts? > > I think intel_device_info is still fundamentally useful. The > disadvantage of having the feature discovery separate from use is > outweighed by having consistent stanzas for those features - it makes > comparing platforms, finding feature sets much easier. (The cost being > that with the setting of the feature flag far away from the code using > it, people updating the cost are more likely to forget the flag.) > > One end goal of this madness, is that we can recompile the kernel module > to only support a single sku and dce the rest. But what are the > diminishing returns here? Without measurement, I'd say a single > platform. > > So that dictates what can be in the static intel_device_info, features > that are constant across a whole platform. And as you point out, we > don't need a pointer to the device_info itself, just a platform field > which is an index into the device_info block, with plenty of room for > subplatform flags. > > While that says how we hook up device_info, I don't think that reflects > on the use of feature flags themselves, or our ability to statically > determine a reduced feature set. > > So not a box, just a mere wet paper bag. To check if I follow.. we are talking about potentially abolishing device info in favour of constructing something at probe time, which could potentially have fewer and overall smaller static data portion? Because I don't see how we can eliminate the pciidlist itself, or even shrink it's size? It has to have one entry per device id, just the question for what we use driver_data for? Static vs runtime I think shouldn't have effect on the per platform builds. As long as all feature tests are done via macros, or small static inlines, code can still be compiled out. I do have a small nagging feeling about this series as well, but I have managed to convince myself it is better than the device id listed in i915_drv.h. So don't know.. we can always drop it and just expand platform mask to u64 to solve the immediate need and leave the rest for later. Regards, Tvrtko
On 26/03/2019 08:39, Jani Nikula wrote: > On Tue, 26 Mar 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Concept of a sub-platform already exist in our code (like ULX and ULT >> platform variants and similar),implemented via the macros which check a >> list of device ids to determine a match. >> >> With this patch we consolidate device ids checking into a single function >> called during early driver load. >> >> A few low bits in the platform mask are reserved for sub-platform >> identification and defined as a per-platform namespace. >> >> At the same time it future proofs the platform_mask handling by preparing >> the code for easy extending, and tidies the very verbose WARN strings >> generated when IS_PLATFORM macros are embedded into a WARN type >> statements. >> >> v2: Fixed IS_SUBPLATFORM. Updated commit msg. >> v3: Chris was right, there is an ordering problem. >> >> v4: >> * Catch-up with new sub-platforms. >> * Rebase for RUNTIME_INFO. >> * Drop subplatform mask union tricks and convert platform_mask to an >> array for extensibility. >> >> v5: >> * Fix subplatform check. >> * Protect against forgetting to expand subplatform bits. >> * Remove platform enum tallying. >> * Add subplatform to error state. (Chris) >> * Drop macros and just use static inlines. >> * Remove redundant IRONLAKE_M. (Ville) >> >> v6: >> * Split out Ironlake change. >> * Optimize subplatform check. >> * Use __always_inline. (Lucas) >> * Add platform_mask comment. (Paulo) >> * Pass stored runtime info in error capture. (Chris) >> >> v7: >> * Rebased for new AML ULX device id. >> * Bump platform mask array size for EHL. >> * Stop mentioning device ids in intel_device_subplatform_init by using >> the trick of splitting macros i915_pciids.h. (Jani) >> * AML seems to be either a subplatform of KBL or CFL so express it like >> that. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: Jose Souza <jose.souza@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v6 >> --- >> drivers/gpu/drm/i915/i915_drv.c | 8 +- >> drivers/gpu/drm/i915/i915_drv.h | 124 +++++++++++++------ >> drivers/gpu/drm/i915/i915_gpu_error.c | 3 + >> drivers/gpu/drm/i915/i915_pci.c | 2 +- >> drivers/gpu/drm/i915/intel_device_info.c | 145 +++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_device_info.h | 27 ++++- >> 6 files changed, 267 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 5465b99b4392..74255374cc6b 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -868,6 +868,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) >> if (i915_inject_load_failure()) >> return -ENODEV; >> >> + intel_device_info_subplatform_init(dev_priv); >> + >> spin_lock_init(&dev_priv->irq_lock); >> spin_lock_init(&dev_priv->gpu_error.lock); >> mutex_init(&dev_priv->backlight_lock); >> @@ -1718,10 +1720,12 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) >> if (drm_debug & DRM_UT_DRIVER) { >> struct drm_printer p = drm_debug_printer("i915 device info:"); >> >> - drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s gen=%i\n", >> + drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (subplatform=0x%x) gen=%i\n", >> INTEL_DEVID(dev_priv), >> INTEL_REVID(dev_priv), >> intel_platform_name(INTEL_INFO(dev_priv)->platform), >> + intel_subplatform(RUNTIME_INFO(dev_priv), >> + INTEL_INFO(dev_priv)->platform), >> INTEL_GEN(dev_priv)); >> >> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p); >> @@ -1764,8 +1768,6 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent) >> memcpy(device_info, match_info, sizeof(*device_info)); >> RUNTIME_INFO(i915)->device_id = pdev->device; >> >> - BUILD_BUG_ON(INTEL_MAX_PLATFORMS > >> - BITS_PER_TYPE(device_info->platform_mask)); >> BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask)); >> >> return i915; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 48c3b139f36f..c29ebfd94065 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2298,7 +2298,68 @@ static inline unsigned int i915_sg_segment_size(void) >> #define IS_REVID(p, since, until) \ >> (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) >> >> -#define IS_PLATFORM(dev_priv, p) (INTEL_INFO(dev_priv)->platform_mask & BIT(p)) >> +static __always_inline unsigned int >> +__platform_mask_index(const struct intel_runtime_info *info, >> + enum intel_platform p) >> +{ >> + const unsigned int pbits = >> + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; >> + >> + /* Expand the platform_mask array if this fails. */ >> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS > >> + pbits * ARRAY_SIZE(info->platform_mask)); >> + >> + return p / pbits; >> +} >> + >> +static __always_inline unsigned int >> +__platform_mask_bit(const struct intel_runtime_info *info, >> + enum intel_platform p) >> +{ >> + const unsigned int pbits = >> + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; >> + >> + return p % pbits + INTEL_SUBPLATFORM_BITS; >> +} >> + >> +static inline u32 >> +intel_subplatform(const struct intel_runtime_info *info, >> + enum intel_platform p) >> +{ >> + const unsigned int pi = __platform_mask_index(info, p); >> + >> + return info->platform_mask[pi] & INTEL_SUBPLATFORM_BITS; >> +} >> + >> +static __always_inline bool >> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p) >> +{ >> + const struct intel_runtime_info *info = RUNTIME_INFO(i915); >> + const unsigned int pi = __platform_mask_index(info, p); >> + const unsigned int pb = __platform_mask_bit(info, p); >> + >> + BUILD_BUG_ON(!__builtin_constant_p(p)); >> + >> + return info->platform_mask[pi] & BIT(pb); >> +} >> + >> +static __always_inline bool >> +IS_SUBPLATFORM(const struct drm_i915_private *i915, >> + enum intel_platform p, unsigned int s) >> +{ >> + const struct intel_runtime_info *info = RUNTIME_INFO(i915); >> + const unsigned int pi = __platform_mask_index(info, p); >> + const unsigned int pb = __platform_mask_bit(info, p); >> + const unsigned int msb = BITS_PER_TYPE(info->platform_mask[0]) - 1; >> + const u32 mask = info->platform_mask[pi]; >> + >> + BUILD_BUG_ON(!__builtin_constant_p(p)); >> + BUILD_BUG_ON(!__builtin_constant_p(s)); >> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); >> + >> + /* Shift and test on the MSB position so sign flag can be used. */ >> + return ((mask << (msb - pb)) & (mask << (msb - s))) & BIT(msb); >> +} >> >> #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile) >> >> @@ -2337,43 +2398,32 @@ static inline unsigned int i915_sg_segment_size(void) >> #define IS_ELKHARTLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) >> #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \ >> (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00) >> -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \ >> - ((INTEL_DEVID(dev_priv) & 0xf) == 0x6 || \ >> - (INTEL_DEVID(dev_priv) & 0xf) == 0xb || \ >> - (INTEL_DEVID(dev_priv) & 0xf) == 0xe)) >> -/* ULX machines are also considered ULT. */ >> -#define IS_BDW_ULX(dev_priv) (IS_BROADWELL(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0xf) == 0xe) >> +#define IS_BDW_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULT) >> +#define IS_BDW_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULX) >> #define IS_BDW_GT3(dev_priv) (IS_BROADWELL(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> -#define IS_HSW_ULT(dev_priv) (IS_HASWELL(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00) >> +#define IS_HSW_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULT) >> #define IS_HSW_GT3(dev_priv) (IS_HASWELL(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> #define IS_HSW_GT1(dev_priv) (IS_HASWELL(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 1) >> /* ULX machines are also considered ULT. */ >> -#define IS_HSW_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x0A0E || \ >> - INTEL_DEVID(dev_priv) == 0x0A1E) >> -#define IS_SKL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x1906 || \ >> - INTEL_DEVID(dev_priv) == 0x1913 || \ >> - INTEL_DEVID(dev_priv) == 0x1916 || \ >> - INTEL_DEVID(dev_priv) == 0x1921 || \ >> - INTEL_DEVID(dev_priv) == 0x1926) >> -#define IS_SKL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x190E || \ >> - INTEL_DEVID(dev_priv) == 0x1915 || \ >> - INTEL_DEVID(dev_priv) == 0x191E) >> -#define IS_KBL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x5906 || \ >> - INTEL_DEVID(dev_priv) == 0x5913 || \ >> - INTEL_DEVID(dev_priv) == 0x5916 || \ >> - INTEL_DEVID(dev_priv) == 0x5921 || \ >> - INTEL_DEVID(dev_priv) == 0x5926) >> -#define IS_KBL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x590E || \ >> - INTEL_DEVID(dev_priv) == 0x5915 || \ >> - INTEL_DEVID(dev_priv) == 0x591E) >> -#define IS_AML_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x591C || \ >> - INTEL_DEVID(dev_priv) == 0x87C0 || \ >> - INTEL_DEVID(dev_priv) == 0x87CA) >> +#define IS_HSW_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULX) >> +#define IS_SKL_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULT) >> +#define IS_SKL_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULX) >> +#define IS_KBL_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULT) >> +#define IS_KBL_ULX(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULX) >> +#define IS_AML_ULX(dev_priv) \ >> + (IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_AML) || \ >> + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_AML)) >> #define IS_SKL_GT2(dev_priv) (IS_SKYLAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 2) >> #define IS_SKL_GT3(dev_priv) (IS_SKYLAKE(dev_priv) && \ >> @@ -2384,16 +2434,16 @@ static inline unsigned int i915_sg_segment_size(void) >> INTEL_INFO(dev_priv)->gt == 2) >> #define IS_KBL_GT3(dev_priv) (IS_KABYLAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> -#define IS_CFL_ULT(dev_priv) (IS_COFFEELAKE(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0) >> +#define IS_CFL_ULT(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_ULT) >> #define IS_CFL_GT2(dev_priv) (IS_COFFEELAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 2) >> #define IS_CFL_GT3(dev_priv) (IS_COFFEELAKE(dev_priv) && \ >> INTEL_INFO(dev_priv)->gt == 3) >> -#define IS_CNL_WITH_PORT_F(dev_priv) (IS_CANNONLAKE(dev_priv) && \ >> - (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004) >> -#define IS_ICL_WITH_PORT_F(dev_priv) (IS_ICELAKE(dev_priv) && \ >> - INTEL_DEVID(dev_priv) != 0x8A51) >> +#define IS_CNL_WITH_PORT_F(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, INTEL_SUBPLATFORM_PORTF) >> +#define IS_ICL_WITH_PORT_F(dev_priv) \ >> + IS_SUBPLATFORM(dev_priv, INTEL_ICELAKE, INTEL_SUBPLATFORM_PORTF) >> >> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support) >> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c >> index a9557f92756f..0c980b899056 100644 >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >> @@ -677,6 +677,9 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m, >> err_printf(m, "Reset count: %u\n", error->reset_count); >> err_printf(m, "Suspend count: %u\n", error->suspend_count); >> err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform)); >> + err_printf(m, "Subplatform: 0x%x\n", >> + intel_subplatform(&error->runtime_info, >> + error->device_info.platform)); >> err_print_pciid(m, m->i915); >> >> err_printf(m, "IOMMU enabled?: %d\n", error->iommu); >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >> index 716f2f95c57d..39251586349a 100644 >> --- a/drivers/gpu/drm/i915/i915_pci.c >> +++ b/drivers/gpu/drm/i915/i915_pci.c >> @@ -32,7 +32,7 @@ >> #include "i915_globals.h" >> #include "i915_selftest.h" >> >> -#define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) >> +#define PLATFORM(x) .platform = (x) >> #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) >> >> #define I845_PIPE_OFFSETS \ >> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >> index e0ac908bb4e9..85da87e14c02 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -714,6 +714,151 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) >> return 0; >> } >> >> +#undef INTEL_VGA_DEVICE >> +#define INTEL_VGA_DEVICE(id, info) (id) >> + >> +static const u16 hsw_ult_ids[] = { >> + INTEL_HSW_ULT_GT1_IDS(0), >> + INTEL_HSW_ULT_GT2_IDS(0), >> + INTEL_HSW_ULT_GT3_IDS(0) >> +}; >> + >> +static const u16 hsw_ulx_ids[] = { >> + INTEL_HSW_ULX_GT1_IDS(0), >> + INTEL_HSW_ULX_GT2_IDS(0) >> +}; >> + >> +static const u16 bdw_ult_ids[] = { >> + INTEL_BDW_ULT_GT1_IDS(0), >> + INTEL_BDW_ULT_GT2_IDS(0), >> + INTEL_BDW_ULT_GT3_IDS(0), >> + INTEL_BDW_ULT_RSVD_IDS(0) >> +}; >> + >> +static const u16 bdw_ulx_ids[] = { >> + INTEL_BDW_ULX_GT1_IDS(0), >> + INTEL_BDW_ULX_GT2_IDS(0), >> + INTEL_BDW_ULX_GT3_IDS(0), >> + INTEL_BDW_ULX_RSVD_IDS(0) >> +}; >> + >> +static const u16 skl_ult_ids[] = { >> + INTEL_SKL_ULT_GT1_IDS(0), >> + INTEL_SKL_ULT_GT2_IDS(0), >> + INTEL_SKL_ULT_GT3_IDS(0) >> +}; >> + >> +static const u16 skl_ulx_ids[] = { >> + INTEL_SKL_ULX_GT1_IDS(0), >> + INTEL_SKL_ULX_GT2_IDS(0) >> +}; >> + >> +static const u16 kbl_ult_ids[] = { >> + INTEL_KBL_ULT_GT1_IDS(0), >> + INTEL_KBL_ULT_GT2_IDS(0), >> + INTEL_KBL_ULT_GT3_IDS(0) >> +}; >> + >> +static const u16 kbl_ulx_ids[] = { >> + INTEL_KBL_ULX_GT1_IDS(0), >> + INTEL_KBL_ULX_GT2_IDS(0) >> +}; >> + >> +static const u16 kbl_aml_ids[] = { >> + INTEL_AML_KBL_GT2_IDS(0) >> +}; >> + >> +static const u16 cfl_ult_ids[] = { >> + INTEL_CFL_U_GT2_IDS(0), >> + INTEL_CFL_U_GT3_IDS(0), >> + INTEL_WHL_U_GT1_IDS(0), >> + INTEL_WHL_U_GT2_IDS(0), >> + INTEL_WHL_U_GT3_IDS(0) >> +}; >> + >> +static const u16 cfl_aml_ids[] = { >> + INTEL_AML_CFL_GT2_IDS(0) >> +}; >> + >> +static const u16 cnl_portf_ids[] = { >> + INTEL_CNL_PORT_F_IDS(0) >> +}; >> + >> +static const u16 icl_portf_ids[] = { >> + INTEL_ICL_PORT_F_IDS(0) >> +}; > > What's the benefit of having platform split in the arrays and an if > ladder in the function below? > > I think I'd go with just the feature arrays. Yeah agreed, it will look much better that way. Regards, Tvrtko > > BR, > Jani. > >> + >> +static bool find_devid(u16 id, const u16 *p, unsigned int num) >> +{ >> + for (; num; num--, p++) { >> + if (*p == id) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +void intel_device_info_subplatform_init(struct drm_i915_private *i915) >> +{ >> + const struct intel_device_info *info = INTEL_INFO(i915); >> + const struct intel_runtime_info *rinfo = RUNTIME_INFO(i915); >> + const unsigned int pi = __platform_mask_index(rinfo, info->platform); >> + const unsigned int pb = __platform_mask_bit(rinfo, info->platform); >> + u16 devid = INTEL_DEVID(i915); >> + u32 mask = 0; >> + >> + RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb); >> + >> + if (IS_HASWELL(i915)) { >> + if (find_devid(devid, hsw_ult_ids, ARRAY_SIZE(hsw_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, hsw_ulx_ids, >> + ARRAY_SIZE(hsw_ulx_ids))) >> + /* ULX machines are also considered ULT. */ >> + mask = BIT(INTEL_SUBPLATFORM_ULT) | >> + BIT(INTEL_SUBPLATFORM_ULX); >> + } else if (IS_BROADWELL(i915)) { >> + if (find_devid(devid, bdw_ult_ids, ARRAY_SIZE(bdw_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, bdw_ulx_ids, >> + ARRAY_SIZE(bdw_ulx_ids))) >> + /* ULX machines are also considered ULT. */ >> + mask = BIT(INTEL_SUBPLATFORM_ULT) | >> + BIT(INTEL_SUBPLATFORM_ULX); >> + } else if (IS_SKYLAKE(i915)) { >> + if (find_devid(devid, skl_ult_ids, ARRAY_SIZE(skl_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, skl_ulx_ids, >> + ARRAY_SIZE(skl_ulx_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULX); >> + } else if (IS_KABYLAKE(i915)) { >> + if (find_devid(devid, kbl_ult_ids, ARRAY_SIZE(kbl_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, kbl_ulx_ids, >> + ARRAY_SIZE(kbl_ulx_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULX); >> + else if (find_devid(devid, kbl_aml_ids, >> + ARRAY_SIZE(kbl_aml_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_AML); >> + } else if (IS_COFFEELAKE(i915)) { >> + if (find_devid(devid, cfl_ult_ids, ARRAY_SIZE(cfl_ult_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_ULT); >> + else if (find_devid(devid, cfl_aml_ids, >> + ARRAY_SIZE(cfl_aml_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_AML); >> + } else if (IS_CANNONLAKE(i915)) { >> + if (find_devid(devid, cnl_portf_ids, ARRAY_SIZE(cnl_portf_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_PORTF); >> + } else if (IS_ICELAKE(i915)) { >> + if (find_devid(devid, icl_portf_ids, ARRAY_SIZE(icl_portf_ids))) >> + mask = BIT(INTEL_SUBPLATFORM_PORTF); >> + } >> + >> + GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS); >> + >> + RUNTIME_INFO(i915)->platform_mask[pi] |= mask; >> +} >> + >> /** >> * intel_device_info_runtime_init - initialize runtime info >> * @dev_priv: the i915 device >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >> index 98acefaacec9..da1152eef46b 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.h >> +++ b/drivers/gpu/drm/i915/intel_device_info.h >> @@ -77,6 +77,21 @@ enum intel_platform { >> INTEL_MAX_PLATFORMS >> }; >> >> +/* >> + * Subplatform bits share the same namespace per parent platform. In other words >> + * it is fine for the same bit to be used on multiple parent platforms. >> + */ >> + >> +#define INTEL_SUBPLATFORM_BITS (3) >> + >> +/* HSW/BDW/SKL/KBL/CFL */ >> +#define INTEL_SUBPLATFORM_ULT (0) >> +#define INTEL_SUBPLATFORM_ULX (1) >> +#define INTEL_SUBPLATFORM_AML (2) >> + >> +/* CNL/ICL */ >> +#define INTEL_SUBPLATFORM_PORTF (0) >> + >> enum intel_ppgtt_type { >> INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE, >> INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING, >> @@ -160,7 +175,6 @@ struct intel_device_info { >> intel_engine_mask_t engine_mask; /* Engines supported by the HW */ >> >> enum intel_platform platform; >> - u32 platform_mask; >> >> enum intel_ppgtt_type ppgtt_type; >> unsigned int ppgtt_size; /* log2, e.g. 31/32/48 bits */ >> @@ -197,6 +211,16 @@ struct intel_device_info { >> }; >> >> struct intel_runtime_info { >> + /* >> + * Platform mask is used for optimizing or-ed IS_PLATFORM calls into >> + * into single runtime conditionals, and also to provide groundwork >> + * for future per platform, or per SKU build optimizations. >> + * >> + * Array can be extended when necessary if the corresponding >> + * BUILD_BUG_ON is hit. >> + */ >> + u32 platform_mask[2]; >> + >> u16 device_id; >> >> u8 num_sprites[I915_MAX_PIPES]; >> @@ -271,6 +295,7 @@ static inline void sseu_set_eus(struct sseu_dev_info *sseu, >> >> const char *intel_platform_name(enum intel_platform platform); >> >> +void intel_device_info_subplatform_init(struct drm_i915_private *dev_priv); >> void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); >> void intel_device_info_dump_flags(const struct intel_device_info *info, >> struct drm_printer *p); >
Quoting Tvrtko Ursulin (2019-03-27 11:35:41) > > On 26/03/2019 09:53, Chris Wilson wrote: > > Quoting Jani Nikula (2019-03-26 09:34:45) > >> Not to block this series, but looking further outside the box... > >> > >> I've still got the constant vs. runtime device info split > >> unfinished. We've got so many things that are mostly constant, but > >> occasionally need changes. And we've got so many things that could be > >> device info flags, but would lead to proliferation of plenty of almost > >> identical device info structures. Like this ULX/ULT and GT number. > >> > >> So I guess I'm wondering if we're doing the right thing by assigning > >> device info pointers to the struct pci_device_id driver_data member in > >> pciidlist[] table. > >> > >> For one thing, that's a whole lot of bits that could be used directly > >> for assigning platform and subplatform, or features. > >> > >> Of course, we'd then need another table besides pciidlist[] to map to > >> the device info, but we're sort of doing some of that with the ULX/ULT > >> parts. > >> > >> I just overall feel that there must be a better way to do all this, and > >> we just haven't figured it out yet, and we're partially putting > >> ourselves into a box we can't break out of. > >> > >> Thoughts? > > > > I think intel_device_info is still fundamentally useful. The > > disadvantage of having the feature discovery separate from use is > > outweighed by having consistent stanzas for those features - it makes > > comparing platforms, finding feature sets much easier. (The cost being > > that with the setting of the feature flag far away from the code using > > it, people updating the cost are more likely to forget the flag.) > > > > One end goal of this madness, is that we can recompile the kernel module > > to only support a single sku and dce the rest. But what are the > > diminishing returns here? Without measurement, I'd say a single > > platform. > > > > So that dictates what can be in the static intel_device_info, features > > that are constant across a whole platform. And as you point out, we > > don't need a pointer to the device_info itself, just a platform field > > which is an index into the device_info block, with plenty of room for > > subplatform flags. > > > > While that says how we hook up device_info, I don't think that reflects > > on the use of feature flags themselves, or our ability to statically > > determine a reduced feature set. > > > > So not a box, just a mere wet paper bag. > > To check if I follow.. we are talking about potentially abolishing > device info in favour of constructing something at probe time, which > could potentially have fewer and overall smaller static data portion? > > Because I don't see how we can eliminate the pciidlist itself, or even > shrink it's size? It has to have one entry per device id, just the > question for what we use driver_data for? Correct. What I think Jani was suggesting was that instead of encoding the device_info pointer into driver_data, we encode the platform/subplatform id. We then use the platform portion to lookup the device_info, and subplatform to annotate the runtime_info. > Static vs runtime I think shouldn't have effect on the per platform > builds. As long as all feature tests are done via macros, or small > static inlines, code can still be compiled out. > > I do have a small nagging feeling about this series as well, but I have > managed to convince myself it is better than the device id listed in > i915_drv.h. I still feel the pain from having the endless chains of || and so welcome the removal of devid from the macros. > So don't know.. we can always drop it and just expand > platform mask to u64 to solve the immediate need and leave the rest for > later. Imo, this series is an improvement, and doesn't prevent us from changing our minds later (although not back to devid macros please!). -Chris
On Wed, 27 Mar 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Tvrtko Ursulin (2019-03-27 11:35:41) >> >> On 26/03/2019 09:53, Chris Wilson wrote: >> > Quoting Jani Nikula (2019-03-26 09:34:45) >> >> Not to block this series, but looking further outside the box... >> >> >> >> I've still got the constant vs. runtime device info split >> >> unfinished. We've got so many things that are mostly constant, but >> >> occasionally need changes. And we've got so many things that could be >> >> device info flags, but would lead to proliferation of plenty of almost >> >> identical device info structures. Like this ULX/ULT and GT number. >> >> >> >> So I guess I'm wondering if we're doing the right thing by assigning >> >> device info pointers to the struct pci_device_id driver_data member in >> >> pciidlist[] table. >> >> >> >> For one thing, that's a whole lot of bits that could be used directly >> >> for assigning platform and subplatform, or features. >> >> >> >> Of course, we'd then need another table besides pciidlist[] to map to >> >> the device info, but we're sort of doing some of that with the ULX/ULT >> >> parts. >> >> >> >> I just overall feel that there must be a better way to do all this, and >> >> we just haven't figured it out yet, and we're partially putting >> >> ourselves into a box we can't break out of. >> >> >> >> Thoughts? >> > >> > I think intel_device_info is still fundamentally useful. The >> > disadvantage of having the feature discovery separate from use is >> > outweighed by having consistent stanzas for those features - it makes >> > comparing platforms, finding feature sets much easier. (The cost being >> > that with the setting of the feature flag far away from the code using >> > it, people updating the cost are more likely to forget the flag.) >> > >> > One end goal of this madness, is that we can recompile the kernel module >> > to only support a single sku and dce the rest. But what are the >> > diminishing returns here? Without measurement, I'd say a single >> > platform. >> > >> > So that dictates what can be in the static intel_device_info, features >> > that are constant across a whole platform. And as you point out, we >> > don't need a pointer to the device_info itself, just a platform field >> > which is an index into the device_info block, with plenty of room for >> > subplatform flags. >> > >> > While that says how we hook up device_info, I don't think that reflects >> > on the use of feature flags themselves, or our ability to statically >> > determine a reduced feature set. >> > >> > So not a box, just a mere wet paper bag. >> >> To check if I follow.. we are talking about potentially abolishing >> device info in favour of constructing something at probe time, which >> could potentially have fewer and overall smaller static data portion? >> >> Because I don't see how we can eliminate the pciidlist itself, or even >> shrink it's size? It has to have one entry per device id, just the >> question for what we use driver_data for? > > Correct. What I think Jani was suggesting was that instead of encoding > the device_info pointer into driver_data, we encode the > platform/subplatform id. We then use the platform portion to lookup the > device_info, and subplatform to annotate the runtime_info. > >> Static vs runtime I think shouldn't have effect on the per platform >> builds. As long as all feature tests are done via macros, or small >> static inlines, code can still be compiled out. >> >> I do have a small nagging feeling about this series as well, but I have >> managed to convince myself it is better than the device id listed in >> i915_drv.h. > > I still feel the pain from having the endless chains of || and so > welcome the removal of devid from the macros. > >> So don't know.. we can always drop it and just expand >> platform mask to u64 to solve the immediate need and leave the rest for >> later. > > Imo, this series is an improvement, and doesn't prevent us from changing > our minds later (although not back to devid macros please!). Okay, let me still ask this: Why do we have gt member in device info, leading to plenty of device info duplication? Why should we add ULT/ULX as separate tables instead of flags in device info where they'd fid perfectly well. This choice is purely arbitrary. The only reason ULT/ULX isn't in device info is because it would lead to so much duplication. A lot of this could go away if we were able to encode a bunch of the flags in a single array similar to pciidtable. (I agree this series, modulo some nitpicks, is an improvement over status quo, I'm just playing the devil's advocate and trying to come up with something better.) BR, Jani.
On 27/03/2019 12:03, Jani Nikula wrote: > On Wed, 27 Mar 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Quoting Tvrtko Ursulin (2019-03-27 11:35:41) >>> >>> On 26/03/2019 09:53, Chris Wilson wrote: >>>> Quoting Jani Nikula (2019-03-26 09:34:45) >>>>> Not to block this series, but looking further outside the box... >>>>> >>>>> I've still got the constant vs. runtime device info split >>>>> unfinished. We've got so many things that are mostly constant, but >>>>> occasionally need changes. And we've got so many things that could be >>>>> device info flags, but would lead to proliferation of plenty of almost >>>>> identical device info structures. Like this ULX/ULT and GT number. >>>>> >>>>> So I guess I'm wondering if we're doing the right thing by assigning >>>>> device info pointers to the struct pci_device_id driver_data member in >>>>> pciidlist[] table. >>>>> >>>>> For one thing, that's a whole lot of bits that could be used directly >>>>> for assigning platform and subplatform, or features. >>>>> >>>>> Of course, we'd then need another table besides pciidlist[] to map to >>>>> the device info, but we're sort of doing some of that with the ULX/ULT >>>>> parts. >>>>> >>>>> I just overall feel that there must be a better way to do all this, and >>>>> we just haven't figured it out yet, and we're partially putting >>>>> ourselves into a box we can't break out of. >>>>> >>>>> Thoughts? >>>> >>>> I think intel_device_info is still fundamentally useful. The >>>> disadvantage of having the feature discovery separate from use is >>>> outweighed by having consistent stanzas for those features - it makes >>>> comparing platforms, finding feature sets much easier. (The cost being >>>> that with the setting of the feature flag far away from the code using >>>> it, people updating the cost are more likely to forget the flag.) >>>> >>>> One end goal of this madness, is that we can recompile the kernel module >>>> to only support a single sku and dce the rest. But what are the >>>> diminishing returns here? Without measurement, I'd say a single >>>> platform. >>>> >>>> So that dictates what can be in the static intel_device_info, features >>>> that are constant across a whole platform. And as you point out, we >>>> don't need a pointer to the device_info itself, just a platform field >>>> which is an index into the device_info block, with plenty of room for >>>> subplatform flags. >>>> >>>> While that says how we hook up device_info, I don't think that reflects >>>> on the use of feature flags themselves, or our ability to statically >>>> determine a reduced feature set. >>>> >>>> So not a box, just a mere wet paper bag. >>> >>> To check if I follow.. we are talking about potentially abolishing >>> device info in favour of constructing something at probe time, which >>> could potentially have fewer and overall smaller static data portion? >>> >>> Because I don't see how we can eliminate the pciidlist itself, or even >>> shrink it's size? It has to have one entry per device id, just the >>> question for what we use driver_data for? >> >> Correct. What I think Jani was suggesting was that instead of encoding >> the device_info pointer into driver_data, we encode the >> platform/subplatform id. We then use the platform portion to lookup the >> device_info, and subplatform to annotate the runtime_info. >> >>> Static vs runtime I think shouldn't have effect on the per platform >>> builds. As long as all feature tests are done via macros, or small >>> static inlines, code can still be compiled out. >>> >>> I do have a small nagging feeling about this series as well, but I have >>> managed to convince myself it is better than the device id listed in >>> i915_drv.h. >> >> I still feel the pain from having the endless chains of || and so >> welcome the removal of devid from the macros. >> >>> So don't know.. we can always drop it and just expand >>> platform mask to u64 to solve the immediate need and leave the rest for >>> later. >> >> Imo, this series is an improvement, and doesn't prevent us from changing >> our minds later (although not back to devid macros please!). > > Okay, let me still ask this: > > Why do we have gt member in device info, leading to plenty of device > info duplication? Why should we add ULT/ULX as separate tables instead > of flags in device info where they'd fid perfectly well. This choice is > purely arbitrary. The only reason ULT/ULX isn't in device info is > because it would lead to so much duplication. Agreed with everything here. Perhaps for instance moving GT into runtime would further improve things. Someone mentioned GT somewhere around here.. So same approach with id tables/macros as with subplatform. If the downside of not being to compile out GT configurations within a single platform is not a concern. > A lot of this could go away if we were able to encode a bunch of the > flags in a single array similar to pciidtable. I don't see how we can encode everything to distinguish a platform in an unsigned long, not even on 64-bit? > (I agree this series, modulo some nitpicks, is an improvement over > status quo, I'm just playing the devil's advocate and trying to come up > with something better.) In this case an updated version should be in your mailbox. Regards, Tvrtko
On Wed, 27 Mar 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 27/03/2019 12:03, Jani Nikula wrote: >> A lot of this could go away if we were able to encode a bunch of the >> flags in a single array similar to pciidtable. > > I don't see how we can encode everything to distinguish a platform in an > unsigned long, not even on 64-bit? Me neither. That's why I'm blabbering here instead of writing a patch. :p BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 5465b99b4392..74255374cc6b 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -868,6 +868,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) if (i915_inject_load_failure()) return -ENODEV; + intel_device_info_subplatform_init(dev_priv); + spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); mutex_init(&dev_priv->backlight_lock); @@ -1718,10 +1720,12 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) if (drm_debug & DRM_UT_DRIVER) { struct drm_printer p = drm_debug_printer("i915 device info:"); - drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s gen=%i\n", + drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (subplatform=0x%x) gen=%i\n", INTEL_DEVID(dev_priv), INTEL_REVID(dev_priv), intel_platform_name(INTEL_INFO(dev_priv)->platform), + intel_subplatform(RUNTIME_INFO(dev_priv), + INTEL_INFO(dev_priv)->platform), INTEL_GEN(dev_priv)); intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p); @@ -1764,8 +1768,6 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent) memcpy(device_info, match_info, sizeof(*device_info)); RUNTIME_INFO(i915)->device_id = pdev->device; - BUILD_BUG_ON(INTEL_MAX_PLATFORMS > - BITS_PER_TYPE(device_info->platform_mask)); BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask)); return i915; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 48c3b139f36f..c29ebfd94065 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2298,7 +2298,68 @@ static inline unsigned int i915_sg_segment_size(void) #define IS_REVID(p, since, until) \ (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) -#define IS_PLATFORM(dev_priv, p) (INTEL_INFO(dev_priv)->platform_mask & BIT(p)) +static __always_inline unsigned int +__platform_mask_index(const struct intel_runtime_info *info, + enum intel_platform p) +{ + const unsigned int pbits = + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; + + /* Expand the platform_mask array if this fails. */ + BUILD_BUG_ON(INTEL_MAX_PLATFORMS > + pbits * ARRAY_SIZE(info->platform_mask)); + + return p / pbits; +} + +static __always_inline unsigned int +__platform_mask_bit(const struct intel_runtime_info *info, + enum intel_platform p) +{ + const unsigned int pbits = + BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS; + + return p % pbits + INTEL_SUBPLATFORM_BITS; +} + +static inline u32 +intel_subplatform(const struct intel_runtime_info *info, + enum intel_platform p) +{ + const unsigned int pi = __platform_mask_index(info, p); + + return info->platform_mask[pi] & INTEL_SUBPLATFORM_BITS; +} + +static __always_inline bool +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p) +{ + const struct intel_runtime_info *info = RUNTIME_INFO(i915); + const unsigned int pi = __platform_mask_index(info, p); + const unsigned int pb = __platform_mask_bit(info, p); + + BUILD_BUG_ON(!__builtin_constant_p(p)); + + return info->platform_mask[pi] & BIT(pb); +} + +static __always_inline bool +IS_SUBPLATFORM(const struct drm_i915_private *i915, + enum intel_platform p, unsigned int s) +{ + const struct intel_runtime_info *info = RUNTIME_INFO(i915); + const unsigned int pi = __platform_mask_index(info, p); + const unsigned int pb = __platform_mask_bit(info, p); + const unsigned int msb = BITS_PER_TYPE(info->platform_mask[0]) - 1; + const u32 mask = info->platform_mask[pi]; + + BUILD_BUG_ON(!__builtin_constant_p(p)); + BUILD_BUG_ON(!__builtin_constant_p(s)); + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); + + /* Shift and test on the MSB position so sign flag can be used. */ + return ((mask << (msb - pb)) & (mask << (msb - s))) & BIT(msb); +} #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile) @@ -2337,43 +2398,32 @@ static inline unsigned int i915_sg_segment_size(void) #define IS_ELKHARTLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \ (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00) -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \ - ((INTEL_DEVID(dev_priv) & 0xf) == 0x6 || \ - (INTEL_DEVID(dev_priv) & 0xf) == 0xb || \ - (INTEL_DEVID(dev_priv) & 0xf) == 0xe)) -/* ULX machines are also considered ULT. */ -#define IS_BDW_ULX(dev_priv) (IS_BROADWELL(dev_priv) && \ - (INTEL_DEVID(dev_priv) & 0xf) == 0xe) +#define IS_BDW_ULT(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULT) +#define IS_BDW_ULX(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, INTEL_SUBPLATFORM_ULX) #define IS_BDW_GT3(dev_priv) (IS_BROADWELL(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 3) -#define IS_HSW_ULT(dev_priv) (IS_HASWELL(dev_priv) && \ - (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00) +#define IS_HSW_ULT(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULT) #define IS_HSW_GT3(dev_priv) (IS_HASWELL(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 3) #define IS_HSW_GT1(dev_priv) (IS_HASWELL(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 1) /* ULX machines are also considered ULT. */ -#define IS_HSW_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x0A0E || \ - INTEL_DEVID(dev_priv) == 0x0A1E) -#define IS_SKL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x1906 || \ - INTEL_DEVID(dev_priv) == 0x1913 || \ - INTEL_DEVID(dev_priv) == 0x1916 || \ - INTEL_DEVID(dev_priv) == 0x1921 || \ - INTEL_DEVID(dev_priv) == 0x1926) -#define IS_SKL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x190E || \ - INTEL_DEVID(dev_priv) == 0x1915 || \ - INTEL_DEVID(dev_priv) == 0x191E) -#define IS_KBL_ULT(dev_priv) (INTEL_DEVID(dev_priv) == 0x5906 || \ - INTEL_DEVID(dev_priv) == 0x5913 || \ - INTEL_DEVID(dev_priv) == 0x5916 || \ - INTEL_DEVID(dev_priv) == 0x5921 || \ - INTEL_DEVID(dev_priv) == 0x5926) -#define IS_KBL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x590E || \ - INTEL_DEVID(dev_priv) == 0x5915 || \ - INTEL_DEVID(dev_priv) == 0x591E) -#define IS_AML_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x591C || \ - INTEL_DEVID(dev_priv) == 0x87C0 || \ - INTEL_DEVID(dev_priv) == 0x87CA) +#define IS_HSW_ULX(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, INTEL_SUBPLATFORM_ULX) +#define IS_SKL_ULT(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULT) +#define IS_SKL_ULX(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, INTEL_SUBPLATFORM_ULX) +#define IS_KBL_ULT(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULT) +#define IS_KBL_ULX(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_ULX) +#define IS_AML_ULX(dev_priv) \ + (IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, INTEL_SUBPLATFORM_AML) || \ + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_AML)) #define IS_SKL_GT2(dev_priv) (IS_SKYLAKE(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 2) #define IS_SKL_GT3(dev_priv) (IS_SKYLAKE(dev_priv) && \ @@ -2384,16 +2434,16 @@ static inline unsigned int i915_sg_segment_size(void) INTEL_INFO(dev_priv)->gt == 2) #define IS_KBL_GT3(dev_priv) (IS_KABYLAKE(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 3) -#define IS_CFL_ULT(dev_priv) (IS_COFFEELAKE(dev_priv) && \ - (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0) +#define IS_CFL_ULT(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, INTEL_SUBPLATFORM_ULT) #define IS_CFL_GT2(dev_priv) (IS_COFFEELAKE(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 2) #define IS_CFL_GT3(dev_priv) (IS_COFFEELAKE(dev_priv) && \ INTEL_INFO(dev_priv)->gt == 3) -#define IS_CNL_WITH_PORT_F(dev_priv) (IS_CANNONLAKE(dev_priv) && \ - (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004) -#define IS_ICL_WITH_PORT_F(dev_priv) (IS_ICELAKE(dev_priv) && \ - INTEL_DEVID(dev_priv) != 0x8A51) +#define IS_CNL_WITH_PORT_F(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, INTEL_SUBPLATFORM_PORTF) +#define IS_ICL_WITH_PORT_F(dev_priv) \ + IS_SUBPLATFORM(dev_priv, INTEL_ICELAKE, INTEL_SUBPLATFORM_PORTF) #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index a9557f92756f..0c980b899056 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -677,6 +677,9 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m, err_printf(m, "Reset count: %u\n", error->reset_count); err_printf(m, "Suspend count: %u\n", error->suspend_count); err_printf(m, "Platform: %s\n", intel_platform_name(error->device_info.platform)); + err_printf(m, "Subplatform: 0x%x\n", + intel_subplatform(&error->runtime_info, + error->device_info.platform)); err_print_pciid(m, m->i915); err_printf(m, "IOMMU enabled?: %d\n", error->iommu); diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 716f2f95c57d..39251586349a 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -32,7 +32,7 @@ #include "i915_globals.h" #include "i915_selftest.h" -#define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) +#define PLATFORM(x) .platform = (x) #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) #define I845_PIPE_OFFSETS \ diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index e0ac908bb4e9..85da87e14c02 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -714,6 +714,151 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv) return 0; } +#undef INTEL_VGA_DEVICE +#define INTEL_VGA_DEVICE(id, info) (id) + +static const u16 hsw_ult_ids[] = { + INTEL_HSW_ULT_GT1_IDS(0), + INTEL_HSW_ULT_GT2_IDS(0), + INTEL_HSW_ULT_GT3_IDS(0) +}; + +static const u16 hsw_ulx_ids[] = { + INTEL_HSW_ULX_GT1_IDS(0), + INTEL_HSW_ULX_GT2_IDS(0) +}; + +static const u16 bdw_ult_ids[] = { + INTEL_BDW_ULT_GT1_IDS(0), + INTEL_BDW_ULT_GT2_IDS(0), + INTEL_BDW_ULT_GT3_IDS(0), + INTEL_BDW_ULT_RSVD_IDS(0) +}; + +static const u16 bdw_ulx_ids[] = { + INTEL_BDW_ULX_GT1_IDS(0), + INTEL_BDW_ULX_GT2_IDS(0), + INTEL_BDW_ULX_GT3_IDS(0), + INTEL_BDW_ULX_RSVD_IDS(0) +}; + +static const u16 skl_ult_ids[] = { + INTEL_SKL_ULT_GT1_IDS(0), + INTEL_SKL_ULT_GT2_IDS(0), + INTEL_SKL_ULT_GT3_IDS(0) +}; + +static const u16 skl_ulx_ids[] = { + INTEL_SKL_ULX_GT1_IDS(0), + INTEL_SKL_ULX_GT2_IDS(0) +}; + +static const u16 kbl_ult_ids[] = { + INTEL_KBL_ULT_GT1_IDS(0), + INTEL_KBL_ULT_GT2_IDS(0), + INTEL_KBL_ULT_GT3_IDS(0) +}; + +static const u16 kbl_ulx_ids[] = { + INTEL_KBL_ULX_GT1_IDS(0), + INTEL_KBL_ULX_GT2_IDS(0) +}; + +static const u16 kbl_aml_ids[] = { + INTEL_AML_KBL_GT2_IDS(0) +}; + +static const u16 cfl_ult_ids[] = { + INTEL_CFL_U_GT2_IDS(0), + INTEL_CFL_U_GT3_IDS(0), + INTEL_WHL_U_GT1_IDS(0), + INTEL_WHL_U_GT2_IDS(0), + INTEL_WHL_U_GT3_IDS(0) +}; + +static const u16 cfl_aml_ids[] = { + INTEL_AML_CFL_GT2_IDS(0) +}; + +static const u16 cnl_portf_ids[] = { + INTEL_CNL_PORT_F_IDS(0) +}; + +static const u16 icl_portf_ids[] = { + INTEL_ICL_PORT_F_IDS(0) +}; + +static bool find_devid(u16 id, const u16 *p, unsigned int num) +{ + for (; num; num--, p++) { + if (*p == id) + return true; + } + + return false; +} + +void intel_device_info_subplatform_init(struct drm_i915_private *i915) +{ + const struct intel_device_info *info = INTEL_INFO(i915); + const struct intel_runtime_info *rinfo = RUNTIME_INFO(i915); + const unsigned int pi = __platform_mask_index(rinfo, info->platform); + const unsigned int pb = __platform_mask_bit(rinfo, info->platform); + u16 devid = INTEL_DEVID(i915); + u32 mask = 0; + + RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb); + + if (IS_HASWELL(i915)) { + if (find_devid(devid, hsw_ult_ids, ARRAY_SIZE(hsw_ult_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULT); + else if (find_devid(devid, hsw_ulx_ids, + ARRAY_SIZE(hsw_ulx_ids))) + /* ULX machines are also considered ULT. */ + mask = BIT(INTEL_SUBPLATFORM_ULT) | + BIT(INTEL_SUBPLATFORM_ULX); + } else if (IS_BROADWELL(i915)) { + if (find_devid(devid, bdw_ult_ids, ARRAY_SIZE(bdw_ult_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULT); + else if (find_devid(devid, bdw_ulx_ids, + ARRAY_SIZE(bdw_ulx_ids))) + /* ULX machines are also considered ULT. */ + mask = BIT(INTEL_SUBPLATFORM_ULT) | + BIT(INTEL_SUBPLATFORM_ULX); + } else if (IS_SKYLAKE(i915)) { + if (find_devid(devid, skl_ult_ids, ARRAY_SIZE(skl_ult_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULT); + else if (find_devid(devid, skl_ulx_ids, + ARRAY_SIZE(skl_ulx_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULX); + } else if (IS_KABYLAKE(i915)) { + if (find_devid(devid, kbl_ult_ids, ARRAY_SIZE(kbl_ult_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULT); + else if (find_devid(devid, kbl_ulx_ids, + ARRAY_SIZE(kbl_ulx_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULX); + else if (find_devid(devid, kbl_aml_ids, + ARRAY_SIZE(kbl_aml_ids))) + mask = BIT(INTEL_SUBPLATFORM_AML); + } else if (IS_COFFEELAKE(i915)) { + if (find_devid(devid, cfl_ult_ids, ARRAY_SIZE(cfl_ult_ids))) + mask = BIT(INTEL_SUBPLATFORM_ULT); + else if (find_devid(devid, cfl_aml_ids, + ARRAY_SIZE(cfl_aml_ids))) + mask = BIT(INTEL_SUBPLATFORM_AML); + } else if (IS_CANNONLAKE(i915)) { + if (find_devid(devid, cnl_portf_ids, ARRAY_SIZE(cnl_portf_ids))) + mask = BIT(INTEL_SUBPLATFORM_PORTF); + } else if (IS_ICELAKE(i915)) { + if (find_devid(devid, icl_portf_ids, ARRAY_SIZE(icl_portf_ids))) + mask = BIT(INTEL_SUBPLATFORM_PORTF); + } + + GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS); + + RUNTIME_INFO(i915)->platform_mask[pi] |= mask; +} + /** * intel_device_info_runtime_init - initialize runtime info * @dev_priv: the i915 device diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 98acefaacec9..da1152eef46b 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -77,6 +77,21 @@ enum intel_platform { INTEL_MAX_PLATFORMS }; +/* + * Subplatform bits share the same namespace per parent platform. In other words + * it is fine for the same bit to be used on multiple parent platforms. + */ + +#define INTEL_SUBPLATFORM_BITS (3) + +/* HSW/BDW/SKL/KBL/CFL */ +#define INTEL_SUBPLATFORM_ULT (0) +#define INTEL_SUBPLATFORM_ULX (1) +#define INTEL_SUBPLATFORM_AML (2) + +/* CNL/ICL */ +#define INTEL_SUBPLATFORM_PORTF (0) + enum intel_ppgtt_type { INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE, INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING, @@ -160,7 +175,6 @@ struct intel_device_info { intel_engine_mask_t engine_mask; /* Engines supported by the HW */ enum intel_platform platform; - u32 platform_mask; enum intel_ppgtt_type ppgtt_type; unsigned int ppgtt_size; /* log2, e.g. 31/32/48 bits */ @@ -197,6 +211,16 @@ struct intel_device_info { }; struct intel_runtime_info { + /* + * Platform mask is used for optimizing or-ed IS_PLATFORM calls into + * into single runtime conditionals, and also to provide groundwork + * for future per platform, or per SKU build optimizations. + * + * Array can be extended when necessary if the corresponding + * BUILD_BUG_ON is hit. + */ + u32 platform_mask[2]; + u16 device_id; u8 num_sprites[I915_MAX_PIPES]; @@ -271,6 +295,7 @@ static inline void sseu_set_eus(struct sseu_dev_info *sseu, const char *intel_platform_name(enum intel_platform platform); +void intel_device_info_subplatform_init(struct drm_i915_private *dev_priv); void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); void intel_device_info_dump_flags(const struct intel_device_info *info, struct drm_printer *p);