[RFC PATCH] drm/i915: replace platform flags with a platform enum

[RFC PATCH] drm/i915: replace platform flags with a platform enum

[Jani Nikula]

The platform flags in device info are (mostly) mutually
exclusive. Replace the flags with an enum. Add the platform enum also
for platforms that previously didn't have a flag, and give them codename
logging in dmesg.

Pineview remains an exception, the platform being G33 for that.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Untested TGIF patch. ;)
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  1 +
 drivers/gpu/drm/i915/i915_drv.h          | 76 +++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_gpu_error.c    |  1 +
 drivers/gpu/drm/i915/i915_pci.c          | 53 +++++++++++++---------
 drivers/gpu/drm/i915/intel_device_info.c | 40 ++++++++++++++++-
 5 files changed, 117 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b7f42c448a44..982f0bdb768f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -77,6 +77,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
 	const struct intel_device_info *info = INTEL_INFO(dev_priv);
 
 	seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv));
+	seq_printf(m, "platform: %s\n", intel_platform_name(info->platform));
 	seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv));
 #define PRINT_FLAG(x)  seq_printf(m, #x ": %s\n", yesno(info->x))
 	DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index be67aeece749..b8a1b66599dc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -659,24 +659,8 @@ struct intel_csr {
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func) \
-	/* Keep is_* in chronological order */ \
 	func(is_mobile); \
-	func(is_i85x); \
-	func(is_i915g); \
-	func(is_i945gm); \
-	func(is_g33); \
-	func(is_g4x); \
 	func(is_pineview); \
-	func(is_broadwater); \
-	func(is_crestline); \
-	func(is_ivybridge); \
-	func(is_valleyview); \
-	func(is_cherryview); \
-	func(is_haswell); \
-	func(is_broadwell); \
-	func(is_skylake); \
-	func(is_broxton); \
-	func(is_kabylake); \
 	func(is_alpha_support); \
 	/* Keep has_* in alphabetical order */ \
 	func(has_64bit_reloc); \
@@ -726,6 +710,34 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
 }
 
