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