On Fri, Sep 27, 2024 at 03:16:23PM +0300, Jani Nikula wrote: > On Thu, 26 Sep 2024, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote: > > On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote: > >> On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote: > >> > On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote: > >> >>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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
