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

[Tvrtko Ursulin]

On 14/04/2021 14:13, Jani Nikula wrote:
> 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.

Adding masks later would not a big deal, but another cycle of changing 
"xxx_VER == n" to "IS_xxx_VER(n)" is a churn which could presumably be 
avoided.

It is moot yes, but I don't see a clear case for doing the reversal as 
part of this series. With a disclaimer that I only glanced over the 
commit messages today for the first time.

So I think from me its neither ack or nack, at least since I don't 
understand the attractiveness of using the "ver == n" and numerical 
range check forms everywhere. As said, if we are churning I'd rather go 
the other direction. But that's a soft objection only so feel free to 
proceed.

Regards,

Tvrtko