[Intel-gfx] [PATCH 1/4] drm/i915: Nuke not needed members of dram_info

[Intel-gfx] [PATCH 1/4] drm/i915: Nuke not needed members of dram_info

[José Roberto de Souza]

Valid, ranks and bandwidth_kbps are set into dram_info but are not
used anywhere else so nuking it.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c   |  4 +--
 drivers/gpu/drm/i915/i915_drv.h   |  3 --
 drivers/gpu/drm/i915/intel_dram.c | 47 +++++++------------------------
 3 files changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f5666b44ea9d..a1cc60de99f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -609,8 +609,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
 
 	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.
+	 * Fill the dram structure to get the system dram info. This will be
+	 * used for memory latency calculation.
 	 */
 	intel_dram_detect(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8376cff5ba86..250e92910fa1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1134,11 +1134,8 @@ struct drm_i915_private {
 	} wm;
 
 	struct dram_info {
-		bool valid;
 		bool is_16gb_dimm;
 		u8 num_channels;
-		u8 ranks;
-		u32 bandwidth_kbps;
 		bool symmetric_memory;
 		enum intel_dram_type {
 			INTEL_DRAM_UNKNOWN,
diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
index 4754296a250e..694fbd8c9cd4 100644
--- a/drivers/gpu/drm/i915/intel_dram.c
+++ b/drivers/gpu/drm/i915/intel_dram.c
@@ -201,17 +201,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915)
 		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.ranks == 1 || ch1.ranks == 1)
-		dram_info->ranks = 1;
-	else
-		dram_info->ranks = max(ch0.ranks, ch1.ranks);
-
-	if (dram_info->ranks == 0) {
+	if (ch0.ranks == 0 && ch1.ranks == 0) {
 		drm_info(&i915->drm, "couldn't get memory rank information\n");
 		return -EINVAL;
 	}
@@ -269,16 +259,12 @@ skl_get_dram_info(struct drm_i915_private *i915)
 	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) {
+	if (dram_info->num_channels * mem_freq_khz == 0) {
 		drm_info(&i915->drm,
 			 "Couldn't get system memory bandwidth\n");
 		return -EINVAL;
 	}
 
-	dram_info->valid = true;
 	return 0;
 }
 
@@ -365,7 +351,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
 	struct dram_info *dram_info = &i915->dram_info;
 	u32 dram_channels;
 	u32 mem_freq_khz, val;
-	u8 num_active_channels;
+	u8 num_active_channels, valid_ranks = 0;
 	int i;
 
 	val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0);
@@ -375,10 +361,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
 	dram_channels = val & 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) {
+	if (mem_freq_khz * num_active_channels == 0) {
 		drm_info(&i915->drm,
 			 "Couldn't get system memory bandwidth\n");
 		return -EINVAL;
@@ -410,27 +393,18 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
 			    dimm.size, dimm.width, dimm.ranks,
 			    intel_dram_type_str(type));
 
-		/*
-		 * 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->ranks == 0)
-			dram_info->ranks = dimm.ranks;
-		else if (dimm.ranks == 1)
-			dram_info->ranks = 1;
+		if (valid_ranks == 0)
+			valid_ranks = dimm.ranks;
 
 		if (type != INTEL_DRAM_UNKNOWN)
 			dram_info->type = type;
 	}
 
-	if (dram_info->type == INTEL_DRAM_UNKNOWN || dram_info->ranks == 0) {
+	if (dram_info->type == INTEL_DRAM_UNKNOWN || valid_ranks == 0) {
 		drm_info(&i915->drm, "couldn't get memory information\n");
 		return -EINVAL;
 	}
 
-	dram_info->valid = true;
-
 	return 0;
 }
 
@@ -456,11 +430,10 @@ void intel_dram_detect(struct drm_i915_private *i915)
 	if (ret)
 		return;
 
-	drm_dbg_kms(&i915->drm, "DRAM bandwidth: %u kBps, channels: %u\n",
-		    dram_info->bandwidth_kbps, dram_info->num_channels);
+	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
 
-	drm_dbg_kms(&i915->drm, "DRAM ranks: %u, 16Gb DIMMs: %s\n",
-		    dram_info->ranks, yesno(dram_info->is_16gb_dimm));
+	drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n",
+		    yesno(dram_info->is_16gb_dimm));
 }
 
 static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap)
-- 
2.30.0

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

[Intel-gfx] [PATCH 2/4] drm/i915/gen11+: Only load DRAM information from pcode

[José Roberto de Souza]

Up to now we were reading some DRAM information from MCHBAR register
and from pcode what is already not good but some GEN12(TGL-H and ADL-S)
platforms have MCHBAR DRAM information in different offsets.

This was notified to HW team that decided that the best alternative is
always apply the 16gb_dimm watermark adjustment for GEN12+ platforms
and read the remaning DRAM information needed to other display
programming from pcode.

So here moving the DRAM pcode function to intel_dram.c, removing
the duplicated fields from intel_qgv_info, setting and using
information from dram_info.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 98 +++++--------------------
 drivers/gpu/drm/i915/i915_drv.c         |  5 +-
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_dram.c       | 77 ++++++++++++++++++-
 4 files changed, 97 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index bd060404d249..1368bd96ed73 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -19,77 +19,9 @@ struct intel_qgv_point {
 
 struct intel_qgv_info {
 	struct intel_qgv_point points[I915_NUM_QGV_POINTS];
-	u8 num_points;
-	u8 num_channels;
 	u8 t_bl;
-	enum intel_dram_type dram_type;
 };
 
-static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv,
-					  struct intel_qgv_info *qi)
-{
-	u32 val = 0;
-	int ret;
-
-	ret = sandybridge_pcode_read(dev_priv,
-				     ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
-				     ICL_PCODE_MEM_SS_READ_GLOBAL_INFO,
-				     &val, NULL);
-	if (ret)
-		return ret;
-
-	if (IS_GEN(dev_priv, 12)) {
-		switch (val & 0xf) {
-		case 0:
-			qi->dram_type = INTEL_DRAM_DDR4;
-			break;
-		case 3:
-			qi->dram_type = INTEL_DRAM_LPDDR4;
-			break;
-		case 4:
-			qi->dram_type = INTEL_DRAM_DDR3;
-			break;
-		case 5:
-			qi->dram_type = INTEL_DRAM_LPDDR3;
-			break;
-		default:
-			MISSING_CASE(val & 0xf);
-			break;
-		}
-	} else if (IS_GEN(dev_priv, 11)) {
-		switch (val & 0xf) {
-		case 0:
-			qi->dram_type = INTEL_DRAM_DDR4;
-			break;
-		case 1:
-			qi->dram_type = INTEL_DRAM_DDR3;
-			break;
-		case 2:
-			qi->dram_type = INTEL_DRAM_LPDDR3;
-			break;
-		case 3:
-			qi->dram_type = INTEL_DRAM_LPDDR4;
-			break;
-		default:
-			MISSING_CASE(val & 0xf);
-			break;
-		}
-	} else {
-		MISSING_CASE(INTEL_GEN(dev_priv));
-		qi->dram_type = INTEL_DRAM_LPDDR3; /* Conservative default */
-	}
-
-	qi->num_channels = (val & 0xf0) >> 4;
-	qi->num_points = (val & 0xf00) >> 8;
-
-	if (IS_GEN(dev_priv, 12))
-		qi->t_bl = qi->dram_type == INTEL_DRAM_DDR4 ? 4 : 16;
-	else if (IS_GEN(dev_priv, 11))
-		qi->t_bl = qi->dram_type == INTEL_DRAM_DDR4 ? 4 : 8;
-
-	return 0;
-}
-
 static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv,
 					 struct intel_qgv_point *sp,
 					 int point)
@@ -139,17 +71,19 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv,
 static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
 			      struct intel_qgv_info *qi)
 {
+	struct dram_info *dram_info = &dev_priv->dram_info;
 	int i, ret;
 
-	ret = icl_pcode_read_mem_global_info(dev_priv, qi);
-	if (ret)
-		return ret;
+	if (IS_GEN(dev_priv, 12))
+		qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 16;
+	else if (IS_GEN(dev_priv, 11))
+		qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 8;
 
 	if (drm_WARN_ON(&dev_priv->drm,
-			qi->num_points > ARRAY_SIZE(qi->points)))
-		qi->num_points = ARRAY_SIZE(qi->points);
+			dram_info->qgv_points > ARRAY_SIZE(qi->points)))
+		dram_info->qgv_points = ARRAY_SIZE(qi->points);
 
-	for (i = 0; i < qi->num_points; i++) {
+	for (i = 0; i < dram_info->qgv_points; i++) {
 		struct intel_qgv_point *sp = &qi->points[i];
 
 		ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i);
@@ -171,12 +105,13 @@ static int icl_calc_bw(int dclk, int num, int den)
 	return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6);
 }
 
-static int icl_sagv_max_dclk(const struct intel_qgv_info *qi)
+static int icl_sagv_max_dclk(struct drm_i915_private *dev_priv,
+			     const struct intel_qgv_info *qi)
 {
 	u16 dclk = 0;
 	int i;
 
-	for (i = 0; i < qi->num_points; i++)
+	for (i = 0; i < dev_priv->dram_info.qgv_points; i++)
 		dclk = max(dclk, qi->points[i].dclk);
 
 	return dclk;
@@ -207,6 +142,7 @@ static const struct intel_sa_info rkl_sa_info = {
 
 static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa)
 {
+	struct dram_info *dram_info = &dev_priv->dram_info;
 	struct intel_qgv_info qi = {};
 	bool is_y_tile = true; /* assume y tile may be used */
 	int num_channels;
@@ -222,10 +158,10 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
 			    "Failed to get memory subsystem information, ignoring bandwidth limits");
 		return ret;
 	}
-	num_channels = qi.num_channels;
+	num_channels = dram_info->num_channels;
 
 	deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2);
-	dclk_max = icl_sagv_max_dclk(&qi);
+	dclk_max = icl_sagv_max_dclk(dev_priv, &qi);
 
 	ipqdepthpch = 16;
 
@@ -241,9 +177,9 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
 		clpchgroup = (sa->deburst * deinterleave / num_channels) << i;
 		bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1;
 
-		bi->num_qgv_points = qi.num_points;
+		bi->num_qgv_points = dram_info->qgv_points;
 
-		for (j = 0; j < qi.num_points; j++) {
+		for (j = 0; j < dram_info->qgv_points; j++) {
 			const struct intel_qgv_point *sp = &qi.points[j];
 			int ct, bw;
 
@@ -274,7 +210,7 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
 	 * SAGV point, but we can't send PCode commands to restrict it
 	 * as it will fail and pointless anyway.
 	 */
-	if (qi.num_points == 1)
+	if (dram_info->qgv_points == 1)
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 	else
 		dev_priv->sagv_status = I915_SAGV_ENABLED;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a1cc60de99f0..66f763fe7a83 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -608,14 +608,15 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
 		goto err_msi;
 
 	intel_opregion_setup(dev_priv);
+
+	intel_pcode_init(dev_priv);
+
 	/*
 	 * Fill the dram structure to get the system dram info. This will be
 	 * used for memory latency calculation.
 	 */
 	intel_dram_detect(dev_priv);
 
-	intel_pcode_init(dev_priv);
-
 	intel_bw_init_hw(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 250e92910fa1..a2ae21082b34 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1144,6 +1144,7 @@ struct drm_i915_private {
 			INTEL_DRAM_LPDDR3,
 			INTEL_DRAM_LPDDR4
 		} type;
+		u8 qgv_points;
 	} dram_info;
 
 	struct intel_bw_info {
diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
index 694fbd8c9cd4..1298823c957c 100644
--- a/drivers/gpu/drm/i915/intel_dram.c
+++ b/drivers/gpu/drm/i915/intel_dram.c
@@ -5,6 +5,7 @@
 
 #include "i915_drv.h"
 #include "intel_dram.h"
+#include "intel_sideband.h"
 
 struct dram_dimm_info {
 	u16 size;
@@ -408,6 +409,78 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
 	return 0;
 }
 
+static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
+{
+	struct dram_info *dram_info = &dev_priv->dram_info;
+	u32 val = 0;
+	int ret;
+
+	ret = sandybridge_pcode_read(dev_priv,
+				     ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
+				     ICL_PCODE_MEM_SS_READ_GLOBAL_INFO,
+				     &val, NULL);
+	if (ret)
+		return ret;
+
+	if (IS_GEN(dev_priv, 12)) {
+		switch (val & 0xf) {
+		case 0:
+			dram_info->type = INTEL_DRAM_DDR4;
+			break;
+		case 3:
+			dram_info->type = INTEL_DRAM_LPDDR4;
+			break;
+		case 4:
+			dram_info->type = INTEL_DRAM_DDR3;
+			break;
+		case 5:
+			dram_info->type = INTEL_DRAM_LPDDR3;
+			break;
+		default:
+			MISSING_CASE(val & 0xf);
+			return -1;
+		}
+	} else {
+		switch (val & 0xf) {
+		case 0:
+			dram_info->type = INTEL_DRAM_DDR4;
+			break;
+		case 1:
+			dram_info->type = INTEL_DRAM_DDR3;
+			break;
+		case 2:
+			dram_info->type = INTEL_DRAM_LPDDR3;
+			break;
+		case 3:
+			dram_info->type = INTEL_DRAM_LPDDR4;
+			break;
+		default:
+			MISSING_CASE(val & 0xf);
+			return -1;
+		}
+	}
+
+	dram_info->num_channels = (val & 0xf0) >> 4;
+	dram_info->qgv_points = (val & 0xf00) >> 8;
+
+	return 0;
+}
+
+static int gen11_get_dram_info(struct drm_i915_private *i915)
+{
+	if (INTEL_GEN(i915) == 11) {
+		int ret = skl_get_dram_info(i915);
+
+		if (ret)
+			return ret;
+	} else {
+		/* Always needed for GEN12+ */
+		i915->dram_info.is_16gb_dimm = true;
+	}
+
+	return icl_pcode_read_mem_global_info(i915);
+}
+
 void intel_dram_detect(struct drm_i915_private *i915)
 {
 	struct dram_info *dram_info = &i915->dram_info;
@@ -423,7 +496,9 @@ void intel_dram_detect(struct drm_i915_private *i915)
 	if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915))
 		return;
 
-	if (IS_GEN9_LP(i915))
+	if (INTEL_GEN(i915) >= 11)
+		ret = gen11_get_dram_info(i915);
+	else if (IS_GEN9_LP(i915))
 		ret = bxt_get_dram_info(i915);
 	else
 		ret = skl_get_dram_info(i915);
-- 
2.30.0

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

[Intel-gfx] [PATCH 3/4] drm/i915: Fail driver probe when unable to load DRAM information

[José Roberto de Souza]

DRAM information is required to properly program display.
Before "drm/i915/gen11+: Only load DRAM information from pcode" we
were failing driver load if unable to fetch DRAM information from
pcode form GEN11+ but we should also extend it to GEN9 plaforms.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c   |  6 +++++-
 drivers/gpu/drm/i915/intel_dram.c | 13 +++++++++----
 drivers/gpu/drm/i915/intel_dram.h |  2 +-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66f763fe7a83..6bfcd3ee6c66 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -615,12 +615,16 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
 	 * Fill the dram structure to get the system dram info. This will be
 	 * used for memory latency calculation.
 	 */
-	intel_dram_detect(dev_priv);
+	ret = intel_dram_detect(dev_priv);
+	if (ret)
+		goto err_dram;
 
 	intel_bw_init_hw(dev_priv);
 
 	return 0;
 
+err_dram:
+	intel_gvt_driver_remove(dev_priv);
 err_msi:
 	if (pdev->msi_enabled)
 		pci_disable_msi(pdev);
diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
index 1298823c957c..4871d48589f9 100644
--- a/drivers/gpu/drm/i915/intel_dram.c
+++ b/drivers/gpu/drm/i915/intel_dram.c
@@ -481,7 +481,7 @@ static int gen11_get_dram_info(struct drm_i915_private *i915)
 	return icl_pcode_read_mem_global_info(i915);
 }
 
-void intel_dram_detect(struct drm_i915_private *i915)
+int intel_dram_detect(struct drm_i915_private *i915)
 {
 	struct dram_info *dram_info = &i915->dram_info;
 	int ret;
@@ -494,7 +494,7 @@ void intel_dram_detect(struct drm_i915_private *i915)
 	dram_info->is_16gb_dimm = !IS_GEN9_LP(i915);
 
 	if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915))
-		return;
+		return 0;
 
 	if (INTEL_GEN(i915) >= 11)
 		ret = gen11_get_dram_info(i915);
@@ -502,13 +502,18 @@ void intel_dram_detect(struct drm_i915_private *i915)
 		ret = bxt_get_dram_info(i915);
 	else
 		ret = skl_get_dram_info(i915);
-	if (ret)
-		return;
+
+	if (ret) {
+		drm_warn(&i915->drm, "Unable to load dram information\n");
+		return ret;
+	}
 
 	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
 
 	drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n",
 		    yesno(dram_info->is_16gb_dimm));
+
+	return 0;
 }
 
 static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap)
diff --git a/drivers/gpu/drm/i915/intel_dram.h b/drivers/gpu/drm/i915/intel_dram.h
index 4ba13c13162c..2a0f283b1a1d 100644
--- a/drivers/gpu/drm/i915/intel_dram.h
+++ b/drivers/gpu/drm/i915/intel_dram.h
@@ -9,6 +9,6 @@
 struct drm_i915_private;
 
 void intel_dram_edram_detect(struct drm_i915_private *i915);
-void intel_dram_detect(struct drm_i915_private *i915);
+int intel_dram_detect(struct drm_i915_private *i915);
 
 #endif /* __INTEL_DRAM_H__ */
-- 
2.30.0

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

[Intel-gfx] [PATCH 4/4] drm/i915: Rename is_16gb_dimm to wm_lv_0_adjust_needed

[José Roberto de Souza]

As it now it is always required for GEN12+ the is_16gb_dimm name
do not make sense for GEN12+.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  2 +-
 drivers/gpu/drm/i915/intel_dram.c | 10 +++++-----
 drivers/gpu/drm/i915/intel_pm.c   |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2ae21082b34..adc008c65b14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1134,7 +1134,7 @@ struct drm_i915_private {
 	} wm;
 
 	struct dram_info {
-		bool is_16gb_dimm;
+		bool wm_lv_0_adjust_needed;
 		u8 num_channels;
 		bool symmetric_memory;
 		enum intel_dram_type {
diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
index 4871d48589f9..a5850f0f25aa 100644
--- a/drivers/gpu/drm/i915/intel_dram.c
+++ b/drivers/gpu/drm/i915/intel_dram.c
@@ -207,7 +207,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915)
 		return -EINVAL;
 	}
 
-	dram_info->is_16gb_dimm = ch0.is_16gb_dimm || ch1.is_16gb_dimm;
+	dram_info->wm_lv_0_adjust_needed = ch0.is_16gb_dimm || ch1.is_16gb_dimm;
 
 	dram_info->symmetric_memory = intel_is_dram_symmetric(&ch0, &ch1);
 
@@ -475,7 +475,7 @@ static int gen11_get_dram_info(struct drm_i915_private *i915)
 			return ret;
 	} else {
 		/* Always needed for GEN12+ */
-		i915->dram_info.is_16gb_dimm = true;
+		i915->dram_info.wm_lv_0_adjust_needed = true;
 	}
 
 	return icl_pcode_read_mem_global_info(i915);
@@ -491,7 +491,7 @@ int intel_dram_detect(struct drm_i915_private *i915)
 	 * This is only used for the level 0 watermark latency
 	 * w/a which does not apply to bxt/glk.
 	 */
-	dram_info->is_16gb_dimm = !IS_GEN9_LP(i915);
+	dram_info->wm_lv_0_adjust_needed = !IS_GEN9_LP(i915);
 
 	if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915))
 		return 0;
@@ -510,8 +510,8 @@ int intel_dram_detect(struct drm_i915_private *i915)
 
 	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
 
-	drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n",
-		    yesno(dram_info->is_16gb_dimm));
+	drm_dbg_kms(&i915->drm, "Watermark level 0 adjustment needed: %s\n",
+		    yesno(dram_info->wm_lv_0_adjust_needed));
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 992fce8b8d13..f778aae19f82 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2930,7 +2930,7 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
 		 * any underrun. If not able to get Dimm info assume 16GB dimm
 		 * to avoid any underrun.
 		 */
-		if (dev_priv->dram_info.is_16gb_dimm)
+		if (dev_priv->dram_info.wm_lv_0_adjust_needed)
 			wm[0] += 1;
 
 	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-- 
2.30.0

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Nuke not needed members of dram_info

[Souza, Jose]

On Wed, 2021-01-20 at 10:52 -0800, Lucas De Marchi wrote:
> On Wed, Jan 20, 2021 at 10:42:46AM -0800, Jose Souza wrote:
> > On Wed, 2021-01-20 at 10:31 -0800, Lucas De Marchi wrote:
> > > On Wed, Jan 20, 2021 at 07:16:08AM -0800, Jose Souza wrote:
> > > > Valid, ranks and bandwidth_kbps are set into dram_info but are not
> > > > used anywhere else so nuking it.
> > > > 
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.c   |  4 +--
> > > > drivers/gpu/drm/i915/i915_drv.h   |  3 --
> > > > drivers/gpu/drm/i915/intel_dram.c | 47 +++++++------------------------
> > > > 3 files changed, 12 insertions(+), 42 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index f5666b44ea9d..a1cc60de99f0 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -609,8 +609,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
> > > > 
> > > > 	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.
> > > > +	 * Fill the dram structure to get the system dram info. This will be
> > > > +	 * used for memory latency calculation.
> > > > 	 */
> > > > 	intel_dram_detect(dev_priv);
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 8376cff5ba86..250e92910fa1 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1134,11 +1134,8 @@ struct drm_i915_private {
> > > > 	} wm;
> > > > 
> > > > 	struct dram_info {
> > > > -		bool valid;
> > > > 		bool is_16gb_dimm;
> > > > 		u8 num_channels;
> > > > -		u8 ranks;
> > > > -		u32 bandwidth_kbps;
> > > > 		bool symmetric_memory;
> > > > 		enum intel_dram_type {
> > > > 			INTEL_DRAM_UNKNOWN,
> > > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
> > > > index 4754296a250e..694fbd8c9cd4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dram.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dram.c
> > > > @@ -201,17 +201,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915)
> > > > 		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.ranks == 1 || ch1.ranks == 1)
> > > > -		dram_info->ranks = 1;
> > > > -	else
> > > > -		dram_info->ranks = max(ch0.ranks, ch1.ranks);
> > > > -
> > > > -	if (dram_info->ranks == 0) {
> > > > +	if (ch0.ranks == 0 && ch1.ranks == 0) {
> > > 
> > > previously if any of them were != 0, we would not fall here.
> > 
> > This is the same behavior.
> 
> indeed, I misread the condition
> 
> > 
> > > 
> > > 
> > > > 		drm_info(&i915->drm, "couldn't get memory rank information\n");
> > > > 		return -EINVAL;
> > > > 	}
> > > > @@ -269,16 +259,12 @@ skl_get_dram_info(struct drm_i915_private *i915)
> > > > 	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) {
> > > > +	if (dram_info->num_channels * mem_freq_khz == 0) {
> > > > 		drm_info(&i915->drm,
> > > > 			 "Couldn't get system memory bandwidth\n");
> > > > 		return -EINVAL;
> > > > 	}
> > > > 
> > > > -	dram_info->valid = true;
> > > > 	return 0;
> > > > }
> > > > 
> > > > @@ -365,7 +351,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
> > > > 	struct dram_info *dram_info = &i915->dram_info;
> > > > 	u32 dram_channels;
> > > > 	u32 mem_freq_khz, val;
> > > > -	u8 num_active_channels;
> > > > +	u8 num_active_channels, valid_ranks = 0;
> > > > 	int i;
> > > > 
> > > > 	val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0);
> > > > @@ -375,10 +361,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
> > > > 	dram_channels = val & 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) {
> > > > +	if (mem_freq_khz * num_active_channels == 0) {
> > > 
> > > maybe better to replace with a local var?
> > > 
> > > 	bandwidth_kbps = mem_freq_khz * num_active_channels;
> > > 
> > > and then check it where needed.
> > 
> > The only place it is used is in this if to return -EINVAL, same for the SKL function.
> > The multiplication fits under the 80 col limit so don't see why add a local var.
> 
> ok... maybe `if (!mem_freq_khz || !num_active_channels)` then?

That works too but I still prefer keep it as close as possible from the previous check.

> 
> 
> LUcas De Marchi
> 
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > > 		drm_info(&i915->drm,
> > > > 			 "Couldn't get system memory bandwidth\n");
> > > > 		return -EINVAL;
> > > > @@ -410,27 +393,18 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
> > > > 			    dimm.size, dimm.width, dimm.ranks,
> > > > 			    intel_dram_type_str(type));
> > > > 
> > > > -		/*
> > > > -		 * 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->ranks == 0)
> > > > -			dram_info->ranks = dimm.ranks;
> > > > -		else if (dimm.ranks == 1)
> > > > -			dram_info->ranks = 1;
> > > > +		if (valid_ranks == 0)
> > > > +			valid_ranks = dimm.ranks;
> > > > 
> > > > 		if (type != INTEL_DRAM_UNKNOWN)
> > > > 			dram_info->type = type;
> > > > 	}
> > > > 
> > > > -	if (dram_info->type == INTEL_DRAM_UNKNOWN || dram_info->ranks == 0) {
> > > > +	if (dram_info->type == INTEL_DRAM_UNKNOWN || valid_ranks == 0) {
> > > > 		drm_info(&i915->drm, "couldn't get memory information\n");
> > > > 		return -EINVAL;
> > > > 	}
> > > > 
> > > > -	dram_info->valid = true;
> > > > -
> > > > 	return 0;
> > > > }
> > > > 
> > > > @@ -456,11 +430,10 @@ void intel_dram_detect(struct drm_i915_private *i915)
> > > > 	if (ret)
> > > > 		return;
> > > > 
> > > > -	drm_dbg_kms(&i915->drm, "DRAM bandwidth: %u kBps, channels: %u\n",
> > > > -		    dram_info->bandwidth_kbps, dram_info->num_channels);
> > > > +	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
> > > > 
> > > > -	drm_dbg_kms(&i915->drm, "DRAM ranks: %u, 16Gb DIMMs: %s\n",
> > > > -		    dram_info->ranks, yesno(dram_info->is_16gb_dimm));
> > > > +	drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n",
> > > > +		    yesno(dram_info->is_16gb_dimm));
> > > > }
> > > > 
> > > > static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap)
> > > > --
> > > > 2.30.0
> > > > 
> > > > _______________________________________________
> > > > 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] ✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Nuke not needed members of dram_info

[Patchwork]

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

== Series Details ==

Series: series starting with [1/4] drm/i915: Nuke not needed members of dram_info
URL   : https://patchwork.freedesktop.org/series/86092/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9650 -> Patchwork_19429
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload:
    - fi-glk-dsi:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-glk-dsi/igt@i915_module_load@reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@i915_module_load@reload.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_prime@i915-to-amd:
    - fi-snb-2520m:       NOTRUN -> [SKIP][3] ([fdo#109271]) +17 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-snb-2520m/igt@amdgpu/amd_prime@i915-to-amd.html

  * igt@gem_exec_fence@basic-busy:
    - fi-glk-dsi:         NOTRUN -> [SKIP][4] ([fdo#109271]) +13 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@gem_exec_fence@basic-busy.html

  * igt@i915_getparams_basic@basic-subslice-total:
    - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#402]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-tgl-y/igt@i915_getparams_basic@basic-subslice-total.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-tgl-y/igt@i915_getparams_basic@basic-subslice-total.html

  * igt@kms_addfb_basic@invalid-set-prop-any:
    - fi-glk-dsi:         [PASS][7] -> [SKIP][8] ([fdo#109271]) +116 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-glk-dsi/igt@kms_addfb_basic@invalid-set-prop-any.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@kms_addfb_basic@invalid-set-prop-any.html

  * igt@runner@aborted:
    - fi-glk-dsi:         NOTRUN -> [FAIL][9] ([i915#2292] / [i915#2295] / [k.org#202321] / [k.org#204565])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_render_tiled_blits@basic:
    - fi-tgl-y:           [DMESG-WARN][10] ([i915#402]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-tgl-y/igt@gem_render_tiled_blits@basic.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-tgl-y/igt@gem_render_tiled_blits@basic.html

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2520m:       [INCOMPLETE][12] -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-snb-2520m/igt@i915_selftest@live@hangcheck.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-snb-2520m/igt@i915_selftest@live@hangcheck.html

  
#### Warnings ####

  * igt@gem_huc_copy@huc-copy:
    - fi-glk-dsi:         [SKIP][14] ([fdo#109271] / [i915#2190]) -> [SKIP][15] ([fdo#109271])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-glk-dsi/igt@gem_huc_copy@huc-copy.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@gem_huc_copy@huc-copy.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-glk-dsi:         [SKIP][16] ([fdo#109271] / [fdo#111827]) -> [SKIP][17] ([fdo#109271]) +8 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-glk-dsi/igt@kms_chamelium@hdmi-hpd-fast.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-glk-dsi:         [SKIP][18] ([fdo#109271] / [i915#533]) -> [SKIP][19] ([fdo#109271])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9650/fi-glk-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/fi-glk-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2292]: https://gitlab.freedesktop.org/drm/intel/issues/2292
  [i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
  [k.org#204565]: https://bugzilla.kernel.org/show_bug.cgi?id=204565


Participating hosts (43 -> 37)
------------------------------

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_9650 -> Patchwork_19429

  CI-20190529: 20190529
  CI_DRM_9650: 3f989d1bb4cfd91e25549f9fd7a750412581dcc4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5960: ace82fcd5f3623f8dde7c220a825873dc53dfae4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_19429: 61037f310a0e0d6ed56e90fd6dd5b90001b71881 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

61037f310a0e drm/i915: Rename is_16gb_dimm to wm_lv_0_adjust_needed
467a885ed1a0 drm/i915: Fail driver probe when unable to load DRAM information
67139f6e2aad drm/i915/gen11+: Only load DRAM information from pcode
4ff0251bf217 drm/i915: Nuke not needed members of dram_info

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19429/index.html

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

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

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Nuke not needed members of dram_info (rev2)

[Patchwork]

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

== Series Details ==

Series: series starting with [1/4] drm/i915: Nuke not needed members of dram_info (rev2)
URL   : https://patchwork.freedesktop.org/series/86092/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9664 -> Patchwork_19441
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload:
    - fi-glk-dsi:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-glk-dsi/igt@i915_module_load@reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@i915_module_load@reload.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][3] ([fdo#109271]) +22 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-bdw-5557u/igt@amdgpu/amd_basic@semaphore.html

  * igt@core_hotunplug@unbind-rebind:
    - fi-bdw-5557u:       NOTRUN -> [WARN][4] ([i915#2283])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-bdw-5557u/igt@core_hotunplug@unbind-rebind.html

  * igt@gem_exec_fence@basic-busy:
    - fi-glk-dsi:         NOTRUN -> [SKIP][5] ([fdo#109271]) +13 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@gem_exec_fence@basic-busy.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-tgl-y:           [PASS][6] -> [DMESG-WARN][7] ([i915#2411] / [i915#402])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-tgl-y/igt@gem_exec_suspend@basic-s3.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-tgl-y/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-guc:         [PASS][8] -> [FAIL][9] ([i915#2203] / [i915#579])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-kbl-guc/igt@i915_pm_rpm@module-reload.html

  * igt@kms_addfb_basic@invalid-set-prop-any:
    - fi-glk-dsi:         [PASS][10] -> [SKIP][11] ([fdo#109271]) +116 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-glk-dsi/igt@kms_addfb_basic@invalid-set-prop-any.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@kms_addfb_basic@invalid-set-prop-any.html

  * igt@prime_self_import@basic-with_one_bo_two_files:
    - fi-tgl-y:           [PASS][12] -> [DMESG-WARN][13] ([i915#402]) +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html

  * igt@runner@aborted:
    - fi-glk-dsi:         NOTRUN -> [FAIL][14] ([i915#2292] / [i915#2295] / [k.org#202321] / [k.org#204565])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@fbdev@read:
    - fi-tgl-y:           [DMESG-WARN][15] ([i915#402]) -> [PASS][16] +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-tgl-y/igt@fbdev@read.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-tgl-y/igt@fbdev@read.html

  * igt@i915_module_load@reload:
    - fi-byt-j1900:       [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-byt-j1900/igt@i915_module_load@reload.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-byt-j1900/igt@i915_module_load@reload.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-kbl-7500u:       [DMESG-WARN][19] ([i915#2868]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html

  
#### Warnings ####

  * igt@gem_huc_copy@huc-copy:
    - fi-glk-dsi:         [SKIP][21] ([fdo#109271] / [i915#2190]) -> [SKIP][22] ([fdo#109271])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-glk-dsi/igt@gem_huc_copy@huc-copy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@gem_huc_copy@huc-copy.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-glk-dsi:         [SKIP][23] ([fdo#109271] / [fdo#111827]) -> [SKIP][24] ([fdo#109271]) +8 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-glk-dsi/igt@kms_chamelium@hdmi-hpd-fast.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-glk-dsi:         [SKIP][25] ([fdo#109271] / [i915#533]) -> [SKIP][26] ([fdo#109271])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9664/fi-glk-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/fi-glk-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2203]: https://gitlab.freedesktop.org/drm/intel/issues/2203
  [i915#2283]: https://gitlab.freedesktop.org/drm/intel/issues/2283
  [i915#2292]: https://gitlab.freedesktop.org/drm/intel/issues/2292
  [i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295
  [i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
  [i915#2868]: https://gitlab.freedesktop.org/drm/intel/issues/2868
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579
  [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
  [k.org#204565]: https://bugzilla.kernel.org/show_bug.cgi?id=204565


Participating hosts (41 -> 37)
------------------------------

  Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


Build changes
-------------

  * Linux: CI_DRM_9664 -> Patchwork_19441

  CI-20190529: 20190529
  CI_DRM_9664: dd119a66c9fc3706c93efa10a7631a4c499d94f3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5962: 22e3daaed82ab7890018a2f2aabf5082cd536023 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_19441: a871e378b7e4c55928f79259f724df0111cc82b8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a871e378b7e4 drm/i915: Rename is_16gb_dimm to wm_lv_0_adjust_needed
1d4c44951302 drm/i915: Fail driver probe when unable to load DRAM information
4901cd6b2e8f drm/i915/gen11+: Only load DRAM information from pcode
51817b52f494 drm/i915: Nuke not needed members of dram_info

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19441/index.html

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

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

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Nuke not needed members of dram_info (rev3)

[Patchwork]

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

== Series Details ==

Series: series starting with [1/4] drm/i915: Nuke not needed members of dram_info (rev3)
URL   : https://patchwork.freedesktop.org/series/86092/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9670 -> Patchwork_19460
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload:
    - fi-glk-dsi:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-glk-dsi/igt@i915_module_load@reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@i915_module_load@reload.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-byt-j1900:       NOTRUN -> [SKIP][3] ([fdo#109271]) +17 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-byt-j1900/igt@amdgpu/amd_basic@userptr.html

  * igt@gem_exec_fence@basic-busy:
    - fi-glk-dsi:         NOTRUN -> [SKIP][4] ([fdo#109271]) +13 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@gem_exec_fence@basic-busy.html

  * igt@i915_hangman@error-state-basic:
    - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#402]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-tgl-y/igt@i915_hangman@error-state-basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-tgl-y/igt@i915_hangman@error-state-basic.html

  * igt@kms_addfb_basic@invalid-set-prop-any:
    - fi-glk-dsi:         [PASS][7] -> [SKIP][8] ([fdo#109271]) +116 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-glk-dsi/igt@kms_addfb_basic@invalid-set-prop-any.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@kms_addfb_basic@invalid-set-prop-any.html

  * igt@runner@aborted:
    - fi-glk-dsi:         NOTRUN -> [FAIL][9] ([i915#2292] / [i915#2295] / [k.org#202321] / [k.org#204565])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-byt-j1900:       [INCOMPLETE][10] ([i915#142] / [i915#2405]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html

  * igt@vgem_basic@setversion:
    - fi-tgl-y:           [DMESG-WARN][12] ([i915#402]) -> [PASS][13] +2 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-tgl-y/igt@vgem_basic@setversion.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-tgl-y/igt@vgem_basic@setversion.html

  
#### Warnings ####

  * igt@gem_huc_copy@huc-copy:
    - fi-glk-dsi:         [SKIP][14] ([fdo#109271] / [i915#2190]) -> [SKIP][15] ([fdo#109271])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-glk-dsi/igt@gem_huc_copy@huc-copy.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@gem_huc_copy@huc-copy.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-glk-dsi:         [SKIP][16] ([fdo#109271] / [fdo#111827]) -> [SKIP][17] ([fdo#109271]) +8 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-glk-dsi/igt@kms_chamelium@hdmi-hpd-fast.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-glk-dsi:         [SKIP][18] ([fdo#109271] / [i915#533]) -> [SKIP][19] ([fdo#109271])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9670/fi-glk-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/fi-glk-dsi/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#142]: https://gitlab.freedesktop.org/drm/intel/issues/142
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2292]: https://gitlab.freedesktop.org/drm/intel/issues/2292
  [i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295
  [i915#2405]: https://gitlab.freedesktop.org/drm/intel/issues/2405
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
  [k.org#204565]: https://bugzilla.kernel.org/show_bug.cgi?id=204565


Participating hosts (41 -> 37)
------------------------------

  Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


Build changes
-------------

  * Linux: CI_DRM_9670 -> Patchwork_19460

  CI-20190529: 20190529
  CI_DRM_9670: 85fd189b9fbfb6e7af8d956d37be012fdd6ae0ad @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5964: 0949766cb9846d7d55fac9cdf31d3d8e8ed1d0c6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_19460: c1f73ccb6d94db8505fdc03789e66427e71dcd6e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c1f73ccb6d94 drm/i915: Rename is_16gb_dimm to wm_lv_0_adjust_needed
7a0944ee9f63 drm/i915: Fail driver probe when unable to load DRAM information
f532480817a9 drm/i915/gen11+: Only load DRAM information from pcode
d61e40e438c6 drm/i915: Nuke not needed members of dram_info

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19460/index.html

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

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

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Nuke not needed members of dram_info

[Lucas De Marchi]

On Wed, Jan 20, 2021 at 07:29:37PM +0000, Jose Souza wrote:
>On Wed, 2021-01-20 at 10:52 -0800, Lucas De Marchi wrote:
>> On Wed, Jan 20, 2021 at 10:42:46AM -0800, Jose Souza wrote:
>> > On Wed, 2021-01-20 at 10:31 -0800, Lucas De Marchi wrote:
>> > > On Wed, Jan 20, 2021 at 07:16:08AM -0800, Jose Souza wrote:
>> > > > Valid, ranks and bandwidth_kbps are set into dram_info but are not
>> > > > used anywhere else so nuking it.
>> > > >
>> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> > > > ---
>> > > > drivers/gpu/drm/i915/i915_drv.c   |  4 +--
>> > > > drivers/gpu/drm/i915/i915_drv.h   |  3 --
>> > > > drivers/gpu/drm/i915/intel_dram.c | 47 +++++++------------------------
>> > > > 3 files changed, 12 insertions(+), 42 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > > > index f5666b44ea9d..a1cc60de99f0 100644
>> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > > > @@ -609,8 +609,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
>> > > >
>> > > > 	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.
>> > > > +	 * Fill the dram structure to get the system dram info. This will be
>> > > > +	 * used for memory latency calculation.
>> > > > 	 */
>> > > > 	intel_dram_detect(dev_priv);
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > > > index 8376cff5ba86..250e92910fa1 100644
>> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > > @@ -1134,11 +1134,8 @@ struct drm_i915_private {
>> > > > 	} wm;
>> > > >
>> > > > 	struct dram_info {
>> > > > -		bool valid;
>> > > > 		bool is_16gb_dimm;
>> > > > 		u8 num_channels;
>> > > > -		u8 ranks;
>> > > > -		u32 bandwidth_kbps;
>> > > > 		bool symmetric_memory;
>> > > > 		enum intel_dram_type {
>> > > > 			INTEL_DRAM_UNKNOWN,
>> > > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
>> > > > index 4754296a250e..694fbd8c9cd4 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_dram.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_dram.c
>> > > > @@ -201,17 +201,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915)
>> > > > 		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.ranks == 1 || ch1.ranks == 1)
>> > > > -		dram_info->ranks = 1;
>> > > > -	else
>> > > > -		dram_info->ranks = max(ch0.ranks, ch1.ranks);
>> > > > -
>> > > > -	if (dram_info->ranks == 0) {
>> > > > +	if (ch0.ranks == 0 && ch1.ranks == 0) {
>> > >
>> > > previously if any of them were != 0, we would not fall here.
>> >
>> > This is the same behavior.
>>
>> indeed, I misread the condition
>>
>> >
>> > >
>> > >
>> > > > 		drm_info(&i915->drm, "couldn't get memory rank information\n");
>> > > > 		return -EINVAL;
>> > > > 	}
>> > > > @@ -269,16 +259,12 @@ skl_get_dram_info(struct drm_i915_private *i915)
>> > > > 	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) {
>> > > > +	if (dram_info->num_channels * mem_freq_khz == 0) {
>> > > > 		drm_info(&i915->drm,
>> > > > 			 "Couldn't get system memory bandwidth\n");
>> > > > 		return -EINVAL;
>> > > > 	}
>> > > >
>> > > > -	dram_info->valid = true;
>> > > > 	return 0;
>> > > > }
>> > > >
>> > > > @@ -365,7 +351,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
>> > > > 	struct dram_info *dram_info = &i915->dram_info;
>> > > > 	u32 dram_channels;
>> > > > 	u32 mem_freq_khz, val;
>> > > > -	u8 num_active_channels;
>> > > > +	u8 num_active_channels, valid_ranks = 0;
>> > > > 	int i;
>> > > >
>> > > > 	val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0);
>> > > > @@ -375,10 +361,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
>> > > > 	dram_channels = val & 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) {
>> > > > +	if (mem_freq_khz * num_active_channels == 0) {
>> > >
>> > > maybe better to replace with a local var?
>> > >
>> > > 	bandwidth_kbps = mem_freq_khz * num_active_channels;
>> > >
>> > > and then check it where needed.
>> >
>> > The only place it is used is in this if to return -EINVAL, same for the SKL function.
>> > The multiplication fits under the 80 col limit so don't see why add a local var.
>>
>> ok... maybe `if (!mem_freq_khz || !num_active_channels)` then?
>
>That works too but I still prefer keep it as close as possible from the previous check.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi
>
>>
>>
>> LUcas De Marchi
>>
>> >
>> > >
>> > > Lucas De Marchi
>> > >
>> > > > 		drm_info(&i915->drm,
>> > > > 			 "Couldn't get system memory bandwidth\n");
>> > > > 		return -EINVAL;
>> > > > @@ -410,27 +393,18 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
>> > > > 			    dimm.size, dimm.width, dimm.ranks,
>> > > > 			    intel_dram_type_str(type));
>> > > >
>> > > > -		/*
>> > > > -		 * 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->ranks == 0)
>> > > > -			dram_info->ranks = dimm.ranks;
>> > > > -		else if (dimm.ranks == 1)
>> > > > -			dram_info->ranks = 1;
>> > > > +		if (valid_ranks == 0)
>> > > > +			valid_ranks = dimm.ranks;
>> > > >
>> > > > 		if (type != INTEL_DRAM_UNKNOWN)
>> > > > 			dram_info->type = type;
>> > > > 	}
>> > > >
>> > > > -	if (dram_info->type == INTEL_DRAM_UNKNOWN || dram_info->ranks == 0) {
>> > > > +	if (dram_info->type == INTEL_DRAM_UNKNOWN || valid_ranks == 0) {
>> > > > 		drm_info(&i915->drm, "couldn't get memory information\n");
>> > > > 		return -EINVAL;
>> > > > 	}
>> > > >
>> > > > -	dram_info->valid = true;
>> > > > -
>> > > > 	return 0;
>> > > > }
>> > > >
>> > > > @@ -456,11 +430,10 @@ void intel_dram_detect(struct drm_i915_private *i915)
>> > > > 	if (ret)
>> > > > 		return;
>> > > >
>> > > > -	drm_dbg_kms(&i915->drm, "DRAM bandwidth: %u kBps, channels: %u\n",
>> > > > -		    dram_info->bandwidth_kbps, dram_info->num_channels);
>> > > > +	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
>> > > >
>> > > > -	drm_dbg_kms(&i915->drm, "DRAM ranks: %u, 16Gb DIMMs: %s\n",
>> > > > -		    dram_info->ranks, yesno(dram_info->is_16gb_dimm));
>> > > > +	drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n",
>> > > > +		    yesno(dram_info->is_16gb_dimm));
>> > > > }
>> > > >
>> > > > static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap)
>> > > > --
>> > > > 2.30.0
>> > > >
>> > > > _______________________________________________
>> > > > 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: [Intel-gfx] [PATCH 2/4] drm/i915/gen11+: Only load DRAM information from pcode

[Lucas De Marchi]

On Wed, Jan 20, 2021 at 07:16:09AM -0800, Jose Souza wrote:
>Up to now we were reading some DRAM information from MCHBAR register
>and from pcode what is already not good but some GEN12(TGL-H and ADL-S)
>platforms have MCHBAR DRAM information in different offsets.
>
>This was notified to HW team that decided that the best alternative is
>always apply the 16gb_dimm watermark adjustment for GEN12+ platforms
>and read the remaning DRAM information needed to other display
>programming from pcode.
>
>So here moving the DRAM pcode function to intel_dram.c, removing
>the duplicated fields from intel_qgv_info, setting and using
>information from dram_info.
>
>Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_bw.c | 98 +++++--------------------
> drivers/gpu/drm/i915/i915_drv.c         |  5 +-
> drivers/gpu/drm/i915/i915_drv.h         |  1 +
> drivers/gpu/drm/i915/intel_dram.c       | 77 ++++++++++++++++++-
> 4 files changed, 97 insertions(+), 84 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>index bd060404d249..1368bd96ed73 100644
>--- a/drivers/gpu/drm/i915/display/intel_bw.c
>+++ b/drivers/gpu/drm/i915/display/intel_bw.c
>@@ -19,77 +19,9 @@ struct intel_qgv_point {
>
> struct intel_qgv_info {
> 	struct intel_qgv_point points[I915_NUM_QGV_POINTS];
>-	u8 num_points;
>-	u8 num_channels;
> 	u8 t_bl;
>-	enum intel_dram_type dram_type;

humn... given this struct already has padding, we could very well leave
the num_points field. See below.

> static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv,
> 					 struct intel_qgv_point *sp,
> 					 int point)
>@@ -139,17 +71,19 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv,
> static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
> 			      struct intel_qgv_info *qi)
> {
>+	struct dram_info *dram_info = &dev_priv->dram_info;
> 	int i, ret;
>
>-	ret = icl_pcode_read_mem_global_info(dev_priv, qi);
>-	if (ret)
>-		return ret;
>+	if (IS_GEN(dev_priv, 12))
>+		qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 16;
>+	else if (IS_GEN(dev_priv, 11))
>+		qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 8;
>
> 	if (drm_WARN_ON(&dev_priv->drm,
>-			qi->num_points > ARRAY_SIZE(qi->points)))
>-		qi->num_points = ARRAY_SIZE(qi->points);
>+			dram_info->qgv_points > ARRAY_SIZE(qi->points)))
>+		dram_info->qgv_points = ARRAY_SIZE(qi->points);

previously we were overriding a member of qi. Now we are overriding a
member of dram_info, which seems to cross the boundaries of what this
code should be doing.

So maybe:

qi->num_points = dev_priv->dram_info->qgv_points;

and leave the check alone and also the renames in this file?
Also, dram_info->qgv_points should be named dram_info->num_qgv_points
for consistency with other structs.

>
>-	for (i = 0; i < qi->num_points; i++) {
>+	for (i = 0; i < dram_info->qgv_points; i++) {
> 		struct intel_qgv_point *sp = &qi->points[i];
>
> 		ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i);
>@@ -171,12 +105,13 @@ static int icl_calc_bw(int dclk, int num, int den)
> 	return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6);
> }
>
>-static int icl_sagv_max_dclk(const struct intel_qgv_info *qi)
>+static int icl_sagv_max_dclk(struct drm_i915_private *dev_priv,
>+			     const struct intel_qgv_info *qi)
> {
> 	u16 dclk = 0;
> 	int i;
>
>-	for (i = 0; i < qi->num_points; i++)
>+	for (i = 0; i < dev_priv->dram_info.qgv_points; i++)
> 		dclk = max(dclk, qi->points[i].dclk);
>
> 	return dclk;
>@@ -207,6 +142,7 @@ static const struct intel_sa_info rkl_sa_info = {
>
> static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa)
> {
>+	struct dram_info *dram_info = &dev_priv->dram_info;
> 	struct intel_qgv_info qi = {};
> 	bool is_y_tile = true; /* assume y tile may be used */
> 	int num_channels;
>@@ -222,10 +158,10 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
> 			    "Failed to get memory subsystem information, ignoring bandwidth limits");
> 		return ret;
> 	}
>-	num_channels = qi.num_channels;
>+	num_channels = dram_info->num_channels;
>
> 	deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2);
>-	dclk_max = icl_sagv_max_dclk(&qi);
>+	dclk_max = icl_sagv_max_dclk(dev_priv, &qi);
>
> 	ipqdepthpch = 16;
>
>@@ -241,9 +177,9 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
> 		clpchgroup = (sa->deburst * deinterleave / num_channels) << i;
> 		bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1;
>
>-		bi->num_qgv_points = qi.num_points;
>+		bi->num_qgv_points = dram_info->qgv_points;


see above, we already have num_ prefix here, let's keep it in dram_info
as well.

>
>-		for (j = 0; j < qi.num_points; j++) {
>+		for (j = 0; j < dram_info->qgv_points; j++) {
> 			const struct intel_qgv_point *sp = &qi.points[j];
> 			int ct, bw;
>
>@@ -274,7 +210,7 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
> 	 * SAGV point, but we can't send PCode commands to restrict it
> 	 * as it will fail and pointless anyway.
> 	 */
>-	if (qi.num_points == 1)
>+	if (dram_info->qgv_points == 1)
> 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
> 	else
> 		dev_priv->sagv_status = I915_SAGV_ENABLED;
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index a1cc60de99f0..66f763fe7a83 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -608,14 +608,15 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
> 		goto err_msi;
>
> 	intel_opregion_setup(dev_priv);
>+
>+	intel_pcode_init(dev_priv);
>+
> 	/*
> 	 * Fill the dram structure to get the system dram info. This will be
> 	 * used for memory latency calculation.
> 	 */
> 	intel_dram_detect(dev_priv);
>
>-	intel_pcode_init(dev_priv);
>-
> 	intel_bw_init_hw(dev_priv);
>
> 	return 0;
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 250e92910fa1..a2ae21082b34 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -1144,6 +1144,7 @@ struct drm_i915_private {
> 			INTEL_DRAM_LPDDR3,
> 			INTEL_DRAM_LPDDR4
> 		} type;
>+		u8 qgv_points;
> 	} dram_info;
>
> 	struct intel_bw_info {
>diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
>index 694fbd8c9cd4..1298823c957c 100644
>--- a/drivers/gpu/drm/i915/intel_dram.c
>+++ b/drivers/gpu/drm/i915/intel_dram.c
>@@ -5,6 +5,7 @@
>
> #include "i915_drv.h"
> #include "intel_dram.h"
>+#include "intel_sideband.h"
>
> struct dram_dimm_info {
> 	u16 size;
>@@ -408,6 +409,78 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
> 	return 0;
> }
>
>+static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
>+{
>+	struct dram_info *dram_info = &dev_priv->dram_info;
>+	u32 val = 0;
>+	int ret;
>+
>+	ret = sandybridge_pcode_read(dev_priv,
>+				     ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
>+				     ICL_PCODE_MEM_SS_READ_GLOBAL_INFO,
>+				     &val, NULL);
>+	if (ret)
>+		return ret;
>+
>+	if (IS_GEN(dev_priv, 12)) {
>+		switch (val & 0xf) {
>+		case 0:
>+			dram_info->type = INTEL_DRAM_DDR4;
>+			break;
>+		case 3:
>+			dram_info->type = INTEL_DRAM_LPDDR4;
>+			break;
>+		case 4:
>+			dram_info->type = INTEL_DRAM_DDR3;
>+			break;
>+		case 5:
>+			dram_info->type = INTEL_DRAM_LPDDR3;
>+			break;
>+		default:
>+			MISSING_CASE(val & 0xf);
>+			return -1;
>+		}
>+	} else {
>+		switch (val & 0xf) {
>+		case 0:
>+			dram_info->type = INTEL_DRAM_DDR4;
>+			break;
>+		case 1:
>+			dram_info->type = INTEL_DRAM_DDR3;
>+			break;
>+		case 2:
>+			dram_info->type = INTEL_DRAM_LPDDR3;
>+			break;
>+		case 3:
>+			dram_info->type = INTEL_DRAM_LPDDR4;
>+			break;
>+		default:
>+			MISSING_CASE(val & 0xf);
>+			return -1;
>+		}
>+	}
>+
>+	dram_info->num_channels = (val & 0xf0) >> 4;
>+	dram_info->qgv_points = (val & 0xf00) >> 8;
>+
>+	return 0;
>+}
>+
>+static int gen11_get_dram_info(struct drm_i915_private *i915)
>+{
>+	if (INTEL_GEN(i915) == 11) {
>+		int ret = skl_get_dram_info(i915);
>+
>+		if (ret)
>+			return ret;
>+	} else {
>+		/* Always needed for GEN12+ */
>+		i915->dram_info.is_16gb_dimm = true;
>+	}
>+
>+	return icl_pcode_read_mem_global_info(i915);
>+}
>+
> void intel_dram_detect(struct drm_i915_private *i915)
> {
> 	struct dram_info *dram_info = &i915->dram_info;
>@@ -423,7 +496,9 @@ void intel_dram_detect(struct drm_i915_private *i915)
> 	if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915))
> 		return;
>
>-	if (IS_GEN9_LP(i915))
>+	if (INTEL_GEN(i915) >= 11)
>+		ret = gen11_get_dram_info(i915);

gen11 and gen12 implementation above are sufficiently different: better
to keep the if/else chain here only

static int gen11_get_dram_info(struct drm_i915_private *i915)
{
	int ret = skl_get_dram_info(i915);
	if (ret)
		return ret;

	return icl_pcode_read_mem_global_info(i915);
}


static int gen12_get_dram_info(struct drm_i915_private *i915)
{
	/* Always needed for GEN12+ */
	i915->dram_info.is_16gb_dimm = true;

	return icl_pcode_read_mem_global_info(i915);
}

Also, now it seems we have lots of dead code since we are not calling
skl_get_dram_info() for tgl. See
tgl_get_dram_type(), skl_dram_get_channels_info, etc

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Rename is_16gb_dimm to wm_lv_0_adjust_needed

[Lucas De Marchi]

On Wed, Jan 20, 2021 at 07:16:11AM -0800, Jose Souza wrote:
>As it now it is always required for GEN12+ the is_16gb_dimm name
>do not make sense for GEN12+.
>
>Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>---
> drivers/gpu/drm/i915/i915_drv.h   |  2 +-
> drivers/gpu/drm/i915/intel_dram.c | 10 +++++-----
> drivers/gpu/drm/i915/intel_pm.c   |  2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index a2ae21082b34..adc008c65b14 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -1134,7 +1134,7 @@ struct drm_i915_private {
> 	} wm;
>
> 	struct dram_info {
>-		bool is_16gb_dimm;
>+		bool wm_lv_0_adjust_needed;
> 		u8 num_channels;
> 		bool symmetric_memory;
> 		enum intel_dram_type {
>diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
>index 4871d48589f9..a5850f0f25aa 100644
>--- a/drivers/gpu/drm/i915/intel_dram.c
>+++ b/drivers/gpu/drm/i915/intel_dram.c
>@@ -207,7 +207,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915)
> 		return -EINVAL;
> 	}
>
>-	dram_info->is_16gb_dimm = ch0.is_16gb_dimm || ch1.is_16gb_dimm;
>+	dram_info->wm_lv_0_adjust_needed = ch0.is_16gb_dimm || ch1.is_16gb_dimm;
>
> 	dram_info->symmetric_memory = intel_is_dram_symmetric(&ch0, &ch1);
>
>@@ -475,7 +475,7 @@ static int gen11_get_dram_info(struct drm_i915_private *i915)
> 			return ret;
> 	} else {
> 		/* Always needed for GEN12+ */
>-		i915->dram_info.is_16gb_dimm = true;
>+		i915->dram_info.wm_lv_0_adjust_needed = true;
> 	}
>
> 	return icl_pcode_read_mem_global_info(i915);
>@@ -491,7 +491,7 @@ int intel_dram_detect(struct drm_i915_private *i915)
> 	 * This is only used for the level 0 watermark latency
> 	 * w/a which does not apply to bxt/glk.
> 	 */
>-	dram_info->is_16gb_dimm = !IS_GEN9_LP(i915);
>+	dram_info->wm_lv_0_adjust_needed = !IS_GEN9_LP(i915);

comment above also needs to be updated. With that:


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

>
> 	if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915))
> 		return 0;
>@@ -510,8 +510,8 @@ int intel_dram_detect(struct drm_i915_private *i915)
>
> 	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
>
>-	drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n",
>-		    yesno(dram_info->is_16gb_dimm));
>+	drm_dbg_kms(&i915->drm, "Watermark level 0 adjustment needed: %s\n",
>+		    yesno(dram_info->wm_lv_0_adjust_needed));
>
> 	return 0;
> }
>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>index 992fce8b8d13..f778aae19f82 100644
>--- a/drivers/gpu/drm/i915/intel_pm.c
>+++ b/drivers/gpu/drm/i915/intel_pm.c
>@@ -2930,7 +2930,7 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
> 		 * any underrun. If not able to get Dimm info assume 16GB dimm
> 		 * to avoid any underrun.
> 		 */
>-		if (dev_priv->dram_info.is_16gb_dimm)
>+		if (dev_priv->dram_info.wm_lv_0_adjust_needed)
> 			wm[0] += 1;
>
> 	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>-- 
>2.30.0
>
>_______________________________________________
>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: [Intel-gfx] [PATCH 2/4] drm/i915/gen11+: Only load DRAM information from pcode

[Souza, Jose]

On Wed, 2021-01-27 at 06:49 -0800, Lucas De Marchi wrote:
> On Wed, Jan 20, 2021 at 07:16:09AM -0800, Jose Souza wrote:
> > Up to now we were reading some DRAM information from MCHBAR register
> > and from pcode what is already not good but some GEN12(TGL-H and ADL-S)
> > platforms have MCHBAR DRAM information in different offsets.
> > 
> > This was notified to HW team that decided that the best alternative is
> > always apply the 16gb_dimm watermark adjustment for GEN12+ platforms
> > and read the remaning DRAM information needed to other display
> > programming from pcode.
> > 
> > So here moving the DRAM pcode function to intel_dram.c, removing
> > the duplicated fields from intel_qgv_info, setting and using
> > information from dram_info.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_bw.c | 98 +++++--------------------
> > drivers/gpu/drm/i915/i915_drv.c         |  5 +-
> > drivers/gpu/drm/i915/i915_drv.h         |  1 +
> > drivers/gpu/drm/i915/intel_dram.c       | 77 ++++++++++++++++++-
> > 4 files changed, 97 insertions(+), 84 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index bd060404d249..1368bd96ed73 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -19,77 +19,9 @@ struct intel_qgv_point {
> > 
> > struct intel_qgv_info {
> > 	struct intel_qgv_point points[I915_NUM_QGV_POINTS];
> > -	u8 num_points;
> > -	u8 num_channels;
> > 	u8 t_bl;
> > -	enum intel_dram_type dram_type;
> 
> humn... given this struct already has padding, we could very well leave
> the num_points field. See below.
> 
> > static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv,
> > 					 struct intel_qgv_point *sp,
> > 					 int point)
> > @@ -139,17 +71,19 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv,
> > static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
> > 			      struct intel_qgv_info *qi)
> > {
> > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > 	int i, ret;
> > 
> > -	ret = icl_pcode_read_mem_global_info(dev_priv, qi);
> > -	if (ret)
> > -		return ret;
> > +	if (IS_GEN(dev_priv, 12))
> > +		qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 16;
> > +	else if (IS_GEN(dev_priv, 11))
> > +		qi->t_bl = dev_priv->dram_info.type == INTEL_DRAM_DDR4 ? 4 : 8;
> > 
> > 	if (drm_WARN_ON(&dev_priv->drm,
> > -			qi->num_points > ARRAY_SIZE(qi->points)))
> > -		qi->num_points = ARRAY_SIZE(qi->points);
> > +			dram_info->qgv_points > ARRAY_SIZE(qi->points)))
> > +		dram_info->qgv_points = ARRAY_SIZE(qi->points);
> 
> previously we were overriding a member of qi. Now we are overriding a
> member of dram_info, which seems to cross the boundaries of what this
> code should be doing.
> 
> So maybe:
> 
> qi->num_points = dev_priv->dram_info->qgv_points;

Okay, reasonable enough to keep this duplicated.

> 
> and leave the check alone and also the renames in this file?
> Also, dram_info->qgv_points should be named dram_info->num_qgv_points
> for consistency with other structs.
> 
> > 
> > -	for (i = 0; i < qi->num_points; i++) {
> > +	for (i = 0; i < dram_info->qgv_points; i++) {
> > 		struct intel_qgv_point *sp = &qi->points[i];
> > 
> > 		ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i);
> > @@ -171,12 +105,13 @@ static int icl_calc_bw(int dclk, int num, int den)
> > 	return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6);
> > }
> > 
> > -static int icl_sagv_max_dclk(const struct intel_qgv_info *qi)
> > +static int icl_sagv_max_dclk(struct drm_i915_private *dev_priv,
> > +			     const struct intel_qgv_info *qi)
> > {
> > 	u16 dclk = 0;
> > 	int i;
> > 
> > -	for (i = 0; i < qi->num_points; i++)
> > +	for (i = 0; i < dev_priv->dram_info.qgv_points; i++)
> > 		dclk = max(dclk, qi->points[i].dclk);
> > 
> > 	return dclk;
> > @@ -207,6 +142,7 @@ static const struct intel_sa_info rkl_sa_info = {
> > 
> > static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa)
> > {
> > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > 	struct intel_qgv_info qi = {};
> > 	bool is_y_tile = true; /* assume y tile may be used */
> > 	int num_channels;
> > @@ -222,10 +158,10 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
> > 			    "Failed to get memory subsystem information, ignoring bandwidth limits");
> > 		return ret;
> > 	}
> > -	num_channels = qi.num_channels;
> > +	num_channels = dram_info->num_channels;
> > 
> > 	deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2);
> > -	dclk_max = icl_sagv_max_dclk(&qi);
> > +	dclk_max = icl_sagv_max_dclk(dev_priv, &qi);
> > 
> > 	ipqdepthpch = 16;
> > 
> > @@ -241,9 +177,9 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
> > 		clpchgroup = (sa->deburst * deinterleave / num_channels) << i;
> > 		bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1;
> > 
> > -		bi->num_qgv_points = qi.num_points;
> > +		bi->num_qgv_points = dram_info->qgv_points;
> 
> 
> see above, we already have num_ prefix here, let's keep it in dram_info
> as well.

Done, but will keep num_point in intel_qgv_info. We can rename it latter.

> 
> > 
> > -		for (j = 0; j < qi.num_points; j++) {
> > +		for (j = 0; j < dram_info->qgv_points; j++) {
> > 			const struct intel_qgv_point *sp = &qi.points[j];
> > 			int ct, bw;
> > 
> > @@ -274,7 +210,7 @@ static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
> > 	 * SAGV point, but we can't send PCode commands to restrict it
> > 	 * as it will fail and pointless anyway.
> > 	 */
> > -	if (qi.num_points == 1)
> > +	if (dram_info->qgv_points == 1)
> > 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
> > 	else
> > 		dev_priv->sagv_status = I915_SAGV_ENABLED;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index a1cc60de99f0..66f763fe7a83 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -608,14 +608,15 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
> > 		goto err_msi;
> > 
> > 	intel_opregion_setup(dev_priv);
> > +
> > +	intel_pcode_init(dev_priv);
> > +
> > 	/*
> > 	 * Fill the dram structure to get the system dram info. This will be
> > 	 * used for memory latency calculation.
> > 	 */
> > 	intel_dram_detect(dev_priv);
> > 
> > -	intel_pcode_init(dev_priv);
> > -
> > 	intel_bw_init_hw(dev_priv);
> > 
> > 	return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 250e92910fa1..a2ae21082b34 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1144,6 +1144,7 @@ struct drm_i915_private {
> > 			INTEL_DRAM_LPDDR3,
> > 			INTEL_DRAM_LPDDR4
> > 		} type;
> > +		u8 qgv_points;
> > 	} dram_info;
> > 
> > 	struct intel_bw_info {
> > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
> > index 694fbd8c9cd4..1298823c957c 100644
> > --- a/drivers/gpu/drm/i915/intel_dram.c
> > +++ b/drivers/gpu/drm/i915/intel_dram.c
> > @@ -5,6 +5,7 @@
> > 
> > #include "i915_drv.h"
> > #include "intel_dram.h"
> > +#include "intel_sideband.h"
> > 
> > struct dram_dimm_info {
> > 	u16 size;
> > @@ -408,6 +409,78 @@ static int bxt_get_dram_info(struct drm_i915_private *i915)
> > 	return 0;
> > }
> > 
> > +static int icl_pcode_read_mem_global_info(struct drm_i915_private *dev_priv)
> > +{
> > +	struct dram_info *dram_info = &dev_priv->dram_info;
> > +	u32 val = 0;
> > +	int ret;
> > +
> > +	ret = sandybridge_pcode_read(dev_priv,
> > +				     ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
> > +				     ICL_PCODE_MEM_SS_READ_GLOBAL_INFO,
> > +				     &val, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (IS_GEN(dev_priv, 12)) {
> > +		switch (val & 0xf) {
> > +		case 0:
> > +			dram_info->type = INTEL_DRAM_DDR4;
> > +			break;
> > +		case 3:
> > +			dram_info->type = INTEL_DRAM_LPDDR4;
> > +			break;
> > +		case 4:
> > +			dram_info->type = INTEL_DRAM_DDR3;
> > +			break;
> > +		case 5:
> > +			dram_info->type = INTEL_DRAM_LPDDR3;
> > +			break;
> > +		default:
> > +			MISSING_CASE(val & 0xf);
> > +			return -1;
> > +		}
> > +	} else {
> > +		switch (val & 0xf) {
> > +		case 0:
> > +			dram_info->type = INTEL_DRAM_DDR4;
> > +			break;
> > +		case 1:
> > +			dram_info->type = INTEL_DRAM_DDR3;
> > +			break;
> > +		case 2:
> > +			dram_info->type = INTEL_DRAM_LPDDR3;
> > +			break;
> > +		case 3:
> > +			dram_info->type = INTEL_DRAM_LPDDR4;
> > +			break;
> > +		default:
> > +			MISSING_CASE(val & 0xf);
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	dram_info->num_channels = (val & 0xf0) >> 4;
> > +	dram_info->qgv_points = (val & 0xf00) >> 8;
> > +
> > +	return 0;
> > +}
> > +
> > +static int gen11_get_dram_info(struct drm_i915_private *i915)
> > +{
> > +	if (INTEL_GEN(i915) == 11) {
> > +		int ret = skl_get_dram_info(i915);
> > +
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		/* Always needed for GEN12+ */
> > +		i915->dram_info.is_16gb_dimm = true;
> > +	}
> > +
> > +	return icl_pcode_read_mem_global_info(i915);
> > +}
> > +
> > void intel_dram_detect(struct drm_i915_private *i915)
> > {
> > 	struct dram_info *dram_info = &i915->dram_info;
> > @@ -423,7 +496,9 @@ void intel_dram_detect(struct drm_i915_private *i915)
> > 	if (INTEL_GEN(i915) < 9 || !HAS_DISPLAY(i915))
> > 		return;
> > 
> > -	if (IS_GEN9_LP(i915))
> > +	if (INTEL_GEN(i915) >= 11)
> > +		ret = gen11_get_dram_info(i915);
> 
> gen11 and gen12 implementation above are sufficiently different: better
> to keep the if/else chain here only
> 
> static int gen11_get_dram_info(struct drm_i915_private *i915)
> {
> 	int ret = skl_get_dram_info(i915);
> 	if (ret)
> 		return ret;
> 
> 	return icl_pcode_read_mem_global_info(i915);
> }
> 
> 
> static int gen12_get_dram_info(struct drm_i915_private *i915)
> {
> 	/* Always needed for GEN12+ */
> 	i915->dram_info.is_16gb_dimm = true;
> 
> 	return icl_pcode_read_mem_global_info(i915);
> }

Done

> 
> Also, now it seems we have lots of dead code since we are not calling
> skl_get_dram_info() for tgl. See
> tgl_get_dram_type(), skl_dram_get_channels_info, etc

Can't find those.

> 
> Lucas De Marchi

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