[Intel-gfx] [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 at 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