* [PATCH] drm/i915: Introduce concept of a sub-platform
@ 2019-03-15 12:26 Tvrtko Ursulin
2019-03-15 13:16 ` Chris Wilson
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 12:26 UTC (permalink / raw)
To: Intel-gfx; +Cc: Jani Nikula, Lucas De Marchi
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.
The approach is also beneficial to driver size, with an combined shrink of
code and strings of around 1.7 kiB.
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.
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>
---
drivers/gpu/drm/i915/i915_drv.c | 7 +-
drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
drivers/gpu/drm/i915/i915_pci.c | 2 +-
drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
5 files changed, 179 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0d743907e7bc..3218350cd225 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -863,6 +863,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);
@@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
INTEL_DEVID(dev_priv),
INTEL_REVID(dev_priv),
intel_platform_name(INTEL_INFO(dev_priv)->platform),
+ RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
INTEL_GEN(dev_priv));
intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
@@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2281,7 +2281,46 @@ 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))
+#define __IS_PLATFORM(dev_priv, p) \
+({ \
+ const unsigned int pbits__ = \
+ BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
+ INTEL_SUBPLATFORM_BITS; \
+ const unsigned int pi__ = (p) / pbits__; \
+ const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
+\
+ BUILD_BUG_ON(!__builtin_constant_p(p)); \
+\
+ (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
+})
+
+#define __IS_SUBPLATFORM(dev_priv, p, s) \
+({ \
+ const unsigned int pbits__ = \
+ BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
+ INTEL_SUBPLATFORM_BITS; \
+ const unsigned int pi__ = (p) / pbits__; \
+ const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
+\
+ BUILD_BUG_ON(!__builtin_constant_p(p)); \
+ BUILD_BUG_ON(!__builtin_constant_p(s)); \
+ BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
+\
+ (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
+})
+
+static inline bool
+IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
+{
+ return __IS_PLATFORM(i915, p);
+}
+
+static inline bool
+IS_SUBPLATFORM(const struct drm_i915_private *i915,
+ enum intel_platform p, unsigned int s)
+{
+ return __IS_SUBPLATFORM(i915, p, s);
+}
#define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830)
#define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G)
@@ -2296,11 +2335,15 @@ static inline unsigned int i915_sg_segment_size(void)
#define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45)
#define IS_GM45(dev_priv) IS_PLATFORM(dev_priv, INTEL_GM45)
#define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv))
-#define IS_PINEVIEW_G(dev_priv) (INTEL_DEVID(dev_priv) == 0xa001)
-#define IS_PINEVIEW_M(dev_priv) (INTEL_DEVID(dev_priv) == 0xa011)
+#define IS_PINEVIEW_G(dev_priv) \
+ IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_G)
+#define IS_PINEVIEW_M(dev_priv) \
+ IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_M)
#define IS_PINEVIEW(dev_priv) IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
#define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33)
-#define IS_IRONLAKE_M(dev_priv) (INTEL_DEVID(dev_priv) == 0x0046)
+#define IS_IRONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
+#define IS_IRONLAKE_M(dev_priv) \
+ IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, INTEL_SUBPLATFORM_IRONLAKE_M)
#define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
#define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \
INTEL_INFO(dev_priv)->gt == 1)
@@ -2318,42 +2361,31 @@ static inline unsigned int i915_sg_segment_size(void)
#define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile)
#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)
+#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_ULX)
#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) && \
@@ -2364,16 +2396,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_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 3cf697e8f1fa..2b042618e3ca 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 aac19b1c419c..b8a7e17cb443 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -707,6 +707,85 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
return 0;
}
+void intel_device_info_subplatform_init(struct drm_i915_private *i915)
+{
+ const unsigned int pbits =
+ BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
+ INTEL_SUBPLATFORM_BITS;
+ const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
+ const unsigned int pb =
+ INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
+ struct intel_runtime_info *info = RUNTIME_INFO(i915);
+ u16 devid = INTEL_DEVID(i915);
+
+ BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
+ pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
+
+ info->platform_mask[pi] = BIT(pb);
+
+ if (IS_PINEVIEW(i915)) {
+ if (devid == 0xa001)
+ info->platform_mask[pi] |=
+ BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
+ else if (devid == 0xa011)
+ info->platform_mask[pi] |=
+ BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
+ } else if (IS_IRONLAKE(i915)) {
+ if (devid == 0x0046)
+ info->platform_mask[pi] |=
+ BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
+ } else if (IS_HASWELL(i915)) {
+ if ((devid & 0xFF00) == 0x0A00)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ /* ULX machines are also considered ULT. */
+ if (devid == 0x0A0E || devid == 0x0A1E)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ } else if (IS_BROADWELL(i915)) {
+ if ((devid & 0xf) == 0x6 ||
+ (devid & 0xf) == 0xb ||
+ (devid & 0xf) == 0xe)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ /* ULX machines are also considered ULT. */
+ if ((devid & 0xf) == 0xe)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ } else if (IS_SKYLAKE(i915)) {
+ if (devid == 0x1906 ||
+ devid == 0x1913 ||
+ devid == 0x1916 ||
+ devid == 0x1921 ||
+ devid == 0x1926)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ else if (devid == 0x190E ||
+ devid == 0x1915 ||
+ devid == 0x191E)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ } else if (IS_KABYLAKE(i915)) {
+ if (devid == 0x5906 ||
+ devid == 0x5913 ||
+ devid == 0x5916 ||
+ devid == 0x5921 ||
+ devid == 0x5926)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ else if (devid == 0x590E ||
+ devid == 0x5915 ||
+ devid == 0x591E)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ else if (devid == 0x591C ||
+ devid == 0x87C0)
+ info->platform_mask[pi] |=
+ BIT(INTEL_SUBPLATFORM_AML_ULX);
+ } else if (IS_COFFEELAKE(i915)) {
+ if ((devid & 0x00F0) == 0x00A0)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ } else if (IS_CANNONLAKE(i915)) {
+ if ((devid & 0x0004) == 0x0004)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
+ } else if (IS_ICELAKE(i915)) {
+ if (devid != 0x8A51)
+ info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
+ }
+}
+
/**
* 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 047d10bdd455..b03fbd2e451a 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -44,7 +44,7 @@ enum intel_platform {
INTEL_I915G,
INTEL_I915GM,
INTEL_I945G,
- INTEL_I945GM,
+ INTEL_I945GM = 8,
INTEL_G33,
INTEL_PINEVIEW,
/* gen4 */
@@ -55,7 +55,7 @@ enum intel_platform {
/* gen5 */
INTEL_IRONLAKE,
/* gen6 */
- INTEL_SANDYBRIDGE,
+ INTEL_SANDYBRIDGE = 16,
/* gen7 */
INTEL_IVYBRIDGE,
INTEL_VALLEYVIEW,
@@ -66,7 +66,7 @@ enum intel_platform {
/* gen9 */
INTEL_SKYLAKE,
INTEL_BROXTON,
- INTEL_KABYLAKE,
+ INTEL_KABYLAKE = 24,
INTEL_GEMINILAKE,
INTEL_COFFEELAKE,
/* gen10 */
@@ -76,6 +76,24 @@ 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)
+
+#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
+
+#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
+#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
+
+#define INTEL_SUBPLATFORM_ULT (0)
+#define INTEL_SUBPLATFORM_ULX (1)
+#define INTEL_SUBPLATFORM_AML_ULX (2)
+
+#define INTEL_SUBPLATFORM_PORTF (0)
+
enum intel_ppgtt {
INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING,
@@ -160,7 +178,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 ppgtt;
unsigned int page_sizes; /* page sizes supported by the HW */
@@ -195,6 +212,8 @@ struct intel_device_info {
};
struct intel_runtime_info {
+ u32 platform_mask[1];
+
u16 device_id;
u8 num_sprites[I915_MAX_PIPES];
@@ -269,6 +288,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);
--
2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
@ 2019-03-15 13:16 ` Chris Wilson
2019-03-15 13:32 ` Tvrtko Ursulin
2019-03-15 14:09 ` Ville Syrjälä
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2019-03-15 13:16 UTC (permalink / raw)
To: Intel-gfx, Tvrtko Ursulin; +Cc: Jani Nikula, Lucas De Marchi
Quoting Tvrtko Ursulin (2019-03-15 12:26:33)
> 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.
>
> The approach is also beneficial to driver size, with an combined shrink of
> code and strings of around 1.7 kiB.
>
> 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.
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
> 5 files changed, 179 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0d743907e7bc..3218350cd225 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -863,6 +863,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);
> @@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
> INTEL_DEVID(dev_priv),
> INTEL_REVID(dev_priv),
> intel_platform_name(INTEL_INFO(dev_priv)->platform),
> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
I hope that's a one-off!
> INTEL_GEN(dev_priv));
>
> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2281,7 +2281,46 @@ 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))
> +#define __IS_PLATFORM(dev_priv, p) \
> +({ \
> + const unsigned int pbits__ = \
> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> + INTEL_SUBPLATFORM_BITS; \
> + const unsigned int pi__ = (p) / pbits__; \
> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
Oh, p is a compile time constant, so these can all be evaluated at
compile time. So not a worry.
> +\
> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> +\
> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
> +})
> +
> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
> +({ \
> + const unsigned int pbits__ = \
> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> + INTEL_SUBPLATFORM_BITS; \
> + const unsigned int pi__ = (p) / pbits__; \
> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> +\
> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
> +\
> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
> +})
> +
> +static inline bool
> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
> +{
> + return __IS_PLATFORM(i915, p);
> +}
> +
> +static inline bool
> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
> + enum intel_platform p, unsigned int s)
> +{
> + return __IS_SUBPLATFORM(i915, p, s);
> +}
Ok, that all makes sense as a custom bitmap.
> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> +{
> + const unsigned int pbits =
> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
> + INTEL_SUBPLATFORM_BITS;
> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
> + const unsigned int pb =
> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
> + u16 devid = INTEL_DEVID(i915);
> +
> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
> +
> + info->platform_mask[pi] = BIT(pb);
> +
> + if (IS_PINEVIEW(i915)) {
> + if (devid == 0xa001)
> + info->platform_mask[pi] |=
> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> + else if (devid == 0xa011)
> + info->platform_mask[pi] |=
> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
if (IS_PINEVIEW(i915)) {
int subplatform = 0;
if (devid == 0xa001)
subplatform = INTEL_SUBPLATFORM_PINEVIEW_G;
else if (devid == 0xa001)
subplatform = INTEL_SUBPLATFORM_PINEVIEW_M;
else
MISSING_CASE(devid);
info->platform_mask[pi] |= BIT(subplatform);
WARN_ON(!IS_SUBPLATFORM(i915, INTEL_PLATFORM_PINEVIEW, subplatform));
}
So we catch missing ids, and validate subplatform against
SUBPLATFORM_BITS.
> /**
> * 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 047d10bdd455..b03fbd2e451a 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -44,7 +44,7 @@ enum intel_platform {
> INTEL_I915G,
> INTEL_I915GM,
> INTEL_I945G,
> - INTEL_I945GM,
> + INTEL_I945GM = 8,
> INTEL_G33,
> INTEL_PINEVIEW,
> /* gen4 */
> @@ -55,7 +55,7 @@ enum intel_platform {
> /* gen5 */
> INTEL_IRONLAKE,
> /* gen6 */
> - INTEL_SANDYBRIDGE,
> + INTEL_SANDYBRIDGE = 16,
> /* gen7 */
> INTEL_IVYBRIDGE,
> INTEL_VALLEYVIEW,
> @@ -66,7 +66,7 @@ enum intel_platform {
> /* gen9 */
> INTEL_SKYLAKE,
> INTEL_BROXTON,
> - INTEL_KABYLAKE,
> + INTEL_KABYLAKE = 24,
Looks like you are just keeping a tally, and no I have no idea how to
add a comment to make that clear.
/* tally */
/* tally so far */
> INTEL_GEMINILAKE,
> INTEL_COFFEELAKE,
> /* gen10 */
> @@ -76,6 +76,24 @@ 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)
> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
I can't believe you haven't done i852/i855! (Or whatever that variant
was called.)
Couldn't spot anything wrong, and so long as subplatform remains clear
in the error state, hint hint, I'm happy.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 13:16 ` Chris Wilson
@ 2019-03-15 13:32 ` Tvrtko Ursulin
2019-03-15 13:42 ` Chris Wilson
0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 13:32 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx; +Cc: Jani Nikula, Lucas De Marchi
On 15/03/2019 13:16, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-15 12:26:33)
>> 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.
>>
>> The approach is also beneficial to driver size, with an combined shrink of
>> code and strings of around 1.7 kiB.
>>
>> 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.
>>
>> 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>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 7 +-
>> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
>> drivers/gpu/drm/i915/i915_pci.c | 2 +-
>> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
>> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
>> 5 files changed, 179 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 0d743907e7bc..3218350cd225 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -863,6 +863,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);
>> @@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
>> INTEL_DEVID(dev_priv),
>> INTEL_REVID(dev_priv),
>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
>
> I hope that's a one-off!
It's indeed horrible. I was thinking of adding a helper but decided to
wait for some general acks first.
>> INTEL_GEN(dev_priv));
>>
>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2281,7 +2281,46 @@ 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))
>> +#define __IS_PLATFORM(dev_priv, p) \
>> +({ \
>> + const unsigned int pbits__ = \
>> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> + INTEL_SUBPLATFORM_BITS; \
>> + const unsigned int pi__ = (p) / pbits__; \
>> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>
> Oh, p is a compile time constant, so these can all be evaluated at
> compile time. So not a worry.
>
>> +\
>> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> +\
>> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>> +})
>> +
>> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
>> +({ \
>> + const unsigned int pbits__ = \
>> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> + INTEL_SUBPLATFORM_BITS; \
>> + const unsigned int pi__ = (p) / pbits__; \
>> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>> +\
>> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
>> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
>> +\
>> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
>> +})
>> +
>> +static inline bool
>> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
>> +{
>> + return __IS_PLATFORM(i915, p);
>> +}
>> +
>> +static inline bool
>> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
>> + enum intel_platform p, unsigned int s)
>> +{
>> + return __IS_SUBPLATFORM(i915, p, s);
>> +}
>
> Ok, that all makes sense as a custom bitmap.
Hm head scratch.. why I needed macro and a static inline? I'll check if
I can get away with only the latter. This fiddling was all about keeping
WARN_ON strings readable (and short!).
>
>> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
>> +{
>> + const unsigned int pbits =
>> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
>> + INTEL_SUBPLATFORM_BITS;
>> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
>> + const unsigned int pb =
>> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
>> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
>> + u16 devid = INTEL_DEVID(i915);
>> +
>> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
>> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
>> +
>> + info->platform_mask[pi] = BIT(pb);
>> +
>> + if (IS_PINEVIEW(i915)) {
>> + if (devid == 0xa001)
>> + info->platform_mask[pi] |=
>> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
>> + else if (devid == 0xa011)
>> + info->platform_mask[pi] |=
>> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
>
> if (IS_PINEVIEW(i915)) {
> int subplatform = 0;
>
> if (devid == 0xa001)
> subplatform = INTEL_SUBPLATFORM_PINEVIEW_G;
> else if (devid == 0xa001)
> subplatform = INTEL_SUBPLATFORM_PINEVIEW_M;
> else
> MISSING_CASE(devid);
With MISSING_CASE you are talking specifically for Pineview since it
only has a total of two ids? You expect more parts to ship? :))
Otherwise the logic here is only to mention ids which are special, not
all platform knows of. So as a general pattern MISSING_CASE wouldn't work.
>
> info->platform_mask[pi] |= BIT(subplatform);
> WARN_ON(!IS_SUBPLATFORM(i915, INTEL_PLATFORM_PINEVIEW, subplatform));
> }
>
> So we catch missing ids, and validate subplatform against
> SUBPLATFORM_BITS.
Not so straightforward with the ULT and ULX bunch since some of those
are both.
What I have added locally however is this:
GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS);
RUNTIME_INFO(i915)->platform_mask[pi] |= mask;
Moved the mask assignment to end so that I can check subplatform did not
overflow the allocated space.
>> /**
>> * 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 047d10bdd455..b03fbd2e451a 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -44,7 +44,7 @@ enum intel_platform {
>> INTEL_I915G,
>> INTEL_I915GM,
>> INTEL_I945G,
>> - INTEL_I945GM,
>> + INTEL_I945GM = 8,
>> INTEL_G33,
>> INTEL_PINEVIEW,
>> /* gen4 */
>> @@ -55,7 +55,7 @@ enum intel_platform {
>> /* gen5 */
>> INTEL_IRONLAKE,
>> /* gen6 */
>> - INTEL_SANDYBRIDGE,
>> + INTEL_SANDYBRIDGE = 16,
>> /* gen7 */
>> INTEL_IVYBRIDGE,
>> INTEL_VALLEYVIEW,
>> @@ -66,7 +66,7 @@ enum intel_platform {
>> /* gen9 */
>> INTEL_SKYLAKE,
>> INTEL_BROXTON,
>> - INTEL_KABYLAKE,
>> + INTEL_KABYLAKE = 24,
>
> Looks like you are just keeping a tally, and no I have no idea how to
> add a comment to make that clear.
>
> /* tally */
> /* tally so far */
Yeah.. I kept re-counting to see how it will look when the array is
expanded, whether I should reserve more suplatfoms bits right now for
cut-off to fall somewhere reasonable.
I can remove these markers, they are not that useful.
>
>> INTEL_GEMINILAKE,
>> INTEL_COFFEELAKE,
>> /* gen10 */
>> @@ -76,6 +76,24 @@ 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)
>
>> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
>
> I can't believe you haven't done i852/i855! (Or whatever that variant
> was called.)
IS_I85X? I don't see any sub-platforms there.
> Couldn't spot anything wrong, and so long as subplatform remains clear
> in the error state, hint hint, I'm happy.
Like how? With a string like platform names? Would be possible but is it
really needed on top of devid?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 13:32 ` Tvrtko Ursulin
@ 2019-03-15 13:42 ` Chris Wilson
0 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2019-03-15 13:42 UTC (permalink / raw)
To: Intel-gfx, Tvrtko Ursulin; +Cc: Jani Nikula, Lucas De Marchi
Quoting Tvrtko Ursulin (2019-03-15 13:32:54)
>
> On 15/03/2019 13:16, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-15 12:26:33)
> >> 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.
> >>
> >> The approach is also beneficial to driver size, with an combined shrink of
> >> code and strings of around 1.7 kiB.
> >>
> >> 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.
> >>
> >> 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>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> >> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
> >> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> >> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
> >> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
> >> 5 files changed, 179 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 0d743907e7bc..3218350cd225 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -863,6 +863,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);
> >> @@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
> >> INTEL_DEVID(dev_priv),
> >> INTEL_REVID(dev_priv),
> >> intel_platform_name(INTEL_INFO(dev_priv)->platform),
> >> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
> >
> > I hope that's a one-off!
>
> It's indeed horrible. I was thinking of adding a helper but decided to
> wait for some general acks first.
>
> >> INTEL_GEN(dev_priv));
> >>
> >> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
> >> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2281,7 +2281,46 @@ 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))
> >> +#define __IS_PLATFORM(dev_priv, p) \
> >> +({ \
> >> + const unsigned int pbits__ = \
> >> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> >> + INTEL_SUBPLATFORM_BITS; \
> >> + const unsigned int pi__ = (p) / pbits__; \
> >> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> >
> > Oh, p is a compile time constant, so these can all be evaluated at
> > compile time. So not a worry.
> >
> >> +\
> >> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> >> +\
> >> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
> >> +})
> >> +
> >> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
> >> +({ \
> >> + const unsigned int pbits__ = \
> >> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> >> + INTEL_SUBPLATFORM_BITS; \
> >> + const unsigned int pi__ = (p) / pbits__; \
> >> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> >> +\
> >> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> >> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
> >> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
> >> +\
> >> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
> >> +})
> >> +
> >> +static inline bool
> >> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
> >> +{
> >> + return __IS_PLATFORM(i915, p);
> >> +}
> >> +
> >> +static inline bool
> >> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >> + enum intel_platform p, unsigned int s)
> >> +{
> >> + return __IS_SUBPLATFORM(i915, p, s);
> >> +}
> >
> > Ok, that all makes sense as a custom bitmap.
>
> Hm head scratch.. why I needed macro and a static inline? I'll check if
> I can get away with only the latter. This fiddling was all about keeping
> WARN_ON strings readable (and short!).
>
> >
> >> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> >> +{
> >> + const unsigned int pbits =
> >> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
> >> + INTEL_SUBPLATFORM_BITS;
> >> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
> >> + const unsigned int pb =
> >> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
> >> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
> >> + u16 devid = INTEL_DEVID(i915);
> >> +
> >> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> >> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
> >> +
> >> + info->platform_mask[pi] = BIT(pb);
> >> +
> >> + if (IS_PINEVIEW(i915)) {
> >> + if (devid == 0xa001)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> >> + else if (devid == 0xa011)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
> >
> > if (IS_PINEVIEW(i915)) {
> > int subplatform = 0;
> >
> > if (devid == 0xa001)
> > subplatform = INTEL_SUBPLATFORM_PINEVIEW_G;
> > else if (devid == 0xa001)
> > subplatform = INTEL_SUBPLATFORM_PINEVIEW_M;
> > else
> > MISSING_CASE(devid);
>
> With MISSING_CASE you are talking specifically for Pineview since it
> only has a total of two ids? You expect more parts to ship? :))
>
> Otherwise the logic here is only to mention ids which are special, not
> all platform knows of. So as a general pattern MISSING_CASE wouldn't work.
Hmm. I worried about forgetting to add subplatform fields in future.
would
int subplatform = 0;
switch (devid) {
case BORING_A:
...
case BORING_Z:
break;
case SUB_AA:
...
case SUB_AZ:
subplatform = A;
break;
case SUB_BA:
...
case SUB_BZ:
subplatform = B;
break;
default:
CI_MISSING_CASE(devid);
break;
}
compile away the jumptable for the defaults when !CI?
However, that does imply that we see an example of every subplatform in
CI. Which, off course, we do!
> > info->platform_mask[pi] |= BIT(subplatform);
> > WARN_ON(!IS_SUBPLATFORM(i915, INTEL_PLATFORM_PINEVIEW, subplatform));
> > }
> >
> > So we catch missing ids, and validate subplatform against
> > SUBPLATFORM_BITS.
>
> Not so straightforward with the ULT and ULX bunch since some of those
> are both.
>
> What I have added locally however is this:
>
> GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS);
>
> RUNTIME_INFO(i915)->platform_mask[pi] |= mask;
>
> Moved the mask assignment to end so that I can check subplatform did not
> overflow the allocated space.
Fair enough.
> >> /**
> >> * 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 047d10bdd455..b03fbd2e451a 100644
> >> --- a/drivers/gpu/drm/i915/intel_device_info.h
> >> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> >> @@ -44,7 +44,7 @@ enum intel_platform {
> >> INTEL_I915G,
> >> INTEL_I915GM,
> >> INTEL_I945G,
> >> - INTEL_I945GM,
> >> + INTEL_I945GM = 8,
> >> INTEL_G33,
> >> INTEL_PINEVIEW,
> >> /* gen4 */
> >> @@ -55,7 +55,7 @@ enum intel_platform {
> >> /* gen5 */
> >> INTEL_IRONLAKE,
> >> /* gen6 */
> >> - INTEL_SANDYBRIDGE,
> >> + INTEL_SANDYBRIDGE = 16,
> >> /* gen7 */
> >> INTEL_IVYBRIDGE,
> >> INTEL_VALLEYVIEW,
> >> @@ -66,7 +66,7 @@ enum intel_platform {
> >> /* gen9 */
> >> INTEL_SKYLAKE,
> >> INTEL_BROXTON,
> >> - INTEL_KABYLAKE,
> >> + INTEL_KABYLAKE = 24,
> >
> > Looks like you are just keeping a tally, and no I have no idea how to
> > add a comment to make that clear.
> >
> > /* tally */
> > /* tally so far */
>
> Yeah.. I kept re-counting to see how it will look when the array is
> expanded, whether I should reserve more suplatfoms bits right now for
> cut-off to fall somewhere reasonable.
>
> I can remove these markers, they are not that useful.
>
> >
> >> INTEL_GEMINILAKE,
> >> INTEL_COFFEELAKE,
> >> /* gen10 */
> >> @@ -76,6 +76,24 @@ 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)
> >
> >> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
> >
> > I can't believe you haven't done i852/i855! (Or whatever that variant
> > was called.)
>
> IS_I85X? I don't see any sub-platforms there.
The gpu is slightly different; but nothing so far for us to do.
Basically, in my grand plan, where there is just one header that defines
all known pci-ids, features, and marketing strings we would need to
differentiate.
> > Couldn't spot anything wrong, and so long as subplatform remains clear
> > in the error state, hint hint, I'm happy.
>
> Like how? With a string like platform names? Would be possible but is it
> really needed on top of devid?
Because I have every devid memorized!
I'd take the subplatform %x; that's enough to see a pattern or to
investigate if might be subplatform specific.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
2019-03-15 13:16 ` Chris Wilson
@ 2019-03-15 14:09 ` Ville Syrjälä
2019-03-15 14:21 ` Tvrtko Ursulin
2019-03-15 14:36 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2019-03-15 14:09 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Jani Nikula, Intel-gfx, Lucas De Marchi
On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>
> The approach is also beneficial to driver size, with an combined shrink of
> code and strings of around 1.7 kiB.
>
> 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.
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
> 5 files changed, 179 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0d743907e7bc..3218350cd225 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -863,6 +863,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);
> @@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
> INTEL_DEVID(dev_priv),
> INTEL_REVID(dev_priv),
> intel_platform_name(INTEL_INFO(dev_priv)->platform),
> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
> INTEL_GEN(dev_priv));
>
> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2281,7 +2281,46 @@ 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))
> +#define __IS_PLATFORM(dev_priv, p) \
> +({ \
> + const unsigned int pbits__ = \
> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> + INTEL_SUBPLATFORM_BITS; \
> + const unsigned int pi__ = (p) / pbits__; \
> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> +\
> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> +\
> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
> +})
> +
> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
> +({ \
> + const unsigned int pbits__ = \
> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> + INTEL_SUBPLATFORM_BITS; \
> + const unsigned int pi__ = (p) / pbits__; \
> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> +\
> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
> +\
> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
> +})
> +
> +static inline bool
> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
> +{
> + return __IS_PLATFORM(i915, p);
> +}
> +
> +static inline bool
> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
> + enum intel_platform p, unsigned int s)
> +{
> + return __IS_SUBPLATFORM(i915, p, s);
> +}
>
> #define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830)
> #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G)
> @@ -2296,11 +2335,15 @@ static inline unsigned int i915_sg_segment_size(void)
> #define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45)
> #define IS_GM45(dev_priv) IS_PLATFORM(dev_priv, INTEL_GM45)
> #define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv))
> -#define IS_PINEVIEW_G(dev_priv) (INTEL_DEVID(dev_priv) == 0xa001)
> -#define IS_PINEVIEW_M(dev_priv) (INTEL_DEVID(dev_priv) == 0xa011)
> +#define IS_PINEVIEW_G(dev_priv) \
> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_G)
> +#define IS_PINEVIEW_M(dev_priv) \
> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_M)
> #define IS_PINEVIEW(dev_priv) IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
I have a feeling we should just use the normal IS_MOBILE() thing
here. But untangling that mess might be a bit of work.
> #define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33)
> -#define IS_IRONLAKE_M(dev_priv) (INTEL_DEVID(dev_priv) == 0x0046)
> +#define IS_IRONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
> +#define IS_IRONLAKE_M(dev_priv) \
> + IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, INTEL_SUBPLATFORM_IRONLAKE_M)
This is clearly just IS_MOBILE().
Or we just keep the current pci id checks. Frankly I see no benefit in
abstracting these few exceptions.
> #define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
> #define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \
> INTEL_INFO(dev_priv)->gt == 1)
> @@ -2318,42 +2361,31 @@ static inline unsigned int i915_sg_segment_size(void)
> #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile)
> #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)
> +#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_ULX)
Why is AML_ULX different from normal ULX?
> #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) && \
> @@ -2364,16 +2396,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)
God that PORTF thing is ugly. I would suggest we just add port_mask
or something along those lines into the device info.
Then we could perhaps even avoid adding more and more branches to
intel_setup_outputs(). Although I'm not sure we need anything for that
since we'll look at the VBT anyway (which *maybe* doesn't advertize
ports that aren't present?).
>
> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 3cf697e8f1fa..2b042618e3ca 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 aac19b1c419c..b8a7e17cb443 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -707,6 +707,85 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> +{
> + const unsigned int pbits =
> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
> + INTEL_SUBPLATFORM_BITS;
> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
> + const unsigned int pb =
> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
> + u16 devid = INTEL_DEVID(i915);
> +
> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
> +
> + info->platform_mask[pi] = BIT(pb);
> +
> + if (IS_PINEVIEW(i915)) {
> + if (devid == 0xa001)
> + info->platform_mask[pi] |=
> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> + else if (devid == 0xa011)
> + info->platform_mask[pi] |=
> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
> + } else if (IS_IRONLAKE(i915)) {
> + if (devid == 0x0046)
> + info->platform_mask[pi] |=
> + BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
> + } else if (IS_HASWELL(i915)) {
> + if ((devid & 0xFF00) == 0x0A00)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> + /* ULX machines are also considered ULT. */
> + if (devid == 0x0A0E || devid == 0x0A1E)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> + } else if (IS_BROADWELL(i915)) {
> + if ((devid & 0xf) == 0x6 ||
> + (devid & 0xf) == 0xb ||
> + (devid & 0xf) == 0xe)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> + /* ULX machines are also considered ULT. */
> + if ((devid & 0xf) == 0xe)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> + } else if (IS_SKYLAKE(i915)) {
> + if (devid == 0x1906 ||
> + devid == 0x1913 ||
> + devid == 0x1916 ||
> + devid == 0x1921 ||
> + devid == 0x1926)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> + else if (devid == 0x190E ||
> + devid == 0x1915 ||
> + devid == 0x191E)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> + } else if (IS_KABYLAKE(i915)) {
> + if (devid == 0x5906 ||
> + devid == 0x5913 ||
> + devid == 0x5916 ||
> + devid == 0x5921 ||
> + devid == 0x5926)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> + else if (devid == 0x590E ||
> + devid == 0x5915 ||
> + devid == 0x591E)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> + else if (devid == 0x591C ||
> + devid == 0x87C0)
> + info->platform_mask[pi] |=
> + BIT(INTEL_SUBPLATFORM_AML_ULX);
> + } else if (IS_COFFEELAKE(i915)) {
> + if ((devid & 0x00F0) == 0x00A0)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> + } else if (IS_CANNONLAKE(i915)) {
> + if ((devid & 0x0004) == 0x0004)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
> + } else if (IS_ICELAKE(i915)) {
> + if (devid != 0x8A51)
> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
> + }
> +}
> +
> /**
> * 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 047d10bdd455..b03fbd2e451a 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -44,7 +44,7 @@ enum intel_platform {
> INTEL_I915G,
> INTEL_I915GM,
> INTEL_I945G,
> - INTEL_I945GM,
> + INTEL_I945GM = 8,
> INTEL_G33,
> INTEL_PINEVIEW,
> /* gen4 */
> @@ -55,7 +55,7 @@ enum intel_platform {
> /* gen5 */
> INTEL_IRONLAKE,
> /* gen6 */
> - INTEL_SANDYBRIDGE,
> + INTEL_SANDYBRIDGE = 16,
> /* gen7 */
> INTEL_IVYBRIDGE,
> INTEL_VALLEYVIEW,
> @@ -66,7 +66,7 @@ enum intel_platform {
> /* gen9 */
> INTEL_SKYLAKE,
> INTEL_BROXTON,
> - INTEL_KABYLAKE,
> + INTEL_KABYLAKE = 24,
Too much magic. Looks rather fragile.
> INTEL_GEMINILAKE,
> INTEL_COFFEELAKE,
> /* gen10 */
> @@ -76,6 +76,24 @@ 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)
> +
> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
> +
> +#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
> +#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
> +
> +#define INTEL_SUBPLATFORM_ULT (0)
> +#define INTEL_SUBPLATFORM_ULX (1)
> +#define INTEL_SUBPLATFORM_AML_ULX (2)
> +
> +#define INTEL_SUBPLATFORM_PORTF (0)
> +
> enum intel_ppgtt {
> INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
> INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING,
> @@ -160,7 +178,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 ppgtt;
> unsigned int page_sizes; /* page sizes supported by the HW */
> @@ -195,6 +212,8 @@ struct intel_device_info {
> };
>
> struct intel_runtime_info {
> + u32 platform_mask[1];
> +
> u16 device_id;
>
> u8 num_sprites[I915_MAX_PIPES];
> @@ -269,6 +288,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);
> --
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 14:09 ` Ville Syrjälä
@ 2019-03-15 14:21 ` Tvrtko Ursulin
2019-03-15 15:55 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 14:21 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Jani Nikula, Intel-gfx, Lucas De Marchi
On 15/03/2019 14:09, Ville Syrjälä wrote:
> On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>>
>> The approach is also beneficial to driver size, with an combined shrink of
>> code and strings of around 1.7 kiB.
>>
>> 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.
>>
>> 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>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 7 +-
>> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
>> drivers/gpu/drm/i915/i915_pci.c | 2 +-
>> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
>> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
>> 5 files changed, 179 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 0d743907e7bc..3218350cd225 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -863,6 +863,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);
>> @@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
>> INTEL_DEVID(dev_priv),
>> INTEL_REVID(dev_priv),
>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
>> INTEL_GEN(dev_priv));
>>
>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2281,7 +2281,46 @@ 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))
>> +#define __IS_PLATFORM(dev_priv, p) \
>> +({ \
>> + const unsigned int pbits__ = \
>> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> + INTEL_SUBPLATFORM_BITS; \
>> + const unsigned int pi__ = (p) / pbits__; \
>> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>> +\
>> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> +\
>> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>> +})
>> +
>> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
>> +({ \
>> + const unsigned int pbits__ = \
>> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> + INTEL_SUBPLATFORM_BITS; \
>> + const unsigned int pi__ = (p) / pbits__; \
>> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>> +\
>> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
>> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
>> +\
>> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
>> +})
>> +
>> +static inline bool
>> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
>> +{
>> + return __IS_PLATFORM(i915, p);
>> +}
>> +
>> +static inline bool
>> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
>> + enum intel_platform p, unsigned int s)
>> +{
>> + return __IS_SUBPLATFORM(i915, p, s);
>> +}
>>
>> #define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830)
>> #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G)
>> @@ -2296,11 +2335,15 @@ static inline unsigned int i915_sg_segment_size(void)
>> #define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45)
>> #define IS_GM45(dev_priv) IS_PLATFORM(dev_priv, INTEL_GM45)
>> #define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv))
>> -#define IS_PINEVIEW_G(dev_priv) (INTEL_DEVID(dev_priv) == 0xa001)
>> -#define IS_PINEVIEW_M(dev_priv) (INTEL_DEVID(dev_priv) == 0xa011)
>> +#define IS_PINEVIEW_G(dev_priv) \
>> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_G)
>> +#define IS_PINEVIEW_M(dev_priv) \
>> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_M)
>> #define IS_PINEVIEW(dev_priv) IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
>
> I have a feeling we should just use the normal IS_MOBILE() thing
> here. But untangling that mess might be a bit of work.
>
>> #define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33)
>> -#define IS_IRONLAKE_M(dev_priv) (INTEL_DEVID(dev_priv) == 0x0046)
>> +#define IS_IRONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
>> +#define IS_IRONLAKE_M(dev_priv) \
>> + IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, INTEL_SUBPLATFORM_IRONLAKE_M)
>
> This is clearly just IS_MOBILE().
Ok.
> Or we just keep the current pci id checks. Frankly I see no benefit in
> abstracting these few exceptions.
Are you referring to Ironlake only or in general?
>
>> #define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
>> #define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \
>> INTEL_INFO(dev_priv)->gt == 1)
>> @@ -2318,42 +2361,31 @@ static inline unsigned int i915_sg_segment_size(void)
>> #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile)
>> #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)
>> +#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_ULX)
>
> Why is AML_ULX different from normal ULX?
As far as I could it is a different devid which we officially call
Amberlake, but did not add it as separate platform.
Given how this macro is used, which is always in conjuction with
IS_KBL_ULX, I considered just removing it and adding the devid to
KBL_ULX, but did not want to interferre with other people's ideas.
>
>> #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) && \
>> @@ -2364,16 +2396,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)
>
> God that PORTF thing is ugly. I would suggest we just add port_mask
> or something along those lines into the device info.
>
> Then we could perhaps even avoid adding more and more branches to
> intel_setup_outputs(). Although I'm not sure we need anything for that
> since we'll look at the VBT anyway (which *maybe* doesn't advertize
> ports that aren't present?).
No idea, sounds like a question for you display folks to decide
separately of this proposal probably?
>
>>
>> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> index 3cf697e8f1fa..2b042618e3ca 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 aac19b1c419c..b8a7e17cb443 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -707,6 +707,85 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>> return 0;
>> }
>>
>> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
>> +{
>> + const unsigned int pbits =
>> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
>> + INTEL_SUBPLATFORM_BITS;
>> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
>> + const unsigned int pb =
>> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
>> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
>> + u16 devid = INTEL_DEVID(i915);
>> +
>> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
>> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
>> +
>> + info->platform_mask[pi] = BIT(pb);
>> +
>> + if (IS_PINEVIEW(i915)) {
>> + if (devid == 0xa001)
>> + info->platform_mask[pi] |=
>> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
>> + else if (devid == 0xa011)
>> + info->platform_mask[pi] |=
>> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
>> + } else if (IS_IRONLAKE(i915)) {
>> + if (devid == 0x0046)
>> + info->platform_mask[pi] |=
>> + BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
>> + } else if (IS_HASWELL(i915)) {
>> + if ((devid & 0xFF00) == 0x0A00)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> + /* ULX machines are also considered ULT. */
>> + if (devid == 0x0A0E || devid == 0x0A1E)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> + } else if (IS_BROADWELL(i915)) {
>> + if ((devid & 0xf) == 0x6 ||
>> + (devid & 0xf) == 0xb ||
>> + (devid & 0xf) == 0xe)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> + /* ULX machines are also considered ULT. */
>> + if ((devid & 0xf) == 0xe)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> + } else if (IS_SKYLAKE(i915)) {
>> + if (devid == 0x1906 ||
>> + devid == 0x1913 ||
>> + devid == 0x1916 ||
>> + devid == 0x1921 ||
>> + devid == 0x1926)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> + else if (devid == 0x190E ||
>> + devid == 0x1915 ||
>> + devid == 0x191E)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> + } else if (IS_KABYLAKE(i915)) {
>> + if (devid == 0x5906 ||
>> + devid == 0x5913 ||
>> + devid == 0x5916 ||
>> + devid == 0x5921 ||
>> + devid == 0x5926)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> + else if (devid == 0x590E ||
>> + devid == 0x5915 ||
>> + devid == 0x591E)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
>> + else if (devid == 0x591C ||
>> + devid == 0x87C0)
>> + info->platform_mask[pi] |=
>> + BIT(INTEL_SUBPLATFORM_AML_ULX);
>> + } else if (IS_COFFEELAKE(i915)) {
>> + if ((devid & 0x00F0) == 0x00A0)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
>> + } else if (IS_CANNONLAKE(i915)) {
>> + if ((devid & 0x0004) == 0x0004)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
>> + } else if (IS_ICELAKE(i915)) {
>> + if (devid != 0x8A51)
>> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
>> + }
>> +}
>> +
>> /**
>> * 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 047d10bdd455..b03fbd2e451a 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -44,7 +44,7 @@ enum intel_platform {
>> INTEL_I915G,
>> INTEL_I915GM,
>> INTEL_I945G,
>> - INTEL_I945GM,
>> + INTEL_I945GM = 8,
>> INTEL_G33,
>> INTEL_PINEVIEW,
>> /* gen4 */
>> @@ -55,7 +55,7 @@ enum intel_platform {
>> /* gen5 */
>> INTEL_IRONLAKE,
>> /* gen6 */
>> - INTEL_SANDYBRIDGE,
>> + INTEL_SANDYBRIDGE = 16,
>> /* gen7 */
>> INTEL_IVYBRIDGE,
>> INTEL_VALLEYVIEW,
>> @@ -66,7 +66,7 @@ enum intel_platform {
>> /* gen9 */
>> INTEL_SKYLAKE,
>> INTEL_BROXTON,
>> - INTEL_KABYLAKE,
>> + INTEL_KABYLAKE = 24,
>
> Too much magic. Looks rather fragile.
Imagine these hunks gone. This is not required for this patch at all, or
for anything for that matter. What do you think is magic and fragile?
Regards,
Tvrtko
>
>> INTEL_GEMINILAKE,
>> INTEL_COFFEELAKE,
>> /* gen10 */
>> @@ -76,6 +76,24 @@ 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)
>> +
>> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
>> +
>> +#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
>> +#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
>> +
>> +#define INTEL_SUBPLATFORM_ULT (0)
>> +#define INTEL_SUBPLATFORM_ULX (1)
>> +#define INTEL_SUBPLATFORM_AML_ULX (2)
>> +
>> +#define INTEL_SUBPLATFORM_PORTF (0)
>> +
>> enum intel_ppgtt {
>> INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
>> INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING,
>> @@ -160,7 +178,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 ppgtt;
>> unsigned int page_sizes; /* page sizes supported by the HW */
>> @@ -195,6 +212,8 @@ struct intel_device_info {
>> };
>>
>> struct intel_runtime_info {
>> + u32 platform_mask[1];
>> +
>> u16 device_id;
>>
>> u8 num_sprites[I915_MAX_PIPES];
>> @@ -269,6 +288,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);
>> --
>> 2.19.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Introduce concept of a sub-platform
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
2019-03-15 13:16 ` Chris Wilson
2019-03-15 14:09 ` Ville Syrjälä
@ 2019-03-15 14:36 ` Patchwork
2019-03-15 17:09 ` [PATCH v5] " Tvrtko Ursulin
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2019-03-15 14:36 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Introduce concept of a sub-platform
URL : https://patchwork.freedesktop.org/series/58056/
State : failure
== Summary ==
Applying: drm/i915: Introduce concept of a sub-platform
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/i915_drv.c
M drivers/gpu/drm/i915/i915_drv.h
M drivers/gpu/drm/i915/i915_pci.c
M drivers/gpu/drm/i915/intel_device_info.c
M drivers/gpu/drm/i915/intel_device_info.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_device_info.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_device_info.h
Auto-merging drivers/gpu/drm/i915/intel_device_info.c
Auto-merging drivers/gpu/drm/i915/i915_pci.c
Auto-merging drivers/gpu/drm/i915/i915_drv.h
Auto-merging drivers/gpu/drm/i915/i915_drv.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Introduce concept of a sub-platform
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 14:21 ` Tvrtko Ursulin
@ 2019-03-15 15:55 ` Ville Syrjälä
2019-03-15 15:57 ` Ville Syrjälä
2019-03-15 16:05 ` Tvrtko Ursulin
0 siblings, 2 replies; 19+ messages in thread
From: Ville Syrjälä @ 2019-03-15 15:55 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Jani Nikula, Intel-gfx, Lucas De Marchi
On Fri, Mar 15, 2019 at 02:21:57PM +0000, Tvrtko Ursulin wrote:
>
> On 15/03/2019 14:09, Ville Syrjälä wrote:
> > On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
> >>
> >> The approach is also beneficial to driver size, with an combined shrink of
> >> code and strings of around 1.7 kiB.
> >>
> >> 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.
> >>
> >> 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>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> >> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
> >> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> >> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
> >> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
> >> 5 files changed, 179 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 0d743907e7bc..3218350cd225 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -863,6 +863,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);
> >> @@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
> >> INTEL_DEVID(dev_priv),
> >> INTEL_REVID(dev_priv),
> >> intel_platform_name(INTEL_INFO(dev_priv)->platform),
> >> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
> >> INTEL_GEN(dev_priv));
> >>
> >> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
> >> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2281,7 +2281,46 @@ 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))
> >> +#define __IS_PLATFORM(dev_priv, p) \
> >> +({ \
> >> + const unsigned int pbits__ = \
> >> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> >> + INTEL_SUBPLATFORM_BITS; \
> >> + const unsigned int pi__ = (p) / pbits__; \
> >> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> >> +\
> >> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> >> +\
> >> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
> >> +})
> >> +
> >> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
> >> +({ \
> >> + const unsigned int pbits__ = \
> >> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> >> + INTEL_SUBPLATFORM_BITS; \
> >> + const unsigned int pi__ = (p) / pbits__; \
> >> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> >> +\
> >> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> >> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
> >> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
> >> +\
> >> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
> >> +})
> >> +
> >> +static inline bool
> >> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
> >> +{
> >> + return __IS_PLATFORM(i915, p);
> >> +}
> >> +
> >> +static inline bool
> >> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >> + enum intel_platform p, unsigned int s)
> >> +{
> >> + return __IS_SUBPLATFORM(i915, p, s);
> >> +}
> >>
> >> #define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830)
> >> #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G)
> >> @@ -2296,11 +2335,15 @@ static inline unsigned int i915_sg_segment_size(void)
> >> #define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45)
> >> #define IS_GM45(dev_priv) IS_PLATFORM(dev_priv, INTEL_GM45)
> >> #define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv))
> >> -#define IS_PINEVIEW_G(dev_priv) (INTEL_DEVID(dev_priv) == 0xa001)
> >> -#define IS_PINEVIEW_M(dev_priv) (INTEL_DEVID(dev_priv) == 0xa011)
> >> +#define IS_PINEVIEW_G(dev_priv) \
> >> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_G)
> >> +#define IS_PINEVIEW_M(dev_priv) \
> >> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_M)
> >> #define IS_PINEVIEW(dev_priv) IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
> >
> > I have a feeling we should just use the normal IS_MOBILE() thing
> > here. But untangling that mess might be a bit of work.
> >
> >> #define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33)
> >> -#define IS_IRONLAKE_M(dev_priv) (INTEL_DEVID(dev_priv) == 0x0046)
> >> +#define IS_IRONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
> >> +#define IS_IRONLAKE_M(dev_priv) \
> >> + IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, INTEL_SUBPLATFORM_IRONLAKE_M)
> >
> > This is clearly just IS_MOBILE().
>
> Ok.
>
> > Or we just keep the current pci id checks. Frankly I see no benefit in
> > abstracting these few exceptions.
>
> Are you referring to Ironlake only or in general?
pnv and ilk.
>
> >
> >> #define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
> >> #define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \
> >> INTEL_INFO(dev_priv)->gt == 1)
> >> @@ -2318,42 +2361,31 @@ static inline unsigned int i915_sg_segment_size(void)
> >> #define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile)
> >> #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)
> >> +#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_ULX)
> >
> > Why is AML_ULX different from normal ULX?
>
> As far as I could it is a different devid which we officially call
> Amberlake, but did not add it as separate platform.
>
> Given how this macro is used, which is always in conjuction with
> IS_KBL_ULX, I considered just removing it and adding the devid to
> KBL_ULX, but did not want to interferre with other people's ideas.
This whole AML_ULX makes no sense. The "non-ULX" AML is also
docucemented to be a Y SKU, so AFAICS it should use the exact
same codepaths as KBL Y (aka. KBL_ULX). Cc:ing Jose who added this
second AML variant...
I'm really tempted to just go and nuke all CFL,AML,etc. from the
code to make it actually match the spec.
>
> >
> >> #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) && \
> >> @@ -2364,16 +2396,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)
> >
> > God that PORTF thing is ugly. I would suggest we just add port_mask
> > or something along those lines into the device info.
> >
> > Then we could perhaps even avoid adding more and more branches to
> > intel_setup_outputs(). Although I'm not sure we need anything for that
> > since we'll look at the VBT anyway (which *maybe* doesn't advertize
> > ports that aren't present?).
>
> No idea, sounds like a question for you display folks to decide
> separately of this proposal probably?
>
> >
> >>
> >> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> >> index 3cf697e8f1fa..2b042618e3ca 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 aac19b1c419c..b8a7e17cb443 100644
> >> --- a/drivers/gpu/drm/i915/intel_device_info.c
> >> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> >> @@ -707,6 +707,85 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
> >> return 0;
> >> }
> >>
> >> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> >> +{
> >> + const unsigned int pbits =
> >> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
> >> + INTEL_SUBPLATFORM_BITS;
> >> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
> >> + const unsigned int pb =
> >> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
> >> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
> >> + u16 devid = INTEL_DEVID(i915);
> >> +
> >> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> >> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
> >> +
> >> + info->platform_mask[pi] = BIT(pb);
> >> +
> >> + if (IS_PINEVIEW(i915)) {
> >> + if (devid == 0xa001)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> >> + else if (devid == 0xa011)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
> >> + } else if (IS_IRONLAKE(i915)) {
> >> + if (devid == 0x0046)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
> >> + } else if (IS_HASWELL(i915)) {
> >> + if ((devid & 0xFF00) == 0x0A00)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> >> + /* ULX machines are also considered ULT. */
> >> + if (devid == 0x0A0E || devid == 0x0A1E)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> >> + } else if (IS_BROADWELL(i915)) {
> >> + if ((devid & 0xf) == 0x6 ||
> >> + (devid & 0xf) == 0xb ||
> >> + (devid & 0xf) == 0xe)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> >> + /* ULX machines are also considered ULT. */
> >> + if ((devid & 0xf) == 0xe)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> >> + } else if (IS_SKYLAKE(i915)) {
> >> + if (devid == 0x1906 ||
> >> + devid == 0x1913 ||
> >> + devid == 0x1916 ||
> >> + devid == 0x1921 ||
> >> + devid == 0x1926)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> >> + else if (devid == 0x190E ||
> >> + devid == 0x1915 ||
> >> + devid == 0x191E)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> >> + } else if (IS_KABYLAKE(i915)) {
> >> + if (devid == 0x5906 ||
> >> + devid == 0x5913 ||
> >> + devid == 0x5916 ||
> >> + devid == 0x5921 ||
> >> + devid == 0x5926)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> >> + else if (devid == 0x590E ||
> >> + devid == 0x5915 ||
> >> + devid == 0x591E)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
> >> + else if (devid == 0x591C ||
> >> + devid == 0x87C0)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_AML_ULX);
> >> + } else if (IS_COFFEELAKE(i915)) {
> >> + if ((devid & 0x00F0) == 0x00A0)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
> >> + } else if (IS_CANNONLAKE(i915)) {
> >> + if ((devid & 0x0004) == 0x0004)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
> >> + } else if (IS_ICELAKE(i915)) {
> >> + if (devid != 0x8A51)
> >> + info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
> >> + }
> >> +}
> >> +
> >> /**
> >> * 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 047d10bdd455..b03fbd2e451a 100644
> >> --- a/drivers/gpu/drm/i915/intel_device_info.h
> >> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> >> @@ -44,7 +44,7 @@ enum intel_platform {
> >> INTEL_I915G,
> >> INTEL_I915GM,
> >> INTEL_I945G,
> >> - INTEL_I945GM,
> >> + INTEL_I945GM = 8,
> >> INTEL_G33,
> >> INTEL_PINEVIEW,
> >> /* gen4 */
> >> @@ -55,7 +55,7 @@ enum intel_platform {
> >> /* gen5 */
> >> INTEL_IRONLAKE,
> >> /* gen6 */
> >> - INTEL_SANDYBRIDGE,
> >> + INTEL_SANDYBRIDGE = 16,
> >> /* gen7 */
> >> INTEL_IVYBRIDGE,
> >> INTEL_VALLEYVIEW,
> >> @@ -66,7 +66,7 @@ enum intel_platform {
> >> /* gen9 */
> >> INTEL_SKYLAKE,
> >> INTEL_BROXTON,
> >> - INTEL_KABYLAKE,
> >> + INTEL_KABYLAKE = 24,
> >
> > Too much magic. Looks rather fragile.
>
> Imagine these hunks gone. This is not required for this patch at all, or
> for anything for that matter. What do you think is magic and fragile?
Who is going to remeber to keep adding those magic numbers
in the future.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 15:55 ` Ville Syrjälä
@ 2019-03-15 15:57 ` Ville Syrjälä
2019-03-15 16:05 ` Tvrtko Ursulin
1 sibling, 0 replies; 19+ messages in thread
From: Ville Syrjälä @ 2019-03-15 15:57 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Jani Nikula, Intel-gfx, Lucas De Marchi
On Fri, Mar 15, 2019 at 05:55:19PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 15, 2019 at 02:21:57PM +0000, Tvrtko Ursulin wrote:
> >
> > On 15/03/2019 14:09, Ville Syrjälä wrote:
> > > On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin wrote:
> > >> -#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)
> > >> +#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_ULX)
> > >
> > > Why is AML_ULX different from normal ULX?
> >
> > As far as I could it is a different devid which we officially call
> > Amberlake, but did not add it as separate platform.
> >
> > Given how this macro is used, which is always in conjuction with
> > IS_KBL_ULX, I considered just removing it and adding the devid to
> > KBL_ULX, but did not want to interferre with other people's ideas.
>
> This whole AML_ULX makes no sense. The "non-ULX" AML is also
> docucemented to be a Y SKU, so AFAICS it should use the exact
> same codepaths as KBL Y (aka. KBL_ULX). Cc:ing Jose who added this
> second AML variant...
Actually cc this time.
>
> I'm really tempted to just go and nuke all CFL,AML,etc. from the
> code to make it actually match the spec.
>
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 15:55 ` Ville Syrjälä
2019-03-15 15:57 ` Ville Syrjälä
@ 2019-03-15 16:05 ` Tvrtko Ursulin
1 sibling, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 16:05 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Jani Nikula, Intel-gfx, Lucas De Marchi
On 15/03/2019 15:55, Ville Syrjälä wrote:
> On Fri, Mar 15, 2019 at 02:21:57PM +0000, Tvrtko Ursulin wrote:
[snip]
>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>>>> index 047d10bdd455..b03fbd2e451a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>>>> @@ -44,7 +44,7 @@ enum intel_platform {
>>>> INTEL_I915G,
>>>> INTEL_I915GM,
>>>> INTEL_I945G,
>>>> - INTEL_I945GM,
>>>> + INTEL_I945GM = 8,
>>>> INTEL_G33,
>>>> INTEL_PINEVIEW,
>>>> /* gen4 */
>>>> @@ -55,7 +55,7 @@ enum intel_platform {
>>>> /* gen5 */
>>>> INTEL_IRONLAKE,
>>>> /* gen6 */
>>>> - INTEL_SANDYBRIDGE,
>>>> + INTEL_SANDYBRIDGE = 16,
>>>> /* gen7 */
>>>> INTEL_IVYBRIDGE,
>>>> INTEL_VALLEYVIEW,
>>>> @@ -66,7 +66,7 @@ enum intel_platform {
>>>> /* gen9 */
>>>> INTEL_SKYLAKE,
>>>> INTEL_BROXTON,
>>>> - INTEL_KABYLAKE,
>>>> + INTEL_KABYLAKE = 24,
>>>
>>> Too much magic. Looks rather fragile.
>>
>> Imagine these hunks gone. This is not required for this patch at all, or
>> for anything for that matter. What do you think is magic and fragile?
>
> Who is going to remeber to keep adding those magic numbers
> in the future.
They serve no purpose. :) I'll remove them from the next respin. It was
just me counting to see how many fit into one u32 of mask bits if I
reserve N low bits for the subplatform mask.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5] drm/i915: Introduce concept of a sub-platform
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
` (2 preceding siblings ...)
2019-03-15 14:36 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2019-03-15 17:09 ` Tvrtko Ursulin
2019-03-15 17:28 ` Chris Wilson
2019-03-15 17:12 ` [PATCH] " Lucas De Marchi
2019-03-15 18:22 ` ✗ Fi.CI.BAT: failure for drm/i915: Introduce concept of a sub-platform (rev3) Patchwork
5 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 17:09 UTC (permalink / raw)
To: Intel-gfx; +Cc: Jani Nikula, Lucas De Marchi
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)
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>
---
drivers/gpu/drm/i915/i915_drv.c | 7 +-
drivers/gpu/drm/i915/i915_drv.h | 131 ++++++++++++++++-------
drivers/gpu/drm/i915/i915_gpu_error.c | 1 +
drivers/gpu/drm/i915/i915_pci.c | 2 +-
drivers/gpu/drm/i915/intel_device_info.c | 71 ++++++++++++
drivers/gpu/drm/i915/intel_device_info.h | 20 +++-
6 files changed, 187 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a3b00ecc58c9..5551695a1db5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -863,6 +863,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);
@@ -1752,10 +1754,11 @@ 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(dev_priv),
INTEL_GEN(dev_priv));
intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
@@ -1798,8 +1801,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 c65c2e6649df..ea9fb7967c6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2281,7 +2281,66 @@ 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 inline unsigned int
+__platform_mask_index(const struct drm_i915_private *i915,
+ enum intel_platform p)
+{
+ const unsigned int pbits =
+ BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
+ INTEL_SUBPLATFORM_BITS;
+
+ /* Expand the platform_mask array if this fails. */
+ BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
+ pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
+
+ return p / pbits;
+}
+
+static inline unsigned int
+__platform_mask_bit(const struct drm_i915_private *i915, enum intel_platform p)
+{
+ const unsigned int pbits =
+ BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
+ INTEL_SUBPLATFORM_BITS;
+
+ return p % pbits + INTEL_SUBPLATFORM_BITS;
+}
+
+static inline u32 intel_subplatform(const struct drm_i915_private *i915)
+{
+ const unsigned int pi =
+ __platform_mask_index(i915, INTEL_INFO(i915)->platform);
+
+ return RUNTIME_INFO(i915)->platform_mask[pi] & INTEL_SUBPLATFORM_BITS;
+}
+
+static inline bool
+IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
+{
+ const unsigned int pi = __platform_mask_index(i915, p);
+ const unsigned int pb = __platform_mask_bit(i915, p);
+
+ BUILD_BUG_ON(!__builtin_constant_p(p));
+
+ return RUNTIME_INFO(i915)->platform_mask[pi] & BIT(pb);
+}
+
+static inline bool
+IS_SUBPLATFORM(const struct drm_i915_private *i915,
+ enum intel_platform p, unsigned int s)
+{
+ const unsigned int pi = __platform_mask_index(i915, p);
+ const unsigned int pb = __platform_mask_bit(i915, p);
+ const u32 mask = RUNTIME_INFO(i915)->platform_mask[pi];
+
+ BUILD_BUG_ON(!__builtin_constant_p(p));
+ BUILD_BUG_ON(!__builtin_constant_p(s));
+ BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS);
+
+ return (mask & BIT(pb)) && (mask & BIT(s));
+}
+
+#define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile)
#define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830)
#define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G)
@@ -2296,11 +2355,15 @@ static inline unsigned int i915_sg_segment_size(void)
#define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45)
#define IS_GM45(dev_priv) IS_PLATFORM(dev_priv, INTEL_GM45)
#define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv))
-#define IS_PINEVIEW_G(dev_priv) (INTEL_DEVID(dev_priv) == 0xa001)
-#define IS_PINEVIEW_M(dev_priv) (INTEL_DEVID(dev_priv) == 0xa011)
+#define IS_PINEVIEW_G(dev_priv) \
+ IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_G)
+#define IS_PINEVIEW_M(dev_priv) \
+ IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, INTEL_SUBPLATFORM_PINEVIEW_M)
#define IS_PINEVIEW(dev_priv) IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
#define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33)
-#define IS_IRONLAKE_M(dev_priv) (INTEL_DEVID(dev_priv) == 0x0046)
+#define IS_IRONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
+#define IS_IRONLAKE_M(dev_priv) \
+ (IS_PLATFORM(dev_priv, INTEL_IRONLAKE) && IS_MOBILE(dev_priv))
#define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
#define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \
INTEL_INFO(dev_priv)->gt == 1)
@@ -2315,45 +2378,33 @@ static inline unsigned int i915_sg_segment_size(void)
#define IS_COFFEELAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_COFFEELAKE)
#define IS_CANNONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_CANNONLAKE)
#define IS_ICELAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ICELAKE)
-#define IS_MOBILE(dev_priv) (INTEL_INFO(dev_priv)->is_mobile)
#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)
+#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_ULX)
#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) && \
@@ -2364,16 +2415,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 3d8020888604..e3360a31f8d3 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -677,6 +677,7 @@ 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(m->i915));
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 ef7410c492fd..1522385d098b 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 eddf83807957..367ee0e01344 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -707,6 +707,77 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
return 0;
}
+void intel_device_info_subplatform_init(struct drm_i915_private *i915)
+{
+ const unsigned int pi =
+ __platform_mask_index(i915, INTEL_INFO(i915)->platform);
+ const unsigned int pb =
+ __platform_mask_bit(i915, INTEL_INFO(i915)->platform);
+ u16 devid = INTEL_DEVID(i915);
+ u32 mask = 0;
+
+ RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb);
+
+ if (IS_PINEVIEW(i915)) {
+ if (devid == 0xa001)
+ mask |= BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
+ else if (devid == 0xa011)
+ mask |= BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
+ } else if (IS_HASWELL(i915)) {
+ if ((devid & 0xFF00) == 0x0A00)
+ mask |= BIT(INTEL_SUBPLATFORM_ULT);
+ /* ULX machines are also considered ULT. */
+ if (devid == 0x0A0E || devid == 0x0A1E)
+ mask |= BIT(INTEL_SUBPLATFORM_ULX);
+ } else if (IS_BROADWELL(i915)) {
+ if ((devid & 0xf) == 0x6 ||
+ (devid & 0xf) == 0xb ||
+ (devid & 0xf) == 0xe)
+ mask |= BIT(INTEL_SUBPLATFORM_ULT);
+ /* ULX machines are also considered ULT. */
+ if ((devid & 0xf) == 0xe)
+ mask |= BIT(INTEL_SUBPLATFORM_ULX);
+ } else if (IS_SKYLAKE(i915)) {
+ if (devid == 0x1906 ||
+ devid == 0x1913 ||
+ devid == 0x1916 ||
+ devid == 0x1921 ||
+ devid == 0x1926)
+ mask |= BIT(INTEL_SUBPLATFORM_ULT);
+ else if (devid == 0x190E ||
+ devid == 0x1915 ||
+ devid == 0x191E)
+ mask |= BIT(INTEL_SUBPLATFORM_ULX);
+ } else if (IS_KABYLAKE(i915)) {
+ if (devid == 0x5906 ||
+ devid == 0x5913 ||
+ devid == 0x5916 ||
+ devid == 0x5921 ||
+ devid == 0x5926)
+ mask |= BIT(INTEL_SUBPLATFORM_ULT);
+ else if (devid == 0x590E ||
+ devid == 0x5915 ||
+ devid == 0x591E)
+ mask |= BIT(INTEL_SUBPLATFORM_ULX);
+ else if (devid == 0x591C ||
+ devid == 0x87C0)
+ mask |= BIT(INTEL_SUBPLATFORM_AML_ULX);
+ } else if (IS_COFFEELAKE(i915)) {
+ if ((devid & 0x00F0) == 0x00A0)
+ mask |= BIT(INTEL_SUBPLATFORM_ULT);
+ } else if (IS_CANNONLAKE(i915)) {
+ if ((devid & 0x0004) == 0x0004)
+ mask |= BIT(INTEL_SUBPLATFORM_PORTF);
+ } else if (IS_ICELAKE(i915)) {
+ if (devid != 0x8A51)
+ 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 6234570a9b17..81d76fffb7b0 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -76,6 +76,22 @@ 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)
+
+#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
+#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
+
+#define INTEL_SUBPLATFORM_ULT (0)
+#define INTEL_SUBPLATFORM_ULX (1)
+#define INTEL_SUBPLATFORM_AML_ULX (2)
+
+#define INTEL_SUBPLATFORM_PORTF (0)
+
enum intel_ppgtt_type {
INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE,
INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING,
@@ -159,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 */
@@ -196,6 +211,8 @@ struct intel_device_info {
};
struct intel_runtime_info {
+ u32 platform_mask[1];
+
u16 device_id;
u8 num_sprites[I915_MAX_PIPES];
@@ -270,6 +287,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);
--
2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
` (3 preceding siblings ...)
2019-03-15 17:09 ` [PATCH v5] " Tvrtko Ursulin
@ 2019-03-15 17:12 ` Lucas De Marchi
2019-03-15 17:31 ` Tvrtko Ursulin
2019-03-15 18:22 ` ✗ Fi.CI.BAT: failure for drm/i915: Introduce concept of a sub-platform (rev3) Patchwork
5 siblings, 1 reply; 19+ messages in thread
From: Lucas De Marchi @ 2019-03-15 17:12 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Jani Nikula, Intel-gfx
On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>
>The approach is also beneficial to driver size, with an combined shrink of
>code and strings of around 1.7 kiB.
>
>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.
>
>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>
>---
> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
> 5 files changed, 179 insertions(+), 47 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index 0d743907e7bc..3218350cd225 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -863,6 +863,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);
>@@ -1752,10 +1754,11 @@ 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 (%x) gen=%i\n",
> INTEL_DEVID(dev_priv),
> INTEL_REVID(dev_priv),
> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>+ RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
bug here, INTEL_SUBPLATFORM_BITS should be outside of []. Bad things
will happen for platform=32 /o\
> INTEL_GEN(dev_priv));
>
> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>@@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -2281,7 +2281,46 @@ 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))
>+#define __IS_PLATFORM(dev_priv, p) \
>+({ \
>+ const unsigned int pbits__ = \
>+ BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>+ INTEL_SUBPLATFORM_BITS; \
>+ const unsigned int pi__ = (p) / pbits__; \
>+ const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>+\
>+ BUILD_BUG_ON(!__builtin_constant_p(p)); \
>+\
>+ (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
Ugh. That double dword fiddling is way too ugly. IMO it is not buying us
anything. Just use a u64 rather than the double dword. Your approach may
have a small benefit on ARCH=i386, but has the burden of carrying all
this forward. The diff below (only build-tested) is on top of yours,
which is basically equivalent to "move to u64 and then add the
subplatform part".
text data bss dec hex filename
1834620 40454 4176 1879250 1cacd2 drivers/gpu/drm/i915/i915.o.yours
1834710 40454 4176 1879340 1cad2c drivers/gpu/drm/i915/i915.o
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3218350cd225..c8042f4579c6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1754,11 +1754,11 @@ 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 (%x) gen=%i\n",
+ drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (%#llx) gen=%i\n",
INTEL_DEVID(dev_priv),
INTEL_REVID(dev_priv),
intel_platform_name(INTEL_INFO(dev_priv)->platform),
- RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
+ RUNTIME_INFO(dev_priv)->platform_mask,
INTEL_GEN(dev_priv));
intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 34282cf66cb0..bd5d31e07a13 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2283,43 +2283,33 @@ static inline unsigned int i915_sg_segment_size(void)
#define __IS_PLATFORM(dev_priv, p) \
({ \
- const unsigned int pbits__ = \
- BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
- INTEL_SUBPLATFORM_BITS; \
- const unsigned int pi__ = (p) / pbits__; \
- const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
-\
- BUILD_BUG_ON(!__builtin_constant_p(p)); \
-\
- (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
})
#define __IS_SUBPLATFORM(dev_priv, p, s) \
({ \
- const unsigned int pbits__ = \
- BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
- INTEL_SUBPLATFORM_BITS; \
- const unsigned int pi__ = (p) / pbits__; \
- const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
-\
- BUILD_BUG_ON(!__builtin_constant_p(p)); \
- BUILD_BUG_ON(!__builtin_constant_p(s)); \
- BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
-\
- (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
})
-static inline bool
+__always_inline static inline bool
IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
{
- return __IS_PLATFORM(i915, p);
+ const unsigned int pbit = p + INTEL_SUBPLATFORM_BITS;
+
+ BUILD_BUG_ON(!__builtin_constant_p(p));
+
+ return RUNTIME_INFO(i915)->platform_mask & BIT(pbit);
}
-static inline bool
+__always_inline static inline bool
IS_SUBPLATFORM(const struct drm_i915_private *i915,
enum intel_platform p, unsigned int s)
{
- return __IS_SUBPLATFORM(i915, p, s);
+ const unsigned int pbit = p + INTEL_SUBPLATFORM_BITS;
+
+ BUILD_BUG_ON(!__builtin_constant_p(p));
+ BUILD_BUG_ON(!__builtin_constant_p(s));
+ BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS);
+
+ return RUNTIME_INFO(i915)->platform_mask & (BIT(pbit) | BIT(s));
}
#define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index b8a7e17cb443..1cdd25486c04 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -709,80 +709,76 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
void intel_device_info_subplatform_init(struct drm_i915_private *i915)
{
- const unsigned int pbits =
- BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
- INTEL_SUBPLATFORM_BITS;
- const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
- const unsigned int pb =
- INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
- struct intel_runtime_info *info = RUNTIME_INFO(i915);
+ struct intel_runtime_info *rinfo = RUNTIME_INFO(i915);
+ const unsigned int pbit = INTEL_INFO(i915)->platform
+ + INTEL_SUBPLATFORM_BITS;
u16 devid = INTEL_DEVID(i915);
- BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
- pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
+ BUILD_BUG_ON(INTEL_MAX_PLATFORMS - INTEL_SUBPLATFORM_BITS >
+ BITS_PER_TYPE(rinfo->platform_mask));
- info->platform_mask[pi] = BIT(pb);
+ rinfo->platform_mask = BIT_ULL(pbit);
if (IS_PINEVIEW(i915)) {
if (devid == 0xa001)
- info->platform_mask[pi] |=
+ rinfo->platform_mask |=
BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
else if (devid == 0xa011)
- info->platform_mask[pi] |=
+ rinfo->platform_mask |=
BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
} else if (IS_IRONLAKE(i915)) {
if (devid == 0x0046)
- info->platform_mask[pi] |=
+ rinfo->platform_mask |=
BIT(INTEL_SUBPLATFORM_IRONLAKE_M);
} else if (IS_HASWELL(i915)) {
if ((devid & 0xFF00) == 0x0A00)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULT);
/* ULX machines are also considered ULT. */
if (devid == 0x0A0E || devid == 0x0A1E)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULX);
} else if (IS_BROADWELL(i915)) {
if ((devid & 0xf) == 0x6 ||
(devid & 0xf) == 0xb ||
(devid & 0xf) == 0xe)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULT);
/* ULX machines are also considered ULT. */
if ((devid & 0xf) == 0xe)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULX);
} else if (IS_SKYLAKE(i915)) {
if (devid == 0x1906 ||
devid == 0x1913 ||
devid == 0x1916 ||
devid == 0x1921 ||
devid == 0x1926)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULT);
else if (devid == 0x190E ||
devid == 0x1915 ||
devid == 0x191E)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULX);
} else if (IS_KABYLAKE(i915)) {
if (devid == 0x5906 ||
devid == 0x5913 ||
devid == 0x5916 ||
devid == 0x5921 ||
devid == 0x5926)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULT);
else if (devid == 0x590E ||
devid == 0x5915 ||
devid == 0x591E)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULX);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULX);
else if (devid == 0x591C ||
devid == 0x87C0)
- info->platform_mask[pi] |=
+ rinfo->platform_mask |=
BIT(INTEL_SUBPLATFORM_AML_ULX);
} else if (IS_COFFEELAKE(i915)) {
if ((devid & 0x00F0) == 0x00A0)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_ULT);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_ULT);
} else if (IS_CANNONLAKE(i915)) {
if ((devid & 0x0004) == 0x0004)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_PORTF);
} else if (IS_ICELAKE(i915)) {
if (devid != 0x8A51)
- info->platform_mask[pi] |= BIT(INTEL_SUBPLATFORM_PORTF);
+ rinfo->platform_mask |= BIT(INTEL_SUBPLATFORM_PORTF);
}
}
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index b03fbd2e451a..12ff04a25b51 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -212,7 +212,7 @@ struct intel_device_info {
};
struct intel_runtime_info {
- u32 platform_mask[1];
+ u64 platform_mask;
u16 device_id;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v5] drm/i915: Introduce concept of a sub-platform
2019-03-15 17:09 ` [PATCH v5] " Tvrtko Ursulin
@ 2019-03-15 17:28 ` Chris Wilson
2019-03-15 17:36 ` Tvrtko Ursulin
0 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2019-03-15 17:28 UTC (permalink / raw)
To: Intel-gfx, Tvrtko Ursulin; +Cc: Jani Nikula, Lucas De Marchi
Quoting Tvrtko Ursulin (2019-03-15 17:09:28)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 3d8020888604..e3360a31f8d3 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -677,6 +677,7 @@ 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(m->i915));
We didn't take a copy of the runtime_info? Ww should have done at least,
that's even more volatile than device_info.
It'll do for now, but would be nicer if RUNTIME_INFO(i915) wasn't baked
so hard into the subplatform :)
> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> +{
> + const unsigned int pi =
> + __platform_mask_index(i915, INTEL_INFO(i915)->platform);
> + const unsigned int pb =
> + __platform_mask_bit(i915, INTEL_INFO(i915)->platform);
> + u16 devid = INTEL_DEVID(i915);
> + u32 mask = 0;
> +
> + RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb);
> +
> + if (IS_PINEVIEW(i915)) {
> + if (devid == 0xa001)
> + mask |= BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> + else if (devid == 0xa011)
> + mask |= BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
I take it, leaving is-mobile conversion for the display gurus?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 17:12 ` [PATCH] " Lucas De Marchi
@ 2019-03-15 17:31 ` Tvrtko Ursulin
2019-03-15 18:40 ` Lucas De Marchi
0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 17:31 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: Jani Nikula, Intel-gfx
On 15/03/2019 17:12, Lucas De Marchi wrote:
> On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>>
>> The approach is also beneficial to driver size, with an combined
>> shrink of
>> code and strings of around 1.7 kiB.
>>
>> 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.
>>
>> 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>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 7 +-
>> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
>> drivers/gpu/drm/i915/i915_pci.c | 2 +-
>> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
>> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
>> 5 files changed, 179 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 0d743907e7bc..3218350cd225 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -863,6 +863,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);
>> @@ -1752,10 +1754,11 @@ 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 (%x)
>> gen=%i\n",
>> INTEL_DEVID(dev_priv),
>> INTEL_REVID(dev_priv),
>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>> +
>> RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform /
>> (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) -
>> INTEL_SUBPLATFORM_BITS)],
>
> bug here, INTEL_SUBPLATFORM_BITS should be outside of []. Bad things
> will happen for platform=32 /o\
? [32 / (32 - 3)] = [1], for which there is a BUILD_BUG_ON with a
comment saying to increase size of array.
>
>> INTEL_GEN(dev_priv));
>>
>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2281,7 +2281,46 @@ 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))
>> +#define __IS_PLATFORM(dev_priv, p) \
>> +({ \
>> + const unsigned int pbits__ = \
>> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>> + INTEL_SUBPLATFORM_BITS; \
>> + const unsigned int pi__ = (p) / pbits__; \
>> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>> +\
>> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
>> +\
>> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>
>
> Ugh. That double dword fiddling is way too ugly. IMO it is not buying us
> anything. Just use a u64 rather than the double dword. Your approach may
> have a small benefit on ARCH=i386, but has the burden of carrying all
> this forward. The diff below (only build-tested) is on top of yours,
> which is basically equivalent to "move to u64 and then add the
> subplatform part".
>
> text data bss dec hex filename
> 1834620 40454 4176 1879250 1cacd2 drivers/gpu/drm/i915/i915.o.yours
> 1834710 40454 4176 1879340 1cad2c drivers/gpu/drm/i915/i915.o
The cost of going u64 would be higher than what you saw if bits above
were actually used I think. But would have to check the output to be
sure. It was at least a year ago I think I last played with this.
Benefit of the u32 array approach is that it avoids that even on 64-bit
builds.
As it stands v5 of my patch has minimal positive effect on code size
(sub 1k). Maybe a bit better in non-debug builds. But the main point is
about the devid checking consolidation.
It is of course open to discussion.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5] drm/i915: Introduce concept of a sub-platform
2019-03-15 17:28 ` Chris Wilson
@ 2019-03-15 17:36 ` Tvrtko Ursulin
0 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-15 17:36 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx; +Cc: Jani Nikula, Lucas De Marchi
On 15/03/2019 17:28, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-15 17:09:28)
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 3d8020888604..e3360a31f8d3 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -677,6 +677,7 @@ 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(m->i915));
>
> We didn't take a copy of the runtime_info? Ww should have done at least,
> that's even more volatile than device_info.
>
> It'll do for now, but would be nicer if RUNTIME_INFO(i915) wasn't baked
> so hard into the subplatform :)
Could do yeah.
>> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
>> +{
>> + const unsigned int pi =
>> + __platform_mask_index(i915, INTEL_INFO(i915)->platform);
>> + const unsigned int pb =
>> + __platform_mask_bit(i915, INTEL_INFO(i915)->platform);
>> + u16 devid = INTEL_DEVID(i915);
>> + u32 mask = 0;
>> +
>> + RUNTIME_INFO(i915)->platform_mask[pi] = BIT(pb);
>> +
>> + if (IS_PINEVIEW(i915)) {
>> + if (devid == 0xa001)
>> + mask |= BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
>> + else if (devid == 0xa011)
>> + mask |= BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
>
> I take it, leaving is-mobile conversion for the display gurus?
Ville said it will be hard so I did not even look. :) Okay, it doesn't
seem that hard.. but definitely wouldn't stuff it into this patch.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Introduce concept of a sub-platform (rev3)
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
` (4 preceding siblings ...)
2019-03-15 17:12 ` [PATCH] " Lucas De Marchi
@ 2019-03-15 18:22 ` Patchwork
5 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2019-03-15 18:22 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Introduce concept of a sub-platform (rev3)
URL : https://patchwork.freedesktop.org/series/58056/
State : failure
== Summary ==
Applying: drm/i915: Introduce concept of a sub-platform
error: corrupt patch at line 199
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Introduce concept of a sub-platform
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 17:31 ` Tvrtko Ursulin
@ 2019-03-15 18:40 ` Lucas De Marchi
2019-03-18 7:09 ` Tvrtko Ursulin
0 siblings, 1 reply; 19+ messages in thread
From: Lucas De Marchi @ 2019-03-15 18:40 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Jani Nikula, Intel-gfx
On Fri, Mar 15, 2019 at 05:31:05PM +0000, Tvrtko Ursulin wrote:
>
>On 15/03/2019 17:12, Lucas De Marchi wrote:
>>On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>>>
>>>The approach is also beneficial to driver size, with an combined
>>>shrink of
>>>code and strings of around 1.7 kiB.
>>>
>>>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.
>>>
>>>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>
>>>---
>>>drivers/gpu/drm/i915/i915_drv.c | 7 +-
>>>drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
>>>drivers/gpu/drm/i915/i915_pci.c | 2 +-
>>>drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
>>>drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
>>>5 files changed, 179 insertions(+), 47 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>b/drivers/gpu/drm/i915/i915_drv.c
>>>index 0d743907e7bc..3218350cd225 100644
>>>--- a/drivers/gpu/drm/i915/i915_drv.c
>>>+++ b/drivers/gpu/drm/i915/i915_drv.c
>>>@@ -863,6 +863,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);
>>>@@ -1752,10 +1754,11 @@ 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 (%x)
>>>gen=%i\n",
>>> INTEL_DEVID(dev_priv),
>>> INTEL_REVID(dev_priv),
>>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>>>+
>>>RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform
>>>/ (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) -
>>>INTEL_SUBPLATFORM_BITS)],
>>
>>bug here, INTEL_SUBPLATFORM_BITS should be outside of []. Bad things
>>will happen for platform=32 /o\
>
>? [32 / (32 - 3)] = [1], for which there is a BUILD_BUG_ON with a
>comment saying to increase size of array.
I think I missed a "(" and thought that would be (32 / 32) - 3
anyway, you are printing confusing information here since you only print
one u32.
>
>>
>>> INTEL_GEN(dev_priv));
>>>
>>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>>>@@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>>>--- a/drivers/gpu/drm/i915/i915_drv.h
>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
>>>@@ -2281,7 +2281,46 @@ 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))
>>>+#define __IS_PLATFORM(dev_priv, p) \
>>>+({ \
>>>+ const unsigned int pbits__ = \
>>>+ BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>>>+ INTEL_SUBPLATFORM_BITS; \
>>>+ const unsigned int pi__ = (p) / pbits__; \
>>>+ const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
>>>+\
>>>+ BUILD_BUG_ON(!__builtin_constant_p(p)); \
>>>+\
>>>+ (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>>
>>
>>Ugh. That double dword fiddling is way too ugly. IMO it is not buying us
>>anything. Just use a u64 rather than the double dword. Your approach may
>>have a small benefit on ARCH=i386, but has the burden of carrying all
>>this forward. The diff below (only build-tested) is on top of yours,
>>which is basically equivalent to "move to u64 and then add the
>>subplatform part".
>>
>> text data bss dec hex filename
>>1834620 40454 4176 1879250 1cacd2 drivers/gpu/drm/i915/i915.o.yours
>>1834710 40454 4176 1879340 1cad2c drivers/gpu/drm/i915/i915.o
>
>The cost of going u64 would be higher than what you saw if bits above
>were actually used I think. But would have to check the output to be
>sure. It was at least a year ago I think I last played with this.
I challenge that. My butt feeling here is pretty strong that's not
true.
>Benefit of the u32 array approach is that it avoids that even on
>64-bit builds.
my point is on 64-bit builds... It may suffer a little on m32.
Lucas De Marchi
>As it stands v5 of my patch has minimal positive effect on code size
>(sub 1k). Maybe a bit better in non-debug builds. But the main point
>is about the devid checking consolidation.
>
>It is of course open to discussion.
>
>Regards,
>
>Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-15 18:40 ` Lucas De Marchi
@ 2019-03-18 7:09 ` Tvrtko Ursulin
2019-03-18 18:53 ` Lucas De Marchi
0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2019-03-18 7:09 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: Jani Nikula, Intel-gfx
On 15/03/2019 18:40, Lucas De Marchi wrote:
> On Fri, Mar 15, 2019 at 05:31:05PM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/03/2019 17:12, Lucas De Marchi wrote:
>>> On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>>>>
>>>> The approach is also beneficial to driver size, with an combined
>>>> shrink of
>>>> code and strings of around 1.7 kiB.
>>>>
>>>> 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.
>>>>
>>>> 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>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.c | 7 +-
>>>> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
>>>> drivers/gpu/drm/i915/i915_pci.c | 2 +-
>>>> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
>>>> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
>>>> 5 files changed, 179 insertions(+), 47 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 0d743907e7bc..3218350cd225 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -863,6 +863,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);
>>>> @@ -1752,10 +1754,11 @@ 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 (%x)
>>>> gen=%i\n",
>>>> INTEL_DEVID(dev_priv),
>>>> INTEL_REVID(dev_priv),
>>>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>>>> +
>>>> RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform
>>>> / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) -
>>>> INTEL_SUBPLATFORM_BITS)],
>>>
>>> bug here, INTEL_SUBPLATFORM_BITS should be outside of []. Bad things
>>> will happen for platform=32 /o\
>>
>> ? [32 / (32 - 3)] = [1], for which there is a BUILD_BUG_ON with a
>> comment saying to increase size of array.
>
> I think I missed a "(" and thought that would be (32 / 32) - 3
>
> anyway, you are printing confusing information here since you only print
> one u32.
Confusing how and what is the second u32? You mean when array gets
expanded in the future? Here the actual idea (see next version) is to
only print the subplatform bits.
>>
>>>
>>>> INTEL_GEN(dev_priv));
>>>>
>>>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>>>> @@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2281,7 +2281,46 @@ 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))
>>>> +#define __IS_PLATFORM(dev_priv, p) \
>>>> +({ \
>>>> + const unsigned int pbits__ = \
>>>> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>>>> + INTEL_SUBPLATFORM_BITS; \
>>>> + const unsigned int pi__ = (p) / pbits__; \
>>>> + const unsigned int pb__ = (p) % pbits__ +
>>>> INTEL_SUBPLATFORM_BITS; \
>>>> +\
>>>> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
>>>> +\
>>>> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>>>
>>>
>>> Ugh. That double dword fiddling is way too ugly. IMO it is not buying us
>>> anything. Just use a u64 rather than the double dword. Your approach may
>>> have a small benefit on ARCH=i386, but has the burden of carrying all
>>> this forward. The diff below (only build-tested) is on top of yours,
>>> which is basically equivalent to "move to u64 and then add the
>>> subplatform part".
>>>
>>> text data bss dec hex filename
>>> 1834620 40454 4176 1879250 1cacd2
>>> drivers/gpu/drm/i915/i915.o.yours
>>> 1834710 40454 4176 1879340 1cad2c drivers/gpu/drm/i915/i915.o
>>
>> The cost of going u64 would be higher than what you saw if bits above
>> were actually used I think. But would have to check the output to be
>> sure. It was at least a year ago I think I last played with this.
>
> I challenge that. My butt feeling here is pretty strong that's not
> true.
I hope you meant gut feeling. Anyways, I said I wasn't 100% sure of the
details, right? Having exercised my memory lanes I think the biggest
negative effect of the move to u64 was in conjunction with this
subplatform idea. This is due the "if (mask & platform) && (mask &
subplatform)" expanding to 2 u64 constants per check. I have a remedy
for that, but lets not get bogged down and code size which is not the
main consideration here. Idea was to consolidate and simplify whilst
preserving the two beneficial effects which I already explain.
>> Benefit of the u32 array approach is that it avoids that even on
>> 64-bit builds.
>
> my point is on 64-bit builds... It may suffer a little on m32.
Which is why I hinted the u32 array approach does not suffer on either.
No increase on 64-bit, no real increase on 32-bit.
Regards,
Tvrtko
>
> Lucas De Marchi
>
>> As it stands v5 of my patch has minimal positive effect on code size
>> (sub 1k). Maybe a bit better in non-debug builds. But the main point
>> is about the devid checking consolidation.
>>
>> It is of course open to discussion.
>>
>> Regards,
>>
>> Tvrtko
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Introduce concept of a sub-platform
2019-03-18 7:09 ` Tvrtko Ursulin
@ 2019-03-18 18:53 ` Lucas De Marchi
0 siblings, 0 replies; 19+ messages in thread
From: Lucas De Marchi @ 2019-03-18 18:53 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Jani Nikula, Intel-gfx
On Mon, Mar 18, 2019 at 07:09:59AM +0000, Tvrtko Ursulin wrote:
>
>On 15/03/2019 18:40, Lucas De Marchi wrote:
>>On Fri, Mar 15, 2019 at 05:31:05PM +0000, Tvrtko Ursulin wrote:
>>>
>>>On 15/03/2019 17:12, Lucas De Marchi wrote:
>>>>On Fri, Mar 15, 2019 at 12:26:33PM +0000, Tvrtko Ursulin 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.
>>>>>
>>>>>The approach is also beneficial to driver size, with an
>>>>>combined shrink of
>>>>>code and strings of around 1.7 kiB.
>>>>>
>>>>>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.
>>>>>
>>>>>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>
>>>>>---
>>>>>drivers/gpu/drm/i915/i915_drv.c | 7 +-
>>>>>drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
>>>>>drivers/gpu/drm/i915/i915_pci.c | 2 +-
>>>>>drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
>>>>>drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
>>>>>5 files changed, 179 insertions(+), 47 deletions(-)
>>>>>
>>>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>>>b/drivers/gpu/drm/i915/i915_drv.c
>>>>>index 0d743907e7bc..3218350cd225 100644
>>>>>--- a/drivers/gpu/drm/i915/i915_drv.c
>>>>>+++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>>@@ -863,6 +863,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);
>>>>>@@ -1752,10 +1754,11 @@ 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
>>>>>(%x) gen=%i\n",
>>>>> INTEL_DEVID(dev_priv),
>>>>> INTEL_REVID(dev_priv),
>>>>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>>>>>+ RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform
>>>>>/ (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) -
>>>>>INTEL_SUBPLATFORM_BITS)],
>>>>
>>>>bug here, INTEL_SUBPLATFORM_BITS should be outside of []. Bad things
>>>>will happen for platform=32 /o\
>>>
>>>? [32 / (32 - 3)] = [1], for which there is a BUILD_BUG_ON with a
>>>comment saying to increase size of array.
>>
>>I think I missed a "(" and thought that would be (32 / 32) - 3
>>
>>anyway, you are printing confusing information here since you only print
>>one u32.
>
>Confusing how and what is the second u32? You mean when array gets
because you are printing here only one u32 and hence this number could
be more than one platform.
>expanded in the future? Here the actual idea (see next version) is to
>only print the subplatform bits.
yep, much better there.
>
>>>
>>>>
>>>>> INTEL_GEN(dev_priv));
>>>>>
>>>>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>>>>>@@ -1798,8 +1801,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 dccb6006aabf..34282cf66cb0 100644
>>>>>--- a/drivers/gpu/drm/i915/i915_drv.h
>>>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>>@@ -2281,7 +2281,46 @@ 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))
>>>>>+#define __IS_PLATFORM(dev_priv, p) \
>>>>>+({ \
>>>>>+ const unsigned int pbits__ = \
>>>>>+ BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
>>>>>+ INTEL_SUBPLATFORM_BITS; \
>>>>>+ const unsigned int pi__ = (p) / pbits__; \
>>>>>+ const unsigned int pb__ = (p) % pbits__ +
>>>>>INTEL_SUBPLATFORM_BITS; \
>>>>>+\
>>>>>+ BUILD_BUG_ON(!__builtin_constant_p(p)); \
>>>>>+\
>>>>>+ (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
>>>>
>>>>
>>>>Ugh. That double dword fiddling is way too ugly. IMO it is not buying us
>>>>anything. Just use a u64 rather than the double dword. Your approach may
>>>>have a small benefit on ARCH=i386, but has the burden of carrying all
>>>>this forward. The diff below (only build-tested) is on top of yours,
>>>>which is basically equivalent to "move to u64 and then add the
>>>>subplatform part".
>>>>
>>>> text data bss dec hex filename
>>>>1834620 40454 4176 1879250 1cacd2
>>>>drivers/gpu/drm/i915/i915.o.yours
>>>>1834710 40454 4176 1879340 1cad2c drivers/gpu/drm/i915/i915.o
>>>
>>>The cost of going u64 would be higher than what you saw if bits
>>>above were actually used I think. But would have to check the
>>>output to be sure. It was at least a year ago I think I last
>>>played with this.
>>
>>I challenge that. My butt feeling here is pretty strong that's not
>>true.
>
>I hope you meant gut feeling. Anyways, I said I wasn't 100% sure of
yep, of course :-/
>the details, right? Having exercised my memory lanes I think the
>biggest negative effect of the move to u64 was in conjunction with
>this subplatform idea. This is due the "if (mask & platform) && (mask
>& subplatform)" expanding to 2 u64 constants per check. I have a
>remedy for that, but lets not get bogged down and code size which is
>not the main consideration here. Idea was to consolidate and simplify
>whilst preserving the two beneficial effects which I already explain.
>
>>>Benefit of the u32 array approach is that it avoids that even on
>>>64-bit builds.
>>
>>my point is on 64-bit builds... It may suffer a little on m32.
>
>Which is why I hinted the u32 array approach does not suffer on
>either. No increase on 64-bit, no real increase on 32-bit.
I actually think the main advantage of multiple dwords is to be ready
for the next bump when it arrives. So ok. I still don't like open coded
bitmask though.
Lucas De Marchi
>
>Regards,
>
>Tvrtko
>
>>
>>Lucas De Marchi
>>
>>>As it stands v5 of my patch has minimal positive effect on code
>>>size (sub 1k). Maybe a bit better in non-debug builds. But the
>>>main point is about the devid checking consolidation.
>>>
>>>It is of course open to discussion.
>>>
>>>Regards,
>>>
>>>Tvrtko
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-03-18 18:53 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-15 12:26 [PATCH] drm/i915: Introduce concept of a sub-platform Tvrtko Ursulin
2019-03-15 13:16 ` Chris Wilson
2019-03-15 13:32 ` Tvrtko Ursulin
2019-03-15 13:42 ` Chris Wilson
2019-03-15 14:09 ` Ville Syrjälä
2019-03-15 14:21 ` Tvrtko Ursulin
2019-03-15 15:55 ` Ville Syrjälä
2019-03-15 15:57 ` Ville Syrjälä
2019-03-15 16:05 ` Tvrtko Ursulin
2019-03-15 14:36 ` ✗ Fi.CI.BAT: failure for " Patchwork
2019-03-15 17:09 ` [PATCH v5] " Tvrtko Ursulin
2019-03-15 17:28 ` Chris Wilson
2019-03-15 17:36 ` Tvrtko Ursulin
2019-03-15 17:12 ` [PATCH] " Lucas De Marchi
2019-03-15 17:31 ` Tvrtko Ursulin
2019-03-15 18:40 ` Lucas De Marchi
2019-03-18 7:09 ` Tvrtko Ursulin
2019-03-18 18:53 ` Lucas De Marchi
2019-03-15 18:22 ` ✗ Fi.CI.BAT: failure for drm/i915: Introduce concept of a sub-platform (rev3) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.