Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name in defines.

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 20/06/2023 17:30, Jani Nikula wrote:
> On Thu, 15 Jun 2023, Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com> wrote:
>> Replace all occurences of ADL with ALDERLAKE, TGL with TIGERLAKE,
>> MTL with METEORLAKE, RKL with ROCKETLAKE, JSL with JASPERLAKE,
>> KBL with KABYLAKE and SKL with SKYLAKE in platform and subplatform
>> defines. This way there is a consistent pattern to how platforms
>> are referred. While the change is minor and could be combined to
>> have lesser patches, splitting to per subpaltform for easier
>> cherrypicks, if needed.
> 
> First of all, I'll note that changes like these need maintainer acks
> before merging. Simple review for correctness is not enough!
> 
> While discussing this, there was perhaps a slight preference for moving
> towards acronyms for brevity instead of expanding all of them to full
> names. It can get a bit unwieldy.
> 
> For background, the reasons for having IS_<TLA>_DISPLAY_STEP() are
> two-fold: the steppings used to be platform specific, so it made sense
> to tie platform and stepping together, and IS_<LONG_NAME>_DISPLAY_STEP()
> was considered too long combined.
> 
> Nowadays, we've abstracted steppings in code to be independent of
> platforms, so we could use IS_<LONG_NAME>() && IS_DISPLAY_STEP(), and
> throw out all the IS_<TLA>_DISPLAY_STEP() macros. They're orthogonal
> things, and it actually bugs me to have so many platform specific
> wrappers for the combos.
> 
> If in addition we moved to acronyms, we could have IS_<TLA>() &&
> IS_DISPLAY_STEP(), and it would be pretty short and nice.
> 
> That said, all of these changes are a lot of churn, so I'd rather not
> make them lightly.

I don't have a strong opinion on whether to churn or not. Or for the TLA vs LONG_NAME. As long as we do not mix the two. I think this translates to "If you do something, make it consistent, make it readable and make it tidy.". :)

Historically we were not avoiding churn if we could sense a real gain. In this case we could drop some combo macros, as Jani points out, gain some consistency, what else?

As minimum compact the numerous pointlessly expanded(*) MTL checks into a single platform check. But that is even orthogonal to the renames. Like:

	if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_B0, STEP_FOREVER) ||
	    IS_MTL_GRAPHICS_STEP(i915, P, STEP_B0, STEP_FOREVER))

Into possibly:

	if (IS_MTL(i915) && IS_GRAPHICS_STEP(i915, STEP_B0, STEP_FOREVER))

But that could also be:

	if (IS_MTL_GRAPHICS_STEP(i915, STEP_B0, STEP_FOREVER))

Regards,

Tvrtko