[PATCH 0/3] Claw back the IS_<platform> optimisation

[PATCH 0/3] Claw back the IS_<platform> optimisation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reorganise the IS_<platform> code a bit inspired by the pre-processor
code generation as used by Chris in the self test patches.

This avoids some source code duplication and makes maintenance easier.

In the last patch try to claw back the multiple IS_<platform> optimisation,
as the commit message says:

    If we use only power of two values for the platform enum
    values we can let the compiler optimize some common
    checks to a single conditional.

    For example code like this:

        if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))

    Goes from this:

       5c3c5:       8b 83 d8 06 00 00       mov    0x6d8(%rbx),%eax
       5c3cb:       83 f8 12                cmp    $0x12,%eax
       5c3ce:       0f 84 f3 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>
       5c3d4:       83 f8 15                cmp    $0x15,%eax
       5c3d7:       0f 84 ea 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>

    To this:

       5c1d5:       f7 83 d8 06 00 00 00    testl  $0x240000,0x6d8(%rbx)
       5c1dc:       00 24 00
       5c1df:       0f 85 da 00 00 00       jne    5c2bf <fw_domain_init+0x18f>

    It is not much but there is value in this that as long as we
    have to have conditions like the above sprinkled troughout the
    code, we can at least have the generate binary a bit smarter.

    Until we get to more than 32 platforms there is no downside
    to this approach.

Patch series saves 1890 bytes of binary:

   text    data     bss     dec     hex filename
1103970   26273     580 1130823  114147 i915.ko.0
1102080   26273     580 1128933  1139e5 i915.ko.1

Tvrtko Ursulin (3):
  drm/i915: Introduct i915_platforms.h
  drm/i915: Generate all IS_<platform> macros
  drm/i915: Number the platform enum strategically

 drivers/gpu/drm/i915/i915_drv.h          | 69 +++++++++-----------------------
 drivers/gpu/drm/i915/i915_platforms.h    | 34 ++++++++++++++++
 drivers/gpu/drm/i915/intel_device_info.c | 38 ++++--------------
 3 files changed, 61 insertions(+), 80 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_platforms.h

-- 
2.7.4

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

[PATCH 1/3] drm/i915: Introduct i915_platforms.h

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Inspired by the same approach used by Chris Wilson in the self
test patches.

We add a separate header file containing the list of our
platforms and then use the pre-processor to generate all
other places which use that list. This avoids having to
list them multiple times and avoids the maintenance
burden.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 30 +++-------------------------
 drivers/gpu/drm/i915/i915_platforms.h    | 34 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_device_info.c | 30 +++-------------------------
 3 files changed, 40 insertions(+), 54 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_platforms.h

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1480e733312a..ea06d3ff59da 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -825,34 +825,10 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
 }
 
-/* Keep in gen based order, and chronological order within a gen */
 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_PINEVIEW,
-	INTEL_I965G,
-	INTEL_I965GM,
-	INTEL_G45,
-	INTEL_GM45,
-	INTEL_IRONLAKE,
-	INTEL_SANDYBRIDGE,
-	INTEL_IVYBRIDGE,
-	INTEL_VALLEYVIEW,
-	INTEL_HASWELL,
-	INTEL_BROADWELL,
-	INTEL_CHERRYVIEW,
-	INTEL_SKYLAKE,
-	INTEL_BROXTON,
-	INTEL_KABYLAKE,
-	INTEL_GEMINILAKE,
+#define i915_platform(name, value) INTEL_##name = value,
+#include "i915_platforms.h"
+#undef i915_platform
 };
 
 struct intel_device_info {
diff --git a/drivers/gpu/drm/i915/i915_platforms.h b/drivers/gpu/drm/i915/i915_platforms.h
new file mode 100644
index 000000000000..b44ea1dd9c15
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_platforms.h
@@ -0,0 +1,34 @@
+/*
+ * List each platform as i915_platform(name, id).
+ *
+ * Names must be legal C identifiers and the ids must be unique integers.
+ *
+ * Keep in gen based order, and chronological order within a gen.
+ */
+
+i915_platform(UNINITIALIZED,  0)
+i915_platform(I830,	      1)
+i915_platform(I845G,	      2)
+i915_platform(I85X,	      3)
+i915_platform(I865G,	      4)
+i915_platform(I915G,	      5)
+i915_platform(I915GM,	      6)
+i915_platform(I945G,	      7)
+i915_platform(I945GM,	      8)
+i915_platform(G33,	      9)
+i915_platform(PINEVIEW,	     10)
+i915_platform(I965G,	     11)
+i915_platform(I965GM,	     12)
+i915_platform(G45,	     13)
+i915_platform(GM45,	     14)
+i915_platform(IRONLAKE,	     15)
+i915_platform(SANDYBRIDGE,   16)
+i915_platform(IVYBRIDGE,     17)
+i915_platform(VALLEYVIEW,    18)
+i915_platform(HASWELL,	     19)
+i915_platform(BROADWELL,     20)
+i915_platform(CHERRYVIEW,    21)
+i915_platform(SKYLAKE,	     22)
+i915_platform(BROXTON,	     23)
+i915_platform(KABYLAKE,	     24)
+i915_platform(GEMINILAKE,    25)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index c46415b8c1b9..5192d388d10e 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -24,35 +24,11 @@
 
 #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(PINEVIEW),
-	PLATFORM_NAME(I965G),
-	PLATFORM_NAME(I965GM),
-	PLATFORM_NAME(G45),
-	PLATFORM_NAME(GM45),
-	PLATFORM_NAME(IRONLAKE),
-	PLATFORM_NAME(SANDYBRIDGE),
-	PLATFORM_NAME(IVYBRIDGE),
-	PLATFORM_NAME(VALLEYVIEW),
-	PLATFORM_NAME(HASWELL),
-	PLATFORM_NAME(BROADWELL),
-	PLATFORM_NAME(CHERRYVIEW),
-	PLATFORM_NAME(SKYLAKE),
-	PLATFORM_NAME(BROXTON),
-	PLATFORM_NAME(KABYLAKE),
-	PLATFORM_NAME(GEMINILAKE),
+#define i915_platform(name, id) [id] = #name,
+#include "i915_platforms.h"
+#undef i915_platform
 };
-#undef PLATFORM_NAME
 
 const char *intel_platform_name(enum intel_platform platform)
 {
-- 
2.7.4

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

[PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Instead of listing them individually we can generate them
using the new i915_platforms.h header.

Also convert them to a static inline function which
interestingly makes the code smaller as well.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea06d3ff59da..fb8f4b7cd1ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2579,36 +2579,29 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define IS_REVID(p, since, until) \
 	(INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
 
-#define IS_I830(dev_priv)	((dev_priv)->info.platform == INTEL_I830)
-#define IS_I845G(dev_priv)	((dev_priv)->info.platform == INTEL_I845G)
-#define IS_I85X(dev_priv)	((dev_priv)->info.platform == INTEL_I85X)
-#define IS_I865G(dev_priv)	((dev_priv)->info.platform == INTEL_I865G)
-#define IS_I915G(dev_priv)	((dev_priv)->info.platform == INTEL_I915G)
-#define IS_I915GM(dev_priv)	((dev_priv)->info.platform == INTEL_I915GM)
-#define IS_I945G(dev_priv)	((dev_priv)->info.platform == INTEL_I945G)
-#define IS_I945GM(dev_priv)	((dev_priv)->info.platform == INTEL_I945GM)
-#define IS_I965G(dev_priv)	((dev_priv)->info.platform == INTEL_I965G)
-#define IS_I965GM(dev_priv)	((dev_priv)->info.platform == INTEL_I965GM)
-#define IS_G45(dev_priv)	((dev_priv)->info.platform == INTEL_G45)
-#define IS_GM45(dev_priv)	((dev_priv)->info.platform == INTEL_GM45)
+/*
+ * This block generates the IS_<platform> helper functions in the format of:
+ *
+ * static inline bool IS_SKYLAKE(const struct drm_i915_private *dev_priv);
+ * ...
+ *
+ * One for each platform listed in i915_platforms.h is generated.
+ */
+#define i915_platform(name, id) \
+static inline bool IS_##name(const struct drm_i915_private *dev_priv) \
+{ \
+	return (dev_priv)->info.platform == INTEL_##name; \
+}
+#include "i915_platforms.h"
+#undef i915_platform
+
 #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(dev_priv)	((dev_priv)->info.platform == INTEL_PINEVIEW)
-#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.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.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_GEMINILAKE(dev_priv)	((dev_priv)->info.platform == INTEL_GEMINILAKE)
 #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)
-- 
2.7.4

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

[PATCH 3/3] drm/i915: Number the platform enum strategically

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

If we use only power of two values for the platform enum
values we can let the compiler optimize some common
checks to a single conditional.

For example code like this:

	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))

Goes from this:

   5c3c5:       8b 83 d8 06 00 00       mov    0x6d8(%rbx),%eax
   5c3cb:       83 f8 12                cmp    $0x12,%eax
   5c3ce:       0f 84 f3 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>
   5c3d4:       83 f8 15                cmp    $0x15,%eax
   5c3d7:       0f 84 ea 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>

To this:

   5c1d5:       f7 83 d8 06 00 00 00    testl  $0x240000,0x6d8(%rbx)
   5c1dc:       00 24 00
   5c1df:       0f 85 da 00 00 00       jne    5c2bf <fw_domain_init+0x18f>

It is not much but there is value in this that as long as we
have to have conditions like the above sprinkled troughout the
code, we can at least have the generate binary a bit smarter.

Until we get to more than 32 platforms there is no downside
to this approach.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  2 +-
 drivers/gpu/drm/i915/i915_platforms.h    | 50 ++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_device_info.c | 10 ++++---
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fb8f4b7cd1ae..347d5c6ffc1b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2590,7 +2590,7 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define i915_platform(name, id) \
 static inline bool IS_##name(const struct drm_i915_private *dev_priv) \
 { \
-	return (dev_priv)->info.platform == INTEL_##name; \
+	return !!((dev_priv)->info.platform & INTEL_##name); \
 }
 #include "i915_platforms.h"
 #undef i915_platform
diff --git a/drivers/gpu/drm/i915/i915_platforms.h b/drivers/gpu/drm/i915/i915_platforms.h
index b44ea1dd9c15..4118f152eac9 100644
--- a/drivers/gpu/drm/i915/i915_platforms.h
+++ b/drivers/gpu/drm/i915/i915_platforms.h
@@ -7,28 +7,28 @@
  */
 
 i915_platform(UNINITIALIZED,  0)
-i915_platform(I830,	      1)
-i915_platform(I845G,	      2)
-i915_platform(I85X,	      3)
-i915_platform(I865G,	      4)
-i915_platform(I915G,	      5)
-i915_platform(I915GM,	      6)
-i915_platform(I945G,	      7)
-i915_platform(I945GM,	      8)
-i915_platform(G33,	      9)
-i915_platform(PINEVIEW,	     10)
-i915_platform(I965G,	     11)
-i915_platform(I965GM,	     12)
-i915_platform(G45,	     13)
-i915_platform(GM45,	     14)
-i915_platform(IRONLAKE,	     15)
-i915_platform(SANDYBRIDGE,   16)
-i915_platform(IVYBRIDGE,     17)
-i915_platform(VALLEYVIEW,    18)
-i915_platform(HASWELL,	     19)
-i915_platform(BROADWELL,     20)
-i915_platform(CHERRYVIEW,    21)
-i915_platform(SKYLAKE,	     22)
-i915_platform(BROXTON,	     23)
-i915_platform(KABYLAKE,	     24)
-i915_platform(GEMINILAKE,    25)
+i915_platform(I830,	      BIT(1))
+i915_platform(I845G,	      BIT(2))
+i915_platform(I85X,	      BIT(3))
+i915_platform(I865G,	      BIT(4))
+i915_platform(I915G,	      BIT(5))
+i915_platform(I915GM,	      BIT(6))
+i915_platform(I945G,	      BIT(7))
+i915_platform(I945GM,	      BIT(8))
+i915_platform(G33,	      BIT(9))
+i915_platform(PINEVIEW,	     BIT(10))
+i915_platform(I965G,	     BIT(11))
+i915_platform(I965GM,	     BIT(12))
+i915_platform(G45,	     BIT(13))
+i915_platform(GM45,	     BIT(14))
+i915_platform(IRONLAKE,	     BIT(15))
+i915_platform(SANDYBRIDGE,   BIT(16))
+i915_platform(IVYBRIDGE,     BIT(17))
+i915_platform(VALLEYVIEW,    BIT(18))
+i915_platform(HASWELL,	     BIT(19))
+i915_platform(BROADWELL,     BIT(20))
+i915_platform(CHERRYVIEW,    BIT(21))
+i915_platform(SKYLAKE,	     BIT(22))
+i915_platform(BROXTON,	     BIT(23))
+i915_platform(KABYLAKE,	     BIT(24))
+i915_platform(GEMINILAKE,    BIT(25))
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 5192d388d10e..26df6363e265 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -25,18 +25,20 @@
 #include "i915_drv.h"
 
 static const char * const platform_names[] = {
-#define i915_platform(name, id) [id] = #name,
+#define i915_platform(name, id) [__builtin_ffs(id)] = #name,
 #include "i915_platforms.h"
 #undef i915_platform
 };
 
 const char *intel_platform_name(enum intel_platform platform)
 {
-	if (WARN_ON_ONCE(platform >= ARRAY_SIZE(platform_names) ||
-			 platform_names[platform] == NULL))
+	unsigned int idx = ffs(platform);
+
+	if (WARN_ON_ONCE(idx >= ARRAY_SIZE(platform_names) ||
+			 platform_names[idx] == NULL))
 		return "<unknown>";
 
-	return platform_names[platform];
+	return platform_names[idx];
 }
 
 void intel_device_info_dump(struct drm_i915_private *dev_priv)
-- 
2.7.4

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

Re: [PATCH 3/3] drm/i915: Number the platform enum strategically

[Chris Wilson]

On Thu, Dec 08, 2016 at 09:49:59AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> If we use only power of two values for the platform enum
> values we can let the compiler optimize some common
> checks to a single conditional.
> 
> For example code like this:
> 
> 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> 
> Goes from this:
> 
>    5c3c5:       8b 83 d8 06 00 00       mov    0x6d8(%rbx),%eax
>    5c3cb:       83 f8 12                cmp    $0x12,%eax
>    5c3ce:       0f 84 f3 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>
>    5c3d4:       83 f8 15                cmp    $0x15,%eax
>    5c3d7:       0f 84 ea 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>
> 
> To this:
> 
>    5c1d5:       f7 83 d8 06 00 00 00    testl  $0x240000,0x6d8(%rbx)
>    5c1dc:       00 24 00
>    5c1df:       0f 85 da 00 00 00       jne    5c2bf <fw_domain_init+0x18f>
> 
> It is not much but there is value in this that as long as we
> have to have conditions like the above sprinkled troughout the
> code, we can at least have the generate binary a bit smarter.
> 
> Until we get to more than 32 platforms there is no downside
> to this approach.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          |  2 +-
>  drivers/gpu/drm/i915/i915_platforms.h    | 50 ++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_device_info.c | 10 ++++---
>  3 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fb8f4b7cd1ae..347d5c6ffc1b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2590,7 +2590,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define i915_platform(name, id) \
>  static inline bool IS_##name(const struct drm_i915_private *dev_priv) \
>  { \
> -	return (dev_priv)->info.platform == INTEL_##name; \
> +	return !!((dev_priv)->info.platform & INTEL_##name); \

Redundant !!. It's an integer in a bool context, the !! are implied by
spec and gcc.

>  }
>  #include "i915_platforms.h"
>  #undef i915_platform
> diff --git a/drivers/gpu/drm/i915/i915_platforms.h b/drivers/gpu/drm/i915/i915_platforms.h
> index b44ea1dd9c15..4118f152eac9 100644
> --- a/drivers/gpu/drm/i915/i915_platforms.h
> +++ b/drivers/gpu/drm/i915/i915_platforms.h
> @@ -7,28 +7,28 @@
>   */
>  
>  i915_platform(UNINITIALIZED,  0)
> -i915_platform(I830,	      1)
> +i915_platform(I830,	      BIT(1))

Could start from BIT(0) or are we still anticipating support for i810?
And i740?

> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 5192d388d10e..26df6363e265 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -25,18 +25,20 @@
>  #include "i915_drv.h"
>  
>  static const char * const platform_names[] = {
> -#define i915_platform(name, id) [id] = #name,
> +#define i915_platform(name, id) [__builtin_ffs(id)] = #name,
>  #include "i915_platforms.h"
>  #undef i915_platform
>  };
>  
>  const char *intel_platform_name(enum intel_platform platform)
>  {
> -	if (WARN_ON_ONCE(platform >= ARRAY_SIZE(platform_names) ||
> -			 platform_names[platform] == NULL))
> +	unsigned int idx = ffs(platform);

Ah, ffs() is offset by one, and our id's are offset by another 1.

Using i915_platforms.h for a single list is a good improvement. And I
think we can get the best of both worlds: a concise identifier in the
logs and error state; and concise code. Looks good with a few fixes.
-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: [PATCH 1/3] drm/i915: Introduct i915_platforms.h

[Jani Nikula]

On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Inspired by the same approach used by Chris Wilson in the self
> test patches.
>
> We add a separate header file containing the list of our
> platforms and then use the pre-processor to generate all
> other places which use that list. This avoids having to
> list them multiple times and avoids the maintenance
> burden.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 30 +++-------------------------
>  drivers/gpu/drm/i915/i915_platforms.h    | 34 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_device_info.c | 30 +++-------------------------
>  3 files changed, 40 insertions(+), 54 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_platforms.h
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1480e733312a..ea06d3ff59da 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -825,34 +825,10 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>  	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
>  }
>  
> -/* Keep in gen based order, and chronological order within a gen */
>  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_PINEVIEW,
> -	INTEL_I965G,
> -	INTEL_I965GM,
> -	INTEL_G45,
> -	INTEL_GM45,
> -	INTEL_IRONLAKE,
> -	INTEL_SANDYBRIDGE,
> -	INTEL_IVYBRIDGE,
> -	INTEL_VALLEYVIEW,
> -	INTEL_HASWELL,
> -	INTEL_BROADWELL,
> -	INTEL_CHERRYVIEW,
> -	INTEL_SKYLAKE,
> -	INTEL_BROXTON,
> -	INTEL_KABYLAKE,
> -	INTEL_GEMINILAKE,
> +#define i915_platform(name, value) INTEL_##name = value,
> +#include "i915_platforms.h"
> +#undef i915_platform
>  };
>  
>  struct intel_device_info {
> diff --git a/drivers/gpu/drm/i915/i915_platforms.h b/drivers/gpu/drm/i915/i915_platforms.h
> new file mode 100644
> index 000000000000..b44ea1dd9c15
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_platforms.h
> @@ -0,0 +1,34 @@
> +/*
> + * List each platform as i915_platform(name, id).
> + *
> + * Names must be legal C identifiers and the ids must be unique integers.
> + *
> + * Keep in gen based order, and chronological order within a gen.
> + */
> +
> +i915_platform(UNINITIALIZED,  0)
> +i915_platform(I830,	      1)
> +i915_platform(I845G,	      2)
> +i915_platform(I85X,	      3)
> +i915_platform(I865G,	      4)
> +i915_platform(I915G,	      5)
> +i915_platform(I915GM,	      6)
> +i915_platform(I945G,	      7)
> +i915_platform(I945GM,	      8)
> +i915_platform(G33,	      9)
> +i915_platform(PINEVIEW,	     10)
> +i915_platform(I965G,	     11)
> +i915_platform(I965GM,	     12)
> +i915_platform(G45,	     13)
> +i915_platform(GM45,	     14)
> +i915_platform(IRONLAKE,	     15)
> +i915_platform(SANDYBRIDGE,   16)
> +i915_platform(IVYBRIDGE,     17)
> +i915_platform(VALLEYVIEW,    18)
> +i915_platform(HASWELL,	     19)
> +i915_platform(BROADWELL,     20)
> +i915_platform(CHERRYVIEW,    21)
> +i915_platform(SKYLAKE,	     22)
> +i915_platform(BROXTON,	     23)
> +i915_platform(KABYLAKE,	     24)
> +i915_platform(GEMINILAKE,    25)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index c46415b8c1b9..5192d388d10e 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -24,35 +24,11 @@
>  
>  #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(PINEVIEW),
> -	PLATFORM_NAME(I965G),
> -	PLATFORM_NAME(I965GM),
> -	PLATFORM_NAME(G45),
> -	PLATFORM_NAME(GM45),
> -	PLATFORM_NAME(IRONLAKE),
> -	PLATFORM_NAME(SANDYBRIDGE),
> -	PLATFORM_NAME(IVYBRIDGE),
> -	PLATFORM_NAME(VALLEYVIEW),
> -	PLATFORM_NAME(HASWELL),
> -	PLATFORM_NAME(BROADWELL),
> -	PLATFORM_NAME(CHERRYVIEW),
> -	PLATFORM_NAME(SKYLAKE),
> -	PLATFORM_NAME(BROXTON),
> -	PLATFORM_NAME(KABYLAKE),
> -	PLATFORM_NAME(GEMINILAKE),
> +#define i915_platform(name, id) [id] = #name,
> +#include "i915_platforms.h"
> +#undef i915_platform
>  };
> -#undef PLATFORM_NAME

I was thinking we could improve the printed names here, for example:

	[INTEL_I965G] = "I965G/Broadwater"
	[INTEL_VALLEYVIEW] = "Valleyview/Baytrail"

But I suppose less repetition is better.

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



>  
>  const char *intel_platform_name(enum intel_platform platform)
>  {

-- 
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: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Jani Nikula]

On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Instead of listing them individually we can generate them
> using the new i915_platforms.h header.
>
> Also convert them to a static inline function which
> interestingly makes the code smaller as well.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

NAK. Absolutely opposed to this.

A large part of my work involves digging through the source tree, and a
crucial part of that is looking up definitions and references, both for
macros and functions. Not having the macro/function definitions breaks
that workflow. Losing that, source code archeology gets *much*
harder. The losses are much greater than the gains.

BR,
Jani.



> ---
>  drivers/gpu/drm/i915/i915_drv.h | 39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea06d3ff59da..fb8f4b7cd1ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2579,36 +2579,29 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define IS_REVID(p, since, until) \
>  	(INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>  
> -#define IS_I830(dev_priv)	((dev_priv)->info.platform == INTEL_I830)
> -#define IS_I845G(dev_priv)	((dev_priv)->info.platform == INTEL_I845G)
> -#define IS_I85X(dev_priv)	((dev_priv)->info.platform == INTEL_I85X)
> -#define IS_I865G(dev_priv)	((dev_priv)->info.platform == INTEL_I865G)
> -#define IS_I915G(dev_priv)	((dev_priv)->info.platform == INTEL_I915G)
> -#define IS_I915GM(dev_priv)	((dev_priv)->info.platform == INTEL_I915GM)
> -#define IS_I945G(dev_priv)	((dev_priv)->info.platform == INTEL_I945G)
> -#define IS_I945GM(dev_priv)	((dev_priv)->info.platform == INTEL_I945GM)
> -#define IS_I965G(dev_priv)	((dev_priv)->info.platform == INTEL_I965G)
> -#define IS_I965GM(dev_priv)	((dev_priv)->info.platform == INTEL_I965GM)
> -#define IS_G45(dev_priv)	((dev_priv)->info.platform == INTEL_G45)
> -#define IS_GM45(dev_priv)	((dev_priv)->info.platform == INTEL_GM45)
> +/*
> + * This block generates the IS_<platform> helper functions in the format of:
> + *
> + * static inline bool IS_SKYLAKE(const struct drm_i915_private *dev_priv);
> + * ...
> + *
> + * One for each platform listed in i915_platforms.h is generated.
> + */
> +#define i915_platform(name, id) \
> +static inline bool IS_##name(const struct drm_i915_private *dev_priv) \
> +{ \
> +	return (dev_priv)->info.platform == INTEL_##name; \
> +}
> +#include "i915_platforms.h"
> +#undef i915_platform
> +
>  #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(dev_priv)	((dev_priv)->info.platform == INTEL_PINEVIEW)
> -#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.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.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_GEMINILAKE(dev_priv)	((dev_priv)->info.platform == INTEL_GEMINILAKE)
>  #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)

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

✓ Fi.CI.BAT: success for Claw back the IS_<platform> optimisation

[Patchwork]

== Series Details ==

Series: Claw back the IS_<platform> optimisation
URL   : https://patchwork.freedesktop.org/series/16544/
State : success

== Summary ==

Series 16544v1 Claw back the IS_<platform> optimisation
https://patchwork.freedesktop.org/api/1.0/series/16544/revisions/1/mbox/


fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

17b1372b7ee9bfd08072bdbeab11b372e82c073b drm-tip: 2016y-12m-08d-10h-13m-45s UTC integration manifest
759148f4 drm/i915: Number the platform enum strategically
4ceaea9 drm/i915: Generate all IS_<platform> macros
01e2445 drm/i915: Introduct i915_platforms.h

== Logs ==

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

Re: [PATCH 1/3] drm/i915: Introduct i915_platforms.h

[Joonas Lahtinen]

On to, 2016-12-08 at 12:41 +0200, Jani Nikula wrote:
> > On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> > 
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Inspired by the same approach used by Chris Wilson in the self
> > test patches.
> > 
> > We add a separate header file containing the list of our
> > platforms and then use the pre-processor to generate all
> > other places which use that list. This avoids having to
> > list them multiple times and avoids the maintenance
> > burden.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

<SNIP>

> I was thinking we could improve the printed names here, for example:
> 
> 	[INTEL_I965G] = "I965G/Broadwater"
> 	[INTEL_VALLEYVIEW] = "Valleyview/Baytrail"
> 

+1 on this idea!

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Introduct i915_platforms.h

[Tvrtko Ursulin]

On 08/12/2016 13:16, Joonas Lahtinen wrote:
> On to, 2016-12-08 at 12:41 +0200, Jani Nikula wrote:
>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Inspired by the same approach used by Chris Wilson in the self
>>> test patches.
>>>
>>> We add a separate header file containing the list of our
>>> platforms and then use the pre-processor to generate all
>>> other places which use that list. This avoids having to
>>> list them multiple times and avoids the maintenance
>>> burden.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> <SNIP>
>
>> I was thinking we could improve the printed names here, for example:
>>
>> 	[INTEL_I965G] = "I965G/Broadwater"
>> 	[INTEL_VALLEYVIEW] = "Valleyview/Baytrail"
>>
>
> +1 on this idea!

Too much prettiness for the kernel I would have thought. Don't see the 
benefit personally.

Regards,

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

Re: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Tvrtko Ursulin]

On 08/12/2016 10:46, Jani Nikula wrote:
> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Instead of listing them individually we can generate them
>> using the new i915_platforms.h header.
>>
>> Also convert them to a static inline function which
>> interestingly makes the code smaller as well.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> NAK. Absolutely opposed to this.

Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs 
any longer?

> A large part of my work involves digging through the source tree, and a
> crucial part of that is looking up definitions and references, both for
> macros and functions. Not having the macro/function definitions breaks
> that workflow. Losing that, source code archeology gets *much*
> harder. The losses are much greater than the gains.

Hm, I struggle to see that point on the same magnitude of a disaster 
scale as you. I would have thought we all know what IS_SKYLAKE & co are 
so it would be no big deal.

Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.

Regards,

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

Re: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Jani Nikula]

On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 08/12/2016 10:46, Jani Nikula wrote:
>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Instead of listing them individually we can generate them
>>> using the new i915_platforms.h header.
>>>
>>> Also convert them to a static inline function which
>>> interestingly makes the code smaller as well.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>
>> NAK. Absolutely opposed to this.
>
> Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs 
> any longer?

Only when dropped without rationale. I needed to make it clear in no
uncertain terms how important this is to me.

>> A large part of my work involves digging through the source tree, and a
>> crucial part of that is looking up definitions and references, both for
>> macros and functions. Not having the macro/function definitions breaks
>> that workflow. Losing that, source code archeology gets *much*
>> harder. The losses are much greater than the gains.
>
> Hm, I struggle to see that point on the same magnitude of a disaster 
> scale as you. I would have thought we all know what IS_SKYLAKE & co are 
> so it would be no big deal.

Sure we know what they are; I want to be able to see all the
*references* to them as well, using GNU global. That fails if they're
not defined in the first place. And no, git grep is not the same.

> Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.

Then all the things passed as parameter would have to be defined.


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

[PATCH v2] drm/i915: Number the platform enum strategically

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

If we use only power of two values for the platform enum
values we can let the compiler optimize some common
checks to a single conditional.

For example code like this:

	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))

