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

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

On Fri, Sep 27, 2024 at 03:16:23PM +0300, Jani Nikula wrote:
> On Thu, 26 Sep 2024, Rodrigo Vivi <rodrigo.vivi@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@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?
> 
> 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/.

Yeap, let's go with that then!

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

yeap, forget about this...

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