> -----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.
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