Goes from this:

   5c3c5:       8b 83 d8 06 00 00       mov    0x6d8(%rbx),%eax
   5c3cb:       83 f8 12                cmp    $0x12,%eax
   5c3ce:       0f 84 f3 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>
   5c3d4:       83 f8 15                cmp    $0x15,%eax
   5c3d7:       0f 84 ea 00 00 00       je     5c4c7 <fw_domain_init+0x1a7>

To this:

   5c1d5:       f7 83 d8 06 00 00 00    testl  $0x240000,0x6d8(%rbx)
   5c1dc:       00 24 00
   5c1df:       0f 85 da 00 00 00       jne    5c2bf <fw_domain_init+0x18f>

It is not much but there is value in this that as long as we
have to have conditions like the above sprinkled troughout the
code, we can at least have the generate binary a bit smarter.

Until we get to more than 32 platforms there is no downside
to this approach.

v2:
 * Remove !! from the comparison. (Chris Wilson)
 * Simplify the patch by moving the BIT() to the generator.
   (Michal Wajdeczko)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  4 ++--
 drivers/gpu/drm/i915/intel_device_info.c | 10 ++++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fb8f4b7cd1ae..3be6a19f22eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -826,7 +826,7 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 }
 
 enum intel_platform {
-#define i915_platform(name, value) INTEL_##name = value,
+#define i915_platform(name, value) INTEL_##name = BIT(value),
 #include "i915_platforms.h"
 #undef i915_platform
 };
