On Mon, 05 Jun 2023, "Srivatsa, Anusha" <[email protected]> wrote: >> -----Original Message----- >> From: Jani Nikula <[email protected]> >> Sent: Monday, June 5, 2023 8:14 AM >> To: Srivatsa, Anusha <[email protected]>; intel- >> [email protected] >> Subject: Re: [Intel-gfx] [PATCH 1/5] drm/i915/adlp: s/ADLP/ALDERLAKE_P for >> display and graphics step >> >> On Tue, 30 May 2023, Anusha Srivatsa <[email protected]> wrote: >> > Driver refers to the platfrom Alderlake P as ADLP in places and >> > ALDERLAKE_P in some. Making the consistent change to avoid confusion >> > of the right naming convention for the platform. >> >> $ git grep "#define IS_.*_DISPLAY_STEP" -- drivers/gpu/drm/i915/i915_drv.h >> drivers/gpu/drm/i915/i915_drv.h:#define IS_KBL_DISPLAY_STEP(i915, since, >> until) \ drivers/gpu/drm/i915/i915_drv.h:#define IS_JSL_EHL_DISPLAY_STEP(p, >> since, until) \ drivers/gpu/drm/i915/i915_drv.h:#define >> IS_TGL_DISPLAY_STEP(__i915, since, until) \ >> drivers/gpu/drm/i915/i915_drv.h:#define IS_RKL_DISPLAY_STEP(p, since, until) >> \ >> drivers/gpu/drm/i915/i915_drv.h:#define IS_ADLS_DISPLAY_STEP(__i915, since, >> until) \ drivers/gpu/drm/i915/i915_drv.h:#define >> IS_ADLP_DISPLAY_STEP(__i915, since, until) \ >> drivers/gpu/drm/i915/i915_drv.h:#define IS_MTL_DISPLAY_STEP(__i915, since, >> until) \ drivers/gpu/drm/i915/i915_drv.h:#define IS_DG2_DISPLAY_STEP(__i915, >> since, until) \ >> >> They all use the acronym. Do you suggest to rename all of them, or just >> ADL-P? > > Got the idea after looking at subplatform defines in i915_drv.h: > > #define IS_METEORLAKE_M(i915) \ > IS_SUBPLATFORM(i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_M) > #define IS_METEORLAKE_P(i915) \ > IS_SUBPLATFORM(i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_P) > #define IS_DG2_G10(i915) \ > IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G10) > #define IS_DG2_G11(i915) \ > IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G11) > #define IS_DG2_G12(i915) \ > IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G12) > #define IS_ADLS_RPLS(i915) \ > IS_SUBPLATFORM(i915, INTEL_ALDERLAKE_S, INTEL_SUBPLATFORM_RPL) > #define IS_ADLP_N(i915) \ > IS_SUBPLATFORM(i915, INTEL_ALDERLAKE_P, INTEL_SUBPLATFORM_N) > #define IS_ADLP_RPLP(i915) \ > IS_SUBPLATFORM(i915, INTEL_ALDERLAKE_P, INTEL_SUBPLATFORM_RPL) > #define IS_ADLP_RPLU(i915) \ > IS_SUBPLATFORM(i915, INTEL_ALDERLAKE_P, INTEL_SUBPLATFORM_RPLU) > > We are using the same platform name for platform and sub-platform defines for > Meteor Lake and DG2, but somehow for flavors of Alder Lake, the sub-platform > has acronym. The idea was that developers should not think if the full name > or acronym has to be used. And that resulted in the series. But now that you > have pointed out the above other such occurrences, I am leaning towards > having them changed as well. That is for a platform defined as TIGERLAKE, All > of its steppings etc should have > TIGERLAKE_(TIGERLAKE_MEDIA_,TIGERLAKE_DISPLAY_, TIGERLAKE_GRAPHICS_ ) etc > instead of having TIGERLAKE in some places and TGL in its stepping or > sub-platform defines. > > This was the naming is uniform and consistent.
One could also make the case for switching all of them use the acronym instead for brevity. BR, Jani. > > Anusha >> BR, >> Jani. >> >> >> >> > >> > Signed-off-by: Anusha Srivatsa <[email protected]> >> > --- >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 2 +- >> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 +- >> > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++---- >> > drivers/gpu/drm/i915/display/skl_universal_plane.c | 4 ++-- >> > drivers/gpu/drm/i915/i915_drv.h | 4 ++-- >> > 5 files changed, 10 insertions(+), 10 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c >> > b/drivers/gpu/drm/i915/display/intel_cdclk.c >> > index 6bed75f1541a..013678caaca8 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> > @@ -3541,7 +3541,7 @@ void intel_init_cdclk_hooks(struct drm_i915_private >> *dev_priv) >> > } else if (IS_ALDERLAKE_P(dev_priv)) { >> > dev_priv->display.funcs.cdclk = &tgl_cdclk_funcs; >> > /* Wa_22011320316:adl-p[a0] */ >> > - if (IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) >> > + if (IS_ALDERLAKE_P_DISPLAY_STEP(dev_priv, STEP_A0, >> STEP_B0)) >> > dev_priv->display.cdclk.table = >> adlp_a_step_cdclk_table; >> > else if (IS_ADLP_RPLU(dev_priv)) >> > dev_priv->display.cdclk.table = rplu_cdclk_table; diff >> > -- >> git >> > a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > index 6b2d8a1e2aa9..81f3ce5a0a1e 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > @@ -3781,7 +3781,7 @@ static void adlp_cmtg_clock_gating_wa(struct >> > drm_i915_private *i915, struct inte { >> > u32 val; >> > >> > - if (!IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0) || >> > + if (!IS_ALDERLAKE_P_DISPLAY_STEP(i915, STEP_A0, STEP_B0) || >> > pll->info->id != DPLL_ID_ICL_DPLL0) >> > return; >> > /* >> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c >> > b/drivers/gpu/drm/i915/display/intel_psr.c >> > index ea0389c5f656..c25457dae315 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_psr.c >> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c >> > @@ -639,7 +639,7 @@ static void hsw_activate_psr2(struct intel_dp >> *intel_dp) >> > } >> > >> > /* Wa_22012278275:adl-p */ >> > - if (IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_E0)) { >> > + if (IS_ALDERLAKE_P_DISPLAY_STEP(dev_priv, STEP_A0, STEP_E0)) { >> > static const u8 map[] = { >> > 2, /* 5 lines */ >> > 1, /* 6 lines */ >> > @@ -807,7 +807,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp >> *intel_dp, >> > return; >> > >> > /* Wa_16011303918:adl-p */ >> > - if (IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) >> > + if (IS_ALDERLAKE_P_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) >> > return; >> > >> > /* >> > @@ -975,7 +975,7 @@ static bool intel_psr2_config_valid(struct intel_dp >> *intel_dp, >> > return false; >> > } >> > >> > - if (IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) { >> > + if (IS_ALDERLAKE_P_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) { >> > drm_dbg_kms(&dev_priv->drm, "PSR2 not completely >> functional in this stepping\n"); >> > return false; >> > } >> > @@ -1033,7 +1033,7 @@ static bool intel_psr2_config_valid(struct >> > intel_dp *intel_dp, >> > >> > /* Wa_16011303918:adl-p */ >> > if (crtc_state->vrr.enable && >> > - IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) { >> > + IS_ALDERLAKE_P_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) { >> > drm_dbg_kms(&dev_priv->drm, >> > "PSR2 not enabled, not compatible with HW stepping >> + VRR\n"); >> > return false; >> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c >> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> > index 36070d86550f..2019e0a87bd3 100644 >> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c >> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> > @@ -2174,7 +2174,7 @@ static bool skl_plane_has_rc_ccs(struct >> drm_i915_private *i915, >> > return false; >> > >> > /* Wa_22011186057 */ >> > - if (IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0)) >> > + if (IS_ALDERLAKE_P_DISPLAY_STEP(i915, STEP_A0, STEP_B0)) >> > return false; >> > >> > if (DISPLAY_VER(i915) >= 11) >> > @@ -2200,7 +2200,7 @@ static bool gen12_plane_has_mc_ccs(struct >> drm_i915_private *i915, >> > return false; >> > >> > /* Wa_22011186057 */ >> > - if (IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0)) >> > + if (IS_ALDERLAKE_P_DISPLAY_STEP(i915, STEP_A0, STEP_B0)) >> > return false; >> > >> > /* Wa_14013215631 */ >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> > b/drivers/gpu/drm/i915/i915_drv.h index f1205ed3ba71..1a50b8b2f00d >> > 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > @@ -669,11 +669,11 @@ IS_SUBPLATFORM(const struct drm_i915_private >> *i915, >> > (IS_ALDERLAKE_S(__i915) && \ >> > IS_GRAPHICS_STEP(__i915, since, until)) >> > >> > -#define IS_ADLP_DISPLAY_STEP(__i915, since, until) \ >> > +#define IS_ALDERLAKE_P_DISPLAY_STEP(__i915, since, until) \ >> > (IS_ALDERLAKE_P(__i915) && \ >> > IS_DISPLAY_STEP(__i915, since, until)) >> > >> > -#define IS_ADLP_GRAPHICS_STEP(__i915, since, until) \ >> > +#define IS_ALDERLAKE_P_GRAPHICS_STEP(__i915, since, until) \ >> > (IS_ALDERLAKE_P(__i915) && \ >> > IS_GRAPHICS_STEP(__i915, since, until)) >> >> -- >> Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center
