[PATCH v2 0/9] drm/i915/display: platform identification with display->is.<PLATFORM>

[Jani Nikula]

On Thu, 26 Sep 2024, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote:
>> On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> > On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
>> >>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala at linux.intel.com> 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)
>> 
>> Fair enough.
>> 
>> >From implementation POV having a sub-struct is easier than not.
>
> how the subplatform would appear in this case?

For example, RPL-S:


	if (display->platform.alderlake_s_raptorlake_s)

But the main platform also matches its subplatforms:

	if (display->platform.alderlake_s)

This is the same as with the patches at hand. Except for the
uppercase/lowercase difference, and s/is/platform/.

>> >>  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.
>> 
>> Well, they're maybe the exception for new platforms, but i915 display
>> does have to deal with a lot of legacy with a lot of platform checks.
>> 
>> > grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
>> > because wherever we can we check by graphics/display version rather than
>> > platform.
>> 
>> i915 display has only one use of IS_LUNARLAKE(), but there are 1k+ other
>> uses of IS_<PLATFORM>.
>> 
>> Incidentally, this is the reason I'm procrastinating about the change at
>> all.
>> 
>> > 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.
>> 
>> The downside with that is that you can't deal with subplatforms as
>> easily. It becomes
>> 
>> 	if (display->platform == PLATFORM_LUNARLAKE ||
>> 	    (display->platform == PLATFORM_ALDERLAKE_P &&
>> 	     display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N))
>> 
>> or similar. Definitely not a fan.
>
> unless the subplatform already includes the platform?

Oh, yeah, it would.

> But well, I also don't have a good suggestion here.
> The '.is' struct is strange indeed, but at least covers all the past
> and future strange cases.
>
> But I also wouldn't mind if we decide to get the verbose path,
> but try to at least making the subplatform already infering the
> platform in a way that this case could only be:
>
>        if (display->platform == PLATFORM_LUNARLAKE ||
>             display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)
>
>
> or perhaps do in a way that we don't even need the subplatform struct?
>
>        if (display->platform == PLATFORM_LUNARLAKE ||
>             display->platform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)

How would that even be possible? display->platform can't be multiple
things at the same time, unless it's a bitmask. If it's a bitmask, you
need a way to nicely check for it instead of having it everywhere.

The alternatives are using a macro, or using bitfields - which is
exactly what the patch at hand does. We've come full circle.


BR,
Jani.


>
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > 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
>> 
>> -- 
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel