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

Mon Sep 28 16:31:58 UTC 2020

________________________________
From: Jani Nikula <jani.nikula at linux.intel.com>
Sent: Monday, September 28, 2020 7:07 PM
To: Surendrakumar Upadhyay, TejaskumarX <tejaskumarx.surendrakumar.upadhyay at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; airlied at linux.ie <airlied at linux.ie>; daniel at ffwll.ch <daniel at ffwll.ch>; intel-gfx at lists.freedesktop.org <intel-gfx at lists.freedesktop.org>; dri-devel at lists.freedesktop.org <dri-devel at lists.freedesktop.org>; linux-kernel at vger.kernel.org <linux-kernel at vger.kernel.org>; Ausmus, James <james.ausmus at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; Souza, Jose <jose.souza at intel.com>; ville.syrjala at linux.intel.com <ville.syrjala at linux.intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; Pandey, Hariom <hariom.pandey at intel.com>
Subject: Re: [PATCH 1/2] drm/i915/jsl: Split EHL/JSL platform info and PCI ids

On Mon, 28 Sep 2020, Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay at intel.com> wrote:
> Split the basic platform definition, macros, and PCI IDs to
> differentiate between EHL and JSL platforms.
>
> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 4 +++-
>  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 +
>  include/drm/i915_pciids.h                | 9 ++++++---
>  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 72a9449b674e..4f20acebb038 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1417,7 +1417,9 @@ 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)
> +#define IS_ELKHARTLAKE(dev_priv) (IS_PLATFORM(dev_priv, INTEL_ELKHARTLAKE) || \
> +                             IS_PLATFORM(dev_priv, INTEL_JASPERLAKE))
> +#define IS_JASPERLAKE(dev_priv)      IS_PLATFORM(dev_priv, INTEL_JASPERLAKE)

I think we've learned from history that we want the platform checks to
be independent. I.e. if you need to split ELK and JSP, you need to make
IS_ELKHARTLAKE() match *only* ELK, and you need to replace every current
IS_ELKHARTLAKE() check with IS_ELKHARTLAKE() || IS_JASPERLAKE().

We've been here before, and we've thought before that we can get by with
the minimal change. It's just postponing the inevitable and generates
confusion.

BR,
Jani.

Tejas :  Replacing IS_ELKHARTLAKE() || IS_JASPERLAKE() everywhere will make lot of changes at each place. To avoid huge change and to differentiate between platforms we have taken this way. Do you think we still change it everywhere? Do you have example where it can harm this change?

>  #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)
> 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/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), \

--
Jani Nikula, Intel Open Source Graphics Center
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20200928/dbd98630/attachment.htm>