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

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

On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@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@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.

>>
>>  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.


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