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

[Lucas De Marchi]

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)

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