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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 17 Jan 2023, Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> 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@intel.com>
> ---
>  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.

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

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.


BR,
Jani.


>  
>  /* DG2 */
>  #define INTEL_DG2_G10_IDS(info) \

-- 
Jani Nikula, Intel Open Source Graphics Center