[PATCH 0/5] Introduce Kabylake Support

[PATCH 0/5] Introduce Kabylake Support

[Rodrigo Vivi]

Kabylake is a Intel® Processor containing Intel® HD Graphics following Skylake.

It is Gen9p5, so it inherits everything from Skylake. So let's define
Kabylake as Skylake and reuse the great job Damien and others did for Skylake.

The caveat is that Kabylake A0 was derivated from Skylake D0, so we needed
some adjusts on revid handling in order to avoid old and not anymore needed
workarounds.

Thanks,
Rodrigo.

Deepak S (1):
  drm/i915/kbl: Add Kabylake PCI ID

Rodrigo Vivi (4):
  drm/i915/kbl: Use propper ddi buffer translation table for Kabylake
    ULT and ULX.
  drm/i915/kbl: Kabylake A0 is based on Skylake H0.
  drm/i915/kbl: Fix DMC load on Kabylake.
  drm/i915/kbl: drm/i915: Avoid GuC loading for now on Kabylake.

 drivers/gpu/drm/i915/i915_drv.c  | 33 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h  | 21 ++++++++++++++++++---
 drivers/gpu/drm/i915/intel_csr.c | 16 ++++++++++------
 drivers/gpu/drm/i915/intel_ddi.c | 10 +++++-----
 include/drm/i915_pciids.h        | 29 +++++++++++++++++++++++++++++
 5 files changed, 94 insertions(+), 15 deletions(-)

-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Rodrigo Vivi]

From: Deepak S <deepak.s@intel.com>

v2: separate out device info into different GT (Damien)
v3: Add is_kabylake to the KBL gt3 structuer (Damien)
    Sort the platforms in older -> newer order (Damien)

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1cb6b82..f42102d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -394,6 +394,34 @@ static const struct intel_device_info intel_broxton_info = {
 	IVB_CURSOR_OFFSETS,
 };
 
+static const struct intel_device_info intel_kabylake_info = {
+	.is_preliminary = 1,
+	.is_skylake = 1,
+	.is_kabylake = 1,
+	.gen = 9, .num_pipes = 3,
+	.need_gfx_hws = 1, .has_hotplug = 1,
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
+	.has_llc = 1,
+	.has_ddi = 1,
+	.has_fbc = 1,
+	GEN_DEFAULT_PIPEOFFSETS,
+	IVB_CURSOR_OFFSETS,
+};
+
+static const struct intel_device_info intel_kabylake_gt3_info = {
+	.is_preliminary = 1,
+	.is_skylake = 1,
+	.is_kabylake = 1,
+	.gen = 9, .num_pipes = 3,
+	.need_gfx_hws = 1, .has_hotplug = 1,
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
+	.has_llc = 1,
+	.has_ddi = 1,
+	.has_fbc = 1,
+	GEN_DEFAULT_PIPEOFFSETS,
+	IVB_CURSOR_OFFSETS,
+};
+
 /*
  * Make sure any device matches here are from most specific to most
  * general.  For example, since the Quanta match is based on the subsystem
@@ -434,7 +462,10 @@ static const struct intel_device_info intel_broxton_info = {
 	INTEL_SKL_GT1_IDS(&intel_skylake_info),	\
 	INTEL_SKL_GT2_IDS(&intel_skylake_info),	\
 	INTEL_SKL_GT3_IDS(&intel_skylake_gt3_info),	\
-	INTEL_BXT_IDS(&intel_broxton_info)
+	INTEL_BXT_IDS(&intel_broxton_info),		\
+	INTEL_KBL_GT1_IDS(&intel_kabylake_info),	\
+	INTEL_KBL_GT2_IDS(&intel_kabylake_info),	\
+	INTEL_KBL_GT3_IDS(&intel_kabylake_gt3_info)
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
 	INTEL_PCI_IDS,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 824e724..f7e9d7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -765,6 +765,7 @@ struct intel_csr {
 	func(is_valleyview) sep \
 	func(is_haswell) sep \
 	func(is_skylake) sep \
+	func(is_kabylake) sep \
 	func(is_preliminary) sep \
 	func(has_fbc) sep \
 	func(has_pipe_cxsr) sep \
@@ -2464,6 +2465,7 @@ struct drm_i915_cmd_table {
 #define IS_BROADWELL(dev)	(!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
 #define IS_SKYLAKE(dev)	(INTEL_INFO(dev)->is_skylake)
 #define IS_BROXTON(dev)	(!INTEL_INFO(dev)->is_skylake && IS_GEN9(dev))
+#define IS_KABYLAKE(dev)	(INTEL_INFO(dev)->is_kabylake)
 #define IS_MOBILE(dev)		(INTEL_INFO(dev)->is_mobile)
 #define IS_HSW_EARLY_SDV(dev)	(IS_HASWELL(dev) && \
 				 (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
index 17c4456..2e7a159 100644
--- a/include/drm/i915_pciids.h
+++ b/include/drm/i915_pciids.h
@@ -291,4 +291,33 @@
 	INTEL_VGA_DEVICE(0x1A84, info), \
 	INTEL_VGA_DEVICE(0x5A84, info)
 
+#define INTEL_KBL_GT1_IDS(info)	\
+	INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \
+	INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \
+	INTEL_VGA_DEVICE(0x5917, info), /* DT  GT1.5 */ \
+	INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \
+	INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \
+	INTEL_VGA_DEVICE(0x5902, info), /* DT  GT1 */ \
+	INTEL_VGA_DEVICE(0x590B, info), /* Halo GT1 */ \
+	INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */
+
+#define INTEL_KBL_GT2_IDS(info)	\
+	INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \
+	INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \
+	INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \
+	INTEL_VGA_DEVICE(0x5912, info), /* DT  GT2 */ \
+	INTEL_VGA_DEVICE(0x591B, info), /* Halo GT2 */ \
+	INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \
+	INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */
+
+#define INTEL_KBL_GT3_IDS(info) \
+	INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \
+	INTEL_VGA_DEVICE(0x592B, info), /* Halo GT3 */ \
+	INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */
+
+#define INTEL_KBL_IDS(info) \
+	INTEL_KBL_GT1_IDS(info), \
+	INTEL_KBL_GT2_IDS(info), \
+	INTEL_KBL_GT3_IDS(info)
+
 #endif /* _I915_PCIIDS_H */
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915/kbl: Use propper ddi buffer translation table for Kabylake ULT and ULX.

[Rodrigo Vivi]

Let's introduce ULT and ULX Kabylake definitions and start
using it for a propper DDI buffer translation.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  8 ++++++++
 drivers/gpu/drm/i915/intel_ddi.c | 10 +++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f7e9d7e..7374a0d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2493,6 +2493,14 @@ struct drm_i915_cmd_table {
 #define IS_SKL_ULX(dev)		(INTEL_DEVID(dev) == 0x190E || \
 				 INTEL_DEVID(dev) == 0x1915 || \
 				 INTEL_DEVID(dev) == 0x191E)