@@ -2590,7 +2590,7 @@ intel_info(const struct drm_i915_private *dev_priv)
 #define i915_platform(name, id) \
 static inline bool IS_##name(const struct drm_i915_private *dev_priv) \
 { \
-	return (dev_priv)->info.platform == INTEL_##name; \
+	return (dev_priv)->info.platform & INTEL_##name; \
 }
 #include "i915_platforms.h"
 #undef i915_platform
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 5192d388d10e..26df6363e265 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -25,18 +25,20 @@
 #include "i915_drv.h"
 
 static const char * const platform_names[] = {
-#define i915_platform(name, id) [id] = #name,
+#define i915_platform(name, id) [__builtin_ffs(id)] = #name,
 #include "i915_platforms.h"
 #undef i915_platform
 };
 
 const char *intel_platform_name(enum intel_platform platform)
 {
-	if (WARN_ON_ONCE(platform >= ARRAY_SIZE(platform_names) ||
-			 platform_names[platform] == NULL))
+	unsigned int idx = ffs(platform);
+
+	if (WARN_ON_ONCE(idx >= ARRAY_SIZE(platform_names) ||
+			 platform_names[idx] == NULL))
 		return "<unknown>";
 
-	return platform_names[platform];
+	return platform_names[idx];
 }
 
 void intel_device_info_dump(struct drm_i915_private *dev_priv)
