Re: [RFC PATCH] drm/i915: replace platform flags with a platform enum

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

On Mon, 21 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 21/11/2016 09:27, Jani Nikula wrote:
>> On Fri, 18 Nov 2016, Tvrtko Ursulin <tvrtko.ursulin@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.

It doesn't persuade me much if something enables sweet optimizations
that nobody can perceive in real life.

Please also note that this brings decent codename debug prints to dmesg,
for all platforms, instead of a bunch of irrelevant is_platform = no for
every platform.

>> 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.

current nightly:
   text	   data	    bss	    dec	    hex	filename
1254149	  43528	   2048	1299725	 13d50d	drivers/gpu/drm/i915/i915.ko

original platform enum patch:
   text	   data	    bss	    dec	    hex	filename
1254456	  43529	   2048	1300033	 13d641	drivers/gpu/drm/i915/i915.ko

the same with BIT() for platform enums:
   text	   data	    bss	    dec	    hex	filename
34811672  43529	   2048	34857249	213e121	drivers/gpu/drm/i915/i915.ko

Holy cow, what happened there?!

Anyway the nightly -> original patch change is just a 300 byte increase
in text size.

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