+/* Keep in chronological order */
+enum intel_platform {
+	INTEL_PLATFORM_UNINITIALIZED = 0,
+	INTEL_I830,
+	INTEL_I845G,
+	INTEL_I85X,
+	INTEL_I865G,
+	INTEL_I915G,
+	INTEL_I915GM,
+	INTEL_I945G,
+	INTEL_I945GM,
+	INTEL_G33,
+	INTEL_G4X,
+	INTEL_PINEVIEW,
+	INTEL_BROADWATER,
+	INTEL_CRESTLINE,
+	INTEL_IRONLAKE,
+	INTEL_SANDYBRIDGE,
+	INTEL_IVYBRIDGE,
+	INTEL_VALLEYVIEW,
+	INTEL_CHERRYVIEW,
+	INTEL_HASWELL,
+	INTEL_BROADWELL,
+	INTEL_SKYLAKE,
+	INTEL_BROXTON,
+	INTEL_KABYLAKE,
+};
+
 struct intel_device_info {
 	u32 display_mmio_offset;
 	u16 device_id;
@@ -733,6 +745,7 @@ struct intel_device_info {
 	u8 num_sprites[I915_MAX_PIPES];
 	u8 gen;
 	u16 gen_mask;
+	enum intel_platform platform;
 	u8 ring_mask; /* Rings supported by the HW */
 	u8 num_rings;
 #define DEFINE_FLAG(name) u8 name:1
@@ -2420,32 +2433,32 @@ intel_info(const struct drm_i915_private *dev_priv)
 
 #define IS_I830(dev_priv)	(INTEL_DEVID(dev_priv) == 0x3577)
 #define IS_845G(dev_priv)	(INTEL_DEVID(dev_priv) == 0x2562)
-#define IS_I85X(dev_priv)	((dev_priv)->info.is_i85x)
+#define IS_I85X(dev_priv)	((dev_priv)->info.platform == INTEL_I85X)
 #define IS_I865G(dev_priv)	(INTEL_DEVID(dev_priv) == 0x2572)
-#define IS_I915G(dev_priv)	((dev_priv)->info.is_i915g)
+#define IS_I915G(dev_priv)	((dev_priv)->info.platform == INTEL_I915G)
 #define IS_I915GM(dev_priv)	(INTEL_DEVID(dev_priv) == 0x2592)
 #define IS_I945G(dev_priv)	(INTEL_DEVID(dev_priv) == 0x2772)
-#define IS_I945GM(dev_priv)	((dev_priv)->info.is_i945gm)
-#define IS_BROADWATER(dev_priv)	((dev_priv)->info.is_broadwater)
-#define IS_CRESTLINE(dev_priv)	((dev_priv)->info.is_crestline)
+#define IS_I945GM(dev_priv)	((dev_priv)->info.platform == INTEL_I945GM)
+#define IS_BROADWATER(dev_priv)	((dev_priv)->info.platform == INTEL_BROADWATER)
+#define IS_CRESTLINE(dev_priv)	((dev_priv)->info.platform == INTEL_CRESTLINE)
 #define IS_GM45(dev_priv)	(INTEL_DEVID(dev_priv) == 0x2A42)
-#define IS_G4X(dev_priv)	((dev_priv)->info.is_g4x)
+#define IS_G4X(dev_priv)	((dev_priv)->info.platform == INTEL_G4X)
 #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(dev_priv)	((dev_priv)->info.is_pineview)
-#define IS_G33(dev_priv)	((dev_priv)->info.is_g33)
+#define IS_G33(dev_priv)	((dev_priv)->info.platform == INTEL_G33)
 #define IS_IRONLAKE_M(dev_priv)	(INTEL_DEVID(dev_priv) == 0x0046)
-#define IS_IVYBRIDGE(dev_priv)	((dev_priv)->info.is_ivybridge)
+#define IS_IVYBRIDGE(dev_priv)	((dev_priv)->info.platform == INTEL_IVYBRIDGE)
 #define IS_IVB_GT1(dev_priv)	(INTEL_DEVID(dev_priv) == 0x0156 || \
 				 INTEL_DEVID(dev_priv) == 0x0152 || \
 				 INTEL_DEVID(dev_priv) == 0x015a)
-#define IS_VALLEYVIEW(dev_priv)	((dev_priv)->info.is_valleyview)
-#define IS_CHERRYVIEW(dev_priv)	((dev_priv)->info.is_cherryview)
-#define IS_HASWELL(dev_priv)	((dev_priv)->info.is_haswell)
-#define IS_BROADWELL(dev_priv)	((dev_priv)->info.is_broadwell)
-#define IS_SKYLAKE(dev_priv)	((dev_priv)->info.is_skylake)
-#define IS_BROXTON(dev_priv)	((dev_priv)->info.is_broxton)
-#define IS_KABYLAKE(dev_priv)	((dev_priv)->info.is_kabylake)
+#define IS_VALLEYVIEW(dev_priv)	((dev_priv)->info.platform == INTEL_VALLEYVIEW)
+#define IS_CHERRYVIEW(dev_priv)	((dev_priv)->info.platform == INTEL_CHERRYVIEW)
+#define IS_HASWELL(dev_priv)	((dev_priv)->info.platform == INTEL_HASWELL)
+#define IS_BROADWELL(dev_priv)	((dev_priv)->info.platform == INTEL_BROADWELL)
+#define IS_SKYLAKE(dev_priv)	((dev_priv)->info.platform == INTEL_SKYLAKE)
+#define IS_BROXTON(dev_priv)	((dev_priv)->info.platform == INTEL_BROXTON)
+#define IS_KABYLAKE(dev_priv)	((dev_priv)->info.platform == INTEL_KABYLAKE)
 #define IS_MOBILE(dev_priv)	((dev_priv)->info.is_mobile)
 #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \
 				    (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00)
@@ -3471,6 +3484,7 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
 	return (struct intel_device_info *)&dev_priv->info;
 }
 