-- 
2.7.4

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

Re: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Jani Nikula]

On Thu, 08 Dec 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 08/12/2016 10:46, Jani Nikula wrote:
>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Instead of listing them individually we can generate them
>>>> using the new i915_platforms.h header.
>>>>
>>>> Also convert them to a static inline function which
>>>> interestingly makes the code smaller as well.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>
>>> NAK. Absolutely opposed to this.
>>
>> Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs 
>> any longer?
>
> Only when dropped without rationale. I needed to make it clear in no
> uncertain terms how important this is to me.
>
>>> A large part of my work involves digging through the source tree, and a
>>> crucial part of that is looking up definitions and references, both for
>>> macros and functions. Not having the macro/function definitions breaks
>>> that workflow. Losing that, source code archeology gets *much*
>>> harder. The losses are much greater than the gains.
>>
>> Hm, I struggle to see that point on the same magnitude of a disaster 
>> scale as you. I would have thought we all know what IS_SKYLAKE & co are 
>> so it would be no big deal.
>
> Sure we know what they are; I want to be able to see all the
> *references* to them as well, using GNU global. That fails if they're
> not defined in the first place. And no, git grep is not the same.
>
>> Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.
>
> Then all the things passed as parameter would have to be defined.

Oh, btw, patch 1/3 also fails this because the INTEL_FOO enumerations
aren't defined as-is but via macros. This is not as important as the
IS_FOO ones, because the former aren't really used throughout the
source, while the latter are.

BR,
Jani.

>
>
> 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: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Tvrtko Ursulin]

On 08/12/2016 13:37, Jani Nikula wrote:
> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 08/12/2016 10:46, Jani Nikula wrote:
>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Instead of listing them individually we can generate them
>>>> using the new i915_platforms.h header.
>>>>
>>>> Also convert them to a static inline function which
>>>> interestingly makes the code smaller as well.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>
>>> NAK. Absolutely opposed to this.
>>
>> Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs
>> any longer?
>
> Only when dropped without rationale. I needed to make it clear in no
> uncertain terms how important this is to me.

Hm ok, I'll give you a benefit of doubt here.

>>> A large part of my work involves digging through the source tree, and a
>>> crucial part of that is looking up definitions and references, both for
>>> macros and functions. Not having the macro/function definitions breaks
>>> that workflow. Losing that, source code archeology gets *much*
>>> harder. The losses are much greater than the gains.
>>
>> Hm, I struggle to see that point on the same magnitude of a disaster
>> scale as you. I would have thought we all know what IS_SKYLAKE & co are
>> so it would be no big deal.
>
> Sure we know what they are; I want to be able to see all the
> *references* to them as well, using GNU global. That fails if they're
> not defined in the first place. And no, git grep is not the same.
>
>> Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.
>
> Then all the things passed as parameter would have to be defined.

