Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name in defines.
> -Original Message- > From: Jani Nikula > Sent: Tuesday, June 20, 2023 9:31 AM > To: Bhadane, Dnyaneshwar ; intel- > g...@lists.freedesktop.org > Cc: Bhadane, Dnyaneshwar ; Srivatsa, > Anusha ; Tvrtko Ursulin > ; Joonas Lahtinen > > Subject: Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name > in defines. > > On Thu, 15 Jun 2023, Dnyaneshwar Bhadane > 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__DISPLAY_STEP() are > two-fold: the steppings used to be platform specific, so it made sense to tie > platform and stepping together, and IS__DISPLAY_STEP() was > considered too long combined. > > Nowadays, we've abstracted steppings in code to be independent of platforms, > so we could use IS_() && IS_DISPLAY_STEP(), and throw out all > the IS__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_() && > 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 -
Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name in defines.
On Wed, 21 Jun 2023, Tvrtko Ursulin 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. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name in defines.
On 20/06/2023 17:30, Jani Nikula wrote: On Thu, 15 Jun 2023, Dnyaneshwar Bhadane 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__DISPLAY_STEP() are two-fold: the steppings used to be platform specific, so it made sense to tie platform and stepping together, and IS__DISPLAY_STEP() was considered too long combined. Nowadays, we've abstracted steppings in code to be independent of platforms, so we could use IS_() && IS_DISPLAY_STEP(), and throw out all the IS__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_() && 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
Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name in defines.
On Thu, 15 Jun 2023, Dnyaneshwar Bhadane 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__DISPLAY_STEP() are two-fold: the steppings used to be platform specific, so it made sense to tie platform and stepping together, and IS__DISPLAY_STEP() was considered too long combined. Nowadays, we've abstracted steppings in code to be independent of platforms, so we could use IS_() && IS_DISPLAY_STEP(), and throw out all the IS__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_() && 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(-) -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 00/11] Replace acronym with full platform name in defines.
> -Original Message- > From: Bhadane, Dnyaneshwar > Sent: Wednesday, June 14, 2023 10:00 PM > To: intel-gfx@lists.freedesktop.org > Cc: Atwood, Matthew S ; Srivatsa, Anusha > ; Bhadane, Dnyaneshwar > > Subject: [PATCH 00/11] Replace acronym with full platform name in defines. > > 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. Thanks for taking this forward. I have reviewed the patches with the only feedback being use of lower case in the subject prefix. But the series also needs platform reordering. It should start from oldest to newest platform. SO Patch 1:SKL Patch 2:KBL Patch 3:RKL And so on. Order being: SLK->KBL->RKL->JSL->TGL->ADLPS and its various flavours->ADLS->MTL Anusha > > 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(-) > > -- > 2.34.1