On Thu, Dec 08, 2016 at 09:49:59AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
