On Wed, 14 Apr 2021, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> On 14/04/2021 14:13, Jani Nikula wrote:
>> On Wed, 14 Apr 2021, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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.
Direct xxx_VER <, >, <= and >= already exist all over the place, and
their numbers trump the == cases. Seems confusing to treat ==
differently.
> 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 wanted to keep using the range check macros for a couple of
reasons. Having (VER >= x && VER <= y) gets long, it needs braces, and
we use a bunch of negation !(VER >= x && VER <= y) vs. VER < x || VER >
y. !IS_GEN_RANGE() has more clarity.
Now, adding IS_GRAPHICS_VER_RANGE() gets long. Dropping the VER for
IS_GRAPHICS_RANGE() gets confusing ("what graphics range?"). Now, if we
use == for specific version check, we can repurpose IS_GRAPHICS_VER() to
do the ranges.
> 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.
Thanks, noted. However, unless stronger objections arise, I think we're
going to go with the patches at hand.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
