Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: h+Q4pW67NS4GhUg+A/sn+bDgMtxKAbpmgMf2knu8cSJ46Kx5KPSe3JeccW5BGadQKxpaGMmzc2
 +Fr+DG2kffDQ==
IronPort-SDR: RgT6PVG/mrKNG0UWJKclspgMmLQsBCPYUaHQW0Zl431vLcx2fkuRZxvDWsp+OSloIZ97vSklIN
 LlZrZzkwaxZA==
From:   Jani Nikula <jani.nikula@linux.intel.com>
To:     "Surendrakumar Upadhyay\, TejaskumarX" 
        <tejaskumarx.surendrakumar.upadhyay@intel.com>,
        "Vivi\, Rodrigo" <rodrigo.vivi@intel.com>,
        "airlied\@linux.ie" <airlied@linux.ie>,
        "daniel\@ffwll.ch" <daniel@ffwll.ch>,
        "intel-gfx\@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "dri-devel\@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Ausmus\, James" <james.ausmus@intel.com>,
        "Roper\, Matthew D" <matthew.d.roper@intel.com>,
        "Souza\, Jose" <jose.souza@intel.com>,
        "ville.syrjala\@linux.intel.com" <ville.syrjala@linux.intel.com>,
        "De Marchi\, Lucas" <lucas.demarchi@intel.com>,
        "Pandey\, Hariom" <hariom.pandey@intel.com>
Subject: Re: [PATCH 1/2] drm/i915/jsl: Split EHL/JSL platform info and PCI ids
In-Reply-To: <SN6PR11MB3421571FA9A490C67E0E9D82DF350@SN6PR11MB3421.namprd11.prod.outlook.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20200928080931.246347-1-tejaskumarx.surendrakumar.upadhyay@intel.com> <20200928080931.246347-2-tejaskumarx.surendrakumar.upadhyay@intel.com> <87d026owdq.fsf@intel.com> <SN6PR11MB3421571FA9A490C67E0E9D82DF350@SN6PR11MB3421.namprd11.prod.outlook.com>
Date:   Mon, 28 Sep 2020 20:14:02 +0300
Message-ID: <874knhq0x1.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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

Please fix your email quoting when interacting on the public lists.

>
> On Mon, 28 Sep 2020, Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@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@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?

If you need to differentiate between the two platforms, IS_ELKHARTLAKE()
must mean only ELK and IS_JASPERLAKE() must mean only JSP.

It's non-negotiable. We've made the mistake before, we're not doing it
again.

There are 32 references to IS_ELKHARTLAKE(). It's slightly painful, but
the alternative is worse.


BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Graphics Center