+const char *intel_platform_name(enum intel_platform platform);
 void intel_device_info_runtime_init(struct drm_i915_private *dev_priv);
 void intel_device_info_dump(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index ae84aa4b1467..46fbeae77758 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -565,6 +565,7 @@ int i915_error_state_to_str(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, "PCI ID: 0x%04x\n", pdev->device);
 	err_printf(m, "PCI Revision: 0x%02x\n", pdev->revision);
 	err_printf(m, "PCI Subsystem: %04x:%04x\n",
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index fce8e198bc76..804038f4f162 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -65,17 +65,19 @@
 
 static const struct intel_device_info intel_i830_info = {
 	GEN2_FEATURES,
+	.platform = INTEL_I830,
 	.is_mobile = 1, .cursor_needs_physical = 1,
 	.num_pipes = 2, /* legal, last one wins */
 };
 
 static const struct intel_device_info intel_845g_info = {
 	GEN2_FEATURES,
+	.platform = INTEL_I845G,
 };
 
 static const struct intel_device_info intel_i85x_info = {
 	GEN2_FEATURES,
-	.is_i85x = 1, .is_mobile = 1,
+	.platform = INTEL_I85X, .is_mobile = 1,
 	.num_pipes = 2, /* legal, last one wins */
 	.cursor_needs_physical = 1,
 	.has_fbc = 1,
@@ -83,6 +85,7 @@ static const struct intel_device_info intel_i85x_info = {
 
 static const struct intel_device_info intel_i865g_info = {
 	GEN2_FEATURES,
+	.platform = INTEL_I865G,
 };
 
 #define GEN3_FEATURES \
@@ -94,12 +97,13 @@ static const struct intel_device_info intel_i865g_info = {
 
 static const struct intel_device_info intel_i915g_info = {
 	GEN3_FEATURES,
-	.is_i915g = 1, .cursor_needs_physical = 1,
+	.platform = INTEL_I915G, .cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
 	.hws_needs_physical = 1,
 };
 static const struct intel_device_info intel_i915gm_info = {
 	GEN3_FEATURES,
+	.platform = INTEL_I915GM,
 	.is_mobile = 1,
 	.cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
@@ -109,13 +113,14 @@ static const struct intel_device_info intel_i915gm_info = {
 };
 static const struct intel_device_info intel_i945g_info = {
 	GEN3_FEATURES,
+	.platform = INTEL_I945G,
 	.has_hotplug = 1, .cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
 	.hws_needs_physical = 1,
 };
 static const struct intel_device_info intel_i945gm_info = {
 	GEN3_FEATURES,
-	.is_i945gm = 1, .is_mobile = 1,
+	.platform = INTEL_I945GM, .is_mobile = 1,
 	.has_hotplug = 1, .cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
 	.supports_tv = 1,
@@ -133,14 +138,14 @@ static const struct intel_device_info intel_i945gm_info = {
 
 static const struct intel_device_info intel_i965g_info = {
 	GEN4_FEATURES,
-	.is_broadwater = 1,
+	.platform = INTEL_BROADWATER,
 	.has_overlay = 1,
 	.hws_needs_physical = 1,
 };
 
 static const struct intel_device_info intel_i965gm_info = {
 	GEN4_FEATURES,
-	.is_crestline = 1,
+	.platform = INTEL_CRESTLINE,
 	.is_mobile = 1, .has_fbc = 1,
 	.has_overlay = 1,
 	.supports_tv = 1,
@@ -149,21 +154,21 @@ static const struct intel_device_info intel_i965gm_info = {
 
 static const struct intel_device_info intel_g33_info = {
 	GEN3_FEATURES,
-	.is_g33 = 1,
+	.platform = INTEL_G33,
 	.has_hotplug = 1,
 	.has_overlay = 1,
 };
 
 static const struct intel_device_info intel_g45_info = {
 	GEN4_FEATURES,
-	.is_g4x = 1,
+	.platform = INTEL_G4X,
 	.has_pipe_cxsr = 1,
 	.ring_mask = RENDER_RING | BSD_RING,
 };
 
 static const struct intel_device_info intel_gm45_info = {
 	GEN4_FEATURES,
-	.is_g4x = 1,
+	.platform = INTEL_G4X,
 	.is_mobile = 1, .has_fbc = 1,
 	.has_pipe_cxsr = 1,
 	.supports_tv = 1,
@@ -172,7 +177,7 @@ static const struct intel_device_info intel_gm45_info = {
 
 static const struct intel_device_info intel_pineview_info = {
 	GEN3_FEATURES,
-	.is_g33 = 1, .is_pineview = 1, .is_mobile = 1,
+	.platform = INTEL_G33, .is_pineview = 1, .is_mobile = 1,
 	.has_hotplug = 1,
 	.has_overlay = 1,
 };
@@ -187,10 +192,12 @@ static const struct intel_device_info intel_pineview_info = {
 
 static const struct intel_device_info intel_ironlake_d_info = {
 	GEN5_FEATURES,
+	.platform = INTEL_IRONLAKE,
 };
 
 static const struct intel_device_info intel_ironlake_m_info = {
 	GEN5_FEATURES,
+	.platform = INTEL_IRONLAKE,
 	.is_mobile = 1,
 };
 
@@ -209,10 +216,12 @@ static const struct intel_device_info intel_ironlake_m_info = {
 
 static const struct intel_device_info intel_sandybridge_d_info = {
 	GEN6_FEATURES,
+	.platform = INTEL_SANDYBRIDGE,
 };
 
 static const struct intel_device_info intel_sandybridge_m_info = {
 	GEN6_FEATURES,
+	.platform = INTEL_SANDYBRIDGE,
 	.is_mobile = 1,
 };
 
@@ -231,20 +240,20 @@ static const struct intel_device_info intel_sandybridge_m_info = {
 
 static const struct intel_device_info intel_ivybridge_d_info = {
 	GEN7_FEATURES,
-	.is_ivybridge = 1,
+	.platform = INTEL_IVYBRIDGE,
 	.has_l3_dpf = 1,
 };
 
 static const struct intel_device_info intel_ivybridge_m_info = {
 	GEN7_FEATURES,
-	.is_ivybridge = 1,
+	.platform = INTEL_IVYBRIDGE,
 	.is_mobile = 1,
 	.has_l3_dpf = 1,
 };
 
 static const struct intel_device_info intel_ivybridge_q_info = {
 	GEN7_FEATURES,
-	.is_ivybridge = 1,
+	.platform = INTEL_IVYBRIDGE,
 	.num_pipes = 0, /* legal, last one wins */
 	.has_l3_dpf = 1,
 };
@@ -265,7 +274,7 @@ static const struct intel_device_info intel_ivybridge_q_info = {
 
 static const struct intel_device_info intel_valleyview_info = {
 	VLV_FEATURES,
-	.is_valleyview = 1,
+	.platform = INTEL_VALLEYVIEW,
 };
 
 #define HSW_FEATURES  \
@@ -281,7 +290,7 @@ static const struct intel_device_info intel_valleyview_info = {
 
 static const struct intel_device_info intel_haswell_info = {
 	HSW_FEATURES,
-	.is_haswell = 1,
+	.platform = INTEL_HASWELL,
 	.has_l3_dpf = 1,
 };
 
@@ -294,13 +303,13 @@ static const struct intel_device_info intel_haswell_info = {
 static const struct intel_device_info intel_broadwell_info = {
 	BDW_FEATURES,
 	.gen = 8,
-	.is_broadwell = 1,
+	.platform = INTEL_BROADWELL,
 };
 
 static const struct intel_device_info intel_broadwell_gt3_info = {
 	BDW_FEATURES,
 	.gen = 8,
-	.is_broadwell = 1,
+	.platform = INTEL_BROADWELL,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 };
 
@@ -308,7 +317,7 @@ static const struct intel_device_info intel_cherryview_info = {
 	.gen = 8, .num_pipes = 3,
 	.has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
-	.is_cherryview = 1,
+	.platform = INTEL_CHERRYVIEW,
 	.has_64bit_reloc = 1,
 	.has_psr = 1,
 	.has_runtime_pm = 1,
@@ -326,7 +335,7 @@ static const struct intel_device_info intel_cherryview_info = {
 
 static const struct intel_device_info intel_skylake_info = {
 	BDW_FEATURES,
-	.is_skylake = 1,
+	.platform = INTEL_SKYLAKE,
 	.gen = 9,
 	.has_csr = 1,
 	.has_guc = 1,
@@ -335,7 +344,7 @@ static const struct intel_device_info intel_skylake_info = {
 
 static const struct intel_device_info intel_skylake_gt3_info = {
 	BDW_FEATURES,
-	.is_skylake = 1,
+	.platform = INTEL_SKYLAKE,
 	.gen = 9,
 	.has_csr = 1,
 	.has_guc = 1,
@@ -344,7 +353,7 @@ static const struct intel_device_info intel_skylake_gt3_info = {
 };
 
 static const struct intel_device_info intel_broxton_info = {
-	.is_broxton = 1,
+	.platform = INTEL_BROXTON,
 	.gen = 9,
 	.has_hotplug = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
@@ -372,7 +381,7 @@ static const struct intel_device_info intel_broxton_info = {
 
 static const struct intel_device_info intel_kabylake_info = {
 	BDW_FEATURES,
-	.is_kabylake = 1,
+	.platform = INTEL_KABYLAKE,
 	.gen = 9,
 	.has_csr = 1,
 	.has_guc = 1,
@@ -381,7 +390,7 @@ static const struct intel_device_info intel_kabylake_info = {
 
 static const struct intel_device_info intel_kabylake_gt3_info = {
 	BDW_FEATURES,
-	.is_kabylake = 1,
+	.platform = INTEL_KABYLAKE,
 	.gen = 9,
 	.has_csr = 1,
 	.has_guc = 1,
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 185e3bbc9ec9..93ed80939f8d 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -24,11 +24,49 @@
 
 #include "i915_drv.h"
 
+#define PLATFORM_NAME(x) [INTEL_##x] = #x
+static const char * const platform_names[] = {
+	PLATFORM_NAME(I830),
+	PLATFORM_NAME(I845G),
+	PLATFORM_NAME(I85X),
+	PLATFORM_NAME(I865G),
+	PLATFORM_NAME(I915G),
+	PLATFORM_NAME(I915GM),
+	PLATFORM_NAME(I945G),
+	PLATFORM_NAME(I945GM),
+	PLATFORM_NAME(G33),
+	PLATFORM_NAME(G4X),
+	PLATFORM_NAME(PINEVIEW),
+	PLATFORM_NAME(BROADWATER),
+	PLATFORM_NAME(CRESTLINE),
+	PLATFORM_NAME(IRONLAKE),
+	PLATFORM_NAME(SANDYBRIDGE),
+	PLATFORM_NAME(IVYBRIDGE),
+	PLATFORM_NAME(VALLEYVIEW),
+	PLATFORM_NAME(CHERRYVIEW),
+	PLATFORM_NAME(HASWELL),
+	PLATFORM_NAME(BROADWELL),
+	PLATFORM_NAME(SKYLAKE),
+	PLATFORM_NAME(BROXTON),
+	PLATFORM_NAME(KABYLAKE),
+};
+#undef PLATFORM_NAME
+
+const char *intel_platform_name(enum intel_platform platform)
+{
+	if (WARN_ON_ONCE(platform >= ARRAY_SIZE(platform_names) ||
+			 platform_names[platform] == NULL))
+		return "<unknown>";
+
+	return platform_names[platform];
+}
+
 void intel_device_info_dump(struct drm_i915_private *dev_priv)
 {
 	const struct intel_device_info *info = &dev_priv->info;
 
-	DRM_DEBUG_DRIVER("i915 device info: gen=%i, pciid=0x%04x rev=0x%02x",
+	DRM_DEBUG_DRIVER("i915 device info: %s gen=%i pciid=0x%04x rev=0x%02x",
+			 intel_platform_name(info->platform),
 			 info->gen,
 			 dev_priv->drm.pdev->device,
 			 dev_priv->drm.pdev->revision);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

[Chris Wilson]

On Fri, Nov 18, 2016 at 04:20:32PM +0200, Jani Nikula wrote:
> The platform flags in device info are (mostly) mutually
> exclusive. Replace the flags with an enum. Add the platform enum also
> for platforms that previously didn't have a flag, and give them codename
> logging in dmesg.
> 
> Pineview remains an exception, the platform being G33 for that.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Untested TGIF patch. ;)
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  1 +
>  drivers/gpu/drm/i915/i915_drv.h          | 76 +++++++++++++++++++-------------
>  drivers/gpu/drm/i915/i915_gpu_error.c    |  1 +
>  drivers/gpu/drm/i915/i915_pci.c          | 53 +++++++++++++---------
>  drivers/gpu/drm/i915/intel_device_info.c | 40 ++++++++++++++++-
>  5 files changed, 117 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b7f42c448a44..982f0bdb768f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -77,6 +77,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
>  	const struct intel_device_info *info = INTEL_INFO(dev_priv);
>  
>  	seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv));
> +	seq_printf(m, "platform: %s\n", intel_platform_name(info->platform));
>  	seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv));
>  #define PRINT_FLAG(x)  seq_printf(m, #x ": %s\n", yesno(info->x))
>  	DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index be67aeece749..b8a1b66599dc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -659,24 +659,8 @@ struct intel_csr {
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func) \
> -	/* Keep is_* in chronological order */ \
>  	func(is_mobile); \
> -	func(is_i85x); \
> -	func(is_i915g); \
> -	func(is_i945gm); \
> -	func(is_g33); \
> -	func(is_g4x); \
>  	func(is_pineview); \
> -	func(is_broadwater); \
> -	func(is_crestline); \
> -	func(is_ivybridge); \
> -	func(is_valleyview); \
> -	func(is_cherryview); \
> -	func(is_haswell); \
> -	func(is_broadwell); \
> -	func(is_skylake); \
> -	func(is_broxton); \
> -	func(is_kabylake); \
>  	func(is_alpha_support); \
>  	/* Keep has_* in alphabetical order */ \
>  	func(has_64bit_reloc); \
> @@ -726,6 +710,34 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>  	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
>  }
>  
> +/* Keep in chronological order */
> +enum intel_platform {
> +	INTEL_PLATFORM_UNINITIALIZED = 0,
> +	INTEL_I830,
> +	INTEL_I845G,
> +	INTEL_I85X,
> +	INTEL_I865G,
> +	INTEL_I915G,
> +	INTEL_I915GM,
> +	INTEL_I945G,
> +	INTEL_I945GM,
> +	INTEL_G33,
> +	INTEL_G4X,
> +	INTEL_PINEVIEW,
> +	INTEL_BROADWATER,
> +	INTEL_CRESTLINE,
> +	INTEL_IRONLAKE,
> +	INTEL_SANDYBRIDGE,
> +	INTEL_IVYBRIDGE,
> +	INTEL_VALLEYVIEW,
> +	INTEL_CHERRYVIEW,
> +	INTEL_HASWELL,
> +	INTEL_BROADWELL,

My earlier review fail. chv/bsw should be after bdw.

I expect Tvrtko to take this for a spin and see how this compares
against the current bittests.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

[Tvrtko Ursulin]

On 18/11/2016 14:20, Jani Nikula wrote:
> The platform flags in device info are (mostly) mutually
> exclusive. Replace the flags with an enum. Add the platform enum also
> for platforms that previously didn't have a flag, and give them codename
> logging in dmesg.

It just saddens me a bit that it prevents the compiler optimisation of 
our IS_THIS || IS_THAT || IS_TAT ugliness. :I On the basis on that I 
cannot quite make myself to support it, although it conceptually does 
make more sense.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: replace platform flags with a platform enum

[Patchwork]

== Series Details ==

Series: drm/i915: replace platform flags with a platform enum
URL   : https://patchwork.freedesktop.org/series/15558/
State : success

== Summary ==

Series 15558v1 drm/i915: replace platform flags with a platform enum
https://patchwork.freedesktop.org/api/1.0/series/15558/revisions/1/mbox/


fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

05104adb7446f310fad4e36f22d4c84ffafa31fc drm-intel-nightly: 2016y-11m-18d-13h-10m-16s UTC integration manifest
00a1f4d drm/i915: replace platform flags with a platform enum

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3056/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

[Jani Nikula]

On Fri, 18 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 18/11/2016 14:20, Jani Nikula wrote:
>> The platform flags in device info are (mostly) mutually
>> exclusive. Replace the flags with an enum. Add the platform enum also
>> for platforms that previously didn't have a flag, and give them codename
>> logging in dmesg.
>
> It just saddens me a bit that it prevents the compiler optimisation of 
> our IS_THIS || IS_THAT || IS_TAT ugliness. :I On the basis on that I 
> cannot quite make myself to support it, although it conceptually does 
> make more sense.

Personally I think making more sense conceptually trumps possible
compiler optimizations, especially when the code paths where this
actually matters are extremely rare. (Can you even point me at an
example where this makes a difference?)

That said, does doing something silly like this make a difference:

enum intel_platform {
	INTEL_PLATFORM_UNINITIALIZED = 0,
	INTEL_I830		= BIT(0),
	INTEL_I845G		= BIT(1),
	INTEL_I85X		= BIT(2),
	INTEL_I865G		= BIT(3),
	INTEL_I915G		= BIT(4),
	INTEL_I915GM		= BIT(5),
	INTEL_I945G		= BIT(6),
	INTEL_I945GM		= BIT(7),
	INTEL_G33		= BIT(8),
	INTEL_G4X		= BIT(9),
	INTEL_PINEVIEW		= BIT(10),
	INTEL_BROADWATER	= BIT(11),
	INTEL_CRESTLINE		= BIT(12),
	INTEL_IRONLAKE		= BIT(13),
	INTEL_SANDYBRIDGE	= BIT(14),
	INTEL_IVYBRIDGE		= BIT(15),
	INTEL_VALLEYVIEW	= BIT(16),
	INTEL_CHERRYVIEW	= BIT(17),
	INTEL_HASWELL		= BIT(18),
	INTEL_BROADWELL		= BIT(19),
	INTEL_SKYLAKE		= BIT(20),
	INTEL_BROXTON		= BIT(21),
	INTEL_KABYLAKE		= BIT(22),
};

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

[Tvrtko Ursulin]

On 21/11/2016 09:27, Jani Nikula wrote:
> On Fri, 18 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 18/11/2016 14:20, Jani Nikula wrote:
>>> The platform flags in device info are (mostly) mutually
>>> exclusive. Replace the flags with an enum. Add the platform enum also
>>> for platforms that previously didn't have a flag, and give them codename
>>> logging in dmesg.
>>
>> It just saddens me a bit that it prevents the compiler optimisation of
>> our IS_THIS || IS_THAT || IS_TAT ugliness. :I On the basis on that I
>> cannot quite make myself to support it, although it conceptually does
>> make more sense.
>
> Personally I think making more sense conceptually trumps possible
> compiler optimizations, especially when the code paths where this
> actually matters are extremely rare. (Can you even point me at an
> example where this makes a difference?)

Without thinking deeply, all instances like IS_HASWELL || IS_BROADWELL, 
IS_SKYLAKE || IS_KABYLAKE, or IS_VALLEYVIEW || IS_CHERRYVIEW. Quick grep 
shows that there is at least around 170 instances of those. There might 
be more, I don't know. At least the ones I listed all currently 
translate to a single conditional in the generated code.

I am not saying these are all on performance critical paths or anything, 
just that on balance, for me the sweetness of the optimisation cancels 
out the ugliness of the code (which is often unavoidable). Losing that 
and ending up with compare ladders in the code would just be a bit sad.

> That said, does doing something silly like this make a difference:
>
> enum intel_platform {
> 	INTEL_PLATFORM_UNINITIALIZED = 0,
> 	INTEL_I830		= BIT(0),
> 	INTEL_I845G		= BIT(1),
> 	INTEL_I85X		= BIT(2),
> 	INTEL_I865G		= BIT(3),
> 	INTEL_I915G		= BIT(4),
> 	INTEL_I915GM		= BIT(5),
> 	INTEL_I945G		= BIT(6),
> 	INTEL_I945GM		= BIT(7),
> 	INTEL_G33		= BIT(8),
> 	INTEL_G4X		= BIT(9),
> 	INTEL_PINEVIEW		= BIT(10),
> 	INTEL_BROADWATER	= BIT(11),
> 	INTEL_CRESTLINE		= BIT(12),
> 	INTEL_IRONLAKE		= BIT(13),
> 	INTEL_SANDYBRIDGE	= BIT(14),
> 	INTEL_IVYBRIDGE		= BIT(15),
> 	INTEL_VALLEYVIEW	= BIT(16),
> 	INTEL_CHERRYVIEW	= BIT(17),
> 	INTEL_HASWELL		= BIT(18),
> 	INTEL_BROADWELL		= BIT(19),
> 	INTEL_SKYLAKE		= BIT(20),
> 	INTEL_BROXTON		= BIT(21),
> 	INTEL_KABYLAKE		= BIT(22),
> };

I could live with that. Probably best to compare the code size before 
and after just to verify there are no surprises.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

[Jani Nikula]

On Mon, 21 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 21/11/2016 09:27, Jani Nikula wrote:
>> On Fri, 18 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 18/11/2016 14:20, Jani Nikula wrote:
>>>> The platform flags in device info are (mostly) mutually
>>>> exclusive. Replace the flags with an enum. Add the platform enum also
>>>> for platforms that previously didn't have a flag, and give them codename
>>>> logging in dmesg.
>>>
>>> It just saddens me a bit that it prevents the compiler optimisation of
>>> our IS_THIS || IS_THAT || IS_TAT ugliness. :I On the basis on that I
>>> cannot quite make myself to support it, although it conceptually does
>>> make more sense.
>>
>> Personally I think making more sense conceptually trumps possible
>> compiler optimizations, especially when the code paths where this
>> actually matters are extremely rare. (Can you even point me at an
>> example where this makes a difference?)
>
> Without thinking deeply, all instances like IS_HASWELL || IS_BROADWELL, 
> IS_SKYLAKE || IS_KABYLAKE, or IS_VALLEYVIEW || IS_CHERRYVIEW. Quick grep 
> shows that there is at least around 170 instances of those. There might 
> be more, I don't know. At least the ones I listed all currently 
> translate to a single conditional in the generated code.
>
> I am not saying these are all on performance critical paths or anything, 
> just that on balance, for me the sweetness of the optimisation cancels 
> out the ugliness of the code (which is often unavoidable). Losing that 
> and ending up with compare ladders in the code would just be a bit sad.

It doesn't persuade me much if something enables sweet optimizations
that nobody can perceive in real life.

Please also note that this brings decent codename debug prints to dmesg,
for all platforms, instead of a bunch of irrelevant is_platform = no for
every platform.

>> That said, does doing something silly like this make a difference:
>>
>> enum intel_platform {
>> 	INTEL_PLATFORM_UNINITIALIZED = 0,
>> 	INTEL_I830		= BIT(0),
>> 	INTEL_I845G		= BIT(1),
>> 	INTEL_I85X		= BIT(2),
>> 	INTEL_I865G		= BIT(3),
>> 	INTEL_I915G		= BIT(4),
>> 	INTEL_I915GM		= BIT(5),
>> 	INTEL_I945G		= BIT(6),
>> 	INTEL_I945GM		= BIT(7),
>> 	INTEL_G33		= BIT(8),
>> 	INTEL_G4X		= BIT(9),
>> 	INTEL_PINEVIEW		= BIT(10),
>> 	INTEL_BROADWATER	= BIT(11),
>> 	INTEL_CRESTLINE		= BIT(12),
>> 	INTEL_IRONLAKE		= BIT(13),
>> 	INTEL_SANDYBRIDGE	= BIT(14),
>> 	INTEL_IVYBRIDGE		= BIT(15),
>> 	INTEL_VALLEYVIEW	= BIT(16),
>> 	INTEL_CHERRYVIEW	= BIT(17),
>> 	INTEL_HASWELL		= BIT(18),
>> 	INTEL_BROADWELL		= BIT(19),
>> 	INTEL_SKYLAKE		= BIT(20),
>> 	INTEL_BROXTON		= BIT(21),
>> 	INTEL_KABYLAKE		= BIT(22),
>> };
>
> I could live with that. Probably best to compare the code size before 
> and after just to verify there are no surprises.

current nightly:
   text	   data	    bss	    dec	    hex	filename
1254149	  43528	   2048	1299725	 13d50d	drivers/gpu/drm/i915/i915.ko

original platform enum patch:
   text	   data	    bss	    dec	    hex	filename
1254456	  43529	   2048	1300033	 13d641	drivers/gpu/drm/i915/i915.ko

the same with BIT() for platform enums:
   text	   data	    bss	    dec	    hex	filename
34811672  43529	   2048	34857249	213e121	drivers/gpu/drm/i915/i915.ko

Holy cow, what happened there?!

Anyway the nightly -> original patch change is just a 300 byte increase
in text size.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

[Ville Syrjälä]

On Fri, Nov 18, 2016 at 04:20:32PM +0200, Jani Nikula wrote:
> The platform flags in device info are (mostly) mutually
> exclusive. Replace the flags with an enum. Add the platform enum also
> for platforms that previously didn't have a flag, and give them codename
> logging in dmesg.
> 
> Pineview remains an exception, the platform being G33 for that.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Untested TGIF patch. ;)
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  1 +
>  drivers/gpu/drm/i915/i915_drv.h          | 76 +++++++++++++++++++-------------
>  drivers/gpu/drm/i915/i915_gpu_error.c    |  1 +
>  drivers/gpu/drm/i915/i915_pci.c          | 53 +++++++++++++---------
>  drivers/gpu/drm/i915/intel_device_info.c | 40 ++++++++++++++++-
>  5 files changed, 117 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b7f42c448a44..982f0bdb768f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -77,6 +77,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
>  	const struct intel_device_info *info = INTEL_INFO(dev_priv);
>  
>  	seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv));
> +	seq_printf(m, "platform: %s\n", intel_platform_name(info->platform));
>  	seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv));
>  #define PRINT_FLAG(x)  seq_printf(m, #x ": %s\n", yesno(info->x))
>  	DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index be67aeece749..b8a1b66599dc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -659,24 +659,8 @@ struct intel_csr {
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func) \
> -	/* Keep is_* in chronological order */ \
>  	func(is_mobile); \
> -	func(is_i85x); \
> -	func(is_i915g); \
> -	func(is_i945gm); \
> -	func(is_g33); \
> -	func(is_g4x); \
>  	func(is_pineview); \
> -	func(is_broadwater); \
> -	func(is_crestline); \
> -	func(is_ivybridge); \
> -	func(is_valleyview); \
> -	func(is_cherryview); \
> -	func(is_haswell); \
> -	func(is_broadwell); \
> -	func(is_skylake); \
> -	func(is_broxton); \
> -	func(is_kabylake); \
>  	func(is_alpha_support); \
>  	/* Keep has_* in alphabetical order */ \
>  	func(has_64bit_reloc); \
> @@ -726,6 +710,34 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>  	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
>  }
>  
> +/* Keep in chronological order */
> +enum intel_platform {
> +	INTEL_PLATFORM_UNINITIALIZED = 0,
> +	INTEL_I830,
> +	INTEL_I845G,
> +	INTEL_I85X,
> +	INTEL_I865G,
> +	INTEL_I915G,
> +	INTEL_I915GM,
> +	INTEL_I945G,
> +	INTEL_I945GM,
> +	INTEL_G33,
> +	INTEL_G4X,
> +	INTEL_PINEVIEW,
> +	INTEL_BROADWATER,
> +	INTEL_CRESTLINE,

The order here is rather wonky.

Assuming we want to keep roughly to the gen based order, then we'd have:

G33/BLB
PNV
965G/BW
965GM/CL
G4X/ELK
GM45/CTG

OTOH if we want to order based on the chipset side of things (which
might be closer to a chronological order I believe) we would have:

965G/BW
965GM/CL
G33/BLB
PNV
G4X/ELK
GM45/CTG

Well, pnv might have to come after elk/ctg to actually to fit into
the chronological order correctly.

This also highlights the issue we're rather inconsistent with the
number vs. codename thing. I think if we want to keep using both we
should draw a clear line where we switch. I think I would at least
flip bw/cl over to the number scheme if we want to keep to the number
scheme for the older parts.

> +	INTEL_IRONLAKE,
> +	INTEL_SANDYBRIDGE,
> +	INTEL_IVYBRIDGE,
> +	INTEL_VALLEYVIEW,
> +	INTEL_CHERRYVIEW,
> +	INTEL_HASWELL,
> +	INTEL_BROADWELL,

And as Chris noted this should be (based on either gen or
chronological order):

IVB
VLV
HSW
BDW
CHV

> +	INTEL_SKYLAKE,
> +	INTEL_BROXTON,
> +	INTEL_KABYLAKE,
> +};
> +
-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx