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

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

On Wed, 14 Apr 2021, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 14/04/2021 14:13, Jani Nikula wrote:
>> On Wed, 14 Apr 2021, Tvrtko Ursulin <tvrtko.ursulin@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx