[PATCH 0/5] Decode memdev info and bandwidth and implemnt latency WA

[PATCH 0/5] Decode memdev info and bandwidth and implemnt latency WA

[Mahesh Kumar]

This series adds support to calculate system memdev parameters and calculate
total system memory bandwidth. This parameters and BW will be used to enable
WM level-0 latency workaround and display memory bandwidth related WA for gen9.

while we are here to enable IPC based on memory configuration lets also 
make sure that we override IPC value set of BIOS incase we decide to
disable IPC in KMS.

Mahesh Kumar (5):
  drm/i915/bxt: Decode memory bandwidth and parameters
  drm/i915/skl+: Decode memory bandwidth and parameters
  drm/i915: Implement 16GB dimm wa for latency level-0
  drm/i915/kbl+: Enable IPC only for symmetric memory configurations
  drm/i915/skl+: don't trust IPC value set by BIOS

 drivers/gpu/drm/i915/i915_drv.c | 308 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  22 +++
 drivers/gpu/drm/i915/i915_reg.h |  51 +++++++
 drivers/gpu/drm/i915/intel_pm.c |  17 ++-
 4 files changed, 395 insertions(+), 3 deletions(-)

-- 
2.16.2

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

[PATCH 1/5] drm/i915/bxt: Decode memory bandwidth and parameters

[Mahesh Kumar]

This patch adds support to decode system memory bandwidth and other
parameters for broxton platform, which will be used for arbitrated
display memory bandwidth calculation in GEN9 based platforms and
WM latency level-0 Work-around calculation on GEN9+ platforms.

Changes since V1:
 - s/memdev_info/dram_info

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 116 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  11 ++++
 drivers/gpu/drm/i915/i915_reg.h |  30 +++++++++++
 3 files changed, 157 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 18a45e7a3d7c..16629601c9f4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1070,6 +1070,116 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
+static int
+bxt_get_dram_info(struct drm_i915_private *dev_priv)
+{
+	struct dram_info *dram_info = &dev_priv->dram_info;
+	u32 dram_channels;
+	u32 mem_freq_khz, val;
+	u8 num_active_channels;
+	int i;
+
+	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
+	mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) *
+				    BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
+
+	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
+					BXT_DRAM_CHANNEL_ACTIVE_MASK;
+	num_active_channels = hweight32(dram_channels);
+
+	/* Each active bit represents 4-byte channel */
+	dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
+
+	if (dram_info->bandwidth_kbps == 0) {
+		DRM_INFO("Couldn't get system memory bandwidth\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
+	 */
+	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
+		u8 rank, size, width;
+		enum dram_rank ch_rank;
+
+		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
+		if (val == 0xFFFFFFFF)
+			continue;
+
+		dram_info->num_channels++;
+		rank = val & BXT_DRAM_RANK_MASK;
+		width = (val >> BXT_DRAM_WIDTH_SHIFT) & BXT_DRAM_WIDTH_MASK;
+		size = (val >> BXT_DRAM_SIZE_SHIFT) & BXT_DRAM_SIZE_MASK;
+		if (rank == BXT_DRAM_RANK_SINGLE)
+			ch_rank = I915_DRAM_RANK_SINGLE;
+		else if (rank == BXT_DRAM_RANK_DUAL)
+			ch_rank = I915_DRAM_RANK_DUAL;
+		else
+			ch_rank = I915_DRAM_RANK_INVALID;
+
+		if (size == BXT_DRAM_SIZE_4GB)
+			size = 4;
+		else if (size == BXT_DRAM_SIZE_6GB)
+			size = 6;
+		else if (size == BXT_DRAM_SIZE_8GB)
+			size = 8;
+		else if (size == BXT_DRAM_SIZE_12GB)
+			size = 12;
+		else if (size == BXT_DRAM_SIZE_16GB)
+			size = 16;
+		else
+			size = 0;
+
+		width = (1 << width) * 8;
+		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
+			      width, rank == BXT_DRAM_RANK_SINGLE ? "single" :
+			      rank == BXT_DRAM_RANK_DUAL ? "dual" : "unknown");
+
+		/*
+		 * If any of the channel is single rank channel,
+		 * worst case output will be same as if single rank
+		 * memory, so consider single rank memory.
+		 */
+		if (dram_info->rank == I915_DRAM_RANK_INVALID)
+			dram_info->rank = ch_rank;
+		else if (ch_rank == I915_DRAM_RANK_SINGLE)
+			dram_info->rank = I915_DRAM_RANK_SINGLE;
+	}
+
+	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
+		DRM_INFO("couldn't get memory rank information\n");
+		return -EINVAL;
+	}
+
+	dram_info->valid = true;
+	return 0;
+}
+
+static void
+intel_get_dram_info(struct drm_i915_private *dev_priv)
+{
+	struct dram_info *dram_info = &dev_priv->dram_info;
+	int ret;
+
+	dram_info->valid = false;
+	dram_info->rank = I915_DRAM_RANK_INVALID;
+	dram_info->bandwidth_kbps = 0;
+	dram_info->num_channels = 0;
+
+	if (!IS_BROXTON(dev_priv))
+		return;
+
+	ret = bxt_get_dram_info(dev_priv);
+	if (ret)
+		return;
+
+	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
+		      dram_info->bandwidth_kbps, dram_info->num_channels);
+	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
+		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
+		      "dual" : "single");
+}
+
 /**
  * i915_driver_init_hw - setup state requiring device access
  * @dev_priv: device private
@@ -1187,6 +1297,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 		goto err_msi;
 
 	intel_opregion_setup(dev_priv);
+	/*
+	 * Fill the dram structure to get the system raw bandwidth and
+	 * dram info. This will be used for memory latency calculation.
+	 */
+	intel_get_dram_info(dev_priv);
+
 
 	return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f49f9988dfa..46f942fa7d60 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1904,6 +1904,17 @@ struct drm_i915_private {
 		bool distrust_bios_wm;
 	} wm;
 
+	struct dram_info {
+		bool valid;
+		u8 num_channels;
+		enum dram_rank {
+			I915_DRAM_RANK_INVALID = 0,
+			I915_DRAM_RANK_SINGLE,
+			I915_DRAM_RANK_DUAL
+		} rank;
+		u32 bandwidth_kbps;
+	} dram_info;
+
 	struct i915_runtime_pm runtime_pm;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5530c470f30d..66900d027570 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9634,6 +9634,36 @@ enum skl_power_gate {
 #define  DC_STATE_DEBUG_MASK_CORES	(1 << 0)
 #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1 << 1)
 
+#define BXT_P_CR_MC_BIOS_REQ_0_0_0	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114)
+#define  BXT_REQ_DATA_MASK			0x3F
+#define  BXT_DRAM_CHANNEL_ACTIVE_SHIFT		12
+#define  BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF
+#define  BXT_MEMORY_FREQ_MULTIPLIER_HZ		133333333
+
+#define BXT_D_CR_DRP0_DUNIT8			0x1000
+#define BXT_D_CR_DRP0_DUNIT9			0x1200
+#define  BXT_D_CR_DRP0_DUNIT_START		8
+#define  BXT_D_CR_DRP0_DUNIT_END		11
+#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO(MCHBAR_MIRROR_BASE_SNB + \
+				      _PICK_EVEN((x) - 8, BXT_D_CR_DRP0_DUNIT8,\
+						 BXT_D_CR_DRP0_DUNIT9))
+#define  BXT_DRAM_RANK_MASK			0x3
+#define  BXT_DRAM_RANK_SINGLE			0x1
+#define  BXT_DRAM_RANK_DUAL			0x3
+#define  BXT_DRAM_WIDTH_MASK			0x3
+#define  BXT_DRAM_WIDTH_SHIFT			4
+#define  BXT_DRAM_WIDTH_X8			0x0
+#define  BXT_DRAM_WIDTH_X16			0x1
+#define  BXT_DRAM_WIDTH_X32			0x2
+#define  BXT_DRAM_WIDTH_X64			0x3
+#define  BXT_DRAM_SIZE_MASK			0x7
+#define  BXT_DRAM_SIZE_SHIFT			6
+#define  BXT_DRAM_SIZE_4GB			0x0
+#define  BXT_DRAM_SIZE_6GB			0x1
+#define  BXT_DRAM_SIZE_8GB			0x2
+#define  BXT_DRAM_SIZE_12GB			0x3
+#define  BXT_DRAM_SIZE_16GB			0x4
+
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
  * since on HSW we can't write to it using I915_WRITE. */
 #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
-- 
2.16.2

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

[PATCH 2/5] drm/i915/skl+: Decode memory bandwidth and parameters

[Mahesh Kumar]

This patch adds support to decode system memory bandwidth and other
parameters for skylake and Gen9+ platforms, which will be used for
arbitrated display memory bandwidth calculation in GEN9 based
platforms and WM latency level-0 Work-around calculation on GEN9+.

Changes Since V1:
 - s/memdev_info/dram_info
 - create a struct to hold channel info

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 131 ++++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h |   7 +++
 drivers/gpu/drm/i915/i915_reg.h |  21 +++++++
 3 files changed, 155 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 16629601c9f4..ddf6bf9b500a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1070,6 +1070,118 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
