Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/jsl: Split EHL/JSL platform info and PCI ids

Ville Syrjälä
2020-10-02T09:59:07Z

On Fri, Oct 02, 2020 at 02:54:34AM +0530, Tejas Upadhyay wrote:
> Split the basic platform definition, macros, and PCI IDs to
> differentiate between EHL and JSL platforms.
>
> Changes since V2 :
> - Added IS_EHL_JSL to replace IS_ELKHARTLAKE
> - EHL/JSL PCI ids split added
> Changes since V1 :
> - IS_ELKHARTLAKE and IS_JASPERLAKE is replaced with
> HAS_PCH_MCC(EHL) and HAS_PCH_JSP(JSL) respectively
> - Reverted EHL/JSL PCI ids split change
>
> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 4 ++--
> drivers/gpu/drm/i915/display/intel_cdclk.c | 4 ++--
> drivers/gpu/drm/i915/display/intel_combo_phy.c | 6 +++---
> drivers/gpu/drm/i915/display/intel_display.c | 8 ++++----
> drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 16 ++++++++--------
> drivers/gpu/drm/i915/gt/intel_sseu.c | 2 +-
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 5 +++--
> drivers/gpu/drm/i915/i915_pci.c | 9 +++++++++
> drivers/gpu/drm/i915/intel_device_info.c | 1 +
> drivers/gpu/drm/i915/intel_device_info.h | 1 +
> drivers/gpu/drm/i915/intel_pch.c | 6 +++---
> include/drm/i915_pciids.h | 9 ++++++---
> 14 files changed, 45 insertions(+), 30 deletions(-)
>
<snip>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 6c580d0d9ea8..5d81a55993e5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1839,7 +1839,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> GEN12_FF_TESSELATION_DOP_GATE_DISABLE);
>
> /* Wa_22010271021:ehl */
> - if (IS_ELKHARTLAKE(i915))
> + if (IS_EHL_JSL(i915))
> wa_masked_en(wal,
> GEN9_CS_DEBUG_MODE1,
> FF_DOP_CLOCK_GATE_DISABLE);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index eef9a821c49c..2c75bcb76932 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1417,7 +1417,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> #define IS_COMETLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_COMETLAKE)
> #define IS_CANNONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_CANNONLAKE)
> #define IS_ICELAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ICELAKE)
> -#define IS_ELKHARTLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE)

Why are we removing this? I would just keep it + add the IS_JASPERLAKE()
too.

> +#define IS_EHL_JSL(dev_priv) (IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) || \
> + IS_PLATFORM(dev_priv, INTEL_JASPERLAKE))

A bit non-standard thing, but I guess it's fine. We already have the
oddball IS_GEN9_{LP,BC}() anyway.

But since JSL is the canonical name used by Bspec I would make that our
"primary" name for this. Ie. I would call the macro IS_JSL_EHL(). And
since I'm a bit ocd about ordering things I would generally make it so
that JSL comes first, followed by EHL, everywhere.

> #define IS_TIGERLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_TIGERLAKE)
> #define IS_ROCKETLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_ROCKETLAKE)
> #define IS_DG1(dev_priv) IS_PLATFORM(dev_priv, INTEL_DG1)
> @@ -1559,7 +1560,7 @@ extern const struct i915_rev_steppings kbl_revids[];
> #define EHL_REVID_A0 0x0
>
> #define IS_EHL_REVID(p, since, until) \
> - (IS_ELKHARTLAKE(p) && IS_REVID(p, since, until))
> + (IS_EHL_JSL(p) && IS_REVID(p, since, until))

That is confusing. Either we want just IS_ELKHARTLAKE() here, or
this macro should also be renamed to contain JSL in the name.

>
> enum {
> TGL_REVID_A0,
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 366ddfc8df6b..8690b69fcf33 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -846,6 +846,14 @@ static const struct intel_device_info ehl_info = {
> .ppgtt_size = 36,
> };
>
> +static const struct intel_device_info jsl_info = {
> + GEN11_FEATURES,
> + PLATFORM(INTEL_JASPERLAKE),
> + .require_force_probe = 1,
> + .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(VCS0) | BIT(VECS0),
> + .ppgtt_size = 36,
> +};
> +
> #define GEN12_FEATURES \
> GEN11_FEATURES, \
> GEN(12), \
> @@ -985,6 +993,7 @@ static const struct pci_device_id pciidlist[] = {
> INTEL_CNL_IDS(&cnl_info),
> INTEL_ICL_11_IDS(&icl_info),
> INTEL_EHL_IDS(&ehl_info),
> + INTEL_JSL_IDS(&jsl_info),
> INTEL_TGL_12_IDS(&tgl_info),
> INTEL_RKL_IDS(&rkl_info),
> {0, 0, 0}
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index adc836f15fde..e67cec8fa2aa 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -62,6 +62,7 @@ static const char * const platform_names[] = {
> PLATFORM_NAME(CANNONLAKE),
> PLATFORM_NAME(ICELAKE),
> PLATFORM_NAME(ELKHARTLAKE),
> + PLATFORM_NAME(JASPERLAKE),
> PLATFORM_NAME(TIGERLAKE),
> PLATFORM_NAME(ROCKETLAKE),
> PLATFORM_NAME(DG1),
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 6a3d607218aa..d92fa041c700 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -79,6 +79,7 @@ enum intel_platform {
> /* gen11 */
> INTEL_ICELAKE,
> INTEL_ELKHARTLAKE,
> + INTEL_JASPERLAKE,
> /* gen12 */
> INTEL_TIGERLAKE,
> INTEL_ROCKETLAKE,
> diff --git a/drivers/gpu/drm/i915/intel_pch.c b/drivers/gpu/drm/i915/intel_pch.c
> index 6c97192e9ca8..72ffb6d4d5fc 100644
> --- a/drivers/gpu/drm/i915/intel_pch.c
> +++ b/drivers/gpu/drm/i915/intel_pch.c
> @@ -115,7 +115,7 @@ intel_pch_type(const struct drm_i915_private *dev_priv, unsigned short id)
> return PCH_ICP;
> case INTEL_PCH_MCC_DEVICE_ID_TYPE:
> drm_dbg_kms(&dev_priv->drm, "Found Mule Creek Canyon PCH\n");
> - drm_WARN_ON(&dev_priv->drm, !IS_ELKHARTLAKE(dev_priv));
> + drm_WARN_ON(&dev_priv->drm, !IS_EHL_JSL(dev_priv));
> return PCH_MCC;
> case INTEL_PCH_TGP_DEVICE_ID_TYPE:
> case INTEL_PCH_TGP2_DEVICE_ID_TYPE:
> @@ -126,7 +126,7 @@ intel_pch_type(const struct drm_i915_private *dev_priv, unsigned short id)
> case INTEL_PCH_JSP_DEVICE_ID_TYPE:
> case INTEL_PCH_JSP2_DEVICE_ID_TYPE:
> drm_dbg_kms(&dev_priv->drm, "Found Jasper Lake PCH\n");
> - drm_WARN_ON(&dev_priv->drm, !IS_ELKHARTLAKE(dev_priv));
> + drm_WARN_ON(&dev_priv->drm, !IS_EHL_JSL(dev_priv));
> return PCH_JSP;
> default:
> return PCH_NONE;
> @@ -157,7 +157,7 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv)
>
> if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv))
> id = INTEL_PCH_TGP_DEVICE_ID_TYPE;
> - else if (IS_ELKHARTLAKE(dev_priv))
> + else if (IS_EHL_JSL(dev_priv))
> id = INTEL_PCH_MCC_DEVICE_ID_TYPE;
> else if (IS_ICELAKE(dev_priv))
> id = INTEL_PCH_ICP_DEVICE_ID_TYPE;
> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> index 7eeecb07c9a1..1b5e09cfa11e 100644
> --- a/include/drm/i915_pciids.h
> +++ b/include/drm/i915_pciids.h
> @@ -579,15 +579,18 @@
> INTEL_VGA_DEVICE(0x8A51, info), \
> INTEL_VGA_DEVICE(0x8A5D, info)
>
> -/* EHL/JSL */
> +/* EHL */
> #define INTEL_EHL_IDS(info) \
> INTEL_VGA_DEVICE(0x4500, info), \
> INTEL_VGA_DEVICE(0x4571, info), \
> INTEL_VGA_DEVICE(0x4551, info), \
> INTEL_VGA_DEVICE(0x4541, info), \
> - INTEL_VGA_DEVICE(0x4E71, info), \
> INTEL_VGA_DEVICE(0x4557, info), \
> - INTEL_VGA_DEVICE(0x4555, info), \
> + INTEL_VGA_DEVICE(0x4555, info)
> +
> +/* JSL */
> +#define INTEL_JSL_IDS(info) \
> + INTEL_VGA_DEVICE(0x4E71, info), \
> INTEL_VGA_DEVICE(0x4E61, info), \
> INTEL_VGA_DEVICE(0x4E57, info), \
> INTEL_VGA_DEVICE(0x4E55, info), \
> --
> 2.28.0

--
Ville Syrjälä
Intel