[Intel-gfx] [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 at 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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at 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