+Tvrtko
+Joonas
> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Monday, June 5, 2023 11:29 AM
> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/5] drm/i915/adlp: s/ADLP/ALDERLAKE_P for
> display and graphics step
>
> On Mon, 05 Jun 2023, "Srivatsa, Anusha" <anusha.srivatsa@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> >> Sent: Monday, June 5, 2023 8:14 AM
> >> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
> >> gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH 1/5] drm/i915/adlp:
> >> s/ADLP/ALDERLAKE_P for display and graphics step
> >>
> >> On Tue, 30 May 2023, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> 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.
That works too.
Anusha
> BR,
> Jani.
>
>
> >
> > Anusha
> >> BR,
> >> Jani.
> >>
> >>
> >>
> >> >
> >> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> >> > ---
> >> > 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
