Re: [RFC 1/2] drm/i915: Add RPL-U sub platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Friday, January 27, 2023 11:35 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>
> Cc: Jani Nikula <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>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [RFC 1/2] drm/i915: Add RPL-U sub platform
> 
> On Fri, Jan 27, 2023 at 01:34:31AM -0800, Borah, Chaitanya Kumar wrote:
> > Hello Jani and Matt,
> >
> > > -----Original Message-----
> > > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > > Sent: Tuesday, January 24, 2023 8:02 PM
> > > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; intel-
> > > gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; Syrjala, Ville
> > > <ville.syrjala@xxxxxxxxx>; Srivatsa, Anusha
> > > <anusha.srivatsa@xxxxxxxxx>; Roper, Matthew D
> > > <matthew.d.roper@xxxxxxxxx>; Atwood, Matthew S
> > > <matthew.s.atwood@xxxxxxxxx>; Borah, Chaitanya Kumar
> > > <chaitanya.kumar.borah@xxxxxxxxx>
> > > Subject: Re: [RFC 1/2] drm/i915: Add RPL-U sub platform
> > >
> > > On Tue, 17 Jan 2023, Chaitanya Kumar Borah
> > > <chaitanya.kumar.borah@xxxxxxxxx> wrote:
> > > > Separate out RPLU device ids and add them to both RPL and newly
> > > > created RPL-U subplatforms.
> > > >
> > > > v2: (Matt)
> > > >     - Sort PCI-IDs numerically
> > > >     - Name the sub-platform to accurately depict what it is for
> > > >     - Make RPL-U part of RPL subplatform
> > > >
> > > > v3: revert to RPL-U subplatform (Jani)
> > > >
> > > > Signed-off-by: Chaitanya Kumar Borah
> > > <chaitanya.kumar.borah@xxxxxxxxx>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
> > > >  drivers/gpu/drm/i915/i915_pci.c          |  1 +
> > > >  drivers/gpu/drm/i915/intel_device_info.c |  8 ++++++++
> > > > drivers/gpu/drm/i915/intel_device_info.h |  2 ++
> > > >  include/drm/i915_pciids.h                | 11 +++++++----
> > > >  5 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > 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),
> > >
> > > You may want to drop this change, see later comment on how and why.
> > >
> > > >  	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..fec8bd116436 100644
> > > > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > > > @@ -199,6 +199,11 @@ static const u16 subplatform_n_ids[] = {
> > > > static const u16 subplatform_rpl_ids[] = {
> > > >  	INTEL_RPLS_IDS(0),
> > > >  	INTEL_RPLP_IDS(0),
> > > > +	INTEL_RPLU_IDS(0)
> > >
> > > Please always include the trailing , at the end to make future changes
> easier.
> > > (However, you may want to drop this change altogether, see later
> > > comment.)
> > >
> > > > +};
> > > > +
> > > > +static const u16 subplatform_rplu_ids[] = {
> > > > +	INTEL_RPLU_IDS(0),
> > > >  };
> > > >
> > > >  static const u16 subplatform_g10_ids[] = { @@ -268,6 +273,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);
> > > > +		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..4a5cd337e4b5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > > > @@ -127,6 +127,8 @@ enum intel_platform {
> > > >   * bit set
> > > >   */
> > > >  #define INTEL_SUBPLATFORM_N    1
> > > > +/* Sub Platform for RPL-U */
> > >
> > > This comment really adds nothing, it's exactly the same as the macro
> name.
> > >
> >
> > Ack.
> >
> > > > +#define INTEL_SUBPLATFORM_RPLU  2
> > > >
> > > >  /* MTL */
> > > >  #define INTEL_SUBPLATFORM_M	0
> > > > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> > > > index 4a4c190f7698..758be5fb09a2 100644
> > > > --- a/include/drm/i915_pciids.h
> > > > +++ b/include/drm/i915_pciids.h
> > > > @@ -684,14 +684,17 @@
> > > >  	INTEL_VGA_DEVICE(0xA78A, info), \
> > > >  	INTEL_VGA_DEVICE(0xA78B, info)
> > > >
> > > > +/* RPL-U */
> > > > +#define INTEL_RPLU_IDS(info) \
> > > > +	INTEL_VGA_DEVICE(0xA721, info), \
> > > > +	INTEL_VGA_DEVICE(0xA7A1, info), \
> > > > +	INTEL_VGA_DEVICE(0xA7A9, info)
> > > > +
> > > >  /* 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(0xA7A1, info), \
> > > > -	INTEL_VGA_DEVICE(0xA7A8, info), \
> > > > -	INTEL_VGA_DEVICE(0xA7A9, info)
> > > > +	INTEL_VGA_DEVICE(0xA7A8, info)
> > >
> > > Changing the INTEL_RPLP_IDS impacts arch/x86/kernel/early-quirks.c
> > > too. As is, this drops the early quirks from RPL-U.
> > >
> >
> > Yes I missed the early quirks part in this revision. Thank you Jani for
> pointing out.
> >
> > > Your options are 1) modify early-quirks.c too, or 2) include RPL-U ids in
> RPL-P:
> > >
> > >   #define INTEL_RPLP_IDS(info) \
> > > +	INTEL_RPLU_IDS(info), \
> > > -	INTEL_VGA_DEVICE(0xA721, info), \
> > >  	INTEL_VGA_DEVICE(0xA7A0, info), \
> > > -	INTEL_VGA_DEVICE(0xA7A1, info), \
> > > -	INTEL_VGA_DEVICE(0xA7A8, info), \
> > > -	INTEL_VGA_DEVICE(0xA7A9, info)
> > > +	INTEL_VGA_DEVICE(0xA7A8, info)
> > >
> > > With option 2, you also don't need to add INTEL_RPLU_IDS separately
> > > to subplatform_rpl_ids[] or pciidlist[].
> > >
> > > I might lean towards option 2, but no strong opinions.
> > >
> >
> > Thank you Jani for your suggestion. I am also inclined to the solution of
> adding RPLU IDs to RPLP platform.
> >
> > To summarize, we would add the INTEL_RPLU_IDS(info) into
> > INTEL_RPLP_IDS(info).  As Jani pointed out, with this, there is no need to
> add INTEL_RPLU_IDS separately to subplatform_rpl_ids[] or pciidlist[]. This
> also aligns with Bspec(55376) as RPL-U ids are listed under "RPL-P SKUs and
> Device IDs Info"
> >
> > @Matt: I remember, we had discussed a similar option in our
> communications. Do you have any strong opinion against it?
> 
> That sounds fine to me.
> 
> Thanks.
> 

Thank you Matt. I have floated a new series based on these comments.

Regards

Chaitanya

> 
> Matt
> 
> >
> > If not I would go ahead with this implementation.
> >
> > Regards
> >
> > Chaitanya
> >
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > >
> > > >  /* DG2 */
> > > >  #define INTEL_DG2_G10_IDS(info) \
> > >
> > > --
> > > Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux