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

[Jani Nikula <jani.nikula@intel.com>]

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