+#define IS_KBL_ULT(dev)		(INTEL_DEVID(dev) == 0x5906 || \
+				 INTEL_DEVID(dev) == 0x5913 || \
+				 INTEL_DEVID(dev) == 0x5916 || \
+				 INTEL_DEVID(dev) == 0x5921 || \
+				 INTEL_DEVID(dev) == 0x5926)
+#define IS_KBL_ULX(dev)		(INTEL_DEVID(dev) == 0x590E || \
+				 INTEL_DEVID(dev) == 0x5915 || \
+				 INTEL_DEVID(dev) == 0x591E)
 #define IS_SKL_GT3(dev)		(IS_SKYLAKE(dev) && \
 				 (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
 #define IS_SKL_GT4(dev)		(IS_SKYLAKE(dev) && \
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b3f1748..8fd4fef 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -353,10 +353,10 @@ static const struct ddi_buf_trans *skl_get_buf_trans_dp(struct drm_device *dev,
 {
 	const struct ddi_buf_trans *ddi_translations;
 
-	if (IS_SKL_ULX(dev)) {
+	if (IS_SKL_ULX(dev) || IS_KBL_ULX(dev)) {
 		ddi_translations = skl_y_ddi_translations_dp;
 		*n_entries = ARRAY_SIZE(skl_y_ddi_translations_dp);
-	} else if (IS_SKL_ULT(dev)) {
+	} else if (IS_SKL_ULT(dev) || IS_KBL_ULT(dev)) {
 		ddi_translations = skl_u_ddi_translations_dp;
 		*n_entries = ARRAY_SIZE(skl_u_ddi_translations_dp);
 	} else {
@@ -373,7 +373,7 @@ static const struct ddi_buf_trans *skl_get_buf_trans_edp(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct ddi_buf_trans *ddi_translations;
 
-	if (IS_SKL_ULX(dev)) {
+	if (IS_SKL_ULX(dev)  || IS_KBL_ULX(dev)) {
 		if (dev_priv->edp_low_vswing) {
 			ddi_translations = skl_y_ddi_translations_edp;
 			*n_entries = ARRAY_SIZE(skl_y_ddi_translations_edp);
@@ -381,7 +381,7 @@ static const struct ddi_buf_trans *skl_get_buf_trans_edp(struct drm_device *dev,
 			ddi_translations = skl_y_ddi_translations_dp;
 			*n_entries = ARRAY_SIZE(skl_y_ddi_translations_dp);
 		}
-	} else if (IS_SKL_ULT(dev)) {
+	} else if (IS_SKL_ULT(dev) || IS_KBL_ULT(dev)) {
 		if (dev_priv->edp_low_vswing) {
 			ddi_translations = skl_u_ddi_translations_edp;
 			*n_entries = ARRAY_SIZE(skl_u_ddi_translations_edp);
@@ -408,7 +408,7 @@ skl_get_buf_trans_hdmi(struct drm_device *dev,
 {
 	const struct ddi_buf_trans *ddi_translations;
 
-	if (IS_SKL_ULX(dev)) {
+	if (IS_SKL_ULX(dev) || IS_KBL_ULX(dev)) {
 		ddi_translations = skl_y_ddi_translations_hdmi;
 		*n_entries = ARRAY_SIZE(skl_y_ddi_translations_hdmi);
 	} else {
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

[Rodrigo Vivi]

Kabylake is gen 9.5 derivated from Skylake H0 stepping.

So we don't need pre-production Skylake workaround and also
firmware loading will use SKL H0 offsets.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7374a0d..580c005 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
 })
 #define INTEL_INFO(p) 	(&__I915__(p)->info)
 #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
-#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
 
 #define IS_I830(dev)		(INTEL_DEVID(dev) == 0x3577)
 #define IS_845G(dev)		(INTEL_DEVID(dev) == 0x2562)
@@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
 
 #define IS_PRELIMINARY_HW(intel_info) ((intel_info)->is_preliminary)
 
+#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision + \
+			 IS_KABYLAKE(p) ? 7 : 0)
+
 #define SKL_REVID_A0		(0x0)
 #define SKL_REVID_B0		(0x1)
 #define SKL_REVID_C0		(0x2)
@@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
 #define SKL_REVID_E0		(0x4)
 #define SKL_REVID_F0		(0x5)
 
+/* KBL A0 is based on SKL H0 */
+#define KBL_REVID_A0		(0x7)
+
 #define BXT_REVID_A0		(0x0)
 #define BXT_REVID_B0		(0x3)
 #define BXT_REVID_C0		(0x9)
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915/kbl: Fix DMC load on Kabylake.

[Rodrigo Vivi]

Kabylake A0 is based on Skylake H0. So let's reuse the
INTEL_REVID that already takes care of this, so the csr
code can find the proper firmware address offset.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 9e530a7..b7d26d7 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -190,9 +190,11 @@ static struct stepping_info bxt_stepping_info[] = {
 
 static char intel_get_stepping(struct drm_device *dev)
 {
-	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(skl_stepping_info)))
-		return skl_stepping_info[dev->pdev->revision].stepping;
+	int revid = INTEL_REVID(dev);
+
+	if (IS_SKYLAKE(dev) &&
+	    revid < ARRAY_SIZE(skl_stepping_info))
+		return skl_stepping_info[revid].stepping;
 	else if (IS_BROXTON(dev) && (dev->pdev->revision <
 				ARRAY_SIZE(bxt_stepping_info)))
 		return bxt_stepping_info[dev->pdev->revision].stepping;
@@ -202,9 +204,11 @@ static char intel_get_stepping(struct drm_device *dev)
 
 static char intel_get_substepping(struct drm_device *dev)
 {
-	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(skl_stepping_info)))
-		return skl_stepping_info[dev->pdev->revision].substepping;
+	int revid = INTEL_REVID(dev);
+
+	if (IS_SKYLAKE(dev) &&
+	    revid < ARRAY_SIZE(skl_stepping_info))
+		return skl_stepping_info[revid].substepping;
 	else if (IS_BROXTON(dev) && (dev->pdev->revision <
 			ARRAY_SIZE(bxt_stepping_info)))
 		return bxt_stepping_info[dev->pdev->revision].substepping;
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915/kbl: drm/i915: Avoid GuC loading for now on Kabylake.

[Rodrigo Vivi]

GuC has no version for KBL published yet and it is not recommended
to load the Skylake one, so let's avoid loading this for now while
we don't have the proper GuC firmware for Kabylake.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 580c005..a532943 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2603,8 +2603,8 @@ struct drm_i915_cmd_table {
 
 #define HAS_CSR(dev)	(IS_GEN9(dev))
 
-#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev))
-#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev))
+#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
+#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
 
 #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
 				    INTEL_INFO(dev)->gen >= 8)
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Jani Nikula]

On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Deepak S <deepak.s@intel.com>
>
> v2: separate out device info into different GT (Damien)
> v3: Add is_kabylake to the KBL gt3 structuer (Damien)
>     Sort the platforms in older -> newer order (Damien)
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Deepak S <deepak.s@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1cb6b82..f42102d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -394,6 +394,34 @@ static const struct intel_device_info intel_broxton_info = {
>  	IVB_CURSOR_OFFSETS,
>  };
>  
> +static const struct intel_device_info intel_kabylake_info = {
> +	.is_preliminary = 1,
> +	.is_skylake = 1,

Now's the time to call the shots, is this really a good idea or not? See
VLV vs. CHV, we (okay, the royal we) still confuse ourselves with
IS_VALLEYVIEW.

Granted, 74 call sites for IS_SKYLAKE(), all of those would need to be
patched. We'd need something like "is skylake family" including SKL and
KBL. Some of them might be changed to be more like feature flags, which
is something we've decided we need to do more anyway.

BR,
Jani.


> +	.is_kabylake = 1,
> +	.gen = 9, .num_pipes = 3,
> +	.need_gfx_hws = 1, .has_hotplug = 1,
> +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> +	.has_llc = 1,
> +	.has_ddi = 1,
> +	.has_fbc = 1,
> +	GEN_DEFAULT_PIPEOFFSETS,
> +	IVB_CURSOR_OFFSETS,
> +};
> +
> +static const struct intel_device_info intel_kabylake_gt3_info = {
> +	.is_preliminary = 1,
> +	.is_skylake = 1,
> +	.is_kabylake = 1,
> +	.gen = 9, .num_pipes = 3,
> +	.need_gfx_hws = 1, .has_hotplug = 1,
> +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> +	.has_llc = 1,
> +	.has_ddi = 1,
> +	.has_fbc = 1,
> +	GEN_DEFAULT_PIPEOFFSETS,
> +	IVB_CURSOR_OFFSETS,
> +};
> +
>  /*
>   * Make sure any device matches here are from most specific to most
>   * general.  For example, since the Quanta match is based on the subsystem
> @@ -434,7 +462,10 @@ static const struct intel_device_info intel_broxton_info = {
>  	INTEL_SKL_GT1_IDS(&intel_skylake_info),	\
>  	INTEL_SKL_GT2_IDS(&intel_skylake_info),	\
>  	INTEL_SKL_GT3_IDS(&intel_skylake_gt3_info),	\
> -	INTEL_BXT_IDS(&intel_broxton_info)
> +	INTEL_BXT_IDS(&intel_broxton_info),		\
> +	INTEL_KBL_GT1_IDS(&intel_kabylake_info),	\
> +	INTEL_KBL_GT2_IDS(&intel_kabylake_info),	\
> +	INTEL_KBL_GT3_IDS(&intel_kabylake_gt3_info)
>  
>  static const struct pci_device_id pciidlist[] = {		/* aka */
>  	INTEL_PCI_IDS,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 824e724..f7e9d7e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -765,6 +765,7 @@ struct intel_csr {
>  	func(is_valleyview) sep \
>  	func(is_haswell) sep \
>  	func(is_skylake) sep \
> +	func(is_kabylake) sep \
>  	func(is_preliminary) sep \
>  	func(has_fbc) sep \
>  	func(has_pipe_cxsr) sep \
> @@ -2464,6 +2465,7 @@ struct drm_i915_cmd_table {
>  #define IS_BROADWELL(dev)	(!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
>  #define IS_SKYLAKE(dev)	(INTEL_INFO(dev)->is_skylake)
>  #define IS_BROXTON(dev)	(!INTEL_INFO(dev)->is_skylake && IS_GEN9(dev))
> +#define IS_KABYLAKE(dev)	(INTEL_INFO(dev)->is_kabylake)
>  #define IS_MOBILE(dev)		(INTEL_INFO(dev)->is_mobile)
>  #define IS_HSW_EARLY_SDV(dev)	(IS_HASWELL(dev) && \
>  				 (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> index 17c4456..2e7a159 100644
> --- a/include/drm/i915_pciids.h
> +++ b/include/drm/i915_pciids.h
> @@ -291,4 +291,33 @@
>  	INTEL_VGA_DEVICE(0x1A84, info), \
>  	INTEL_VGA_DEVICE(0x5A84, info)
>  
> +#define INTEL_KBL_GT1_IDS(info)	\
> +	INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \
> +	INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \
> +	INTEL_VGA_DEVICE(0x5917, info), /* DT  GT1.5 */ \
> +	INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \
> +	INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \
> +	INTEL_VGA_DEVICE(0x5902, info), /* DT  GT1 */ \
> +	INTEL_VGA_DEVICE(0x590B, info), /* Halo GT1 */ \
> +	INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */
> +
> +#define INTEL_KBL_GT2_IDS(info)	\
> +	INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \
> +	INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \
> +	INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \
> +	INTEL_VGA_DEVICE(0x5912, info), /* DT  GT2 */ \
> +	INTEL_VGA_DEVICE(0x591B, info), /* Halo GT2 */ \
> +	INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \
> +	INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */
> +
> +#define INTEL_KBL_GT3_IDS(info) \
> +	INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \
> +	INTEL_VGA_DEVICE(0x592B, info), /* Halo GT3 */ \
> +	INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */
> +
> +#define INTEL_KBL_IDS(info) \
> +	INTEL_KBL_GT1_IDS(info), \
> +	INTEL_KBL_GT2_IDS(info), \
> +	INTEL_KBL_GT3_IDS(info)
> +
>  #endif /* _I915_PCIIDS_H */
> -- 
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Jani Nikula]

On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Deepak S <deepak.s@intel.com>
>
> v2: separate out device info into different GT (Damien)
> v3: Add is_kabylake to the KBL gt3 structuer (Damien)
>     Sort the platforms in older -> newer order (Damien)
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Deepak S <deepak.s@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1cb6b82..f42102d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -394,6 +394,34 @@ static const struct intel_device_info intel_broxton_info = {
>  	IVB_CURSOR_OFFSETS,
>  };
>  
> +static const struct intel_device_info intel_kabylake_info = {
> +	.is_preliminary = 1,
> +	.is_skylake = 1,
> +	.is_kabylake = 1,
> +	.gen = 9, .num_pipes = 3,
> +	.need_gfx_hws = 1, .has_hotplug = 1,
> +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> +	.has_llc = 1,
> +	.has_ddi = 1,
> +	.has_fbc = 1,
> +	GEN_DEFAULT_PIPEOFFSETS,
> +	IVB_CURSOR_OFFSETS,
> +};
> +
> +static const struct intel_device_info intel_kabylake_gt3_info = {
> +	.is_preliminary = 1,
> +	.is_skylake = 1,
> +	.is_kabylake = 1,
> +	.gen = 9, .num_pipes = 3,
> +	.need_gfx_hws = 1, .has_hotplug = 1,
> +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> +	.has_llc = 1,
> +	.has_ddi = 1,
> +	.has_fbc = 1,
> +	GEN_DEFAULT_PIPEOFFSETS,
> +	IVB_CURSOR_OFFSETS,
> +};
> +
>  /*
>   * Make sure any device matches here are from most specific to most
>   * general.  For example, since the Quanta match is based on the subsystem
> @@ -434,7 +462,10 @@ static const struct intel_device_info intel_broxton_info = {
>  	INTEL_SKL_GT1_IDS(&intel_skylake_info),	\
>  	INTEL_SKL_GT2_IDS(&intel_skylake_info),	\
>  	INTEL_SKL_GT3_IDS(&intel_skylake_gt3_info),	\
> -	INTEL_BXT_IDS(&intel_broxton_info)
> +	INTEL_BXT_IDS(&intel_broxton_info),		\
> +	INTEL_KBL_GT1_IDS(&intel_kabylake_info),	\
> +	INTEL_KBL_GT2_IDS(&intel_kabylake_info),	\
> +	INTEL_KBL_GT3_IDS(&intel_kabylake_gt3_info)
>  
>  static const struct pci_device_id pciidlist[] = {		/* aka */
>  	INTEL_PCI_IDS,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 824e724..f7e9d7e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -765,6 +765,7 @@ struct intel_csr {
>  	func(is_valleyview) sep \
>  	func(is_haswell) sep \
>  	func(is_skylake) sep \
> +	func(is_kabylake) sep \
>  	func(is_preliminary) sep \
>  	func(has_fbc) sep \
>  	func(has_pipe_cxsr) sep \
> @@ -2464,6 +2465,7 @@ struct drm_i915_cmd_table {
>  #define IS_BROADWELL(dev)	(!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
>  #define IS_SKYLAKE(dev)	(INTEL_INFO(dev)->is_skylake)
>  #define IS_BROXTON(dev)	(!INTEL_INFO(dev)->is_skylake && IS_GEN9(dev))
> +#define IS_KABYLAKE(dev)	(INTEL_INFO(dev)->is_kabylake)
>  #define IS_MOBILE(dev)		(INTEL_INFO(dev)->is_mobile)
>  #define IS_HSW_EARLY_SDV(dev)	(IS_HASWELL(dev) && \
>  				 (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> index 17c4456..2e7a159 100644
> --- a/include/drm/i915_pciids.h
> +++ b/include/drm/i915_pciids.h
> @@ -291,4 +291,33 @@
>  	INTEL_VGA_DEVICE(0x1A84, info), \
>  	INTEL_VGA_DEVICE(0x5A84, info)
>  
> +#define INTEL_KBL_GT1_IDS(info)	\
> +	INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \
> +	INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \
> +	INTEL_VGA_DEVICE(0x5917, info), /* DT  GT1.5 */ \
> +	INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \
> +	INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \
> +	INTEL_VGA_DEVICE(0x5902, info), /* DT  GT1 */ \
> +	INTEL_VGA_DEVICE(0x590B, info), /* Halo GT1 */ \
> +	INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */
> +
> +#define INTEL_KBL_GT2_IDS(info)	\
> +	INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \
> +	INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \

This will match IS_SKL_GT3, making it a good example of just how
confusing it will be to have IS_SKYLAKE match Kabylake.

> +	INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \
> +	INTEL_VGA_DEVICE(0x5912, info), /* DT  GT2 */ \
> +	INTEL_VGA_DEVICE(0x591B, info), /* Halo GT2 */ \
> +	INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \
> +	INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */
> +
> +#define INTEL_KBL_GT3_IDS(info) \
> +	INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \
> +	INTEL_VGA_DEVICE(0x592B, info), /* Halo GT3 */ \
> +	INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */
> +
> +#define INTEL_KBL_IDS(info) \
> +	INTEL_KBL_GT1_IDS(info), \
> +	INTEL_KBL_GT2_IDS(info), \
> +	INTEL_KBL_GT3_IDS(info)
> +
>  #endif /* _I915_PCIIDS_H */
> -- 
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

[Jani Nikula]

On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Kabylake is gen 9.5 derivated from Skylake H0 stepping.
>
> So we don't need pre-production Skylake workaround and also
> firmware loading will use SKL H0 offsets.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7374a0d..580c005 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
>  })
>  #define INTEL_INFO(p) 	(&__I915__(p)->info)
>  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
> -#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>  
>  #define IS_I830(dev)		(INTEL_DEVID(dev) == 0x3577)
>  #define IS_845G(dev)		(INTEL_DEVID(dev) == 0x2562)
> @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
>  
>  #define IS_PRELIMINARY_HW(intel_info) ((intel_info)->is_preliminary)
>  
> +#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision + \
> +			 IS_KABYLAKE(p) ? 7 : 0)
> +

I am not fond of this at all. It will be really confusing that
->revision is different from INTEL_REVID when checking the workarounds,
and that you'll be using SKL_REVID_* to match KBL revision
ids. Additionally, we'll probably want to start removing SKL workarounds
before KBL workarounds.

Others may disagree, but I'd like KBL revid checks be different from
SKL.

>  #define SKL_REVID_A0		(0x0)
>  #define SKL_REVID_B0		(0x1)
>  #define SKL_REVID_C0		(0x2)
> @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
>  #define SKL_REVID_E0		(0x4)
>  #define SKL_REVID_F0		(0x5)
>  
> +/* KBL A0 is based on SKL H0 */
> +#define KBL_REVID_A0		(0x7)

You can't compare this against INTEL_REVID() now can you...? Or is this
not the one in the spec? Confused already.

BR,
Jani.

> +
>  #define BXT_REVID_A0		(0x0)
>  #define BXT_REVID_B0		(0x3)
>  #define BXT_REVID_C0		(0x9)
> -- 
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Vivi, Rodrigo]

On Tue, 2015-10-06 at 12:09 +0300, Jani Nikula wrote:
> On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > From: Deepak S <deepak.s@intel.com>
> > 
> > v2: separate out device info into different GT (Damien)
> > v3: Add is_kabylake to the KBL gt3 structuer (Damien)
> >     Sort the platforms in older -> newer order (Damien)
> > 
> > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > Signed-off-by: Deepak S <deepak.s@intel.com>
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 33 
> > ++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
> >  include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
> >  3 files changed, 63 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 1cb6b82..f42102d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -394,6 +394,34 @@ static const struct intel_device_info 
> > intel_broxton_info = {
> >  	IVB_CURSOR_OFFSETS,
> >  };
> >  
> > +static const struct intel_device_info intel_kabylake_info = {
> > +	.is_preliminary = 1,
> > +	.is_skylake = 1,
> 
> Now's the time to call the shots, is this really a good idea or not? 
> See
> VLV vs. CHV, we (okay, the royal we) still confuse ourselves with
> IS_VALLEYVIEW.
> 
> Granted, 74 call sites for IS_SKYLAKE(), all of those would need to 
> be
> patched. We'd need something like "is skylake family" including SKL 
> and
> KBL. Some of them might be changed to be more like feature flags, 
> which
> is something we've decided we need to do more anyway.

To be honest I also don't like this approach as I didn't like the chv
is vlv one. I always get confused and I thought about changing
everything many times during last week, but as you pointed out it would
be a change in many entries and not sure what others opinion was since
I never saw any complain about this patch before neither the vlv-chv
ones... 

But if you and Daniel prefer I can do a
s/IS_SKYLAKE(dev)/IS_KABYLAKE(dev) || IS_SKYLAKE(dev)
Just let me know...

The IS_SKYLAKE_FAMILY I don't like much because it will still confuse
people because in ark they will probably be 2 different "families" if
they continue with the current scheme they are using so far.

> 
> BR,
> Jani.
> 
> 
> > +	.is_kabylake = 1,
> > +	.gen = 9, .num_pipes = 3,
> > +	.need_gfx_hws = 1, .has_hotplug = 1,
> > +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | 
> > VEBOX_RING,
> > +	.has_llc = 1,
> > +	.has_ddi = 1,
> > +	.has_fbc = 1,
> > +	GEN_DEFAULT_PIPEOFFSETS,
> > +	IVB_CURSOR_OFFSETS,
> > +};
> > +
> > +static const struct intel_device_info intel_kabylake_gt3_info = {
> > +	.is_preliminary = 1,
> > +	.is_skylake = 1,
> > +	.is_kabylake = 1,
> > +	.gen = 9, .num_pipes = 3,
> > +	.need_gfx_hws = 1, .has_hotplug = 1,
> > +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | 
> > VEBOX_RING | BSD2_RING,
> > +	.has_llc = 1,
> > +	.has_ddi = 1,
> > +	.has_fbc = 1,
> > +	GEN_DEFAULT_PIPEOFFSETS,
> > +	IVB_CURSOR_OFFSETS,
> > +};
> > +
> >  /*
> >   * Make sure any device matches here are from most specific to 
> > most
> >   * general.  For example, since the Quanta match is based on the 
> > subsystem
> > @@ -434,7 +462,10 @@ static const struct intel_device_info 
> > intel_broxton_info = {
> >  	INTEL_SKL_GT1_IDS(&intel_skylake_info),	\
> >  	INTEL_SKL_GT2_IDS(&intel_skylake_info),	\
> >  	INTEL_SKL_GT3_IDS(&intel_skylake_gt3_info),	\
> > -	INTEL_BXT_IDS(&intel_broxton_info)
> > +	INTEL_BXT_IDS(&intel_broxton_info),		\
> > +	INTEL_KBL_GT1_IDS(&intel_kabylake_info),	\
> > +	INTEL_KBL_GT2_IDS(&intel_kabylake_info),	\
> > +	INTEL_KBL_GT3_IDS(&intel_kabylake_gt3_info)
> >  
> >  static const struct pci_device_id pciidlist[] = {		/
> > * aka */
> >  	INTEL_PCI_IDS,
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 824e724..f7e9d7e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -765,6 +765,7 @@ struct intel_csr {
> >  	func(is_valleyview) sep \
> >  	func(is_haswell) sep \
> >  	func(is_skylake) sep \
> > +	func(is_kabylake) sep \
> >  	func(is_preliminary) sep \
> >  	func(has_fbc) sep \
> >  	func(has_pipe_cxsr) sep \
> > @@ -2464,6 +2465,7 @@ struct drm_i915_cmd_table {
> >  #define IS_BROADWELL(dev)	(!INTEL_INFO(dev)->is_valleyview 
> > && IS_GEN8(dev))
> >  #define IS_SKYLAKE(dev)	(INTEL_INFO(dev)->is_skylake)
> >  #define IS_BROXTON(dev)	(!INTEL_INFO(dev)->is_skylake && 
> > IS_GEN9(dev))
> > +#define IS_KABYLAKE(dev)	(INTEL_INFO(dev)->is_kabylake)
> >  #define IS_MOBILE(dev)		(INTEL_INFO(dev)->is_mobile)
> >  #define IS_HSW_EARLY_SDV(dev)	(IS_HASWELL(dev) && \
> >  				 (INTEL_DEVID(dev) & 0xFF00) == 
> > 0x0C00)
> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> > index 17c4456..2e7a159 100644
> > --- a/include/drm/i915_pciids.h
> > +++ b/include/drm/i915_pciids.h
> > @@ -291,4 +291,33 @@
> >  	INTEL_VGA_DEVICE(0x1A84, info), \
> >  	INTEL_VGA_DEVICE(0x5A84, info)
> >  
> > +#define INTEL_KBL_GT1_IDS(info)	\
> > +	INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \
> > +	INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \
> > +	INTEL_VGA_DEVICE(0x5917, info), /* DT  GT1.5 */ \
> > +	INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \
> > +	INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \
> > +	INTEL_VGA_DEVICE(0x5902, info), /* DT  GT1 */ \
> > +	INTEL_VGA_DEVICE(0x590B, info), /* Halo GT1 */ \
> > +	INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */
> > +
> > +#define INTEL_KBL_GT2_IDS(info)	\
> > +	INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \
> > +	INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \
> > +	INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \
> > +	INTEL_VGA_DEVICE(0x5912, info), /* DT  GT2 */ \
> > +	INTEL_VGA_DEVICE(0x591B, info), /* Halo GT2 */ \
> > +	INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \
> > +	INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */
> > +
> > +#define INTEL_KBL_GT3_IDS(info) \
> > +	INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \
> > +	INTEL_VGA_DEVICE(0x592B, info), /* Halo GT3 */ \
> > +	INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */
> > +
> > +#define INTEL_KBL_IDS(info) \
> > +	INTEL_KBL_GT1_IDS(info), \
> > +	INTEL_KBL_GT2_IDS(info), \
> > +	INTEL_KBL_GT3_IDS(info)
> > +
> >  #endif /* _I915_PCIIDS_H */
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

[Vivi, Rodrigo]

On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
> On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Kabylake is gen 9.5 derivated from Skylake H0 stepping.
> > 
> > So we don't need pre-production Skylake workaround and also
> > firmware loading will use SKL H0 offsets.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 7374a0d..580c005 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
> >  })
> >  #define INTEL_INFO(p) 	(&__I915__(p)->info)
> >  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
> > -#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
> >  
> >  #define IS_I830(dev)		(INTEL_DEVID(dev) == 0x3577)
> >  #define IS_845G(dev)		(INTEL_DEVID(dev) == 0x2562)
> > @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
> >  
> >  #define IS_PRELIMINARY_HW(intel_info) ((intel_info)
> > ->is_preliminary)
> >  
> > +#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision + 
> > \
> > +			 IS_KABYLAKE(p) ? 7 : 0)
> > +
> 
> I am not fond of this at all. It will be really confusing that
> ->revision is different from INTEL_REVID when checking the 
> workarounds,
> and that you'll be using SKL_REVID_* to match KBL revision
> ids. 

this is exactly one of the reasons why I did this sum in this way so
they never match...

> Additionally, we'll probably want to start removing SKL workarounds
> before KBL workarounds.

I believe this is another discussion... On HSW BDW I remember I was
removing old Wa as it was no longer needed, but on SKL I saw this REVID
and I believed the idea was to let them there since some devs might be
using preliminary platforms yet for other reasons... I don't see a
problem of letting the old W/a there.

> 
> Others may disagree, but I'd like KBL revid checks be different from
> SKL.
> 
> >  #define SKL_REVID_A0		(0x0)
> >  #define SKL_REVID_B0		(0x1)
> >  #define SKL_REVID_C0		(0x2)
> > @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
> >  #define SKL_REVID_E0		(0x4)
> >  #define SKL_REVID_F0		(0x5)
> >  
> > +/* KBL A0 is based on SKL H0 */
> > +#define KBL_REVID_A0		(0x7)
> 
> You can't compare this against INTEL_REVID() now can you...? Or is 
> this
> not the one in the spec? Confused already.

Yes, this is confusing indeed. It seems that we have many levels of
steppings (according to platform guys) and this platform stepping
returning 0 is our KBL A0, but this correspond to our internal gpu
stepping H0 (same going to skl h0).

Like dmc firmware loading for instance we need to load the firmware for
stepping 7.

So yes, this definition matches BSPec KBL A0.

> 
> BR,
> Jani.
> 
> > +
> >  #define BXT_REVID_A0		(0x0)
> >  #define BXT_REVID_B0		(0x3)
> >  #define BXT_REVID_C0		(0x9)
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 12:09:17PM +0300, Jani Nikula wrote:
> On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > From: Deepak S <deepak.s@intel.com>
> >
> > v2: separate out device info into different GT (Damien)
> > v3: Add is_kabylake to the KBL gt3 structuer (Damien)
> >     Sort the platforms in older -> newer order (Damien)
> >
> > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > Signed-off-by: Deepak S <deepak.s@intel.com>
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 33 ++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
> >  include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
> >  3 files changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1cb6b82..f42102d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -394,6 +394,34 @@ static const struct intel_device_info intel_broxton_info = {
> >  	IVB_CURSOR_OFFSETS,
> >  };
> >  
> > +static const struct intel_device_info intel_kabylake_info = {
> > +	.is_preliminary = 1,
> > +	.is_skylake = 1,
> 
> Now's the time to call the shots, is this really a good idea or not? See
> VLV vs. CHV, we (okay, the royal we) still confuse ourselves with
> IS_VALLEYVIEW.
> 
> Granted, 74 call sites for IS_SKYLAKE(), all of those would need to be
> patched. We'd need something like "is skylake family" including SKL and
> KBL.

That would be just gen>=9 in most case, with potentially BXT handled
first as the special case. So maybe not much better than IS_SKYLAKE
being true for KBL.

Maybe it's best to just have IS_KABYLAKE and go through all the
IS_SKYLAKE checks and add BKL where needed (or change to gen>=9 if
that's OK).

I'm pretty sure I once did a patch to split IS_CHERRYVIEW from
IS_VALLEYVIEW, but I'm not sure where I stashed the work. Maybe I
should dig it up to get rid of the bad example?

> Some of them might be changed to be more like feature flags, which
> is something we've decided we need to do more anyway.

I've also thought about adding a chipset/platform enum or something where
the platforms would sit in some sort of logical order. So mostly a .gen
except we could actually tell apart the .5 gens and whatnot. Might make
the code a bit nicer since we would say eg. '>= HSW' instead of
'gen >= 8 || HSW', but maybe it wouldn't do much else for us. VLV/CHV
would still be fairly problematic since the display gen is so different
from the gt gen.

> 
> BR,
> Jani.
> 
> 
> > +	.is_kabylake = 1,
> > +	.gen = 9, .num_pipes = 3,
> > +	.need_gfx_hws = 1, .has_hotplug = 1,
> > +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> > +	.has_llc = 1,
> > +	.has_ddi = 1,
> > +	.has_fbc = 1,
> > +	GEN_DEFAULT_PIPEOFFSETS,
> > +	IVB_CURSOR_OFFSETS,
> > +};
> > +
> > +static const struct intel_device_info intel_kabylake_gt3_info = {
> > +	.is_preliminary = 1,
> > +	.is_skylake = 1,
> > +	.is_kabylake = 1,
> > +	.gen = 9, .num_pipes = 3,
> > +	.need_gfx_hws = 1, .has_hotplug = 1,
> > +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> > +	.has_llc = 1,
> > +	.has_ddi = 1,
> > +	.has_fbc = 1,
> > +	GEN_DEFAULT_PIPEOFFSETS,
> > +	IVB_CURSOR_OFFSETS,
> > +};
> > +
> >  /*
> >   * Make sure any device matches here are from most specific to most
> >   * general.  For example, since the Quanta match is based on the subsystem
> > @@ -434,7 +462,10 @@ static const struct intel_device_info intel_broxton_info = {
> >  	INTEL_SKL_GT1_IDS(&intel_skylake_info),	\
> >  	INTEL_SKL_GT2_IDS(&intel_skylake_info),	\
> >  	INTEL_SKL_GT3_IDS(&intel_skylake_gt3_info),	\
> > -	INTEL_BXT_IDS(&intel_broxton_info)
> > +	INTEL_BXT_IDS(&intel_broxton_info),		\
> > +	INTEL_KBL_GT1_IDS(&intel_kabylake_info),	\
> > +	INTEL_KBL_GT2_IDS(&intel_kabylake_info),	\
> > +	INTEL_KBL_GT3_IDS(&intel_kabylake_gt3_info)
> >  
> >  static const struct pci_device_id pciidlist[] = {		/* aka */
> >  	INTEL_PCI_IDS,
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 824e724..f7e9d7e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -765,6 +765,7 @@ struct intel_csr {
> >  	func(is_valleyview) sep \
> >  	func(is_haswell) sep \
> >  	func(is_skylake) sep \
> > +	func(is_kabylake) sep \
> >  	func(is_preliminary) sep \
> >  	func(has_fbc) sep \
> >  	func(has_pipe_cxsr) sep \
> > @@ -2464,6 +2465,7 @@ struct drm_i915_cmd_table {
> >  #define IS_BROADWELL(dev)	(!INTEL_INFO(dev)->is_valleyview && IS_GEN8(dev))
> >  #define IS_SKYLAKE(dev)	(INTEL_INFO(dev)->is_skylake)
> >  #define IS_BROXTON(dev)	(!INTEL_INFO(dev)->is_skylake && IS_GEN9(dev))
> > +#define IS_KABYLAKE(dev)	(INTEL_INFO(dev)->is_kabylake)
> >  #define IS_MOBILE(dev)		(INTEL_INFO(dev)->is_mobile)
> >  #define IS_HSW_EARLY_SDV(dev)	(IS_HASWELL(dev) && \
> >  				 (INTEL_DEVID(dev) & 0xFF00) == 0x0C00)
> > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> > index 17c4456..2e7a159 100644
> > --- a/include/drm/i915_pciids.h
> > +++ b/include/drm/i915_pciids.h
> > @@ -291,4 +291,33 @@
> >  	INTEL_VGA_DEVICE(0x1A84, info), \
> >  	INTEL_VGA_DEVICE(0x5A84, info)
> >  
> > +#define INTEL_KBL_GT1_IDS(info)	\
> > +	INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \
> > +	INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \
> > +	INTEL_VGA_DEVICE(0x5917, info), /* DT  GT1.5 */ \
> > +	INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \
> > +	INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \
> > +	INTEL_VGA_DEVICE(0x5902, info), /* DT  GT1 */ \
> > +	INTEL_VGA_DEVICE(0x590B, info), /* Halo GT1 */ \
> > +	INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */
> > +
> > +#define INTEL_KBL_GT2_IDS(info)	\
> > +	INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \
> > +	INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \
> > +	INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \
> > +	INTEL_VGA_DEVICE(0x5912, info), /* DT  GT2 */ \
> > +	INTEL_VGA_DEVICE(0x591B, info), /* Halo GT2 */ \
> > +	INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \
> > +	INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */
> > +
> > +#define INTEL_KBL_GT3_IDS(info) \
> > +	INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \
> > +	INTEL_VGA_DEVICE(0x592B, info), /* Halo GT3 */ \
> > +	INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */
> > +
> > +#define INTEL_KBL_IDS(info) \
> > +	INTEL_KBL_GT1_IDS(info), \
> > +	INTEL_KBL_GT2_IDS(info), \
> > +	INTEL_KBL_GT3_IDS(info)
> > +
> >  #endif /* _I915_PCIIDS_H */
> > -- 
> > 2.4.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 3819 bytes --]

cc'ing Ben to get his opinion...

On Tue, Oct 6, 2015 at 10:43 AM Vivi, Rodrigo <rodrigo.vivi@intel.com>
wrote:

> On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
> > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > Kabylake is gen 9.5 derivated from Skylake H0 stepping.
> > >
> > > So we don't need pre-production Skylake workaround and also
> > > firmware loading will use SKL H0 offsets.
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7374a0d..580c005 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
> > >  })
> > >  #define INTEL_INFO(p)      (&__I915__(p)->info)
> > >  #define INTEL_DEVID(p)     (INTEL_INFO(p)->device_id)
> > > -#define INTEL_REVID(p)     (__I915__(p)->dev->pdev->revision)
> > >
> > >  #define IS_I830(dev)               (INTEL_DEVID(dev) == 0x3577)
> > >  #define IS_845G(dev)               (INTEL_DEVID(dev) == 0x2562)
> > > @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
> > >
> > >  #define IS_PRELIMINARY_HW(intel_info) ((intel_info)
> > > ->is_preliminary)
> > >
> > > +#define INTEL_REVID(p)     (__I915__(p)->dev->pdev->revision +
> > > \
> > > +                    IS_KABYLAKE(p) ? 7 : 0)
> > > +
> >
> > I am not fond of this at all. It will be really confusing that
> > ->revision is different from INTEL_REVID when checking the
> > workarounds,
> > and that you'll be using SKL_REVID_* to match KBL revision
> > ids.
>
> this is exactly one of the reasons why I did this sum in this way so
> they never match...
>
> > Additionally, we'll probably want to start removing SKL workarounds
> > before KBL workarounds.
>
> I believe this is another discussion... On HSW BDW I remember I was
> removing old Wa as it was no longer needed, but on SKL I saw this REVID
> and I believed the idea was to let them there since some devs might be
> using preliminary platforms yet for other reasons... I don't see a
> problem of letting the old W/a there.
>
> >
> > Others may disagree, but I'd like KBL revid checks be different from
> > SKL.
> >
> > >  #define SKL_REVID_A0               (0x0)
> > >  #define SKL_REVID_B0               (0x1)
> > >  #define SKL_REVID_C0               (0x2)
> > > @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
> > >  #define SKL_REVID_E0               (0x4)
> > >  #define SKL_REVID_F0               (0x5)
> > >
> > > +/* KBL A0 is based on SKL H0 */
> > > +#define KBL_REVID_A0               (0x7)
> >
> > You can't compare this against INTEL_REVID() now can you...? Or is
> > this
> > not the one in the spec? Confused already.
>
> Yes, this is confusing indeed. It seems that we have many levels of
> steppings (according to platform guys) and this platform stepping
> returning 0 is our KBL A0, but this correspond to our internal gpu
> stepping H0 (same going to skl h0).
>
> Like dmc firmware loading for instance we need to load the firmware for
> stepping 7.
>
> So yes, this definition matches BSPec KBL A0.
>
> >
> > BR,
> > Jani.
> >
> > > +
> > >  #define BXT_REVID_A0               (0x0)
> > >  #define BXT_REVID_B0               (0x3)
> > >  #define BXT_REVID_C0               (0x9)
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 5420 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

[Ben Widawsky]

On Tue, Oct 06, 2015 at 08:51:13PM +0000, Rodrigo Vivi wrote:
> cc'ing Ben to get his opinion...
> 

Of course anything is possible wrt the delta of KBL features vs SKL. With the
knowledge we have, we can make a pretty educated guess that there will be no
changes, and with an equally high level of confidence say that if there are
changes, they will be very minor and self contained.

I am in favor of this minimalistic patch myself. I think both the result, and
reduced amount of churn make this patch favorable to the requested alternative.
Some finer comments below.

> On Tue, Oct 6, 2015 at 10:43 AM Vivi, Rodrigo <rodrigo.vivi@intel.com>
> wrote:
> 
> > On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
> > > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > > Kabylake is gen 9.5 derivated from Skylake H0 stepping.

Don't call it 9.5... some people don't like that... Just say it's a SKL
derivative.

> > > >
> > > > So we don't need pre-production Skylake workaround and also
> > > > firmware loading will use SKL H0 offsets.

In fact we know some of these workarounds to be harmful.

> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 7374a0d..580c005 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
> > > >  })
> > > >  #define INTEL_INFO(p)      (&__I915__(p)->info)
> > > >  #define INTEL_DEVID(p)     (INTEL_INFO(p)->device_id)
> > > > -#define INTEL_REVID(p)     (__I915__(p)->dev->pdev->revision)
> > > >
> > > >  #define IS_I830(dev)               (INTEL_DEVID(dev) == 0x3577)
> > > >  #define IS_845G(dev)               (INTEL_DEVID(dev) == 0x2562)
> > > > @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
> > > >
> > > >  #define IS_PRELIMINARY_HW(intel_info) ((intel_info)
> > > > ->is_preliminary)
> > > >
> > > > +#define INTEL_REVID(p)     (__I915__(p)->dev->pdev->revision +
> > > > \
> > > > +                    IS_KABYLAKE(p) ? 7 : 0)
> > > > +
> > >
> > > I am not fond of this at all. It will be really confusing that
> > > ->revision is different from INTEL_REVID when checking the
> > > workarounds,
> > > and that you'll be using SKL_REVID_* to match KBL revision
> > > ids.
> >
> > this is exactly one of the reasons why I did this sum in this way so
> > they never match...
> >

Jani, I do understand your distaste with this patch. However, I think this is a
very reasonable, and more importantly, readable wart in the code. It's very
obvious what and why the macro does what it does.

> > > Additionally, we'll probably want to start removing SKL workarounds
> > > before KBL workarounds.
> >

I don't see a reason for this. Maybe you have thought of something I haven't?
We're not using any of the early SKL workarounds on KBL as a result of this
patch, so it shouldn't matter.

> > I believe this is another discussion... On HSW BDW I remember I was
> > removing old Wa as it was no longer needed, but on SKL I saw this REVID
> > and I believed the idea was to let them there since some devs might be
> > using preliminary platforms yet for other reasons... I don't see a
> > problem of letting the old W/a there.
> >

I'm very much in favor of killing pre-production support (not that anyone
asked).

> > >
> > > Others may disagree, but I'd like KBL revid checks be different from
> > > SKL.
> > >
> > > >  #define SKL_REVID_A0               (0x0)
> > > >  #define SKL_REVID_B0               (0x1)
> > > >  #define SKL_REVID_C0               (0x2)
> > > > @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
> > > >  #define SKL_REVID_E0               (0x4)
> > > >  #define SKL_REVID_F0               (0x5)
> > > >
> > > > +/* KBL A0 is based on SKL H0 */
> > > > +#define KBL_REVID_A0               (0x7)
> > >
> > > You can't compare this against INTEL_REVID() now can you...? Or is
> > > this
> > > not the one in the spec? Confused already.
> >
> > Yes, this is confusing indeed. It seems that we have many levels of
> > steppings (according to platform guys) and this platform stepping
> > returning 0 is our KBL A0, but this correspond to our internal gpu
> > stepping H0 (same going to skl h0).
> >
> > Like dmc firmware loading for instance we need to load the firmware for
> > stepping 7.
> >
> > So yes, this definition matches BSPec KBL A0.
> >

Maybe amend the comment to say that the actual PCI header has revid 0 (or
whatever it was). With that, it's pretty clear - and yes, it is a value which
can and should be used to compare with INTEL_REVID, but as stated above, the
comparison isn't needed, and if/when it is, we can/should revisit the more
intrusive change you're suggesting.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

[Jani Nikula]

On Wed, 07 Oct 2015, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Tue, Oct 06, 2015 at 08:51:13PM +0000, Rodrigo Vivi wrote:
>> cc'ing Ben to get his opinion...
>> 
>
> Of course anything is possible wrt the delta of KBL features vs SKL. With the
> knowledge we have, we can make a pretty educated guess that there will be no
> changes, and with an equally high level of confidence say that if there are
> changes, they will be very minor and self contained.
>
> I am in favor of this minimalistic patch myself. I think both the result, and
> reduced amount of churn make this patch favorable to the requested alternative.
> Some finer comments below.
>
>> On Tue, Oct 6, 2015 at 10:43 AM Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> wrote:
>> 
>> > On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
>> > > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > > > Kabylake is gen 9.5 derivated from Skylake H0 stepping.
>
> Don't call it 9.5... some people don't like that... Just say it's a SKL
> derivative.
>
>> > > >
>> > > > So we don't need pre-production Skylake workaround and also
>> > > > firmware loading will use SKL H0 offsets.
>
> In fact we know some of these workarounds to be harmful.
>
>> > > >
>> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
>> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > > > b/drivers/gpu/drm/i915/i915_drv.h
>> > > > index 7374a0d..580c005 100644
>> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > > @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
>> > > >  })
>> > > >  #define INTEL_INFO(p)      (&__I915__(p)->info)
>> > > >  #define INTEL_DEVID(p)     (INTEL_INFO(p)->device_id)
>> > > > -#define INTEL_REVID(p)     (__I915__(p)->dev->pdev->revision)
>> > > >
>> > > >  #define IS_I830(dev)               (INTEL_DEVID(dev) == 0x3577)
>> > > >  #define IS_845G(dev)               (INTEL_DEVID(dev) == 0x2562)
>> > > > @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
>> > > >
>> > > >  #define IS_PRELIMINARY_HW(intel_info) ((intel_info)
>> > > > ->is_preliminary)
>> > > >
>> > > > +#define INTEL_REVID(p)     (__I915__(p)->dev->pdev->revision +
>> > > > \
>> > > > +                    IS_KABYLAKE(p) ? 7 : 0)
>> > > > +
>> > >
>> > > I am not fond of this at all. It will be really confusing that
>> > > ->revision is different from INTEL_REVID when checking the
>> > > workarounds,
>> > > and that you'll be using SKL_REVID_* to match KBL revision
>> > > ids.
>> >
>> > this is exactly one of the reasons why I did this sum in this way so
>> > they never match...
>> >
>
> Jani, I do understand your distaste with this patch. However, I think
> this is a very reasonable, and more importantly, readable wart in the
> code. It's very obvious what and why the macro does what it does.

INTEL_REVID() is supposed to be a shorthand for getting at
->dev->pdev->revision. I think it's rather surprising that on one
platform this would not hold. It's a clever trick, I'll give you that.

>> > > Additionally, we'll probably want to start removing SKL workarounds
>> > > before KBL workarounds.
>> >
>
> I don't see a reason for this. Maybe you have thought of something I haven't?
> We're not using any of the early SKL workarounds on KBL as a result of this
> patch, so it shouldn't matter.
>
>> > I believe this is another discussion... On HSW BDW I remember I was
>> > removing old Wa as it was no longer needed, but on SKL I saw this REVID
>> > and I believed the idea was to let them there since some devs might be
>> > using preliminary platforms yet for other reasons... I don't see a
>> > problem of letting the old W/a there.
>> >
>
> I'm very much in favor of killing pre-production support (not that anyone
> asked).

It was specifically dropping pre-production SKL workarounds I meant
above.

You might be interested in having a look at [1] I wrote to address the
revid/workaround part of the SKL vs. KBL discussion. With that, I think
we could keep INTEL_REVID() as it is, make IS_SKL_REVID() match only
SKL, and add IS_KBL_REVID() to match only KBL. As you say, most early
SKL workarounds don't impact KBL, so we wouldn't have to touch so many
places in the code to add KBL workarounds.

