[Intel-gfx] [PATCH v2 08/12] drm/i915: finish removal of gen_mask

[Jani Nikula]

On Wed, 14 Apr 2021, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
> On 13/04/2021 06:09, Lucas De Marchi wrote:
>> Now that it's not used anywhere, remove it from struct
>> intel_device_info. To allow a period in which code will be converted to
>> the new macro, keep IS_GEN_RANGE() around, just redefining it to use
>> the new fields. The size advantage from IS_GEN_RANGE() using a mask is
>> not that big as it has pretty limited use througout the driver:
>> 
>>     text    data     bss     dec     hex filename
>> 2758497   95965    6496 2860958  2ba79e drivers/gpu/drm/i915/i915.ko.old
>> 2758586   95953    6496 2861035  2ba7eb drivers/gpu/drm/i915/i915.ko.new
>
> This delta refers to this patch - I mean this point in the series? 
> Asking because it may not be 100% representative since some of the 
> previous patches have already removed some gen mask usages.
>
> While I am here, I am a bit fond of the mask approach and wonder if 
> using it for all (gt/media/whatelse) new fields would still make sense.
>
> Presence of the range check helpers suggests that it might, but I 
> haven't looked at how prevalent their usage ends up after the series is 
> done. So just in principle, I don't see why not still go with masks 
> since that guarantees elegant check at each range check site. It would 
> be all hidden in the macro implementation so easy.
>
> Also for historical reference, another reason why I went for masks 
> everywhere approach is that at some point we had a feature request to 
> allow compiling out platforms/gens. I *think* that was much easier to do 
> with masking and in experiments back then I was able for instance to 
> build just for Gen9+ and drop like 30% of the binary size.
>
> Oh I found the branch now.. The reason for IS_GEN(p, v) was also in that 
> series. I don't know if I ever RFC-ed or trybotted it.. google suggests 
> no and I neither can find it in my mailboxes. I could send out the old 
> patches for reference? But to be honest I have no idea if this feature 
> request (targeted driver builds) will ever resurface..

I completely agreed with the direction of using the masks way back when,
especially with the goal of the conditional/targeted compilation.

I think the question now is whether we want to keep maintaining them
just for the sake of the masks. Keeping them means having three masks
instead of one. And we wouldn't be using most of the benefits with them,
we'd mostly just get the downsides.

Having the masks per se is not such a big deal, but they're also not
such a big deal to add back later on if needed. It's the codebase all
over that's the hard part. And arguably it's not getting that much
different with the series at hand; the direct uses of INTEL_GEN() and
DISPLAY_VER() vastly outnumber IS_GEN(), IS_GEN_RANGE() and
IS_DISPLAY_RANGE() which could benefit from the mask.

We'd still be retaining the range macros as IS_GRAPHICS_VER(),
IS_MEDIA_VER() and IS_DISPLAY_VER(), although more for clarity than for
any other reason.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center