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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote:
> 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.

how the subplatform would appear in this case?

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

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)

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