I also think that patch, incl. adding separate IS_KBL_REVID, is
orthogonal to whether IS_SKYLAKE() should match KBL or not.

[1] http://mid.gmane.org/1444131676-12187-3-git-send-email-jani.nikula@intel.com

>> > >
>> > > Others may disagree, but I'd like KBL revid checks be different from
>> > > SKL.
>> > >
>> > > >  #define SKL_REVID_A0               (0x0)
>> > > >  #define SKL_REVID_B0               (0x1)
>> > > >  #define SKL_REVID_C0               (0x2)
>> > > > @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
>> > > >  #define SKL_REVID_E0               (0x4)
>> > > >  #define SKL_REVID_F0               (0x5)
>> > > >
>> > > > +/* KBL A0 is based on SKL H0 */
>> > > > +#define KBL_REVID_A0               (0x7)
>> > >
>> > > You can't compare this against INTEL_REVID() now can you...? Or is
>> > > this
>> > > not the one in the spec? Confused already.
>> >
>> > Yes, this is confusing indeed. It seems that we have many levels of
>> > steppings (according to platform guys) and this platform stepping
>> > returning 0 is our KBL A0, but this correspond to our internal gpu
>> > stepping H0 (same going to skl h0).
>> >
>> > Like dmc firmware loading for instance we need to load the firmware for
>> > stepping 7.
>> >
>> > So yes, this definition matches BSPec KBL A0.
>> >
>
> Maybe amend the comment to say that the actual PCI header has revid 0 (or
> whatever it was). With that, it's pretty clear - and yes, it is a value which
> can and should be used to compare with INTEL_REVID, but as stated above, the
> comparison isn't needed, and if/when it is, we can/should revisit the more
> intrusive change you're suggesting.