+static int
+skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
+{
+	u8 l_rank, s_rank;
+	u8 l_size, s_size;
+	u8 l_width, s_width;
+	enum dram_rank rank;
+
+	if (!val)
+		return -1;
+
+	l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & SKL_DRAM_SIZE_MASK;
+	s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & SKL_DRAM_SIZE_MASK;
+	l_width = (val >> SKL_DRAM_WIDTH_L_SHIFT) & SKL_DRAM_WIDTH_MASK;
+	s_width = (val >> SKL_DRAM_WIDTH_S_SHIFT) & SKL_DRAM_WIDTH_MASK;
+	l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & SKL_DRAM_RANK_MASK;
+	s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & SKL_DRAM_RANK_MASK;
+
+	if (l_size == 0 && s_size == 0)
+		return -1;
+
+	DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
+		      l_size, (1 << l_width) * 8, l_rank ? "dual" : "single",
+		      s_size, (1 << s_width) * 8, s_rank ? "dual" : "single");
+
+	if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == SKL_DRAM_RANK_DUAL)
+		rank = I915_DRAM_RANK_DUAL;
+	else if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) &&
+		 (s_size && s_rank == SKL_DRAM_RANK_SINGLE))
+		rank = I915_DRAM_RANK_DUAL;
+	else
+		rank = I915_DRAM_RANK_SINGLE;
+
+	ch->l_info.size = l_size;
+	ch->s_info.size = s_size;
+	ch->l_info.width = l_width;
+	ch->s_info.width = s_width;
+	ch->l_info.rank = l_rank;
+	ch->s_info.rank = s_rank;
+	ch->rank = rank;
+
+	return 0;
+}
+
+static int
+skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
+{
+	struct dram_info *dram_info = &dev_priv->dram_info;
+	struct dram_channel_info ch0, ch1;
+	u32 val;
+	int ret;
+
+	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
+	ret = skl_dram_get_channel_info(&ch0, val);
+	if (ret == 0)
+		dram_info->num_channels++;
+
+	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
+	ret = skl_dram_get_channel_info(&ch1, val);
+	if (ret == 0)
+		dram_info->num_channels++;
+
+	if (dram_info->num_channels == 0) {
+		DRM_INFO("Number of memory channels is zero\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * If any of the channel is single rank channel, worst case output
+	 * will be same as if single rank memory, so consider single rank
+	 * memory.
+	 */
+	if (ch0.rank == I915_DRAM_RANK_SINGLE ||
+	    ch1.rank == I915_DRAM_RANK_SINGLE)
+		dram_info->rank = I915_DRAM_RANK_SINGLE;
+	else
+		dram_info->rank = max(ch0.rank, ch1.rank);
+
+	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
+		DRM_INFO("couldn't get memory rank information\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int
+skl_get_dram_info(struct drm_i915_private *dev_priv)
+{
+	struct dram_info *dram_info = &dev_priv->dram_info;
+	u32 mem_freq_khz, val;
+	int ret;
+
+	ret = skl_dram_get_channels_info(dev_priv);
+	if (ret)
+		return ret;
+
+	val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
+	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
+				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
+
+	dram_info->bandwidth_kbps = dram_info->num_channels *
+							mem_freq_khz * 8;
+
+	if (dram_info->bandwidth_kbps == 0) {
+		DRM_INFO("Couldn't get system memory bandwidth\n");
+		return -EINVAL;
+	}
+
+	dram_info->valid = true;
+	return 0;
+}
+
 static int
 bxt_get_dram_info(struct drm_i915_private *dev_priv)
 {
@@ -1159,6 +1271,7 @@ static void
 intel_get_dram_info(struct drm_i915_private *dev_priv)
 {
 	struct dram_info *dram_info = &dev_priv->dram_info;
+	char bandwidth_str[32];
 	int ret;
 
 	dram_info->valid = false;
@@ -1166,15 +1279,25 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 	dram_info->bandwidth_kbps = 0;
 	dram_info->num_channels = 0;
 
-	if (!IS_BROXTON(dev_priv))
+	if (INTEL_GEN(dev_priv) < 9)
 		return;
 
-	ret = bxt_get_dram_info(dev_priv);
+	/* Need to calculate bandwidth only for Gen9 */
+	if (IS_BROXTON(dev_priv))
+		ret = bxt_get_dram_info(dev_priv);
+	else if (INTEL_GEN(dev_priv) == 9)
+		ret = skl_get_dram_info(dev_priv);
+	else
+		ret = skl_dram_get_channels_info(dev_priv);
 	if (ret)
 		return;
 
-	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
-		      dram_info->bandwidth_kbps, dram_info->num_channels);
+	if (dram_info->bandwidth_kbps)
+		sprintf(bandwidth_str, "%d KBps", dram_info->bandwidth_kbps);
+	else
+		sprintf(bandwidth_str, "unknown");
+	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
+		      bandwidth_str, dram_info->num_channels);
 	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
 		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
 		      "dual" : "single");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 46f942fa7d60..2d12fc152b49 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2128,6 +2128,13 @@ struct drm_i915_private {
 	 */
 };
 
+struct dram_channel_info {
+	struct info {
+		u8 size, width, rank;
+	} l_info, s_info;
+	enum dram_rank rank;
+};
+
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
 {
 	return container_of(dev, struct drm_i915_private, drm);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 66900d027570..e4d61167fb64 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9664,6 +9664,27 @@ enum skl_power_gate {
 #define  BXT_DRAM_SIZE_12GB			0x3
 #define  BXT_DRAM_SIZE_16GB			0x4
 
+#define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
+#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
+#define  SKL_REQ_DATA_MASK			(0xF << 0)
+
+#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
+#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
+#define  SKL_DRAM_SIZE_MASK			0x3F
+#define  SKL_DRAM_SIZE_L_SHIFT			0
+#define  SKL_DRAM_SIZE_S_SHIFT			16
+#define  SKL_DRAM_WIDTH_MASK			0x3
+#define  SKL_DRAM_WIDTH_L_SHIFT			8
+#define  SKL_DRAM_WIDTH_S_SHIFT			24
+#define  SKL_DRAM_WIDTH_X8			0x0
+#define  SKL_DRAM_WIDTH_X16			0x1
+#define  SKL_DRAM_WIDTH_X32			0x2
+#define  SKL_DRAM_RANK_MASK			0x1
+#define  SKL_DRAM_RANK_L_SHIFT			10
+#define  SKL_DRAM_RANK_S_SHIFT			26
+#define  SKL_DRAM_RANK_SINGLE			0x0
+#define  SKL_DRAM_RANK_DUAL			0x1
+
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
  * since on HSW we can't write to it using I915_WRITE. */
 #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
-- 
2.16.2

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

[PATCH 3/5] drm/i915: Implement 16GB dimm wa for latency level-0

[Mahesh Kumar]

Memory with 16GB dimms require an increase of 1us in level-0 latency.
This patch implements the same.
Bspec: 4381

changes since V1:
 - s/memdev_info/dram_info
 - make skl_is_16gb_dimm pure function

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 40 ++++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h |  3 +++
 drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++++
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ddf6bf9b500a..86bc2e685522 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1070,6 +1070,25 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 	intel_gvt_sanitize_options(dev_priv);
 }
 
+static bool
+skl_is_16gb_dimm(u8 rank, u8 size, u8 width)
+{
+	if (size == 16 && width == SKL_DRAM_WIDTH_X8 &&
+	    rank == SKL_DRAM_RANK_SINGLE)
+		return true;
+	else if (size == 32 && width == SKL_DRAM_WIDTH_X8 &&
+		 rank == SKL_DRAM_RANK_DUAL)
+		return true;
+	else if (size == 8 && width == SKL_DRAM_WIDTH_X16 &&
+		 rank == SKL_DRAM_RANK_SINGLE)
+		return true;
+	else if (size == 16 && width == SKL_DRAM_WIDTH_X16 &&
+		 rank == SKL_DRAM_RANK_DUAL)
+		return true;
+
+	return false;
+}
+
 static int
 skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 {
@@ -1077,6 +1096,7 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 	u8 l_size, s_size;
 	u8 l_width, s_width;
 	enum dram_rank rank;
+	bool l_16gb_dimm, s_16gb_dimm;
 
 	if (!val)
 		return -1;
@@ -1103,6 +1123,13 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 	else
 		rank = I915_DRAM_RANK_SINGLE;
 
+	l_16gb_dimm = skl_is_16gb_dimm(l_rank, l_size, l_width);
+	s_16gb_dimm = skl_is_16gb_dimm(s_rank, s_size, s_width);
+
+	if (l_16gb_dimm || s_16gb_dimm)
+		ch->is_16gb_dimm = true;
+	else
+		ch->is_16gb_dimm = false;
 	ch->l_info.size = l_size;
 	ch->s_info.size = s_size;
 	ch->l_info.width = l_width;
@@ -1137,6 +1164,8 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 		return -EINVAL;
 	}
 
+	dram_info->valid_dimm = true;
+
 	/*
 	 * If any of the channel is single rank channel, worst case output
 	 * will be same as if single rank memory, so consider single rank
@@ -1152,6 +1181,10 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 		DRM_INFO("couldn't get memory rank information\n");
 		return -EINVAL;
 	}
+
+	if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
+		dram_info->is_16gb_dimm = true;
+
 	return 0;
 }
 
@@ -1263,6 +1296,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
 		return -EINVAL;
 	}
 
+	dram_info->valid_dimm = true;
 	dram_info->valid = true;
 	return 0;
 }
@@ -1275,6 +1309,8 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 	int ret;
 
 	dram_info->valid = false;
+	dram_info->valid_dimm = false;
+	dram_info->is_16gb_dimm = false;
 	dram_info->rank = I915_DRAM_RANK_INVALID;
 	dram_info->bandwidth_kbps = 0;
 	dram_info->num_channels = 0;
@@ -1298,9 +1334,9 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
 		sprintf(bandwidth_str, "unknown");
 	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
 		      bandwidth_str, dram_info->num_channels);
-	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
+	DRM_DEBUG_KMS("DRAM rank: %s rank 16GB-dimm:%s\n",
 		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
-		      "dual" : "single");
+		      "dual" : "single", yesno(dram_info->is_16gb_dimm));
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2d12fc152b49..854f3c828e01 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1906,6 +1906,8 @@ struct drm_i915_private {
 
 	struct dram_info {
 		bool valid;
+		bool valid_dimm;
+		bool is_16gb_dimm;
 		u8 num_channels;
 		enum dram_rank {
 			I915_DRAM_RANK_INVALID = 0,
@@ -2133,6 +2135,7 @@ struct dram_channel_info {
 		u8 size, width, rank;
 	} l_info, s_info;
 	enum dram_rank rank;
+	bool is_16gb_dimm;
 };
 
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7312ecb73415..2446f53adf21 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2874,6 +2874,19 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
 			}
 		}
 
+		/*
+		 * WA Level-0 adjustment for 16GB DIMMs: SKL+
+		 * If we could not get dimm info enable this WA to prevent from
+		 * any underrun. If not able to get Dimm info assume 16GB dimm
+		 * to avoid any underrun.
+		 */
+		if (dev_priv->dram_info.valid_dimm) {
+			if (dev_priv->dram_info.is_16gb_dimm)
+				wm[0] += 1;
+		} else {
+			wm[0] += 1;
+		}
+
 	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		uint64_t sskpd = I915_READ64(MCH_SSKPD);
 
-- 
2.16.2

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

[PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Mahesh Kumar]

IPC may cause underflows if not used with dual channel symmetric
memory configuration. Disable IPC for non symmetric configurations in
affected platforms.
Display WA #1141

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 43 ++++++++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c |  2 +-
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 86bc2e685522..2273664166bc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
 	return 0;
 }
 
+static bool
+intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
+			  u32 val_ch0, u32 val_ch1,
+			  struct dram_channel_info *ch0)
+{
+	/* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
+	if (INTEL_GEN(dev_priv) > 9 &&
+	    !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
+		return true;
+
+	if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
+		return true;
+
+	if (val_ch0 != val_ch1)
+		return false;
+
+	if (ch0->s_info.size == 0)
+		return true;
+	if (ch0->l_info.size == ch0->s_info.size &&
+	    ch0->l_info.width == ch0->s_info.width &&
+	    ch0->l_info.rank == ch0->s_info.rank)
+		return true;
+
+	return false;
+}
+
 static int
 skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 {
 	struct dram_info *dram_info = &dev_priv->dram_info;
 	struct dram_channel_info ch0, ch1;
-	u32 val;
+	u32 val_ch0, val_ch1;
 	int ret;
 
-	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch0, val);
+	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
+	ret = skl_dram_get_channel_info(&ch0, val_ch0);
 	if (ret == 0)
 		dram_info->num_channels++;
 
-	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
-	ret = skl_dram_get_channel_info(&ch1, val);
+	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
+	ret = skl_dram_get_channel_info(&ch1, val_ch1);
 	if (ret == 0)
 		dram_info->num_channels++;
 
@@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
 	if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
 		dram_info->is_16gb_dimm = true;
 
+	if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
+		dev_priv->ipc_capable_mem = true;
+	else
+		dev_priv->ipc_capable_mem = false;
+
+	DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
+		      dev_priv->ipc_capable_mem ? "" : "not ");
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 854f3c828e01..036d6554c017 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2112,6 +2112,7 @@ struct drm_i915_private {
 	bool chv_phy_assert[2];
 
 	bool ipc_enabled;
+	bool ipc_capable_mem;
 
 	/* Used to save the pipe-to-encoder mapping for audio */
 	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2446f53adf21..39e400d5f555 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv)
 	u32 val;
 
 	/* Display WA #0477 WaDisableIPC: skl */
-	if (IS_SKYLAKE(dev_priv)) {
+	if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
 		dev_priv->ipc_enabled = false;
 		return;
 	}
-- 
2.16.2

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

[PATCH 5/5] drm/i915/skl+: don't trust IPC value set by BIOS

[Mahesh Kumar]

If KMS decide to disable IPC make sure we override IPC configuration set
by BIOS.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 39e400d5f555..77b34fe546a8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6097,10 +6097,8 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv)
 	u32 val;
 
 	/* Display WA #0477 WaDisableIPC: skl */
-	if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
+	if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem)
 		dev_priv->ipc_enabled = false;
-		return;
-	}
 
 	val = I915_READ(DISP_ARB_CTL2);
 
-- 
2.16.2

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

✗ Fi.CI.SPARSE: warning for Decode memdev info and bandwidth and implemnt latency WA (rev2)

[Patchwork]

== Series Details ==

Series: Decode memdev info and bandwidth and implemnt latency WA (rev2)
URL   : https://patchwork.freedesktop.org/series/46481/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/bxt: Decode memory bandwidth and parameters
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3645:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3656:16: warning: expression using sizeof(void)

Commit: drm/i915/skl+: Decode memory bandwidth and parameters
+drivers/gpu/drm/i915/i915_drv.c:1149:35: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_drv.c:1149:35: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3656:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3663:16: warning: expression using sizeof(void)

Commit: drm/i915: Implement 16GB dimm wa for latency level-0
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3663:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3666:16: warning: expression using sizeof(void)

Commit: drm/i915/kbl+: Enable IPC only for symmetric memory configurations
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3666:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3667:16: warning: expression using sizeof(void)

Commit: drm/i915/skl+: don't trust IPC value set by BIOS
Okay!

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

✓ Fi.CI.BAT: success for Decode memdev info and bandwidth and implemnt latency WA (rev2)

[Patchwork]

== Series Details ==

Series: Decode memdev info and bandwidth and implemnt latency WA (rev2)
URL   : https://patchwork.freedesktop.org/series/46481/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4551 -> Patchwork_9782 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46481/revisions/2/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9782 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-skl-6600u:       PASS -> INCOMPLETE (fdo#104108)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-skl-6700hq:      DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      {fi-cfl-8109u}:     INCOMPLETE (fdo#106070) -> PASS

    
    ==== Warnings ====

    igt@drv_selftest@live_hangcheck:
      fi-cnl-psr:         DMESG-WARN -> DMESG-FAIL (fdo#106560)

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-WARN (fdo#107372) -> DMESG-FAIL (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (50 -> 44) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper 


== Build changes ==

    * Linux: CI_DRM_4551 -> Patchwork_9782

  CI_DRM_4551: 68157d5c225b33a8f480ceb2ea11d2fae680fe65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4576: bcb37a9b20eeec97f15fac2222408cc2e0b77631 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9782: 517bcc78eb3b93372e1998eddcbf9138b8dc4a40 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

517bcc78eb3b drm/i915/skl+: don't trust IPC value set by BIOS
035e7e872a52 drm/i915/kbl+: Enable IPC only for symmetric memory configurations
39a7f7e0e151 drm/i915: Implement 16GB dimm wa for latency level-0
8a96d4539cf7 drm/i915/skl+: Decode memory bandwidth and parameters
2d380e0fb91b drm/i915/bxt: Decode memory bandwidth and parameters

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9782/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Decode memdev info and bandwidth and implemnt latency WA (rev2)

[Patchwork]

== Series Details ==

Series: Decode memdev info and bandwidth and implemnt latency WA (rev2)
URL   : https://patchwork.freedesktop.org/series/46481/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4551_full -> Patchwork_9782_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9782_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9782_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9782_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-blt:
      shard-kbl:          PASS -> SKIP

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          SKIP -> PASS

    igt@kms_vblank@pipe-a-ts-continuation-modeset-hang:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9782_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@2x-wf_vblank-ts-check-interruptible:
      shard-glk:          PASS -> FAIL (fdo#103928)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
      shard-apl:          PASS -> FAIL (fdo#103375)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    
    ==== Possible fixes ====

    igt@gem_softpin@noreloc-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4551 -> Patchwork_9782

  CI_DRM_4551: 68157d5c225b33a8f480ceb2ea11d2fae680fe65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4576: bcb37a9b20eeec97f15fac2222408cc2e0b77631 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9782: 517bcc78eb3b93372e1998eddcbf9138b8dc4a40 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9782/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Implement 16GB dimm wa for latency level-0

[Matt Turner]

On Thu, Jul 26, 2018 at 7:14 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
> Bspec: 4381

Do we know that these numbers are stable?

I don't know if this form is common in the kernel, but in Mesa we
specify the name of the page which should always allow readers to find
it.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Implement 16GB dimm wa for latency level-0

[Kumar, Mahesh]

Hi Matt,


On 7/27/2018 9:21 AM, Matt Turner wrote:
> On Thu, Jul 26, 2018 at 7:14 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
>> Bspec: 4381
> Do we know that these numbers are stable?
yes these numbers are fixed in Bspec
> I don't know if this form is common in the kernel, but in Mesa we
> specify the name of the page which should always allow readers to find
> it.
This is common practice in kernel to give Bspec Number reference instead 
of name of the page.
-Mahesh

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

Re: [PATCH 3/5] drm/i915: Implement 16GB dimm wa for latency level-0

[Rodrigo Vivi]

On Fri, Jul 27, 2018 at 11:40:14AM +0530, Kumar, Mahesh wrote:
> Hi Matt,
> 
> 
> On 7/27/2018 9:21 AM, Matt Turner wrote:
> > On Thu, Jul 26, 2018 at 7:14 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
> > > Bspec: 4381
> > Do we know that these numbers are stable?
> yes these numbers are fixed in Bspec
> > I don't know if this form is common in the kernel, but in Mesa we
> > specify the name of the page which should always allow readers to find
> > it.
> This is common practice in kernel to give Bspec Number reference instead of
> name of the page.

Well, I believe Matt has a good point here.
It is stable for now and for a while, but we had seen changes
on the web interface in use. Also this number doesn't help devs who
don't use the web interface.

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

Re: [PATCH 3/5] drm/i915: Implement 16GB dimm wa for latency level-0

[Kumar, Mahesh]

Hi,


On 7/28/2018 11:18 AM, Rodrigo Vivi wrote:
> On Fri, Jul 27, 2018 at 11:40:14AM +0530, Kumar, Mahesh wrote:
>> Hi Matt,
>>
>>
>> On 7/27/2018 9:21 AM, Matt Turner wrote:
>>> On Thu, Jul 26, 2018 at 7:14 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
>>>> Bspec: 4381
>>> Do we know that these numbers are stable?
>> yes these numbers are fixed in Bspec
>>> I don't know if this form is common in the kernel, but in Mesa we
>>> specify the name of the page which should always allow readers to find
>>> it.
>> This is common practice in kernel to give Bspec Number reference instead of
>> name of the page.
> Well, I believe Matt has a good point here.
> It is stable for now and for a while, but we had seen changes
> on the web interface in use. Also this number doesn't help devs who
> don't use the web interface.
sure, will update this in next version. Will float updated series once 
receive review comments on other patches.

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

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

Re: [PATCH 1/5] drm/i915/bxt: Decode memory bandwidth and parameters

[Rodrigo Vivi]

On Thu, Jul 26, 2018 at 07:44:06PM +0530, Mahesh Kumar wrote:
> This patch adds support to decode system memory bandwidth and other
> parameters for broxton platform, which will be used for arbitrated
> display memory bandwidth calculation in GEN9 based platforms and
> WM latency level-0 Work-around calculation on GEN9+ platforms.
> 
> Changes since V1:
>  - s/memdev_info/dram_info
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 116 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |  11 ++++
>  drivers/gpu/drm/i915/i915_reg.h |  30 +++++++++++
>  3 files changed, 157 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 18a45e7a3d7c..16629601c9f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1070,6 +1070,116 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> +static int
> +bxt_get_dram_info(struct drm_i915_private *dev_priv)
> +{
> +	struct dram_info *dram_info = &dev_priv->dram_info;
> +	u32 dram_channels;
> +	u32 mem_freq_khz, val;
> +	u8 num_active_channels;
> +	int i;
> +
> +	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
> +	mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) *
> +				    BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> +
> +	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
> +					BXT_DRAM_CHANNEL_ACTIVE_MASK;
> +	num_active_channels = hweight32(dram_channels);
> +
> +	/* Each active bit represents 4-byte channel */
> +	dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
> +
> +	if (dram_info->bandwidth_kbps == 0) {
> +		DRM_INFO("Couldn't get system memory bandwidth\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
> +	 */
> +	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> +		u8 rank, size, width;
> +		enum dram_rank ch_rank;
> +
> +		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
> +		if (val == 0xFFFFFFFF)

why? any spec or any comment here?

> +			continue;
> +
> +		dram_info->num_channels++;
> +		rank = val & BXT_DRAM_RANK_MASK;
> +		width = (val >> BXT_DRAM_WIDTH_SHIFT) & BXT_DRAM_WIDTH_MASK;
> +		size = (val >> BXT_DRAM_SIZE_SHIFT) & BXT_DRAM_SIZE_MASK;
> +		if (rank == BXT_DRAM_RANK_SINGLE)
> +			ch_rank = I915_DRAM_RANK_SINGLE;
> +		else if (rank == BXT_DRAM_RANK_DUAL)
> +			ch_rank = I915_DRAM_RANK_DUAL;
> +		else
> +			ch_rank = I915_DRAM_RANK_INVALID;
> +
> +		if (size == BXT_DRAM_SIZE_4GB)
> +			size = 4;
> +		else if (size == BXT_DRAM_SIZE_6GB)
> +			size = 6;
> +		else if (size == BXT_DRAM_SIZE_8GB)
> +			size = 8;
> +		else if (size == BXT_DRAM_SIZE_12GB)
> +			size = 12;
> +		else if (size == BXT_DRAM_SIZE_16GB)
> +			size = 16;
> +		else
> +			size = 0;

switch case? or define these BXT_DRAM_SIZE with enums?

> +
> +		width = (1 << width) * 8;
> +		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
> +			      width, rank == BXT_DRAM_RANK_SINGLE ? "single" :
> +			      rank == BXT_DRAM_RANK_DUAL ? "dual" : "unknown");
> +
> +		/*
> +		 * If any of the channel is single rank channel,
> +		 * worst case output will be same as if single rank
> +		 * memory, so consider single rank memory.
> +		 */
> +		if (dram_info->rank == I915_DRAM_RANK_INVALID)
> +			dram_info->rank = ch_rank;
> +		else if (ch_rank == I915_DRAM_RANK_SINGLE)
> +			dram_info->rank = I915_DRAM_RANK_SINGLE;
> +	}
> +
> +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> +		DRM_INFO("couldn't get memory rank information\n");
> +		return -EINVAL;
> +	}
> +
> +	dram_info->valid = true;
> +	return 0;
> +}
> +
> +static void
> +intel_get_dram_info(struct drm_i915_private *dev_priv)
> +{
> +	struct dram_info *dram_info = &dev_priv->dram_info;
> +	int ret;
> +
> +	dram_info->valid = false;
> +	dram_info->rank = I915_DRAM_RANK_INVALID;
> +	dram_info->bandwidth_kbps = 0;
> +	dram_info->num_channels = 0;
> +
> +	if (!IS_BROXTON(dev_priv))
> +		return;

I think this should be checked earlier and probably with some
macro NEED_EDRAM_INFO(dev_priv)... to prepare for other platforms

> +
> +	ret = bxt_get_dram_info(dev_priv);
> +	if (ret)
> +		return;
> +
> +	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
> +		      dram_info->bandwidth_kbps, dram_info->num_channels);
> +	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
> +		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> +		      "dual" : "single");
> +}
> +
>  /**
>   * i915_driver_init_hw - setup state requiring device access
>   * @dev_priv: device private
> @@ -1187,6 +1297,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_msi;
>  
>  	intel_opregion_setup(dev_priv);
> +	/*
> +	 * Fill the dram structure to get the system raw bandwidth and
> +	 * dram info. This will be used for memory latency calculation.
> +	 */
> +	intel_get_dram_info(dev_priv);
> +
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0f49f9988dfa..46f942fa7d60 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1904,6 +1904,17 @@ struct drm_i915_private {
>  		bool distrust_bios_wm;
>  	} wm;
>  
> +	struct dram_info {
> +		bool valid;

I don't think we need this duplicated and unused "valid" bool.

> +		u8 num_channels;
> +		enum dram_rank {
> +			I915_DRAM_RANK_INVALID = 0,
> +			I915_DRAM_RANK_SINGLE,
> +			I915_DRAM_RANK_DUAL
> +		} rank;
> +		u32 bandwidth_kbps;
> +	} dram_info;
> +
>  	struct i915_runtime_pm runtime_pm;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5530c470f30d..66900d027570 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9634,6 +9634,36 @@ enum skl_power_gate {
>  #define  DC_STATE_DEBUG_MASK_CORES	(1 << 0)
>  #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1 << 1)
>  
> +#define BXT_P_CR_MC_BIOS_REQ_0_0_0	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114)
> +#define  BXT_REQ_DATA_MASK			0x3F
> +#define  BXT_DRAM_CHANNEL_ACTIVE_SHIFT		12
> +#define  BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF
> +#define  BXT_MEMORY_FREQ_MULTIPLIER_HZ		133333333
> +
> +#define BXT_D_CR_DRP0_DUNIT8			0x1000
> +#define BXT_D_CR_DRP0_DUNIT9			0x1200
> +#define  BXT_D_CR_DRP0_DUNIT_START		8
> +#define  BXT_D_CR_DRP0_DUNIT_END		11
> +#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO(MCHBAR_MIRROR_BASE_SNB + \
> +				      _PICK_EVEN((x) - 8, BXT_D_CR_DRP0_DUNIT8,\
> +						 BXT_D_CR_DRP0_DUNIT9))
> +#define  BXT_DRAM_RANK_MASK			0x3
> +#define  BXT_DRAM_RANK_SINGLE			0x1
> +#define  BXT_DRAM_RANK_DUAL			0x3
> +#define  BXT_DRAM_WIDTH_MASK			0x3
> +#define  BXT_DRAM_WIDTH_SHIFT			4
> +#define  BXT_DRAM_WIDTH_X8			0x0
> +#define  BXT_DRAM_WIDTH_X16			0x1
> +#define  BXT_DRAM_WIDTH_X32			0x2
> +#define  BXT_DRAM_WIDTH_X64			0x3
> +#define  BXT_DRAM_SIZE_MASK			0x7
> +#define  BXT_DRAM_SIZE_SHIFT			6
> +#define  BXT_DRAM_SIZE_4GB			0x0
> +#define  BXT_DRAM_SIZE_6GB			0x1
> +#define  BXT_DRAM_SIZE_8GB			0x2
> +#define  BXT_DRAM_SIZE_12GB			0x3
> +#define  BXT_DRAM_SIZE_16GB			0x4

could you please share or point me to this spec?

> +
>  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>   * since on HSW we can't write to it using I915_WRITE. */
>  #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/skl+: Decode memory bandwidth and parameters

[Rodrigo Vivi]

On Thu, Jul 26, 2018 at 07:44:07PM +0530, Mahesh Kumar wrote:
> This patch adds support to decode system memory bandwidth and other
> parameters for skylake and Gen9+ platforms, which will be used for
> arbitrated display memory bandwidth calculation in GEN9 based
> platforms and WM latency level-0 Work-around calculation on GEN9+.
> 
> Changes Since V1:
>  - s/memdev_info/dram_info
>  - create a struct to hold channel info
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 131 ++++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_drv.h |   7 +++
>  drivers/gpu/drm/i915/i915_reg.h |  21 +++++++
>  3 files changed, 155 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 16629601c9f4..ddf6bf9b500a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1070,6 +1070,118 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> +static int
> +skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> +{
> +	u8 l_rank, s_rank;
> +	u8 l_size, s_size;
> +	u8 l_width, s_width;
> +	enum dram_rank rank;
> +
> +	if (!val)
> +		return -1;

-SOMEERRNO?

> +
> +	l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & SKL_DRAM_SIZE_MASK;
> +	s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & SKL_DRAM_SIZE_MASK;
> +	l_width = (val >> SKL_DRAM_WIDTH_L_SHIFT) & SKL_DRAM_WIDTH_MASK;
> +	s_width = (val >> SKL_DRAM_WIDTH_S_SHIFT) & SKL_DRAM_WIDTH_MASK;
> +	l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & SKL_DRAM_RANK_MASK;
> +	s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & SKL_DRAM_RANK_MASK;
> +
> +	if (l_size == 0 && s_size == 0)
> +		return -1;

ditto

> +
> +	DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
> +		      l_size, (1 << l_width) * 8, l_rank ? "dual" : "single",
> +		      s_size, (1 << s_width) * 8, s_rank ? "dual" : "single");
> +
> +	if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == SKL_DRAM_RANK_DUAL)
> +		rank = I915_DRAM_RANK_DUAL;
> +	else if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) &&
> +		 (s_size && s_rank == SKL_DRAM_RANK_SINGLE))
> +		rank = I915_DRAM_RANK_DUAL;
> +	else
> +		rank = I915_DRAM_RANK_SINGLE;
> +
> +	ch->l_info.size = l_size;
> +	ch->s_info.size = s_size;
> +	ch->l_info.width = l_width;
> +	ch->s_info.width = s_width;
> +	ch->l_info.rank = l_rank;
> +	ch->s_info.rank = s_rank;
> +	ch->rank = rank;

could we do this directly without intermediates? not clear if we change
in the middle after printing...

> +
> +	return 0;
> +}
> +
> +static int
> +skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> +{
> +	struct dram_info *dram_info = &dev_priv->dram_info;
> +	struct dram_channel_info ch0, ch1;
> +	u32 val;
> +	int ret;
> +
> +	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch0, val);
> +	if (ret == 0)
> +		dram_info->num_channels++;
> +
> +	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch1, val);
> +	if (ret == 0)
> +		dram_info->num_channels++;
> +
> +	if (dram_info->num_channels == 0) {
> +		DRM_INFO("Number of memory channels is zero\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * If any of the channel is single rank channel, worst case output
> +	 * will be same as if single rank memory, so consider single rank
> +	 * memory.
> +	 */
> +	if (ch0.rank == I915_DRAM_RANK_SINGLE ||
> +	    ch1.rank == I915_DRAM_RANK_SINGLE)
> +		dram_info->rank = I915_DRAM_RANK_SINGLE;
> +	else
> +		dram_info->rank = max(ch0.rank, ch1.rank);
> +
> +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> +		DRM_INFO("couldn't get memory rank information\n");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int
> +skl_get_dram_info(struct drm_i915_private *dev_priv)
> +{
> +	struct dram_info *dram_info = &dev_priv->dram_info;
> +	u32 mem_freq_khz, val;
> +	int ret;
> +
> +	ret = skl_dram_get_channels_info(dev_priv);
> +	if (ret)
> +		return ret;
> +
> +	val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
> +	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
> +				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> +
> +	dram_info->bandwidth_kbps = dram_info->num_channels *
> +							mem_freq_khz * 8;
> +
> +	if (dram_info->bandwidth_kbps == 0) {
> +		DRM_INFO("Couldn't get system memory bandwidth\n");
> +		return -EINVAL;
> +	}
> +
> +	dram_info->valid = true;
> +	return 0;
> +}
> +
>  static int
>  bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  {
> @@ -1159,6 +1271,7 @@ static void
>  intel_get_dram_info(struct drm_i915_private *dev_priv)
>  {
>  	struct dram_info *dram_info = &dev_priv->dram_info;
> +	char bandwidth_str[32];
>  	int ret;
>  
>  	dram_info->valid = false;
> @@ -1166,15 +1279,25 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  	dram_info->bandwidth_kbps = 0;
>  	dram_info->num_channels = 0;
>  
> -	if (!IS_BROXTON(dev_priv))
> +	if (INTEL_GEN(dev_priv) < 9)
>  		return;
>  
> -	ret = bxt_get_dram_info(dev_priv);
> +	/* Need to calculate bandwidth only for Gen9 */
> +	if (IS_BROXTON(dev_priv))
> +		ret = bxt_get_dram_info(dev_priv);
> +	else if (INTEL_GEN(dev_priv) == 9)
> +		ret = skl_get_dram_info(dev_priv);
> +	else
> +		ret = skl_dram_get_channels_info(dev_priv);

I din't get this... why newer platforms only need this partial path?

also this highlights that the names of the functions are confusing...

>  	if (ret)
>  		return;
>  
> -	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
> -		      dram_info->bandwidth_kbps, dram_info->num_channels);
> +	if (dram_info->bandwidth_kbps)
> +		sprintf(bandwidth_str, "%d KBps", dram_info->bandwidth_kbps);
> +	else
> +		sprintf(bandwidth_str, "unknown");
> +	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
> +		      bandwidth_str, dram_info->num_channels);
>  	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
>  		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
>  		      "dual" : "single");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 46f942fa7d60..2d12fc152b49 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2128,6 +2128,13 @@ struct drm_i915_private {
>  	 */
>  };
>  
> +struct dram_channel_info {
> +	struct info {
> +		u8 size, width, rank;
> +	} l_info, s_info;
> +	enum dram_rank rank;

why do we need duplicated rand information?

> +};
> +
>  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
>  {
>  	return container_of(dev, struct drm_i915_private, drm);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 66900d027570..e4d61167fb64 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9664,6 +9664,27 @@ enum skl_power_gate {
>  #define  BXT_DRAM_SIZE_12GB			0x3
>  #define  BXT_DRAM_SIZE_16GB			0x4
>  
> +#define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
> +#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
> +#define  SKL_REQ_DATA_MASK			(0xF << 0)
> +
> +#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
> +#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
> +#define  SKL_DRAM_SIZE_MASK			0x3F
> +#define  SKL_DRAM_SIZE_L_SHIFT			0
> +#define  SKL_DRAM_SIZE_S_SHIFT			16
> +#define  SKL_DRAM_WIDTH_MASK			0x3
> +#define  SKL_DRAM_WIDTH_L_SHIFT			8
> +#define  SKL_DRAM_WIDTH_S_SHIFT			24
> +#define  SKL_DRAM_WIDTH_X8			0x0
> +#define  SKL_DRAM_WIDTH_X16			0x1
> +#define  SKL_DRAM_WIDTH_X32			0x2
> +#define  SKL_DRAM_RANK_MASK			0x1
> +#define  SKL_DRAM_RANK_L_SHIFT			10
> +#define  SKL_DRAM_RANK_S_SHIFT			26
> +#define  SKL_DRAM_RANK_SINGLE			0x0
> +#define  SKL_DRAM_RANK_DUAL			0x1


again, spec pointers please.

> +
>  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>   * since on HSW we can't write to it using I915_WRITE. */
>  #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/bxt: Decode memory bandwidth and parameters

[Rodrigo Vivi]

On Thu, Jul 26, 2018 at 07:44:06PM +0530, Mahesh Kumar wrote:
> This patch adds support to decode system memory bandwidth and other
> parameters for broxton platform, which will be used for arbitrated
> display memory bandwidth calculation in GEN9 based platforms and
> WM latency level-0 Work-around calculation on GEN9+ platforms.
>
> Changes since V1:
>  - s/memdev_info/dram_info
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 116 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h |  11 ++++
>  drivers/gpu/drm/i915/i915_reg.h |  30 +++++++++++
>  3 files changed, 157 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 18a45e7a3d7c..16629601c9f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1070,6 +1070,116 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>
> +static int
> +bxt_get_dram_info(struct drm_i915_private *dev_priv)
> +{
> +	struct dram_info *dram_info = &dev_priv->dram_info;
> +	u32 dram_channels;
> +	u32 mem_freq_khz, val;
> +	u8 num_active_channels;
> +	int i;
> +
> +	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
> +	mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) *
> +				    BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> +
> +	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
> +					BXT_DRAM_CHANNEL_ACTIVE_MASK;
> +	num_active_channels = hweight32(dram_channels);
> +
> +	/* Each active bit represents 4-byte channel */
> +	dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
> +
> +	if (dram_info->bandwidth_kbps == 0) {
> +		DRM_INFO("Couldn't get system memory bandwidth\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
> +	 */
> +	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> +		u8 rank, size, width;
> +		enum dram_rank ch_rank;
> +
> +		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
> +		if (val == 0xFFFFFFFF)
> +			continue;
> +
> +		dram_info->num_channels++;
> +		rank = val & BXT_DRAM_RANK_MASK;
> +		width = (val >> BXT_DRAM_WIDTH_SHIFT) & BXT_DRAM_WIDTH_MASK;
> +		size = (val >> BXT_DRAM_SIZE_SHIFT) & BXT_DRAM_SIZE_MASK;
> +		if (rank == BXT_DRAM_RANK_SINGLE)
> +			ch_rank = I915_DRAM_RANK_SINGLE;
> +		else if (rank == BXT_DRAM_RANK_DUAL)
> +			ch_rank = I915_DRAM_RANK_DUAL;
> +		else
> +			ch_rank = I915_DRAM_RANK_INVALID;
> +
> +		if (size == BXT_DRAM_SIZE_4GB)
> +			size = 4;
> +		else if (size == BXT_DRAM_SIZE_6GB)
> +			size = 6;
> +		else if (size == BXT_DRAM_SIZE_8GB)
> +			size = 8;
> +		else if (size == BXT_DRAM_SIZE_12GB)
> +			size = 12;
> +		else if (size == BXT_DRAM_SIZE_16GB)
> +			size = 16;
> +		else
> +			size = 0;
> +
> +		width = (1 << width) * 8;
> +		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
> +			      width, rank == BXT_DRAM_RANK_SINGLE ? "single" :
> +			      rank == BXT_DRAM_RANK_DUAL ? "dual" : "unknown");
> +
> +		/*
> +		 * If any of the channel is single rank channel,
> +		 * worst case output will be same as if single rank
> +		 * memory, so consider single rank memory.
> +		 */
> +		if (dram_info->rank == I915_DRAM_RANK_INVALID)
> +			dram_info->rank = ch_rank;
> +		else if (ch_rank == I915_DRAM_RANK_SINGLE)
> +			dram_info->rank = I915_DRAM_RANK_SINGLE;
> +	}
> +
> +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> +		DRM_INFO("couldn't get memory rank information\n");
> +		return -EINVAL;
> +	}
> +
> +	dram_info->valid = true;
> +	return 0;
> +}
> +
> +static void
> +intel_get_dram_info(struct drm_i915_private *dev_priv)
> +{
> +	struct dram_info *dram_info = &dev_priv->dram_info;
> +	int ret;
> +
> +	dram_info->valid = false;
> +	dram_info->rank = I915_DRAM_RANK_INVALID;
> +	dram_info->bandwidth_kbps = 0;
> +	dram_info->num_channels = 0;
> +
> +	if (!IS_BROXTON(dev_priv))
> +		return;
> +
> +	ret = bxt_get_dram_info(dev_priv);
> +	if (ret)
> +		return;
> +
> +	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
> +		      dram_info->bandwidth_kbps, dram_info->num_channels);
> +	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
> +		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> +		      "dual" : "single");
> +}
> +
>  /**
>   * i915_driver_init_hw - setup state requiring device access
>   * @dev_priv: device private
> @@ -1187,6 +1297,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_msi;
>
>  	intel_opregion_setup(dev_priv);
> +	/*
> +	 * Fill the dram structure to get the system raw bandwidth and
> +	 * dram info. This will be used for memory latency calculation.
> +	 */
> +	intel_get_dram_info(dev_priv);
> +
>
>  	return 0;
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0f49f9988dfa..46f942fa7d60 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1904,6 +1904,17 @@ struct drm_i915_private {
>  		bool distrust_bios_wm;
>  	} wm;
>
> +	struct dram_info {
> +		bool valid;
> +		u8 num_channels;
> +		enum dram_rank {
> +			I915_DRAM_RANK_INVALID = 0,
> +			I915_DRAM_RANK_SINGLE,
> +			I915_DRAM_RANK_DUAL
> +		} rank;
> +		u32 bandwidth_kbps;
> +	} dram_info;
> +
>  	struct i915_runtime_pm runtime_pm;
>
>  	struct {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5530c470f30d..66900d027570 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9634,6 +9634,36 @@ enum skl_power_gate {
>  #define  DC_STATE_DEBUG_MASK_CORES	(1 << 0)
>  #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1 << 1)
>
> +#define BXT_P_CR_MC_BIOS_REQ_0_0_0	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114)
> +#define  BXT_REQ_DATA_MASK			0x3F
> +#define  BXT_DRAM_CHANNEL_ACTIVE_SHIFT		12
> +#define  BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF

Thanks a lot for the spec pointers. But now that I opened it and
started to reading here it was hard to review and I noticed this
is because the way that it is defined here doesn't respect our
standards as documented:

"
* For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Define bit field
 * contents so that they are already shifted in place, and can be directly
 * OR'd.
"

So please provide a new version that follows the rules and
that it is easier to review. And sorry for not noticing this
on yesterday's review.

> +#define  BXT_MEMORY_FREQ_MULTIPLIER_HZ		133333333
> +
> +#define BXT_D_CR_DRP0_DUNIT8			0x1000
> +#define BXT_D_CR_DRP0_DUNIT9			0x1200
> +#define  BXT_D_CR_DRP0_DUNIT_START		8
> +#define  BXT_D_CR_DRP0_DUNIT_END		11
> +#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO(MCHBAR_MIRROR_BASE_SNB + \
> +				      _PICK_EVEN((x) - 8, BXT_D_CR_DRP0_DUNIT8,\
> +						 BXT_D_CR_DRP0_DUNIT9))
> +#define  BXT_DRAM_RANK_MASK			0x3
> +#define  BXT_DRAM_RANK_SINGLE			0x1
> +#define  BXT_DRAM_RANK_DUAL			0x3
> +#define  BXT_DRAM_WIDTH_MASK			0x3
> +#define  BXT_DRAM_WIDTH_SHIFT			4
> +#define  BXT_DRAM_WIDTH_X8			0x0
> +#define  BXT_DRAM_WIDTH_X16			0x1
> +#define  BXT_DRAM_WIDTH_X32			0x2
> +#define  BXT_DRAM_WIDTH_X64			0x3
> +#define  BXT_DRAM_SIZE_MASK			0x7
> +#define  BXT_DRAM_SIZE_SHIFT			6
> +#define  BXT_DRAM_SIZE_4GB			0x0
> +#define  BXT_DRAM_SIZE_6GB			0x1
> +#define  BXT_DRAM_SIZE_8GB			0x2
> +#define  BXT_DRAM_SIZE_12GB			0x3
> +#define  BXT_DRAM_SIZE_16GB			0x4
> +
>  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>   * since on HSW we can't write to it using I915_WRITE. */
>  #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> --
> 2.16.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Implement 16GB dimm wa for latency level-0

[Rodrigo Vivi]

On Thu, Jul 26, 2018 at 07:44:08PM +0530, Mahesh Kumar wrote:
> Memory with 16GB dimms require an increase of 1us in level-0 latency.
> This patch implements the same.
> Bspec: 4381
> 
> changes since V1:
>  - s/memdev_info/dram_info
>  - make skl_is_16gb_dimm pure function
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++++
>  3 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ddf6bf9b500a..86bc2e685522 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1070,6 +1070,25 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> +static bool
> +skl_is_16gb_dimm(u8 rank, u8 size, u8 width)
> +{
> +	if (size == 16 && width == SKL_DRAM_WIDTH_X8 &&
> +	    rank == SKL_DRAM_RANK_SINGLE)
> +		return true;
> +	else if (size == 32 && width == SKL_DRAM_WIDTH_X8 &&
> +		 rank == SKL_DRAM_RANK_DUAL)
> +		return true;
> +	else if (size == 8 && width == SKL_DRAM_WIDTH_X16 &&
> +		 rank == SKL_DRAM_RANK_SINGLE)
> +		return true;
> +	else if (size == 16 && width == SKL_DRAM_WIDTH_X16 &&
> +		 rank == SKL_DRAM_RANK_DUAL)
> +		return true;

nip: This is right, but it would be easier if order of checks
     were in same order as spec. Well.. but no real need for
     change since I already checked ;)

> +
> +	return false;
> +}
> +
>  static int
>  skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  {
> @@ -1077,6 +1096,7 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  	u8 l_size, s_size;
>  	u8 l_width, s_width;
>  	enum dram_rank rank;
> +	bool l_16gb_dimm, s_16gb_dimm;

I don't think we need this extra locals

>  
>  	if (!val)
>  		return -1;
> @@ -1103,6 +1123,13 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  	else
>  		rank = I915_DRAM_RANK_SINGLE;
>  
> +	l_16gb_dimm = skl_is_16gb_dimm(l_rank, l_size, l_width);
> +	s_16gb_dimm = skl_is_16gb_dimm(s_rank, s_size, s_width);
> +
> +	if (l_16gb_dimm || s_16gb_dimm)
> +		ch->is_16gb_dimm = true;
> +	else
> +		ch->is_16gb_dimm = false;
	ch->is_16gb_dim = skl_is_16gb_dimm(l_rank, l_size, l_width) ||
			  skl_is_16gb_dimm(s_rank, s_size, s_width);

>  	ch->l_info.size = l_size;
>  	ch->s_info.size = s_size;
>  	ch->l_info.width = l_width;
> @@ -1137,6 +1164,8 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  		return -EINVAL;
>  	}
>  
> +	dram_info->valid_dimm = true;
> +
>  	/*
>  	 * If any of the channel is single rank channel, worst case output
>  	 * will be same as if single rank memory, so consider single rank
> @@ -1152,6 +1181,10 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  		DRM_INFO("couldn't get memory rank information\n");
>  		return -EINVAL;
>  	}
> +
> +	if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
> +		dram_info->is_16gb_dimm = true;
> +
>  	return 0;
>  }
>  
> @@ -1263,6 +1296,7 @@ bxt_get_dram_info(struct drm_i915_private *dev_priv)
>  		return -EINVAL;
>  	}
>  
> +	dram_info->valid_dimm = true;
>  	dram_info->valid = true;
>  	return 0;
>  }
> @@ -1275,6 +1309,8 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  	int ret;
>  
>  	dram_info->valid = false;
> +	dram_info->valid_dimm = false;
> +	dram_info->is_16gb_dimm = false;
>  	dram_info->rank = I915_DRAM_RANK_INVALID;
>  	dram_info->bandwidth_kbps = 0;
>  	dram_info->num_channels = 0;
> @@ -1298,9 +1334,9 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>  		sprintf(bandwidth_str, "unknown");
>  	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
>  		      bandwidth_str, dram_info->num_channels);
> -	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
> +	DRM_DEBUG_KMS("DRAM rank: %s rank 16GB-dimm:%s\n",
>  		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> -		      "dual" : "single");
> +		      "dual" : "single", yesno(dram_info->is_16gb_dimm));
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2d12fc152b49..854f3c828e01 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1906,6 +1906,8 @@ struct drm_i915_private {
>  
>  	struct dram_info {
>  		bool valid;
> +		bool valid_dimm;
> +		bool is_16gb_dimm;
>  		u8 num_channels;
>  		enum dram_rank {
>  			I915_DRAM_RANK_INVALID = 0,
> @@ -2133,6 +2135,7 @@ struct dram_channel_info {
>  		u8 size, width, rank;
>  	} l_info, s_info;
>  	enum dram_rank rank;
> +	bool is_16gb_dimm;
>  };
>  
>  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7312ecb73415..2446f53adf21 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2874,6 +2874,19 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
>  			}
>  		}
>  
> +		/*
> +		 * WA Level-0 adjustment for 16GB DIMMs: SKL+
> +		 * If we could not get dimm info enable this WA to prevent from
> +		 * any underrun. If not able to get Dimm info assume 16GB dimm
> +		 * to avoid any underrun.
> +		 */
> +		if (dev_priv->dram_info.valid_dimm) {
> +			if (dev_priv->dram_info.is_16gb_dimm)
> +				wm[0] += 1;
> +		} else {
> +			wm[0] += 1;

well... spec don't cover this case... I believe it is safe,
but only concern would be this creating latency for other cases...
just thinking loud, but I'm probably in favor of doing this anyway...

> +		}
> +
>  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		uint64_t sskpd = I915_READ64(MCH_SSKPD);
>  
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Rodrigo Vivi]

On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
> IPC may cause underflows if not used with dual channel symmetric
> memory configuration. Disable IPC for non symmetric configurations in
> affected platforms.
> Display WA #1141
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 43 ++++++++++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c |  2 +-
>  3 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 86bc2e685522..2273664166bc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  	return 0;
>  }
>
> +static bool
> +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
> +			  u32 val_ch0, u32 val_ch1,
> +			  struct dram_channel_info *ch0)

what about
intel_is_dram_symmetric() ?

> +{
> +	/* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */

move this to the wa call, not the the function check...

> +	if (INTEL_GEN(dev_priv) > 9 &&
> +	    !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))

please don't add CNL pre-prod wa

> +		return true;
> +
> +	if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
> +		return true;

actually remove all platforms checks here...

> +
> +	if (val_ch0 != val_ch1)
> +		return false;
> +
> +	if (ch0->s_info.size == 0)
> +		return true;
> +	if (ch0->l_info.size == ch0->s_info.size &&
> +	    ch0->l_info.width == ch0->s_info.width &&
> +	    ch0->l_info.rank == ch0->s_info.rank)
> +		return true;
> +
> +	return false;

return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
       (ch0->l_info.size == ch0->s_info.size &&
         ch0->l_info.width == ch0->s_info.width &&
         ch0->l_info.rank == ch0->s_info.rank)))

> +}

> +
>  static int
>  skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  {
>  	struct dram_info *dram_info = &dev_priv->dram_info;
>  	struct dram_channel_info ch0, ch1;
> -	u32 val;
> +	u32 val_ch0, val_ch1;
>  	int ret;
>
> -	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch0, val);
> +	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch0, val_ch0);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>
> -	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> -	ret = skl_dram_get_channel_info(&ch1, val);
> +	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> +	ret = skl_dram_get_channel_info(&ch1, val_ch1);
>  	if (ret == 0)
>  		dram_info->num_channels++;
>
> @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>  	if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
>  		dram_info->is_16gb_dimm = true;
>
> +	if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
> +		dev_priv->ipc_capable_mem = true;
> +	else
> +		dev_priv->ipc_capable_mem = false;
> +
> +	DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
> +		      dev_priv->ipc_capable_mem ? "" : "not ");
>  	return 0;
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 854f3c828e01..036d6554c017 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2112,6 +2112,7 @@ struct drm_i915_private {
>  	bool chv_phy_assert[2];
>
>  	bool ipc_enabled;
> +	bool ipc_capable_mem;

I don't think we need to stage this...

>
>  	/* Used to save the pipe-to-encoder mapping for audio */
>  	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2446f53adf21..39e400d5f555 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv)
>  	u32 val;
>
>  	/* Display WA #0477 WaDisableIPC: skl */
> -	if (IS_SKYLAKE(dev_priv)) {
> +	if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
>  		dev_priv->ipc_enabled = false;

This is not the WA 1141 for other platforms than SKL. Please only keep skl here.

For the other WA add 4us across all watermark levels

/* Display WA #1141: skl,kbl,cfl */
if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
   intel_is_dram_symmetric())
   levels += 4;

>  		return;
>  	}
> --
> 2.16.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/bxt: Decode memory bandwidth and parameters

[Kumar, Mahesh]

Hi,


On 8/17/2018 11:13 PM, Rodrigo Vivi wrote:
> On Thu, Jul 26, 2018 at 07:44:06PM +0530, Mahesh Kumar wrote:
>> This patch adds support to decode system memory bandwidth and other
>> parameters for broxton platform, which will be used for arbitrated
>> display memory bandwidth calculation in GEN9 based platforms and
>> WM latency level-0 Work-around calculation on GEN9+ platforms.
>>
>> Changes since V1:
>>   - s/memdev_info/dram_info
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 116 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_drv.h |  11 ++++
>>   drivers/gpu/drm/i915/i915_reg.h |  30 +++++++++++
>>   3 files changed, 157 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 18a45e7a3d7c..16629601c9f4 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1070,6 +1070,116 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>>   	intel_gvt_sanitize_options(dev_priv);
>>   }
>>
>> +static int
>> +bxt_get_dram_info(struct drm_i915_private *dev_priv)
>> +{
>> +	struct dram_info *dram_info = &dev_priv->dram_info;
>> +	u32 dram_channels;
>> +	u32 mem_freq_khz, val;
>> +	u8 num_active_channels;
>> +	int i;
>> +
>> +	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
>> +	mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) *
>> +				    BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
>> +
>> +	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
>> +					BXT_DRAM_CHANNEL_ACTIVE_MASK;
>> +	num_active_channels = hweight32(dram_channels);
>> +
>> +	/* Each active bit represents 4-byte channel */
>> +	dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
>> +
>> +	if (dram_info->bandwidth_kbps == 0) {
>> +		DRM_INFO("Couldn't get system memory bandwidth\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
>> +	 */
>> +	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
>> +		u8 rank, size, width;
>> +		enum dram_rank ch_rank;
>> +
>> +		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
>> +		if (val == 0xFFFFFFFF)
>> +			continue;
>> +
>> +		dram_info->num_channels++;
>> +		rank = val & BXT_DRAM_RANK_MASK;
>> +		width = (val >> BXT_DRAM_WIDTH_SHIFT) & BXT_DRAM_WIDTH_MASK;
>> +		size = (val >> BXT_DRAM_SIZE_SHIFT) & BXT_DRAM_SIZE_MASK;
>> +		if (rank == BXT_DRAM_RANK_SINGLE)
>> +			ch_rank = I915_DRAM_RANK_SINGLE;
>> +		else if (rank == BXT_DRAM_RANK_DUAL)
>> +			ch_rank = I915_DRAM_RANK_DUAL;
>> +		else
>> +			ch_rank = I915_DRAM_RANK_INVALID;
>> +
>> +		if (size == BXT_DRAM_SIZE_4GB)
>> +			size = 4;
>> +		else if (size == BXT_DRAM_SIZE_6GB)
>> +			size = 6;
>> +		else if (size == BXT_DRAM_SIZE_8GB)
>> +			size = 8;
>> +		else if (size == BXT_DRAM_SIZE_12GB)
>> +			size = 12;
>> +		else if (size == BXT_DRAM_SIZE_16GB)
>> +			size = 16;
>> +		else
>> +			size = 0;
>> +
>> +		width = (1 << width) * 8;
>> +		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
>> +			      width, rank == BXT_DRAM_RANK_SINGLE ? "single" :
>> +			      rank == BXT_DRAM_RANK_DUAL ? "dual" : "unknown");
>> +
>> +		/*
>> +		 * If any of the channel is single rank channel,
>> +		 * worst case output will be same as if single rank
>> +		 * memory, so consider single rank memory.
>> +		 */
>> +		if (dram_info->rank == I915_DRAM_RANK_INVALID)
>> +			dram_info->rank = ch_rank;
>> +		else if (ch_rank == I915_DRAM_RANK_SINGLE)
>> +			dram_info->rank = I915_DRAM_RANK_SINGLE;
>> +	}
>> +
>> +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
>> +		DRM_INFO("couldn't get memory rank information\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dram_info->valid = true;
>> +	return 0;
>> +}
>> +
>> +static void
>> +intel_get_dram_info(struct drm_i915_private *dev_priv)
>> +{
>> +	struct dram_info *dram_info = &dev_priv->dram_info;
>> +	int ret;
>> +
>> +	dram_info->valid = false;
>> +	dram_info->rank = I915_DRAM_RANK_INVALID;
>> +	dram_info->bandwidth_kbps = 0;
>> +	dram_info->num_channels = 0;
>> +
>> +	if (!IS_BROXTON(dev_priv))
>> +		return;
>> +
>> +	ret = bxt_get_dram_info(dev_priv);
>> +	if (ret)
>> +		return;
>> +
>> +	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
>> +		      dram_info->bandwidth_kbps, dram_info->num_channels);
>> +	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
>> +		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
>> +		      "dual" : "single");
>> +}
>> +
>>   /**
>>    * i915_driver_init_hw - setup state requiring device access
>>    * @dev_priv: device private
>> @@ -1187,6 +1297,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>>   		goto err_msi;
>>
>>   	intel_opregion_setup(dev_priv);
>> +	/*
>> +	 * Fill the dram structure to get the system raw bandwidth and
>> +	 * dram info. This will be used for memory latency calculation.
>> +	 */
>> +	intel_get_dram_info(dev_priv);
>> +
>>
>>   	return 0;
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 0f49f9988dfa..46f942fa7d60 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1904,6 +1904,17 @@ struct drm_i915_private {
>>   		bool distrust_bios_wm;
>>   	} wm;
>>
>> +	struct dram_info {
>> +		bool valid;
>> +		u8 num_channels;
>> +		enum dram_rank {
>> +			I915_DRAM_RANK_INVALID = 0,
>> +			I915_DRAM_RANK_SINGLE,
>> +			I915_DRAM_RANK_DUAL
>> +		} rank;
>> +		u32 bandwidth_kbps;
>> +	} dram_info;
>> +
>>   	struct i915_runtime_pm runtime_pm;
>>
>>   	struct {
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 5530c470f30d..66900d027570 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9634,6 +9634,36 @@ enum skl_power_gate {
>>   #define  DC_STATE_DEBUG_MASK_CORES	(1 << 0)
>>   #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1 << 1)
>>
>> +#define BXT_P_CR_MC_BIOS_REQ_0_0_0	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114)
>> +#define  BXT_REQ_DATA_MASK			0x3F
>> +#define  BXT_DRAM_CHANNEL_ACTIVE_SHIFT		12
>> +#define  BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF
> Thanks a lot for the spec pointers. But now that I opened it and
> started to reading here it was hard to review and I noticed this
> is because the way that it is defined here doesn't respect our
> standards as documented:
>
> "
> * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Define bit field
>   * contents so that they are already shifted in place, and can be directly
>   * OR'd.
> "
So I'll modify all "_MASK" defines to make them shifted in place, Should 
I redefine all the bit definitions also to be shifted?
BTW these bit-fields are readonly for driver,

-Mahesh
>
> So please provide a new version that follows the rules and
> that it is easier to review. And sorry for not noticing this
> on yesterday's review.
>
>> +#define  BXT_MEMORY_FREQ_MULTIPLIER_HZ		133333333
>> +
>> +#define BXT_D_CR_DRP0_DUNIT8			0x1000
>> +#define BXT_D_CR_DRP0_DUNIT9			0x1200
>> +#define  BXT_D_CR_DRP0_DUNIT_START		8
>> +#define  BXT_D_CR_DRP0_DUNIT_END		11
>> +#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO(MCHBAR_MIRROR_BASE_SNB + \
>> +				      _PICK_EVEN((x) - 8, BXT_D_CR_DRP0_DUNIT8,\
>> +						 BXT_D_CR_DRP0_DUNIT9))
>> +#define  BXT_DRAM_RANK_MASK			0x3
>> +#define  BXT_DRAM_RANK_SINGLE			0x1
>> +#define  BXT_DRAM_RANK_DUAL			0x3
>> +#define  BXT_DRAM_WIDTH_MASK			0x3
>> +#define  BXT_DRAM_WIDTH_SHIFT			4
>> +#define  BXT_DRAM_WIDTH_X8			0x0
>> +#define  BXT_DRAM_WIDTH_X16			0x1
>> +#define  BXT_DRAM_WIDTH_X32			0x2
>> +#define  BXT_DRAM_WIDTH_X64			0x3
>> +#define  BXT_DRAM_SIZE_MASK			0x7
>> +#define  BXT_DRAM_SIZE_SHIFT			6
>> +#define  BXT_DRAM_SIZE_4GB			0x0
>> +#define  BXT_DRAM_SIZE_6GB			0x1
>> +#define  BXT_DRAM_SIZE_8GB			0x2
>> +#define  BXT_DRAM_SIZE_12GB			0x3
>> +#define  BXT_DRAM_SIZE_16GB			0x4
>> +
>>   /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>>    * since on HSW we can't write to it using I915_WRITE. */
>>   #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/5] drm/i915/skl+: Decode memory bandwidth and parameters

[Kumar, Mahesh]

Hi,


On 8/17/2018 4:05 AM, Rodrigo Vivi wrote:
> On Thu, Jul 26, 2018 at 07:44:07PM +0530, Mahesh Kumar wrote:
>> This patch adds support to decode system memory bandwidth and other
>> parameters for skylake and Gen9+ platforms, which will be used for
>> arbitrated display memory bandwidth calculation in GEN9 based
>> platforms and WM latency level-0 Work-around calculation on GEN9+.
>>
>> Changes Since V1:
>>   - s/memdev_info/dram_info
>>   - create a struct to hold channel info
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 131 ++++++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_drv.h |   7 +++
>>   drivers/gpu/drm/i915/i915_reg.h |  21 +++++++
>>   3 files changed, 155 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 16629601c9f4..ddf6bf9b500a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1070,6 +1070,118 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>>   	intel_gvt_sanitize_options(dev_priv);
>>   }
>>   
>> +static int
>> +skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>> +{
>> +	u8 l_rank, s_rank;
>> +	u8 l_size, s_size;
>> +	u8 l_width, s_width;
>> +	enum dram_rank rank;
>> +
>> +	if (!val)
>> +		return -1;
> -SOMEERRNO?
ok sure.
>
>> +
>> +	l_size = (val >> SKL_DRAM_SIZE_L_SHIFT) & SKL_DRAM_SIZE_MASK;
>> +	s_size = (val >> SKL_DRAM_SIZE_S_SHIFT) & SKL_DRAM_SIZE_MASK;
>> +	l_width = (val >> SKL_DRAM_WIDTH_L_SHIFT) & SKL_DRAM_WIDTH_MASK;
>> +	s_width = (val >> SKL_DRAM_WIDTH_S_SHIFT) & SKL_DRAM_WIDTH_MASK;
>> +	l_rank = (val >> SKL_DRAM_RANK_L_SHIFT) & SKL_DRAM_RANK_MASK;
>> +	s_rank = (val >> SKL_DRAM_RANK_S_SHIFT) & SKL_DRAM_RANK_MASK;
>> +
>> +	if (l_size == 0 && s_size == 0)
>> +		return -1;
> ditto
>
>> +
>> +	DRM_DEBUG_KMS("(size:width:rank) L(%dGB:X%d:%s) S(%dGB:X%d:%s)\n",
>> +		      l_size, (1 << l_width) * 8, l_rank ? "dual" : "single",
>> +		      s_size, (1 << s_width) * 8, s_rank ? "dual" : "single");
>> +
>> +	if (l_rank == SKL_DRAM_RANK_DUAL || s_rank == SKL_DRAM_RANK_DUAL)
>> +		rank = I915_DRAM_RANK_DUAL;
>> +	else if ((l_size && l_rank == SKL_DRAM_RANK_SINGLE) &&
>> +		 (s_size && s_rank == SKL_DRAM_RANK_SINGLE))
>> +		rank = I915_DRAM_RANK_DUAL;
>> +	else
>> +		rank = I915_DRAM_RANK_SINGLE;
>> +
>> +	ch->l_info.size = l_size;
>> +	ch->s_info.size = s_size;
>> +	ch->l_info.width = l_width;
>> +	ch->s_info.width = s_width;
>> +	ch->l_info.rank = l_rank;
>> +	ch->s_info.rank = s_rank;
>> +	ch->rank = rank;
> could we do this directly without intermediates? not clear if we change
> in the middle after printing...
This information is needed later while checking is memories are 
symmetric, that's why we need to keep this as well.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>> +{
>> +	struct dram_info *dram_info = &dev_priv->dram_info;
>> +	struct dram_channel_info ch0, ch1;
>> +	u32 val;
>> +	int ret;
>> +
>> +	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>> +	ret = skl_dram_get_channel_info(&ch0, val);
>> +	if (ret == 0)
>> +		dram_info->num_channels++;
>> +
>> +	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>> +	ret = skl_dram_get_channel_info(&ch1, val);
>> +	if (ret == 0)
>> +		dram_info->num_channels++;
>> +
>> +	if (dram_info->num_channels == 0) {
>> +		DRM_INFO("Number of memory channels is zero\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * If any of the channel is single rank channel, worst case output
>> +	 * will be same as if single rank memory, so consider single rank
>> +	 * memory.
>> +	 */
>> +	if (ch0.rank == I915_DRAM_RANK_SINGLE ||
>> +	    ch1.rank == I915_DRAM_RANK_SINGLE)
>> +		dram_info->rank = I915_DRAM_RANK_SINGLE;
>> +	else
>> +		dram_info->rank = max(ch0.rank, ch1.rank);
>> +
>> +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
>> +		DRM_INFO("couldn't get memory rank information\n");
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int
>> +skl_get_dram_info(struct drm_i915_private *dev_priv)
>> +{
>> +	struct dram_info *dram_info = &dev_priv->dram_info;
>> +	u32 mem_freq_khz, val;
>> +	int ret;
>> +
>> +	ret = skl_dram_get_channels_info(dev_priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = I915_READ(SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
>> +	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
>> +				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
>> +
>> +	dram_info->bandwidth_kbps = dram_info->num_channels *
>> +							mem_freq_khz * 8;
>> +
>> +	if (dram_info->bandwidth_kbps == 0) {
>> +		DRM_INFO("Couldn't get system memory bandwidth\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dram_info->valid = true;
>> +	return 0;
>> +}
>> +
>>   static int
>>   bxt_get_dram_info(struct drm_i915_private *dev_priv)
>>   {
>> @@ -1159,6 +1271,7 @@ static void
>>   intel_get_dram_info(struct drm_i915_private *dev_priv)
>>   {
>>   	struct dram_info *dram_info = &dev_priv->dram_info;
>> +	char bandwidth_str[32];
>>   	int ret;
>>   
>>   	dram_info->valid = false;
>> @@ -1166,15 +1279,25 @@ intel_get_dram_info(struct drm_i915_private *dev_priv)
>>   	dram_info->bandwidth_kbps = 0;
>>   	dram_info->num_channels = 0;
>>   
>> -	if (!IS_BROXTON(dev_priv))
>> +	if (INTEL_GEN(dev_priv) < 9)
>>   		return;
>>   
>> -	ret = bxt_get_dram_info(dev_priv);
>> +	/* Need to calculate bandwidth only for Gen9 */
>> +	if (IS_BROXTON(dev_priv))
>> +		ret = bxt_get_dram_info(dev_priv);
>> +	else if (INTEL_GEN(dev_priv) == 9)
>> +		ret = skl_get_dram_info(dev_priv);
>> +	else
>> +		ret = skl_dram_get_channels_info(dev_priv);
> I din't get this... why newer platforms only need this partial path?
>
> also this highlights that the names of the functions are confusing...
In gen9 platform we need complete DRAM info (channel configuration + 
bandwidth supported by DRAM) which will be used by bandwidth related WA, 
and only applicable for GEN9 platforms. Bspec:4380 (Gen9 Watermark 
Calculations: Workaround)
But Channel info is applicable for all the platforms, This will be used 
to check if system has 16GB dimm or if memory channels are not symmetric.
BSpec:4381 (Retrieve Memory Latency Data)

I agree names seems little confusing I used following convention
dram_info -> channels_info -> channel_info

>
>>   	if (ret)
>>   		return;
>>   
>> -	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
>> -		      dram_info->bandwidth_kbps, dram_info->num_channels);
>> +	if (dram_info->bandwidth_kbps)
>> +		sprintf(bandwidth_str, "%d KBps", dram_info->bandwidth_kbps);
>> +	else
>> +		sprintf(bandwidth_str, "unknown");
>> +	DRM_DEBUG_KMS("DRAM bandwidth:%s, total-channels: %u\n",
>> +		      bandwidth_str, dram_info->num_channels);
>>   	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
>>   		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
>>   		      "dual" : "single");
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 46f942fa7d60..2d12fc152b49 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2128,6 +2128,13 @@ struct drm_i915_private {
>>   	 */
>>   };
>>   
>> +struct dram_channel_info {
>> +	struct info {
>> +		u8 size, width, rank;
>> +	} l_info, s_info;
>> +	enum dram_rank rank;
> why do we need duplicated rand information?
first one represents rank of each dimm left/right dimm.
Second one stores the rank of channel.

-Mahesh
>
>> +};
>> +
>>   static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
>>   {
>>   	return container_of(dev, struct drm_i915_private, drm);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 66900d027570..e4d61167fb64 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9664,6 +9664,27 @@ enum skl_power_gate {
>>   #define  BXT_DRAM_SIZE_12GB			0x3
>>   #define  BXT_DRAM_SIZE_16GB			0x4
>>   
>> +#define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
>> +#define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
>> +#define  SKL_REQ_DATA_MASK			(0xF << 0)
>> +
>> +#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C)
>> +#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010)
>> +#define  SKL_DRAM_SIZE_MASK			0x3F
>> +#define  SKL_DRAM_SIZE_L_SHIFT			0
>> +#define  SKL_DRAM_SIZE_S_SHIFT			16
>> +#define  SKL_DRAM_WIDTH_MASK			0x3
>> +#define  SKL_DRAM_WIDTH_L_SHIFT			8
>> +#define  SKL_DRAM_WIDTH_S_SHIFT			24
>> +#define  SKL_DRAM_WIDTH_X8			0x0
>> +#define  SKL_DRAM_WIDTH_X16			0x1
>> +#define  SKL_DRAM_WIDTH_X32			0x2
>> +#define  SKL_DRAM_RANK_MASK			0x1
>> +#define  SKL_DRAM_RANK_L_SHIFT			10
>> +#define  SKL_DRAM_RANK_S_SHIFT			26
>> +#define  SKL_DRAM_RANK_SINGLE			0x0
>> +#define  SKL_DRAM_RANK_DUAL			0x1
>
> again, spec pointers please.
>
>> +
>>   /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>>    * since on HSW we can't write to it using I915_WRITE. */
>>   #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
>> -- 
>> 2.16.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Kumar, Mahesh]

Hi,


On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
> On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
>> IPC may cause underflows if not used with dual channel symmetric
>> memory configuration. Disable IPC for non symmetric configurations in
>> affected platforms.
>> Display WA #1141
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 43 ++++++++++++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c |  2 +-
>>   3 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 86bc2e685522..2273664166bc 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>>   	return 0;
>>   }
>>
>> +static bool
>> +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
>> +			  u32 val_ch0, u32 val_ch1,
>> +			  struct dram_channel_info *ch0)
> what about
> intel_is_dram_symmetric() ?
sounds good.
>> +{
>> +	/* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
> move this to the wa call, not the the function check...
>
>> +	if (INTEL_GEN(dev_priv) > 9 &&
>> +	    !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
> please don't add CNL pre-prod wa
ok sure
>
>> +		return true;
>> +
>> +	if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
>> +		return true;
> actually remove all platforms checks here...
Agree will remove these checks, function will just return if memory is 
symmetric.
>
>> +
>> +	if (val_ch0 != val_ch1)
>> +		return false;
>> +
>> +	if (ch0->s_info.size == 0)
>> +		return true;
>> +	if (ch0->l_info.size == ch0->s_info.size &&
>> +	    ch0->l_info.width == ch0->s_info.width &&
>> +	    ch0->l_info.rank == ch0->s_info.rank)
>> +		return true;
>> +
>> +	return false;
> return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
>         (ch0->l_info.size == ch0->s_info.size &&
>           ch0->l_info.width == ch0->s_info.width &&
>           ch0->l_info.rank == ch0->s_info.rank)))
>
>> +}
>> +
>>   static int
>>   skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>>   {
>>   	struct dram_info *dram_info = &dev_priv->dram_info;
>>   	struct dram_channel_info ch0, ch1;
>> -	u32 val;
>> +	u32 val_ch0, val_ch1;
>>   	int ret;
>>
>> -	val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>> -	ret = skl_dram_get_channel_info(&ch0, val);
>> +	val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>> +	ret = skl_dram_get_channel_info(&ch0, val_ch0);
>>   	if (ret == 0)
>>   		dram_info->num_channels++;
>>
>> -	val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>> -	ret = skl_dram_get_channel_info(&ch1, val);
>> +	val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>> +	ret = skl_dram_get_channel_info(&ch1, val_ch1);
>>   	if (ret == 0)
>>   		dram_info->num_channels++;
>>
>> @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>>   	if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
>>   		dram_info->is_16gb_dimm = true;
>>
>> +	if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
>> +		dev_priv->ipc_capable_mem = true;
>> +	else
>> +		dev_priv->ipc_capable_mem = false;
>> +
>> +	DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
>> +		      dev_priv->ipc_capable_mem ? "" : "not ");
>>   	return 0;
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 854f3c828e01..036d6554c017 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2112,6 +2112,7 @@ struct drm_i915_private {
>>   	bool chv_phy_assert[2];
>>
>>   	bool ipc_enabled;
>> +	bool ipc_capable_mem;
> I don't think we need to stage this...
With above changes storing this info is not needed, But we need to keep 
a flag in dram_info to check if "memory is symmetric", Otherwise we need 
to compute all the memory related info again.

-Mahesh
>>   	/* Used to save the pipe-to-encoder mapping for audio */
>>   	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 2446f53adf21..39e400d5f555 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv)
>>   	u32 val;
>>
>>   	/* Display WA #0477 WaDisableIPC: skl */
>> -	if (IS_SKYLAKE(dev_priv)) {
>> +	if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
>>   		dev_priv->ipc_enabled = false;
> This is not the WA 1141 for other platforms than SKL. Please only keep skl here.
>
> For the other WA add 4us across all watermark levels
>
> /* Display WA #1141: skl,kbl,cfl */
> if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
>     intel_is_dram_symmetric())
>     levels += 4;
>
>>   		return;
>>   	}
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/5] drm/i915/bxt: Decode memory bandwidth and parameters

[Rodrigo Vivi]

On Tue, Aug 21, 2018 at 03:23:06PM +0530, Kumar, Mahesh wrote:
> Hi,
> 
> 
> On 8/17/2018 11:13 PM, Rodrigo Vivi wrote:
> > On Thu, Jul 26, 2018 at 07:44:06PM +0530, Mahesh Kumar wrote:
> > > This patch adds support to decode system memory bandwidth and other
> > > parameters for broxton platform, which will be used for arbitrated
> > > display memory bandwidth calculation in GEN9 based platforms and
> > > WM latency level-0 Work-around calculation on GEN9+ platforms.
> > > 
> > > Changes since V1:
> > >   - s/memdev_info/dram_info
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.c | 116 ++++++++++++++++++++++++++++++++++++++++
> > >   drivers/gpu/drm/i915/i915_drv.h |  11 ++++
> > >   drivers/gpu/drm/i915/i915_reg.h |  30 +++++++++++
> > >   3 files changed, 157 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 18a45e7a3d7c..16629601c9f4 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1070,6 +1070,116 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
> > >   	intel_gvt_sanitize_options(dev_priv);
> > >   }
> > > 
> > > +static int
> > > +bxt_get_dram_info(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > > +	u32 dram_channels;
> > > +	u32 mem_freq_khz, val;
> > > +	u8 num_active_channels;
> > > +	int i;
> > > +
> > > +	val = I915_READ(BXT_P_CR_MC_BIOS_REQ_0_0_0);
> > > +	mem_freq_khz = DIV_ROUND_UP((val & BXT_REQ_DATA_MASK) *
> > > +				    BXT_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> > > +
> > > +	dram_channels = (val >> BXT_DRAM_CHANNEL_ACTIVE_SHIFT) &
> > > +					BXT_DRAM_CHANNEL_ACTIVE_MASK;
> > > +	num_active_channels = hweight32(dram_channels);
> > > +
> > > +	/* Each active bit represents 4-byte channel */
> > > +	dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4);
> > > +
> > > +	if (dram_info->bandwidth_kbps == 0) {
> > > +		DRM_INFO("Couldn't get system memory bandwidth\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Now read each DUNIT8/9/10/11 to check the rank of each dimms.
> > > +	 */
> > > +	for (i = BXT_D_CR_DRP0_DUNIT_START; i <= BXT_D_CR_DRP0_DUNIT_END; i++) {
> > > +		u8 rank, size, width;
> > > +		enum dram_rank ch_rank;
> > > +
> > > +		val = I915_READ(BXT_D_CR_DRP0_DUNIT(i));
> > > +		if (val == 0xFFFFFFFF)
> > > +			continue;
> > > +
> > > +		dram_info->num_channels++;
> > > +		rank = val & BXT_DRAM_RANK_MASK;
> > > +		width = (val >> BXT_DRAM_WIDTH_SHIFT) & BXT_DRAM_WIDTH_MASK;
> > > +		size = (val >> BXT_DRAM_SIZE_SHIFT) & BXT_DRAM_SIZE_MASK;
> > > +		if (rank == BXT_DRAM_RANK_SINGLE)
> > > +			ch_rank = I915_DRAM_RANK_SINGLE;
> > > +		else if (rank == BXT_DRAM_RANK_DUAL)
> > > +			ch_rank = I915_DRAM_RANK_DUAL;
> > > +		else
> > > +			ch_rank = I915_DRAM_RANK_INVALID;
> > > +
> > > +		if (size == BXT_DRAM_SIZE_4GB)
> > > +			size = 4;
> > > +		else if (size == BXT_DRAM_SIZE_6GB)
> > > +			size = 6;
> > > +		else if (size == BXT_DRAM_SIZE_8GB)
> > > +			size = 8;
> > > +		else if (size == BXT_DRAM_SIZE_12GB)
> > > +			size = 12;
> > > +		else if (size == BXT_DRAM_SIZE_16GB)
> > > +			size = 16;
> > > +		else
> > > +			size = 0;
> > > +
> > > +		width = (1 << width) * 8;
> > > +		DRM_DEBUG_KMS("dram size:%dGB width:X%d rank:%s\n", size,
> > > +			      width, rank == BXT_DRAM_RANK_SINGLE ? "single" :
> > > +			      rank == BXT_DRAM_RANK_DUAL ? "dual" : "unknown");
> > > +
> > > +		/*
> > > +		 * If any of the channel is single rank channel,
> > > +		 * worst case output will be same as if single rank
> > > +		 * memory, so consider single rank memory.
> > > +		 */
> > > +		if (dram_info->rank == I915_DRAM_RANK_INVALID)
> > > +			dram_info->rank = ch_rank;
> > > +		else if (ch_rank == I915_DRAM_RANK_SINGLE)
> > > +			dram_info->rank = I915_DRAM_RANK_SINGLE;
> > > +	}
> > > +
> > > +	if (dram_info->rank == I915_DRAM_RANK_INVALID) {
> > > +		DRM_INFO("couldn't get memory rank information\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	dram_info->valid = true;
> > > +	return 0;
> > > +}
> > > +
> > > +static void
> > > +intel_get_dram_info(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > > +	int ret;
> > > +
> > > +	dram_info->valid = false;
> > > +	dram_info->rank = I915_DRAM_RANK_INVALID;
> > > +	dram_info->bandwidth_kbps = 0;
> > > +	dram_info->num_channels = 0;
> > > +
> > > +	if (!IS_BROXTON(dev_priv))
> > > +		return;
> > > +
> > > +	ret = bxt_get_dram_info(dev_priv);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	DRM_DEBUG_KMS("DRAM bandwidth:%d KBps, total-channels: %u\n",
> > > +		      dram_info->bandwidth_kbps, dram_info->num_channels);
> > > +	DRM_DEBUG_KMS("DRAM rank: %s rank\n",
> > > +		      (dram_info->rank == I915_DRAM_RANK_DUAL) ?
> > > +		      "dual" : "single");
> > > +}
> > > +
> > >   /**
> > >    * i915_driver_init_hw - setup state requiring device access
> > >    * @dev_priv: device private
> > > @@ -1187,6 +1297,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
> > >   		goto err_msi;
> > > 
> > >   	intel_opregion_setup(dev_priv);
> > > +	/*
> > > +	 * Fill the dram structure to get the system raw bandwidth and
> > > +	 * dram info. This will be used for memory latency calculation.
> > > +	 */
> > > +	intel_get_dram_info(dev_priv);
> > > +
> > > 
> > >   	return 0;
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0f49f9988dfa..46f942fa7d60 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1904,6 +1904,17 @@ struct drm_i915_private {
> > >   		bool distrust_bios_wm;
> > >   	} wm;
> > > 
> > > +	struct dram_info {
> > > +		bool valid;
> > > +		u8 num_channels;
> > > +		enum dram_rank {
> > > +			I915_DRAM_RANK_INVALID = 0,
> > > +			I915_DRAM_RANK_SINGLE,
> > > +			I915_DRAM_RANK_DUAL
> > > +		} rank;
> > > +		u32 bandwidth_kbps;
> > > +	} dram_info;
> > > +
> > >   	struct i915_runtime_pm runtime_pm;
> > > 
> > >   	struct {
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 5530c470f30d..66900d027570 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9634,6 +9634,36 @@ enum skl_power_gate {
> > >   #define  DC_STATE_DEBUG_MASK_CORES	(1 << 0)
> > >   #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1 << 1)
> > > 
> > > +#define BXT_P_CR_MC_BIOS_REQ_0_0_0	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7114)
> > > +#define  BXT_REQ_DATA_MASK			0x3F
> > > +#define  BXT_DRAM_CHANNEL_ACTIVE_SHIFT		12
> > > +#define  BXT_DRAM_CHANNEL_ACTIVE_MASK		0xF
> > Thanks a lot for the spec pointers. But now that I opened it and
> > started to reading here it was hard to review and I noticed this
> > is because the way that it is defined here doesn't respect our
> > standards as documented:
> > 
> > "
> > * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Define bit field
> >   * contents so that they are already shifted in place, and can be directly
> >   * OR'd.
> > "
> So I'll modify all "_MASK" defines to make them shifted in place, Should I
> redefine all the bit definitions also to be shifted?
> BTW these bit-fields are readonly for driver,

Yes, please define everything with right shift in place so it gets easier to
read.

Thanks,
Rodrigo.

> 
> -Mahesh
> > 
> > So please provide a new version that follows the rules and
> > that it is easier to review. And sorry for not noticing this
> > on yesterday's review.
> > 
> > > +#define  BXT_MEMORY_FREQ_MULTIPLIER_HZ		133333333
> > > +
> > > +#define BXT_D_CR_DRP0_DUNIT8			0x1000
> > > +#define BXT_D_CR_DRP0_DUNIT9			0x1200
> > > +#define  BXT_D_CR_DRP0_DUNIT_START		8
> > > +#define  BXT_D_CR_DRP0_DUNIT_END		11
> > > +#define BXT_D_CR_DRP0_DUNIT(x)	_MMIO(MCHBAR_MIRROR_BASE_SNB + \
> > > +				      _PICK_EVEN((x) - 8, BXT_D_CR_DRP0_DUNIT8,\
> > > +						 BXT_D_CR_DRP0_DUNIT9))
> > > +#define  BXT_DRAM_RANK_MASK			0x3
> > > +#define  BXT_DRAM_RANK_SINGLE			0x1
> > > +#define  BXT_DRAM_RANK_DUAL			0x3
> > > +#define  BXT_DRAM_WIDTH_MASK			0x3
> > > +#define  BXT_DRAM_WIDTH_SHIFT			4
> > > +#define  BXT_DRAM_WIDTH_X8			0x0
> > > +#define  BXT_DRAM_WIDTH_X16			0x1
> > > +#define  BXT_DRAM_WIDTH_X32			0x2
> > > +#define  BXT_DRAM_WIDTH_X64			0x3
> > > +#define  BXT_DRAM_SIZE_MASK			0x7
> > > +#define  BXT_DRAM_SIZE_SHIFT			6
> > > +#define  BXT_DRAM_SIZE_4GB			0x0
> > > +#define  BXT_DRAM_SIZE_6GB			0x1
> > > +#define  BXT_DRAM_SIZE_8GB			0x2
> > > +#define  BXT_DRAM_SIZE_12GB			0x3
> > > +#define  BXT_DRAM_SIZE_16GB			0x4
> > > +
> > >   /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
> > >    * since on HSW we can't write to it using I915_WRITE. */
> > >   #define D_COMP_HSW			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> > > --
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Kumar, Mahesh]

Hi,


On 8/21/2018 8:27 PM, Kumar, Mahesh wrote:
> Hi,
>
>
> On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
>> On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
>>> IPC may cause underflows if not used with dual channel symmetric
>>> memory configuration. Disable IPC for non symmetric configurations in
>>> affected platforms.
>>> Display WA #1141
>>>
>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c | 43 
>>> ++++++++++++++++++++++++++++++++++++-----
>>>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>>>   drivers/gpu/drm/i915/intel_pm.c |  2 +-
>>>   3 files changed, 40 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index 86bc2e685522..2273664166bc 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct 
>>> dram_channel_info *ch, u32 val)
>>>       return 0;
>>>   }
>>>
>>> +static bool
>>> +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
>>> +              u32 val_ch0, u32 val_ch1,
>>> +              struct dram_channel_info *ch0)
>> what about
>> intel_is_dram_symmetric() ?
> sounds good.
>>> +{
>>> +    /* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
>> move this to the wa call, not the the function check...
>>
>>> +    if (INTEL_GEN(dev_priv) > 9 &&
>>> +        !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
>> please don't add CNL pre-prod wa
> ok sure
>>
>>> +        return true;
>>> +
>>> +    if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
>>> +        return true;
>> actually remove all platforms checks here...
> Agree will remove these checks, function will just return if memory is 
> symmetric.
>>
>>> +
>>> +    if (val_ch0 != val_ch1)
>>> +        return false;
>>> +
>>> +    if (ch0->s_info.size == 0)
>>> +        return true;
>>> +    if (ch0->l_info.size == ch0->s_info.size &&
>>> +        ch0->l_info.width == ch0->s_info.width &&
>>> +        ch0->l_info.rank == ch0->s_info.rank)
>>> +        return true;
>>> +
>>> +    return false;
>> return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
>>         (ch0->l_info.size == ch0->s_info.size &&
>>           ch0->l_info.width == ch0->s_info.width &&
>>           ch0->l_info.rank == ch0->s_info.rank)))
>>
>>> +}
>>> +
>>>   static int
>>>   skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>>>   {
>>>       struct dram_info *dram_info = &dev_priv->dram_info;
>>>       struct dram_channel_info ch0, ch1;
>>> -    u32 val;
>>> +    u32 val_ch0, val_ch1;
>>>       int ret;
>>>
>>> -    val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>>> -    ret = skl_dram_get_channel_info(&ch0, val);
>>> +    val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>>> +    ret = skl_dram_get_channel_info(&ch0, val_ch0);
>>>       if (ret == 0)
>>>           dram_info->num_channels++;
>>>
>>> -    val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>>> -    ret = skl_dram_get_channel_info(&ch1, val);
>>> +    val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>>> +    ret = skl_dram_get_channel_info(&ch1, val_ch1);
>>>       if (ret == 0)
>>>           dram_info->num_channels++;
>>>
>>> @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct 
>>> drm_i915_private *dev_priv)
>>>       if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
>>>           dram_info->is_16gb_dimm = true;
>>>
>>> +    if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
>>> +        dev_priv->ipc_capable_mem = true;
>>> +    else
>>> +        dev_priv->ipc_capable_mem = false;
>>> +
>>> +    DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
>>> +              dev_priv->ipc_capable_mem ? "" : "not ");
>>>       return 0;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 854f3c828e01..036d6554c017 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2112,6 +2112,7 @@ struct drm_i915_private {
>>>       bool chv_phy_assert[2];
>>>
>>>       bool ipc_enabled;
>>> +    bool ipc_capable_mem;
>> I don't think we need to stage this...
> With above changes storing this info is not needed, But we need to 
> keep a flag in dram_info to check if "memory is symmetric", Otherwise 
> we need to compute all the memory related info again.
>
> -Mahesh
>>>       /* Used to save the pipe-to-encoder mapping for audio */
>>>       struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>>> b/drivers/gpu/drm/i915/intel_pm.c
>>> index 2446f53adf21..39e400d5f555 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct drm_i915_private 
>>> *dev_priv)
>>>       u32 val;
>>>
>>>       /* Display WA #0477 WaDisableIPC: skl */
>>> -    if (IS_SKYLAKE(dev_priv)) {
>>> +    if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
>>>           dev_priv->ipc_enabled = false;
>> This is not the WA 1141 for other platforms than SKL. Please only 
>> keep skl here.
>>
>> For the other WA add 4us across all watermark levels
>>
>> /* Display WA #1141: skl,kbl,cfl */
>> if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
>>     intel_is_dram_symmetric())
>>     levels += 4;
Adding 4us in latency is already taken care in skl_compute_plane_wm.
Will handle here, enabling of IPC only if memory is symmetric.

-Mahesh
>>
>>>           return;
>>>       }
>>> -- 
>>> 2.16.2
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Rodrigo Vivi]

On Tue, Aug 21, 2018 at 09:30:21PM +0530, Kumar, Mahesh wrote:
> Hi,
> 
> 
> On 8/21/2018 8:27 PM, Kumar, Mahesh wrote:
> > Hi,
> > 
> > 
> > On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
> > > On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
> > > > IPC may cause underflows if not used with dual channel symmetric
> > > > memory configuration. Disable IPC for non symmetric configurations in
> > > > affected platforms.
> > > > Display WA #1141
> > > > 
> > > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/i915_drv.c | 43
> > > > ++++++++++++++++++++++++++++++++++++-----
> > > >   drivers/gpu/drm/i915/i915_drv.h |  1 +
> > > >   drivers/gpu/drm/i915/intel_pm.c |  2 +-
> > > >   3 files changed, 40 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 86bc2e685522..2273664166bc 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct
> > > > dram_channel_info *ch, u32 val)
> > > >       return 0;
> > > >   }
> > > > 
> > > > +static bool
> > > > +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
> > > > +              u32 val_ch0, u32 val_ch1,
> > > > +              struct dram_channel_info *ch0)
> > > what about
> > > intel_is_dram_symmetric() ?
> > sounds good.
> > > > +{
> > > > +    /* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
> > > move this to the wa call, not the the function check...
> > > 
> > > > +    if (INTEL_GEN(dev_priv) > 9 &&
> > > > +        !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
> > > please don't add CNL pre-prod wa
> > ok sure
> > > 
> > > > +        return true;
> > > > +
> > > > +    if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
> > > > +        return true;
> > > actually remove all platforms checks here...
> > Agree will remove these checks, function will just return if memory is
> > symmetric.
> > > 
> > > > +
> > > > +    if (val_ch0 != val_ch1)
> > > > +        return false;
> > > > +
> > > > +    if (ch0->s_info.size == 0)
> > > > +        return true;
> > > > +    if (ch0->l_info.size == ch0->s_info.size &&
> > > > +        ch0->l_info.width == ch0->s_info.width &&
> > > > +        ch0->l_info.rank == ch0->s_info.rank)
> > > > +        return true;
> > > > +
> > > > +    return false;
> > > return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
> > >         (ch0->l_info.size == ch0->s_info.size &&
> > >           ch0->l_info.width == ch0->s_info.width &&
> > >           ch0->l_info.rank == ch0->s_info.rank)))
> > > 
> > > > +}
> > > > +
> > > >   static int
> > > >   skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> > > >   {
> > > >       struct dram_info *dram_info = &dev_priv->dram_info;
> > > >       struct dram_channel_info ch0, ch1;
> > > > -    u32 val;
> > > > +    u32 val_ch0, val_ch1;
> > > >       int ret;
> > > > 
> > > > -    val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > > > -    ret = skl_dram_get_channel_info(&ch0, val);
> > > > +    val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > > > +    ret = skl_dram_get_channel_info(&ch0, val_ch0);
> > > >       if (ret == 0)
> > > >           dram_info->num_channels++;
> > > > 
> > > > -    val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > > > -    ret = skl_dram_get_channel_info(&ch1, val);
> > > > +    val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > > > +    ret = skl_dram_get_channel_info(&ch1, val_ch1);
> > > >       if (ret == 0)
> > > >           dram_info->num_channels++;
> > > > 
> > > > @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct
> > > > drm_i915_private *dev_priv)
> > > >       if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
> > > >           dram_info->is_16gb_dimm = true;
> > > > 
> > > > +    if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
> > > > +        dev_priv->ipc_capable_mem = true;
> > > > +    else
> > > > +        dev_priv->ipc_capable_mem = false;
> > > > +
> > > > +    DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
> > > > +              dev_priv->ipc_capable_mem ? "" : "not ");
> > > >       return 0;
> > > >   }
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 854f3c828e01..036d6554c017 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2112,6 +2112,7 @@ struct drm_i915_private {
> > > >       bool chv_phy_assert[2];
> > > > 
> > > >       bool ipc_enabled;
> > > > +    bool ipc_capable_mem;
> > > I don't think we need to stage this...
> > With above changes storing this info is not needed, But we need to keep
> > a flag in dram_info to check if "memory is symmetric", Otherwise we need
> > to compute all the memory related info again.
> > 
> > -Mahesh
> > > >       /* Used to save the pipe-to-encoder mapping for audio */
> > > >       struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 2446f53adf21..39e400d5f555 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct
> > > > drm_i915_private *dev_priv)
> > > >       u32 val;
> > > > 
> > > >       /* Display WA #0477 WaDisableIPC: skl */
> > > > -    if (IS_SKYLAKE(dev_priv)) {
> > > > +    if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
> > > >           dev_priv->ipc_enabled = false;
> > > This is not the WA 1141 for other platforms than SKL. Please only
> > > keep skl here.
> > > 
> > > For the other WA add 4us across all watermark levels
> > > 
> > > /* Display WA #1141: skl,kbl,cfl */
> > > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
> > >     intel_is_dram_symmetric())
> > >     levels += 4;
> Adding 4us in latency is already taken care in skl_compute_plane_wm.
> Will handle here, enabling of IPC only if memory is symmetric.

But WA1141 describe that we only need to avoid IPC on SKL...
not on every gen9... Everywhere else the 4s should be enough.

so if the 4us is already in place maybe the patch that we need
is to only apply the 4us if memory is symmetric and not skl.

> 
> -Mahesh
> > > 
> > > >           return;
> > > >       }
> > > > -- 
> > > > 2.16.2
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Kumar, Mahesh]

Hi,


On 8/22/2018 12:26 AM, Rodrigo Vivi wrote:
> On Tue, Aug 21, 2018 at 09:30:21PM +0530, Kumar, Mahesh wrote:
>> Hi,
>>
>>
>> On 8/21/2018 8:27 PM, Kumar, Mahesh wrote:
>>> Hi,
>>>
>>>
>>> On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
>>>> On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
>>>>> IPC may cause underflows if not used with dual channel symmetric
>>>>> memory configuration. Disable IPC for non symmetric configurations in
>>>>> affected platforms.
>>>>> Display WA #1141
>>>>>
>>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_drv.c | 43
>>>>> ++++++++++++++++++++++++++++++++++++-----
>>>>>    drivers/gpu/drm/i915/i915_drv.h |  1 +
>>>>>    drivers/gpu/drm/i915/intel_pm.c |  2 +-
>>>>>    3 files changed, 40 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>>> index 86bc2e685522..2273664166bc 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>> @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct
>>>>> dram_channel_info *ch, u32 val)
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> +static bool
>>>>> +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
>>>>> +              u32 val_ch0, u32 val_ch1,
>>>>> +              struct dram_channel_info *ch0)
>>>> what about
>>>> intel_is_dram_symmetric() ?
>>> sounds good.
>>>>> +{
>>>>> +    /* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
>>>> move this to the wa call, not the the function check...
>>>>
>>>>> +    if (INTEL_GEN(dev_priv) > 9 &&
>>>>> +        !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
>>>> please don't add CNL pre-prod wa
>>> ok sure
>>>>> +        return true;
>>>>> +
>>>>> +    if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
>>>>> +        return true;
>>>> actually remove all platforms checks here...
>>> Agree will remove these checks, function will just return if memory is
>>> symmetric.
>>>>> +
>>>>> +    if (val_ch0 != val_ch1)
>>>>> +        return false;
>>>>> +
>>>>> +    if (ch0->s_info.size == 0)
>>>>> +        return true;
>>>>> +    if (ch0->l_info.size == ch0->s_info.size &&
>>>>> +        ch0->l_info.width == ch0->s_info.width &&
>>>>> +        ch0->l_info.rank == ch0->s_info.rank)
>>>>> +        return true;
>>>>> +
>>>>> +    return false;
>>>> return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
>>>>          (ch0->l_info.size == ch0->s_info.size &&
>>>>            ch0->l_info.width == ch0->s_info.width &&
>>>>            ch0->l_info.rank == ch0->s_info.rank)))
>>>>
>>>>> +}
>>>>> +
>>>>>    static int
>>>>>    skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
>>>>>    {
>>>>>        struct dram_info *dram_info = &dev_priv->dram_info;
>>>>>        struct dram_channel_info ch0, ch1;
>>>>> -    u32 val;
>>>>> +    u32 val_ch0, val_ch1;
>>>>>        int ret;
>>>>>
>>>>> -    val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>>>>> -    ret = skl_dram_get_channel_info(&ch0, val);
>>>>> +    val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>>>>> +    ret = skl_dram_get_channel_info(&ch0, val_ch0);
>>>>>        if (ret == 0)
>>>>>            dram_info->num_channels++;
>>>>>
>>>>> -    val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>>>>> -    ret = skl_dram_get_channel_info(&ch1, val);
>>>>> +    val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>>>>> +    ret = skl_dram_get_channel_info(&ch1, val_ch1);
>>>>>        if (ret == 0)
>>>>>            dram_info->num_channels++;
>>>>>
>>>>> @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct
>>>>> drm_i915_private *dev_priv)
>>>>>        if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
>>>>>            dram_info->is_16gb_dimm = true;
>>>>>
>>>>> +    if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
>>>>> +        dev_priv->ipc_capable_mem = true;
>>>>> +    else
>>>>> +        dev_priv->ipc_capable_mem = false;
>>>>> +
>>>>> +    DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
>>>>> +              dev_priv->ipc_capable_mem ? "" : "not ");
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index 854f3c828e01..036d6554c017 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -2112,6 +2112,7 @@ struct drm_i915_private {
>>>>>        bool chv_phy_assert[2];
>>>>>
>>>>>        bool ipc_enabled;
>>>>> +    bool ipc_capable_mem;
>>>> I don't think we need to stage this...
>>> With above changes storing this info is not needed, But we need to keep
>>> a flag in dram_info to check if "memory is symmetric", Otherwise we need
>>> to compute all the memory related info again.
>>>
>>> -Mahesh
>>>>>        /* Used to save the pipe-to-encoder mapping for audio */
>>>>>        struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>>>> b/drivers/gpu/drm/i915/intel_pm.c
>>>>> index 2446f53adf21..39e400d5f555 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>> @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct
>>>>> drm_i915_private *dev_priv)
>>>>>        u32 val;
>>>>>
>>>>>        /* Display WA #0477 WaDisableIPC: skl */
>>>>> -    if (IS_SKYLAKE(dev_priv)) {
>>>>> +    if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
>>>>>            dev_priv->ipc_enabled = false;
>>>> This is not the WA 1141 for other platforms than SKL. Please only
>>>> keep skl here.
>>>>
>>>> For the other WA add 4us across all watermark levels
>>>>
>>>> /* Display WA #1141: skl,kbl,cfl */
>>>> if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
>>>>      intel_is_dram_symmetric())
>>>>      levels += 4;
>> Adding 4us in latency is already taken care in skl_compute_plane_wm.
>> Will handle here, enabling of IPC only if memory is symmetric.
> But WA1141 describe that we only need to avoid IPC on SKL...
> not on every gen9... Everywhere else the 4s should be enough.
>
> so if the 4us is already in place maybe the patch that we need
> is to only apply the 4us if memory is symmetric and not skl.
WA also says enable IPC only if memory is symmetric,
<KBL WA: When IPC is enabled, watermark latency values must be increased 
by 4us across all levels.
IPC must be enabled only with dual channel symmetric memory configurations.>

-Mahesh
>
>> -Mahesh
>>>>>            return;
>>>>>        }
>>>>> -- 
>>>>> 2.16.2
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/5] drm/i915/kbl+: Enable IPC only for symmetric memory configurations

[Rodrigo Vivi]

On Wed, Aug 22, 2018 at 06:32:33PM +0530, Kumar, Mahesh wrote:
> Hi,
> 
> 
> On 8/22/2018 12:26 AM, Rodrigo Vivi wrote:
> > On Tue, Aug 21, 2018 at 09:30:21PM +0530, Kumar, Mahesh wrote:
> > > Hi,
> > > 
> > > 
> > > On 8/21/2018 8:27 PM, Kumar, Mahesh wrote:
> > > > Hi,
> > > > 
> > > > 
> > > > On 8/17/2018 11:50 PM, Rodrigo Vivi wrote:
> > > > > On Thu, Jul 26, 2018 at 07:44:09PM +0530, Mahesh Kumar wrote:
> > > > > > IPC may cause underflows if not used with dual channel symmetric
> > > > > > memory configuration. Disable IPC for non symmetric configurations in
> > > > > > affected platforms.
> > > > > > Display WA #1141
> > > > > > 
> > > > > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/i915_drv.c | 43
> > > > > > ++++++++++++++++++++++++++++++++++++-----
> > > > > >    drivers/gpu/drm/i915/i915_drv.h |  1 +
> > > > > >    drivers/gpu/drm/i915/intel_pm.c |  2 +-
> > > > > >    3 files changed, 40 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > index 86bc2e685522..2273664166bc 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > @@ -1141,21 +1141,47 @@ skl_dram_get_channel_info(struct
> > > > > > dram_channel_info *ch, u32 val)
> > > > > >        return 0;
> > > > > >    }
> > > > > > 
> > > > > > +static bool
> > > > > > +intel_is_dram_ipc_capable(struct drm_i915_private *dev_priv,
> > > > > > +              u32 val_ch0, u32 val_ch1,
> > > > > > +              struct dram_channel_info *ch0)
> > > > > what about
> > > > > intel_is_dram_symmetric() ?
> > > > sounds good.
> > > > > > +{
> > > > > > +    /* Display WA #1141: SKL:all KBL:all CNL:A CNL:B */
> > > > > move this to the wa call, not the the function check...
> > > > > 
> > > > > > +    if (INTEL_GEN(dev_priv) > 9 &&
> > > > > > +        !IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0))
> > > > > please don't add CNL pre-prod wa
> > > > ok sure
> > > > > > +        return true;
> > > > > > +
> > > > > > +    if (!IS_KABYLAKE(dev_priv) && !IS_SKYLAKE(dev_priv))
> > > > > > +        return true;
> > > > > actually remove all platforms checks here...
> > > > Agree will remove these checks, function will just return if memory is
> > > > symmetric.
> > > > > > +
> > > > > > +    if (val_ch0 != val_ch1)
> > > > > > +        return false;
> > > > > > +
> > > > > > +    if (ch0->s_info.size == 0)
> > > > > > +        return true;
> > > > > > +    if (ch0->l_info.size == ch0->s_info.size &&
> > > > > > +        ch0->l_info.width == ch0->s_info.width &&
> > > > > > +        ch0->l_info.rank == ch0->s_info.rank)
> > > > > > +        return true;
> > > > > > +
> > > > > > +    return false;
> > > > > return (val_ch0 == val_ch1 && (ch0->s_info.size == 0 ||
> > > > >          (ch0->l_info.size == ch0->s_info.size &&
> > > > >            ch0->l_info.width == ch0->s_info.width &&
> > > > >            ch0->l_info.rank == ch0->s_info.rank)))
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > >    static int
> > > > > >    skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> > > > > >    {
> > > > > >        struct dram_info *dram_info = &dev_priv->dram_info;
> > > > > >        struct dram_channel_info ch0, ch1;
> > > > > > -    u32 val;
> > > > > > +    u32 val_ch0, val_ch1;
> > > > > >        int ret;
> > > > > > 
> > > > > > -    val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > > > > > -    ret = skl_dram_get_channel_info(&ch0, val);
> > > > > > +    val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > > > > > +    ret = skl_dram_get_channel_info(&ch0, val_ch0);
> > > > > >        if (ret == 0)
> > > > > >            dram_info->num_channels++;
> > > > > > 
> > > > > > -    val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > > > > > -    ret = skl_dram_get_channel_info(&ch1, val);
> > > > > > +    val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > > > > > +    ret = skl_dram_get_channel_info(&ch1, val_ch1);
> > > > > >        if (ret == 0)
> > > > > >            dram_info->num_channels++;
> > > > > > 
> > > > > > @@ -1185,6 +1211,13 @@ skl_dram_get_channels_info(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > >        if (ch0.is_16gb_dimm || ch1.is_16gb_dimm)
> > > > > >            dram_info->is_16gb_dimm = true;
> > > > > > 
> > > > > > +    if (intel_is_dram_ipc_capable(dev_priv, val_ch0, val_ch1, &ch0))
> > > > > > +        dev_priv->ipc_capable_mem = true;
> > > > > > +    else
> > > > > > +        dev_priv->ipc_capable_mem = false;
> > > > > > +
> > > > > > +    DRM_DEBUG_KMS("memory configuration is %sIPC capable\n",
> > > > > > +              dev_priv->ipc_capable_mem ? "" : "not ");
> > > > > >        return 0;
> > > > > >    }
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 854f3c828e01..036d6554c017 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -2112,6 +2112,7 @@ struct drm_i915_private {
> > > > > >        bool chv_phy_assert[2];
> > > > > > 
> > > > > >        bool ipc_enabled;
> > > > > > +    bool ipc_capable_mem;
> > > > > I don't think we need to stage this...
> > > > With above changes storing this info is not needed, But we need to keep
> > > > a flag in dram_info to check if "memory is symmetric", Otherwise we need
> > > > to compute all the memory related info again.
> > > > 
> > > > -Mahesh
> > > > > >        /* Used to save the pipe-to-encoder mapping for audio */
> > > > > >        struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 2446f53adf21..39e400d5f555 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -6097,7 +6097,7 @@ void intel_enable_ipc(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > >        u32 val;
> > > > > > 
> > > > > >        /* Display WA #0477 WaDisableIPC: skl */
> > > > > > -    if (IS_SKYLAKE(dev_priv)) {
> > > > > > +    if (IS_SKYLAKE(dev_priv) || !dev_priv->ipc_capable_mem) {
> > > > > >            dev_priv->ipc_enabled = false;
> > > > > This is not the WA 1141 for other platforms than SKL. Please only
> > > > > keep skl here.
> > > > > 
> > > > > For the other WA add 4us across all watermark levels
> > > > > 
> > > > > /* Display WA #1141: skl,kbl,cfl */
> > > > > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
> > > > >      intel_is_dram_symmetric())
> > > > >      levels += 4;
> > > Adding 4us in latency is already taken care in skl_compute_plane_wm.
> > > Will handle here, enabling of IPC only if memory is symmetric.
> > But WA1141 describe that we only need to avoid IPC on SKL...
> > not on every gen9... Everywhere else the 4s should be enough.
> > 
> > so if the 4us is already in place maybe the patch that we need
> > is to only apply the 4us if memory is symmetric and not skl.
> WA also says enable IPC only if memory is symmetric,
> <KBL WA: When IPC is enabled, watermark latency values must be increased by
> 4us across all levels.
> IPC must be enabled only with dual channel symmetric memory configurations.>

Oh! Indeed. I'm sorry..

so we need something like:

/* Display WA #0477 WaDisableIPC: skl */
/* Display WA #1141: skl,kbl,cfl */
 if (IS_SKYLAKE(dev_priv) ||
    ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) &&
      !intel_is_dram_symmetric())
     	  dev_priv->ipc_enabled = false;

> 
> -Mahesh
> > 
> > > -Mahesh
> > > > > >            return;
> > > > > >        }
> > > > > > -- 
> > > > > > 2.16.2
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx