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