On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
> v2 of [1]. Please read the cover letter there.
>
> This addresses review comments and adds a few more commits on top, in particular
> the last one showcasing the approach.
>
> The main question remains, is this what we want?
I don't know why, but the 'is' thing is still strange.
I know I know... I'm bad with naming myself.
I think about 'platform' but that get too big
if (display->platform.BROADWELL)
I think about 'gen' but then it is overloaded....
then I think about 'ip' is worse...
'version'?
'name'?
if (display->name.HASWELL)...
....
But well, I like the overall simplification here in general.
Without a better name to suggest, I guess let's just move ahead...
One slight concern with the is.foo is whether it complicates finding
things with eg. cscope. But I suppose for platforms that doesn't matter
all that much. For the has_foo stuff it'd be much more relevant.
It does make finding things harder with cscope and gnu global, but git
grep for is.FOO is pretty accurate.
Anyways, can't think of anything particularly elegant myself either,
so go ahead I guess.
So I haven't yet. I just still have that slightly uneasy feeling about
whether this is a good thing or not. That doesn't usually make me shy
away from things, because you can fix stuff later, but getting this
wrong causes so much churn everywhere.
The fact that it's not a macro makes it less flexible for future
changes. The display->is.FOO is somewhat legible, but could be
better. Would all lowercase make it better? I don't know.
More alternatives? Not elegant for sure, but just alternatives:
- Lowercase names:
if (display->is.rocketlake)
what I really dislike is a struct named "is". Going full mesa-way would
be slightly better IMO:
if (display->is_rockelake)
or
if (display->platform_rocketlake)
or
if (display->platform.rocketlake)
Does not help with flexibility or cscope.
- Lowercase macros for display, e.g. is_rocketlake().
if (is_rocketlake(display))
- Macros based on just the platform name, e.g. ROCKETLAKE().
if (ROCKETLAKE(display))
or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
if (PLATFORM_ROCKETLAKE(display))
But that can get a bit long in some if ladders etc.
Does it matter much? I think those would be the exception, particularly
because platform checks are kind of rare these days.
grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
because wherever we can we check by graphics/display version rather than
platform.
Then simply using something similar to what we already have in xe, would
be great IMO:
if (display->platform == PLATFORM_LUNARLAKE)
it may be verbose, but shouldn't be much used to matter in the end.
Lucas De Marchi
- Go through the trouble of making the existing IS_FOO() macros _Generic
and accept either i915 or display pointer. This does postpone making
any further changes, but fairly soon there will need to be two sets of
macros, separate for i915 and display, even though named the same.
Also, the _Generic thing would look up the platform definitions from
different places, which could be error prone.
Yeah, procrastination...
BR,
Jani.
--
Jani Nikula, Intel