They are already -> enum intel_platform?!

Regards,

Tvrtko

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

Re: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Jani Nikula]

On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 08/12/2016 13:37, Jani Nikula wrote:
>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 08/12/2016 10:46, Jani Nikula wrote:
>>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Instead of listing them individually we can generate them
>>>>> using the new i915_platforms.h header.
>>>>>
>>>>> Also convert them to a static inline function which
>>>>> interestingly makes the code smaller as well.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>
>>>> NAK. Absolutely opposed to this.
>>>
>>> Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs
>>> any longer?
>>
>> Only when dropped without rationale. I needed to make it clear in no
>> uncertain terms how important this is to me.
>
> Hm ok, I'll give you a benefit of doubt here.

Thanks; I hope you've observed I don't use it lightly.

>>>> A large part of my work involves digging through the source tree, and a
>>>> crucial part of that is looking up definitions and references, both for
>>>> macros and functions. Not having the macro/function definitions breaks
>>>> that workflow. Losing that, source code archeology gets *much*
>>>> harder. The losses are much greater than the gains.
>>>
>>> Hm, I struggle to see that point on the same magnitude of a disaster
>>> scale as you. I would have thought we all know what IS_SKYLAKE & co are
>>> so it would be no big deal.
>>
>> Sure we know what they are; I want to be able to see all the
>> *references* to them as well, using GNU global. That fails if they're
>> not defined in the first place. And no, git grep is not the same.
>>
>>> Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.
>>
>> Then all the things passed as parameter would have to be defined.
>
> They are already -> enum intel_platform?!

See the other mail; they'd have to be defined directly (as they
currently are in git) instead of via macros (as in patch 1).

Hmm, how about

static inline bool intel_is_platform(const struct drm_i915_private *dev_priv,
				     enum intel_platform platform)
{
	return dev_priv->info.platform == platform;
}

and doing

#define IS_FOO(dev_priv) intel_is_platform(dev_priv, INTEL_FOO)

manually for the ones we actually use (we don't need them all)? If the
function is inline, I don't see how defining N similar functions instead
of passing in the parameter would be more efficient. And you could still
do the optimizations of patchs 3/3 AFAICS.

Suitable compromise?


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: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Tvrtko Ursulin]

On 08/12/2016 14:00, Jani Nikula wrote:
> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 08/12/2016 13:37, Jani Nikula wrote:
>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>>> On 08/12/2016 10:46, Jani Nikula wrote:
>>>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> Instead of listing them individually we can generate them
>>>>>> using the new i915_platforms.h header.
>>>>>>
>>>>>> Also convert them to a static inline function which
>>>>>> interestingly makes the code smaller as well.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>>
>>>>> NAK. Absolutely opposed to this.
>>>>
>>>> Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs
>>>> any longer?
>>>
>>> Only when dropped without rationale. I needed to make it clear in no
>>> uncertain terms how important this is to me.
>>
>> Hm ok, I'll give you a benefit of doubt here.
>
> Thanks; I hope you've observed I don't use it lightly.
>
>>>>> A large part of my work involves digging through the source tree, and a
>>>>> crucial part of that is looking up definitions and references, both for
>>>>> macros and functions. Not having the macro/function definitions breaks
>>>>> that workflow. Losing that, source code archeology gets *much*
>>>>> harder. The losses are much greater than the gains.
>>>>
>>>> Hm, I struggle to see that point on the same magnitude of a disaster
>>>> scale as you. I would have thought we all know what IS_SKYLAKE & co are
>>>> so it would be no big deal.
>>>
>>> Sure we know what they are; I want to be able to see all the
>>> *references* to them as well, using GNU global. That fails if they're
>>> not defined in the first place. And no, git grep is not the same.
>>>
>>>> Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.
>>>
>>> Then all the things passed as parameter would have to be defined.
>>
>> They are already -> enum intel_platform?!
>
> See the other mail; they'd have to be defined directly (as they
> currently are in git) instead of via macros (as in patch 1).
>
> Hmm, how about
>
> static inline bool intel_is_platform(const struct drm_i915_private *dev_priv,
> 				     enum intel_platform platform)
> {
> 	return dev_priv->info.platform == platform;
> }
>
> and doing
>
> #define IS_FOO(dev_priv) intel_is_platform(dev_priv, INTEL_FOO)
>
> manually for the ones we actually use (we don't need them all)? If the
> function is inline, I don't see how defining N similar functions instead
> of passing in the parameter would be more efficient. And you could still
> do the optimizations of patchs 3/3 AFAICS.
>
> Suitable compromise?

Yes, in fact, I was already half way through typing that when this 
e-mail arrived.

Regards,

Tvrtko

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

✗ Fi.CI.BAT: warning for Claw back the IS_<platform> optimisation (rev2)

[Patchwork]

== Series Details ==

Series: Claw back the IS_<platform> optimisation (rev2)
URL   : https://patchwork.freedesktop.org/series/16544/
State : warning

== Summary ==

Series 16544v2 Claw back the IS_<platform> optimisation
https://patchwork.freedesktop.org/api/1.0/series/16544/revisions/2/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup basic-reload-final:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-skl-6700hq)

fi-bdw-5557u     total:247  pass:230  dwarn:3   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:205  dwarn:3   dfail:0   fail:0   skip:39 
fi-byt-j1900     total:247  pass:217  dwarn:3   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:213  dwarn:3   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:225  dwarn:3   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:225  dwarn:3   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:192  dwarn:3   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:223  dwarn:3   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:223  dwarn:3   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:223  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:231  dwarn:3   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:231  dwarn:3   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:213  dwarn:3   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:212  dwarn:3   dfail:0   fail:0   skip:32 

24cc1f39920c0caf747c6bda267ca19b99f21786 drm-tip: 2016y-12m-08d-12h-31m-59s UTC integration manifest
ab139b1 drm/i915: Number the platform enum strategically
e2a3c06 drm/i915: Generate all IS_<platform> macros
db53ca9 drm/i915: Introduct i915_platforms.h

== Logs ==

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