| Message ID | 20230615095421.3135415-1-dnyaneshwar.bhadane@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Replace acronym with full platform name in defines. | expand |
Replace all occurences of MTL with METEORLAKE, ADL with ALDERLAKE, TGL with TIGERLAKE, 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. v2: - Reordered patches by incrementing platform generations. Anusha Srivatsa (5): drm/i915/adlp: s/ADLP/ALDERLAKE_P for display and graphics step drm/i915/rplp: s/ADLP/ALDERLAKE_P for RPLP defines drm/i915/rplu: s/ADLP/ALDERLAKE_P in RPLU defines drm/i915/adln: s/ADLP/ALDERLAKE_P in ADLN defines drm/i915/adls: s/ADLS/ALDERLAKE_S in platform and subplatform defines Dnyaneshwar Bhadane (6): drm/i915/skl: s/SKL/SKYLAKE for platform/subplatform defines drm/i915/kbl: s/KBL/KABYLAKE for platform/subplatform defines drm/i915/tgl: s/RKL/ROCKETLAKE for platform/subplatform defines drm/i915/jsl: s/JSL/JASPERLAKE for platform/subplatform defines drm/i915/tgl: s/TGL/TIGERLAKE for platform/subplatform defines drm/i915/mtl: s/MTL/METEORLAKE for platform/subplatform defines drivers/gpu/drm/i915/display/icl_dsi.c | 4 +- drivers/gpu/drm/i915/display/intel_cdclk.c | 8 +-- .../gpu/drm/i915/display/intel_combo_phy.c | 6 +- drivers/gpu/drm/i915/display/intel_ddi.c | 6 +- .../drm/i915/display/intel_ddi_buf_trans.c | 10 +-- drivers/gpu/drm/i915/display/intel_display.c | 6 +- .../drm/i915/display/intel_display_device.c | 2 +- .../drm/i915/display/intel_display_power.c | 2 +- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 20 +++--- drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_pmdemand.c | 2 +- drivers/gpu/drm/i915/display/intel_psr.c | 20 +++--- .../drm/i915/display/skl_universal_plane.c | 10 +-- drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 +- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 10 +-- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 4 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +- drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- drivers/gpu/drm/i915/gt/intel_sseu.c | 2 +- drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 ++++++++-------- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +- .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 2 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 +- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 64 +++++++++---------- drivers/gpu/drm/i915/i915_perf.c | 4 +- drivers/gpu/drm/i915/intel_clock_gating.c | 4 +- drivers/gpu/drm/i915/intel_step.c | 10 +-- drivers/gpu/drm/i915/soc/intel_pch.c | 6 +- 34 files changed, 143 insertions(+), 143 deletions(-)
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. BR, Jani. > > v2: > - Fix the checkpatch warning. > > Anusha Srivatsa (5): > drm/i915/adlp: s/ADLP/ALDERLAKE_P for display and graphics step > drm/i915/rplp: s/ADLP/ALDERLAKE_P for RPLP defines > drm/i915/adln: s/ADLP/ALDERLAKE_P in ADLN defines > drm/i915/adls: s/ADLS/ALDERLAKE_S in platform and subplatform defines > drm/i915/rplu: s/ADLP/ALDERLAKE_P in RPLU defines > > Dnyaneshwar Bhadane (6): > drm/i915/TGL: s/TGL/TIGERLAKE for platform/subplatform defines > drm/i915/MTL: s/MTL/METEORLAKE for platform/subplatform defines > drm/i915/TGL: s/RKL/ROCKETLAKE for platform/subplatform defines > drm/i915/JSL: s/JSL/JASPERLAKE for platform/subplatform defines > drm/i915/KBL: s/KBL/KABYLAKE for platform/subplatform defines > drm/i915/SKL: s/SKL/SKYLAKE for platform/subplatform defines > > drivers/gpu/drm/i915/display/icl_dsi.c | 4 +- > drivers/gpu/drm/i915/display/intel_cdclk.c | 8 +-- > .../gpu/drm/i915/display/intel_combo_phy.c | 6 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +- > .../drm/i915/display/intel_ddi_buf_trans.c | 10 +-- > drivers/gpu/drm/i915/display/intel_display.c | 6 +- > .../drm/i915/display/intel_display_device.c | 2 +- > .../drm/i915/display/intel_display_power.c | 2 +- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 20 +++--- > drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > drivers/gpu/drm/i915/display/intel_pmdemand.c | 2 +- > drivers/gpu/drm/i915/display/intel_psr.c | 20 +++--- > .../drm/i915/display/skl_universal_plane.c | 10 +-- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 +- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 10 +-- > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- > .../drm/i915/gt/intel_execlists_submission.c | 2 +- > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 4 +- > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +- > drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- > drivers/gpu/drm/i915/gt/intel_sseu.c | 2 +- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 ++++++++-------- > drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +- > .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 2 +- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 +- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 64 +++++++++---------- > drivers/gpu/drm/i915/i915_perf.c | 4 +- > drivers/gpu/drm/i915/intel_clock_gating.c | 4 +- > drivers/gpu/drm/i915/intel_step.c | 10 +-- > drivers/gpu/drm/i915/soc/intel_pch.c | 6 +- > 34 files changed, 143 insertions(+), 143 deletions(-)
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
On Wed, 21 Jun 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > Historically we were not avoiding churn if we could sense a real > gain. I tried but failed to say, if we're going to churn, let's at least go through this properly instead of churning, and then churning again, and again. BR, Jani.
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Tuesday, June 20, 2023 9:31 AM > To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Srivatsa, > Anusha <anusha.srivatsa@intel.com>; Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com>; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com> > Subject: Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name > in defines. > > 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. Yes there was. The main reason being the acronym was more brief/compact. Having said that, thinking out loud it felt having platform defines have full name would make it more clear and readable? For anyone new wanting to contribute, there will be less confusion. > > 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. Agreed. Anusha > > BR, > Jani. > > > > > > > > v2: > > - Fix the checkpatch warning. > > > > Anusha Srivatsa (5): > > drm/i915/adlp: s/ADLP/ALDERLAKE_P for display and graphics step > > drm/i915/rplp: s/ADLP/ALDERLAKE_P for RPLP defines > > drm/i915/adln: s/ADLP/ALDERLAKE_P in ADLN defines > > drm/i915/adls: s/ADLS/ALDERLAKE_S in platform and subplatform defines > > drm/i915/rplu: s/ADLP/ALDERLAKE_P in RPLU defines > > > > Dnyaneshwar Bhadane (6): > > drm/i915/TGL: s/TGL/TIGERLAKE for platform/subplatform defines > > drm/i915/MTL: s/MTL/METEORLAKE for platform/subplatform defines > > drm/i915/TGL: s/RKL/ROCKETLAKE for platform/subplatform defines > > drm/i915/JSL: s/JSL/JASPERLAKE for platform/subplatform defines > > drm/i915/KBL: s/KBL/KABYLAKE for platform/subplatform defines > > drm/i915/SKL: s/SKL/SKYLAKE for platform/subplatform defines > > > > drivers/gpu/drm/i915/display/icl_dsi.c | 4 +- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 8 +-- > > .../gpu/drm/i915/display/intel_combo_phy.c | 6 +- > > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +- > > .../drm/i915/display/intel_ddi_buf_trans.c | 10 +-- > > drivers/gpu/drm/i915/display/intel_display.c | 6 +- > > .../drm/i915/display/intel_display_device.c | 2 +- > > .../drm/i915/display/intel_display_power.c | 2 +- > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 20 +++--- > > drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > > drivers/gpu/drm/i915/display/intel_pmdemand.c | 2 +- > > drivers/gpu/drm/i915/display/intel_psr.c | 20 +++--- > > .../drm/i915/display/skl_universal_plane.c | 10 +-- > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 +- > > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 10 +-- > > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- > > .../drm/i915/gt/intel_execlists_submission.c | 2 +- > > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 4 +- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +- > > drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- > > drivers/gpu/drm/i915/gt/intel_sseu.c | 2 +- > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 ++++++++-------- > > drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 +- > > .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 2 +- > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 +- > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- > > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- > > drivers/gpu/drm/i915/i915_drv.h | 64 +++++++++---------- > > drivers/gpu/drm/i915/i915_perf.c | 4 +- > > drivers/gpu/drm/i915/intel_clock_gating.c | 4 +- > > drivers/gpu/drm/i915/intel_step.c | 10 +-- > > drivers/gpu/drm/i915/soc/intel_pch.c | 6 +- > > 34 files changed, 143 insertions(+), 143 deletions(-) > > -- > Jani Nikula, Intel Open Source Graphics Center