Hello Matt,
> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Tuesday, January 10, 2023 6:32 AM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; jani.nikula@xxxxxxxxxxxxxxx; Shankar,
> Uma <uma.shankar@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>;
> Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; Atwood, Matthew S
> <matthew.s.atwood@xxxxxxxxx>
> Subject: Re: [RFC 1/2] drm/i915: Add rplu sub platform
>
> On Sat, Jan 07, 2023 at 11:06:42AM +0530, Chaitanya Kumar Borah wrote:
> > Adding RPL-U as a sub platform. In RPL-U a new CDCLK step has been
> > added so we need to make a distinction between RPL-P and RPL-U while
> > CDCLK initialization.
> >
> > Adding a sub-platform, enables us to make this differentiation in the
> > code.
> >
> > Signed-off-by: Chaitanya Kumar Borah
> <chaitanya.kumar.borah@xxxxxxxxx>
> > ---
> > arch/x86/kernel/early-quirks.c | 1 +
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/i915_pci.c | 1 +
> > drivers/gpu/drm/i915/intel_device_info.c | 7 +++++++
> > drivers/gpu/drm/i915/intel_device_info.h | 1 +
> > drivers/gpu/drm/i915/intel_step.c | 3 +++
> > include/drm/i915_pciids.h | 7 +++++--
> > 7 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/early-quirks.c
> > b/arch/x86/kernel/early-quirks.c index a6c1867fc7aa..1ba9926c8974
> > 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -559,6 +559,7 @@ static const struct pci_device_id intel_early_ids[]
> __initconst = {
> > INTEL_ADLN_IDS(&gen11_early_ops),
> > INTEL_RPLS_IDS(&gen11_early_ops),
> > INTEL_RPLP_IDS(&gen11_early_ops),
> > + INTEL_RPLU_IDS(&gen11_early_ops),
> > };
> >
> > struct resource intel_graphics_stolen_res __ro_after_init =
> > DEFINE_RES_MEM(0, 0); diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 48fd82722f12..c88e514728a0
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -619,6 +619,8 @@ IS_SUBPLATFORM(const struct drm_i915_private
> *i915,
> > IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_P,
> INTEL_SUBPLATFORM_N)
> > #define IS_ADLP_RPLP(dev_priv) \
> > IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_P,
> INTEL_SUBPLATFORM_RPL)
> > +#define IS_ADLP_RPLU(dev_priv) \
> > + IS_SUBPLATFORM(dev_priv, INTEL_ALDERLAKE_P,
> INTEL_SUBPLATFORM_RPLU)
> > #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \
> > (INTEL_DEVID(dev_priv) & 0xFF00) ==
> 0x0C00) #define
> > IS_BDW_ULT(dev_priv) \ diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c index 6cc65079b18d..e9f3b99b3e00
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -1234,6 +1234,7 @@ static const struct pci_device_id pciidlist[] = {
> > INTEL_DG1_IDS(&dg1_info),
> > INTEL_RPLS_IDS(&adl_s_info),
> > INTEL_RPLP_IDS(&adl_p_info),
> > + INTEL_RPLU_IDS(&adl_p_info),
> > INTEL_DG2_IDS(&dg2_info),
> > INTEL_ATS_M_IDS(&ats_m_info),
> > INTEL_MTL_IDS(&mtl_info),
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > b/drivers/gpu/drm/i915/intel_device_info.c
> > index 849baf6c3b3c..88f3da63948b 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -201,6 +201,10 @@ static const u16 subplatform_rpl_ids[] = {
> > INTEL_RPLP_IDS(0),
> > };
> >
> > +static const u16 subplatform_rplu_ids[] = {
> > + INTEL_RPLU_IDS(0),
> > +};
> > +
> > static const u16 subplatform_g10_ids[] = {
> > INTEL_DG2_G10_IDS(0),
> > INTEL_ATS_M150_IDS(0),
> > @@ -268,6 +272,9 @@ static void
> intel_device_info_subplatform_init(struct drm_i915_private *i915)
> > } else if (find_devid(devid, subplatform_rpl_ids,
> > ARRAY_SIZE(subplatform_rpl_ids))) {
> > mask = BIT(INTEL_SUBPLATFORM_RPL);
> > + } else if (find_devid(devid, subplatform_rplu_ids,
> > + ARRAY_SIZE(subplatform_rplu_ids))) {
> > + mask = BIT(INTEL_SUBPLATFORM_RPLU);
> > } else if (find_devid(devid, subplatform_g10_ids,
> > ARRAY_SIZE(subplatform_g10_ids))) {
> > mask = BIT(INTEL_SUBPLATFORM_G10);
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index d588e5fd2eea..3e3ca5eb073f 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -127,6 +127,7 @@ enum intel_platform {
> > * bit set
> > */
> > #define INTEL_SUBPLATFORM_N 1
> > +#define INTEL_SUBPLATFORM_RPLU 2
> >
> > /* MTL */
> > #define INTEL_SUBPLATFORM_M 0
> > diff --git a/drivers/gpu/drm/i915/intel_step.c
> > b/drivers/gpu/drm/i915/intel_step.c
> > index 84a6fe736a3b..df75057eaa65 100644
> > --- a/drivers/gpu/drm/i915/intel_step.c
> > +++ b/drivers/gpu/drm/i915/intel_step.c
> > @@ -194,6 +194,9 @@ void intel_step_init(struct drm_i915_private *i915)
> > } else if (IS_ADLP_RPLP(i915)) {
> > revids = adlp_rplp_revids;
> > size = ARRAY_SIZE(adlp_rplp_revids);
> > + } else if (IS_ADLP_RPLU(i915)) {
>
> Since the two blocks are identical, it might be slightly preferable to just
> combine the conditions; that will also help make it clear that this is
> intentional and not a copy/paste mistake.
>
> } else if (IS_ADLP_RPLU(i915) || IS_ADLP_RPLP(i915)) {
> ...
>
Since in the new implementation RPLU also belongs to RPL sub platform. This change is dropped in the latest version.
> > + revids = adlp_rplp_revids;
> > + size = ARRAY_SIZE(adlp_rplp_revids);
> > } else if (IS_ALDERLAKE_P(i915)) {
> > revids = adlp_revids;
> > size = ARRAY_SIZE(adlp_revids);
> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> > index 4a4c190f7698..87bb7e26dfb6 100644
> > --- a/include/drm/i915_pciids.h
> > +++ b/include/drm/i915_pciids.h
> > @@ -687,10 +687,13 @@
> > /* RPL-P */
> > #define INTEL_RPLP_IDS(info) \
> > INTEL_VGA_DEVICE(0xA720, info), \
> > - INTEL_VGA_DEVICE(0xA721, info), \
> > INTEL_VGA_DEVICE(0xA7A0, info), \
> > + INTEL_VGA_DEVICE(0xA7A8, info)
> > +
> > +/* RPL-U */
> > +#define INTEL_RPLU_IDS(info) \
> > INTEL_VGA_DEVICE(0xA7A1, info), \
> > - INTEL_VGA_DEVICE(0xA7A8, info), \
> > + INTEL_VGA_DEVICE(0xA721, info), \
> > INTEL_VGA_DEVICE(0xA7A9, info)
>
> I know the bspec orders them like this, but it's probably better to just sort
> these two lists numerically. These bspec pages get reshuffled so often these
> days that trying to match the bspec's strange ordering isn't really worth it.
>
Ack. The new revision has the sequence corrected.
Regards
Chaitanya
>
> Matt
>
> >
> > /* DG2 */
> > --
> > 2.25.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