I am still confused. I fear you may start thinking that no longer
supports my side of the argument. :(


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Daniel Vetter]

On Tue, Oct 06, 2015 at 05:31:30PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-10-06 at 12:09 +0300, Jani Nikula wrote:
> > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > From: Deepak S <deepak.s@intel.com>
> > > 
> > > v2: separate out device info into different GT (Damien)
> > > v3: Add is_kabylake to the KBL gt3 structuer (Damien)
> > >     Sort the platforms in older -> newer order (Damien)
> > > 
> > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > > Signed-off-by: Deepak S <deepak.s@intel.com>
> > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 33 
> > > ++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
> > >  include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
> > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 1cb6b82..f42102d 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -394,6 +394,34 @@ static const struct intel_device_info 
> > > intel_broxton_info = {
> > >  	IVB_CURSOR_OFFSETS,
> > >  };
> > >  
> > > +static const struct intel_device_info intel_kabylake_info = {
> > > +	.is_preliminary = 1,
> > > +	.is_skylake = 1,
> > 
> > Now's the time to call the shots, is this really a good idea or not? 
> > See
> > VLV vs. CHV, we (okay, the royal we) still confuse ourselves with
> > IS_VALLEYVIEW.
> > 
> > Granted, 74 call sites for IS_SKYLAKE(), all of those would need to 
> > be
> > patched. We'd need something like "is skylake family" including SKL 
> > and
> > KBL. Some of them might be changed to be more like feature flags, 
> > which
> > is something we've decided we need to do more anyway.
> 
> To be honest I also don't like this approach as I didn't like the chv
> is vlv one. I always get confused and I thought about changing
> everything many times during last week, but as you pointed out it would
> be a change in many entries and not sure what others opinion was since
> I never saw any complain about this patch before neither the vlv-chv
> ones... 
> 
> But if you and Daniel prefer I can do a
> s/IS_SKYLAKE(dev)/IS_KABYLAKE(dev) || IS_SKYLAKE(dev)
> Just let me know...
> 
> The IS_SKYLAKE_FAMILY I don't like much because it will still confuse
> people because in ark they will probably be 2 different "families" if
> they continue with the current scheme they are using so far.

IS_SKYLAKE_FAMILY == IS_GEN9 pretty much everywhere. The exceptions (where
bxt is special) are probably worth listing explicitly anyway. So yeah
IS_KABYLAKE.

btw the original reasons for putting chv into vlv was that what we really
wanted is HAS_GMCH but getting there was a lot more work. At least that's
how I remember that particular story ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/kbl: Add Kabylake PCI ID

[Ville Syrjälä]

On Wed, Oct 07, 2015 at 03:55:31PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 05:31:30PM +0000, Vivi, Rodrigo wrote:
> > On Tue, 2015-10-06 at 12:09 +0300, Jani Nikula wrote:
> > > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > > From: Deepak S <deepak.s@intel.com>
> > > > 
> > > > v2: separate out device info into different GT (Damien)
> > > > v3: Add is_kabylake to the KBL gt3 structuer (Damien)
> > > >     Sort the platforms in older -> newer order (Damien)
> > > > 
> > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > > > Signed-off-by: Deepak S <deepak.s@intel.com>
> > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 33 
> > > > ++++++++++++++++++++++++++++++++-
> > > >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
> > > >  include/drm/i915_pciids.h       | 29 +++++++++++++++++++++++++++++
> > > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 1cb6b82..f42102d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -394,6 +394,34 @@ static const struct intel_device_info 
> > > > intel_broxton_info = {
> > > >  	IVB_CURSOR_OFFSETS,
> > > >  };
> > > >  
> > > > +static const struct intel_device_info intel_kabylake_info = {
> > > > +	.is_preliminary = 1,
> > > > +	.is_skylake = 1,
> > > 
> > > Now's the time to call the shots, is this really a good idea or not? 
> > > See
> > > VLV vs. CHV, we (okay, the royal we) still confuse ourselves with
> > > IS_VALLEYVIEW.
> > > 
> > > Granted, 74 call sites for IS_SKYLAKE(), all of those would need to 
> > > be
> > > patched. We'd need something like "is skylake family" including SKL 
> > > and
> > > KBL. Some of them might be changed to be more like feature flags, 
> > > which
> > > is something we've decided we need to do more anyway.
> > 
> > To be honest I also don't like this approach as I didn't like the chv
> > is vlv one. I always get confused and I thought about changing
> > everything many times during last week, but as you pointed out it would
> > be a change in many entries and not sure what others opinion was since
> > I never saw any complain about this patch before neither the vlv-chv
> > ones... 
> > 
> > But if you and Daniel prefer I can do a
> > s/IS_SKYLAKE(dev)/IS_KABYLAKE(dev) || IS_SKYLAKE(dev)
> > Just let me know...
> > 
> > The IS_SKYLAKE_FAMILY I don't like much because it will still confuse
> > people because in ark they will probably be 2 different "families" if
> > they continue with the current scheme they are using so far.
> 
> IS_SKYLAKE_FAMILY == IS_GEN9 pretty much everywhere. The exceptions (where
> bxt is special) are probably worth listing explicitly anyway. So yeah
> IS_KABYLAKE.
> 
> btw the original reasons for putting chv into vlv was that what we really
> wanted is HAS_GMCH but getting there was a lot more work. At least that's
> how I remember that particular story ;-)

IIRC I left it like that mainly to minimize the diff between upstream
and internal when chv was still living there. And I could never come up
with any good IS_<whatever> name to cover both platforms, so didn't do
such a change upfront in upstream.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx