[Intel-gfx] [PATCH v6 0/7] mtl: add support for pmdemand

[Intel-gfx] [PATCH v6 0/7] mtl: add support for pmdemand

[Vinod Govindapillai]

pmdemand support patches for MTL

SAGV configuration support for MTL

v2: added one missing patch in the previous version

v3: chekcpatch warning fixes
    update index handling for the icl/tgl QGV point handling
    program pmdemand code simplified

v4: update to debufs and pipe values pmdemand regiters
    removed the macro usage in update_pmdemand_values

V5: Addressing comments from Gustavo and Jani
    And some other fixes for issues from CI

v6: Addressing some comments from Gustavo
    Updates to pmdemand state struct, active phys calculations
    Got rid of suppress warning patch from v5

Mika Kahola (1):
  drm/i915/mtl: Add support for PM DEMAND

Vinod Govindapillai (6):
  drm/i915: fix the derating percentage for MTL
  drm/i915: update the QGV point frequency calculations
  drm/i915: store the peak bw per QGV point
  drm/i915: extract intel_bw_check_qgv_points()
  drm/i915: modify max_bw to return index to intel_bw_info
  drm/i915/mtl: find the best QGV point for the SAGV configuration

 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_bw.c       | 353 ++++++++----
 drivers/gpu/drm/i915/display/intel_bw.h       |   6 +
 drivers/gpu/drm/i915/display/intel_display.c  |  14 +
 .../gpu/drm/i915/display/intel_display_core.h |  11 +
 .../drm/i915/display/intel_display_driver.c   |   7 +
 .../drm/i915/display/intel_display_power.c    |   8 +
 drivers/gpu/drm/i915/display/intel_pmdemand.c | 503 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_pmdemand.h |  24 +
 drivers/gpu/drm/i915/i915_irq.c               |  23 +-
 drivers/gpu/drm/i915/i915_reg.h               |  36 +-
 11 files changed, 865 insertions(+), 121 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_pmdemand.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_pmdemand.h

-- 
2.34.1

[Intel-gfx] [PATCH v6 1/7] drm/i915: fix the derating percentage for MTL

[Vinod Govindapillai]

Follow the values from bspec for the percentage overhead for
efficiency in MTL BW calculations.

Bspec: 64631

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 597d5816ad1b..ab405c48ca3a 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -379,7 +379,7 @@ static const struct intel_sa_info mtl_sa_info = {
 	.deburst = 32,
 	.deprogbwlimit = 38, /* GB/s */
 	.displayrtids = 256,
-	.derating = 20,
+	.derating = 10,
 };
 
 static int icl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel_sa_info *sa)
-- 
2.34.1

[Intel-gfx] [PATCH v6 2/7] drm/i915: update the QGV point frequency calculations

[Vinod Govindapillai]

From MTL onwwards, pcode locks the QGV point based on peak BW of
the intended QGV point passed by the driver. So the peak BW
calculation must match the value expected by the pcode. Update
the calculations as per the Bspec.

v2: use DIV_ROUND_* macro for the calculations (Ville)

Bspec: 64636

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index ab405c48ca3a..c8075a37c3ab 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -182,7 +182,7 @@ static int mtl_read_qgv_point_info(struct drm_i915_private *dev_priv,
 	val2 = intel_uncore_read(&dev_priv->uncore,
 				 MTL_MEM_SS_INFO_QGV_POINT_HIGH(point));
 	dclk = REG_FIELD_GET(MTL_DCLK_MASK, val);
-	sp->dclk = DIV_ROUND_UP((16667 * dclk), 1000);
+	sp->dclk = DIV_ROUND_CLOSEST(16667 * dclk + 500, 1000);
 	sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val);
 	sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);
 
-- 
2.34.1

[Intel-gfx] [PATCH v6 3/7] drm/i915: store the peak bw per QGV point

[Vinod Govindapillai]

In MTL onwards, pcode locks the GV point based on the peak BW
of a QGV point. So store the peak BW of all the QGV points.

v2: use DIV_ROUND_CLOSEST() for the peakBW calculation

Bspec: 64636

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c           | 8 ++++++--
 drivers/gpu/drm/i915/display/intel_display_core.h | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index c8075a37c3ab..db117638d23b 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -534,10 +534,14 @@ static int tgl_get_bw_info(struct drm_i915_private *dev_priv, const struct intel
 
 			bi->deratedbw[j] = min(maxdebw,
 					       bw * (100 - sa->derating) / 100);
+			bi->peakbw[j] = DIV_ROUND_CLOSEST(sp->dclk *
+							  num_channels *
+							  qi.channel_width, 8);
 
 			drm_dbg_kms(&dev_priv->drm,
-				    "BW%d / QGV %d: num_planes=%d deratedbw=%u\n",
-				    i, j, bi->num_planes, bi->deratedbw[j]);
+				    "BW%d / QGV %d: num_planes=%d deratedbw=%u peakbw: %u\n",
+				    i, j, bi->num_planes, bi->deratedbw[j],
+				    bi->peakbw[j]);
 		}
 
 		for (j = 0; j < qi.num_psf_points; j++) {
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index e36f88a39b86..9f66d734edf6 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -314,6 +314,8 @@ struct intel_display {
 			unsigned int deratedbw[I915_NUM_QGV_POINTS];
 			/* for each PSF GV point */
 			unsigned int psf_bw[I915_NUM_PSF_GV_POINTS];
+			/* Peak BW for each QGV point */
+			unsigned int peakbw[I915_NUM_QGV_POINTS];
 			u8 num_qgv_points;
 			u8 num_psf_gv_points;
 			u8 num_planes;
-- 
2.34.1

[Intel-gfx] [PATCH v6 4/7] drm/i915: extract intel_bw_check_qgv_points()

[Vinod Govindapillai]

Extract intel_bw_check_qgv_points() from intel_bw_atomic_check
to facilitate future platform variations in handling SAGV
configurations.

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 235 +++++++++++++-----------
 1 file changed, 130 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index db117638d23b..d83aafd0cc2b 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -803,6 +803,128 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
 	return to_intel_bw_state(bw_state);
 }
 
+static int icl_find_qgv_points(struct drm_i915_private *i915,
+			       unsigned int data_rate,
+			       unsigned int num_active_planes,
+			       const struct intel_bw_state *old_bw_state,
+			       struct intel_bw_state *new_bw_state)
+{
+	unsigned int max_bw_point = 0;
+	unsigned int max_bw = 0;
+	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
+	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
+	u16 psf_points = 0;
+	u16 qgv_points = 0;
+	int i;
+	int ret;
+
+	ret = intel_atomic_lock_global_state(&new_bw_state->base);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_qgv_points; i++) {
+		unsigned int max_data_rate;
+
+		if (DISPLAY_VER(i915) > 11)
+			max_data_rate = tgl_max_bw(i915, num_active_planes, i);
+		else
+			max_data_rate = icl_max_bw(i915, num_active_planes, i);
+		/*
+		 * We need to know which qgv point gives us
+		 * maximum bandwidth in order to disable SAGV
+		 * if we find that we exceed SAGV block time
+		 * with watermarks. By that moment we already
+		 * have those, as it is calculated earlier in
+		 * intel_atomic_check,
+		 */
+		if (max_data_rate > max_bw) {
+			max_bw_point = i;
+			max_bw = max_data_rate;
+		}
+		if (max_data_rate >= data_rate)
+			qgv_points |= BIT(i);
+
+		drm_dbg_kms(&i915->drm, "QGV point %d: max bw %d required %d\n",
+			    i, max_data_rate, data_rate);
+	}
+
+	for (i = 0; i < num_psf_gv_points; i++) {
+		unsigned int max_data_rate = adl_psf_bw(i915, i);
+
+		if (max_data_rate >= data_rate)
+			psf_points |= BIT(i);
+
+		drm_dbg_kms(&i915->drm, "PSF GV point %d: max bw %d"
+			    " required %d\n",
+			    i, max_data_rate, data_rate);
+	}
+
+	/*
+	 * BSpec states that we always should have at least one allowed point
+	 * left, so if we couldn't - simply reject the configuration for obvious
+	 * reasons.
+	 */
+	if (qgv_points == 0) {
+		drm_dbg_kms(&i915->drm, "No QGV points provide sufficient memory"
+			    " bandwidth %d for display configuration(%d active planes).\n",
+			    data_rate, num_active_planes);
+		return -EINVAL;
+	}
+
+	if (num_psf_gv_points > 0 && psf_points == 0) {
+		drm_dbg_kms(&i915->drm, "No PSF GV points provide sufficient memory"
+			    " bandwidth %d for display configuration(%d active planes).\n",
+			    data_rate, num_active_planes);
+		return -EINVAL;
+	}
+
+	/*
+	 * Leave only single point with highest bandwidth, if
+	 * we can't enable SAGV due to the increased memory latency it may
+	 * cause.
+	 */
+	if (!intel_can_enable_sagv(i915, new_bw_state)) {
+		qgv_points = BIT(max_bw_point);
+		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
+			    max_bw_point);
+	}
+
+	/*
+	 * We store the ones which need to be masked as that is what PCode
+	 * actually accepts as a parameter.
+	 */
+	new_bw_state->qgv_points_mask =
+		~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
+		  ADLS_PCODE_REQ_PSF_PT(psf_points)) &
+		icl_qgv_points_mask(i915);
+
+	/*
+	 * If the actual mask had changed we need to make sure that
+	 * the commits are serialized(in case this is a nomodeset, nonblocking)
+	 */
+	if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
+		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int intel_bw_check_qgv_points(struct drm_i915_private *i915,
+				     const struct intel_bw_state *old_bw_state,
+				     struct intel_bw_state *new_bw_state)
+{
+	unsigned int data_rate = intel_bw_data_rate(i915, new_bw_state);
+	unsigned int num_active_planes =
+			intel_bw_num_active_planes(i915, new_bw_state);
+
+	data_rate = DIV_ROUND_UP(data_rate, 1000);
+
+	return icl_find_qgv_points(i915, data_rate, num_active_planes,
+				   old_bw_state, new_bw_state);
+}
+
 static bool intel_bw_state_changed(struct drm_i915_private *i915,
 				   const struct intel_bw_state *old_bw_state,
 				   const struct intel_bw_state *new_bw_state)
@@ -1049,20 +1171,14 @@ static int intel_bw_check_data_rate(struct intel_atomic_state *state, bool *chan
 
 int intel_bw_atomic_check(struct intel_atomic_state *state)
 {
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_bw_state *old_bw_state;
-	struct intel_bw_state *new_bw_state;
-	unsigned int data_rate;
-	unsigned int num_active_planes;
-	int i, ret;
-	u16 qgv_points = 0, psf_points = 0;
-	unsigned int max_bw_point = 0, max_bw = 0;
-	unsigned int num_qgv_points = dev_priv->display.bw.max[0].num_qgv_points;
-	unsigned int num_psf_gv_points = dev_priv->display.bw.max[0].num_psf_gv_points;
 	bool changed = false;
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct intel_bw_state *new_bw_state;
+	const struct intel_bw_state *old_bw_state;
+	int ret;
 
 	/* FIXME earlier gens need some checks too */
-	if (DISPLAY_VER(dev_priv) < 11)
+	if (DISPLAY_VER(i915) < 11)
 		return 0;
 
 	ret = intel_bw_check_data_rate(state, &changed);
@@ -1073,8 +1189,8 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
 	new_bw_state = intel_atomic_get_new_bw_state(state);
 
 	if (new_bw_state &&
-	    intel_can_enable_sagv(dev_priv, old_bw_state) !=
-	    intel_can_enable_sagv(dev_priv, new_bw_state))
+	    intel_can_enable_sagv(i915, old_bw_state) !=
+	    intel_can_enable_sagv(i915, new_bw_state))
 		changed = true;
 
 	/*
@@ -1084,101 +1200,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
 	if (!changed)
 		return 0;
 
-	ret = intel_atomic_lock_global_state(&new_bw_state->base);
+	ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state);
 	if (ret)
 		return ret;
 
-	data_rate = intel_bw_data_rate(dev_priv, new_bw_state);
-	data_rate = DIV_ROUND_UP(data_rate, 1000);
-
-	num_active_planes = intel_bw_num_active_planes(dev_priv, new_bw_state);
-
-	for (i = 0; i < num_qgv_points; i++) {
-		unsigned int max_data_rate;
-
-		if (DISPLAY_VER(dev_priv) > 11)
-			max_data_rate = tgl_max_bw(dev_priv, num_active_planes, i);
-		else
-			max_data_rate = icl_max_bw(dev_priv, num_active_planes, i);
-		/*
-		 * We need to know which qgv point gives us
-		 * maximum bandwidth in order to disable SAGV
-		 * if we find that we exceed SAGV block time
-		 * with watermarks. By that moment we already
-		 * have those, as it is calculated earlier in
-		 * intel_atomic_check,
-		 */
-		if (max_data_rate > max_bw) {
-			max_bw_point = i;
-			max_bw = max_data_rate;
-		}
-		if (max_data_rate >= data_rate)
-			qgv_points |= BIT(i);
-
-		drm_dbg_kms(&dev_priv->drm, "QGV point %d: max bw %d required %d\n",
-			    i, max_data_rate, data_rate);
-	}
-
-	for (i = 0; i < num_psf_gv_points; i++) {
-		unsigned int max_data_rate = adl_psf_bw(dev_priv, i);
-
-		if (max_data_rate >= data_rate)
-			psf_points |= BIT(i);
-
-		drm_dbg_kms(&dev_priv->drm, "PSF GV point %d: max bw %d"
-			    " required %d\n",
-			    i, max_data_rate, data_rate);
-	}
-
-	/*
-	 * BSpec states that we always should have at least one allowed point
-	 * left, so if we couldn't - simply reject the configuration for obvious
-	 * reasons.
-	 */
-	if (qgv_points == 0) {
-		drm_dbg_kms(&dev_priv->drm, "No QGV points provide sufficient memory"
-			    " bandwidth %d for display configuration(%d active planes).\n",
-			    data_rate, num_active_planes);
-		return -EINVAL;
-	}
-
-	if (num_psf_gv_points > 0 && psf_points == 0) {
-		drm_dbg_kms(&dev_priv->drm, "No PSF GV points provide sufficient memory"
-			    " bandwidth %d for display configuration(%d active planes).\n",
-			    data_rate, num_active_planes);
-		return -EINVAL;
-	}
-
-	/*
-	 * Leave only single point with highest bandwidth, if
-	 * we can't enable SAGV due to the increased memory latency it may
-	 * cause.
-	 */
-	if (!intel_can_enable_sagv(dev_priv, new_bw_state)) {
-		qgv_points = BIT(max_bw_point);
-		drm_dbg_kms(&dev_priv->drm, "No SAGV, using single QGV point %d\n",
-			    max_bw_point);
-	}
-
-	/*
-	 * We store the ones which need to be masked as that is what PCode
-	 * actually accepts as a parameter.
-	 */
-	new_bw_state->qgv_points_mask =
-		~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
-		  ADLS_PCODE_REQ_PSF_PT(psf_points)) &
-		icl_qgv_points_mask(dev_priv);
-
-	/*
-	 * If the actual mask had changed we need to make sure that
-	 * the commits are serialized(in case this is a nomodeset, nonblocking)
-	 */
-	if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
-		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
-- 
2.34.1

[Intel-gfx] [PATCH v6 5/7] drm/i915: modify max_bw to return index to intel_bw_info

[Vinod Govindapillai]

MTL uses the peak BW of a QGV point to lock the required QGV
point instead of the QGV index. Instead of passing the deratedbw
of the selected bw_info, return the index to the selected
bw_info so that either deratedbw or peakbw can be used based on
the platform.

v2: use idx to store index returned by max_bw_index functions

v3: return UINT_MAX in icl_max_bw_index in case no match found

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 27 ++++++++++++++++---------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index d83aafd0cc2b..f466b4e087bb 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -593,8 +593,8 @@ static void dg2_get_bw_info(struct drm_i915_private *i915)
 	i915->display.sagv.status = I915_SAGV_NOT_CONTROLLED;
 }
 
-static unsigned int icl_max_bw(struct drm_i915_private *dev_priv,
-			       int num_planes, int qgv_point)
+static unsigned int icl_max_bw_index(struct drm_i915_private *dev_priv,
+				     int num_planes, int qgv_point)
 {
 	int i;
 
@@ -615,14 +615,14 @@ static unsigned int icl_max_bw(struct drm_i915_private *dev_priv,
 			return UINT_MAX;
 
 		if (num_planes >= bi->num_planes)
-			return bi->deratedbw[qgv_point];
+			return i;
 	}
 
-	return 0;
+	return UINT_MAX;
 }
 
-static unsigned int tgl_max_bw(struct drm_i915_private *dev_priv,
-			       int num_planes, int qgv_point)
+static unsigned int tgl_max_bw_index(struct drm_i915_private *dev_priv,
+				     int num_planes, int qgv_point)
 {
 	int i;
 
@@ -643,10 +643,10 @@ static unsigned int tgl_max_bw(struct drm_i915_private *dev_priv,
 			return UINT_MAX;
 
 		if (num_planes <= bi->num_planes)
-			return bi->deratedbw[qgv_point];
+			return i;
 	}
 
-	return dev_priv->display.bw.max[0].deratedbw[qgv_point];
+	return 0;
 }
 
 static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv,
@@ -823,12 +823,19 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 		return ret;
 
 	for (i = 0; i < num_qgv_points; i++) {
+		unsigned int idx;
 		unsigned int max_data_rate;
 
 		if (DISPLAY_VER(i915) > 11)
-			max_data_rate = tgl_max_bw(i915, num_active_planes, i);
+			idx = tgl_max_bw_index(i915, num_active_planes, i);
 		else
-			max_data_rate = icl_max_bw(i915, num_active_planes, i);
+			idx = icl_max_bw_index(i915, num_active_planes, i);
+
+		if (idx > ARRAY_SIZE(i915->display.bw.max))
+			continue;
+
+		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
+
 		/*
 		 * We need to know which qgv point gives us
 		 * maximum bandwidth in order to disable SAGV
-- 
2.34.1

[Intel-gfx] [PATCH v6 6/7] drm/i915/mtl: find the best QGV point for the SAGV configuration

[Vinod Govindapillai]

From MTL onwards, we need to find the best QGV point based on
the required data rate and pass the peak BW of that point to
the punit to lock the corresponding QGV point.

Bspec: 64636

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 87 ++++++++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_bw.h |  6 ++
 2 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index f466b4e087bb..60da961b0768 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -803,6 +803,85 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
 	return to_intel_bw_state(bw_state);
 }
 
+static int mtl_find_qgv_points(struct drm_i915_private *i915,
+			       unsigned int data_rate,
+			       unsigned int num_active_planes,
+			       const struct intel_bw_state *old_bw_state,
+			       struct intel_bw_state *new_bw_state)
+{
+	unsigned int best_rate = UINT_MAX;
+	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
+	unsigned int qgv_peak_bw  = 0;
+	int i;
+	int ret;
+
+	ret = intel_atomic_lock_global_state(&new_bw_state->base);
+	if (ret)
+		return ret;
+
+	/*
+	 * If SAGV cannot be enabled, disable the pcode SAGV by passing all 1's
+	 * for qgv peak bw in PM Demand request. So assign UINT_MAX if SAGV is
+	 * not enabled. PM Demand code will clamp the value for the register
+	 */
+	if (!intel_can_enable_sagv(i915, new_bw_state)) {
+		new_bw_state->qgv_point_peakbw = UINT_MAX;
+		drm_dbg_kms(&i915->drm, "No SAGV, use UINT_MAX as peak bw.");
+		goto out;
+	}
+
+	/*
+	 * Find the best QGV point by comparing the data_rate with max data rate
+	 * offered per plane group
+	 */
+	for (i = 0; i < num_qgv_points; i++) {
+		unsigned int bw_index =
+			tgl_max_bw_index(i915, num_active_planes, i);
+		unsigned int max_data_rate;
+
+		if (bw_index > ARRAY_SIZE(i915->display.bw.max))
+			continue;
+
+		max_data_rate = i915->display.bw.max[bw_index].deratedbw[i];
+
+		if (max_data_rate < data_rate)
+			continue;
+
+		if (max_data_rate - data_rate < best_rate) {
+			best_rate = max_data_rate - data_rate;
+			qgv_peak_bw = i915->display.bw.max[bw_index].peakbw[i];
+		}
+
+		drm_dbg_kms(&i915->drm, "QGV point %d: max bw %d required %d qgv_peak_bw: %d\n",
+			    i, max_data_rate, data_rate, qgv_peak_bw);
+	}
+
+	drm_dbg_kms(&i915->drm, "Matching peaks QGV bw: %d for required data rate: %d\n",
+		    qgv_peak_bw, data_rate);
+
+	/*
+	 * The display configuration cannot be supported if no QGV point
+	 * satisfying the required data rate is found
+	 */
+	if (qgv_peak_bw == 0) {
+		drm_dbg_kms(&i915->drm, "No QGV points for bw %d for display configuration(%d active planes).\n",
+			    data_rate, num_active_planes);
+		return -EINVAL;
+	}
+
+	/* MTL PM DEMAND expects QGV BW parameter in multiples of 100 mbps */
+	new_bw_state->qgv_point_peakbw = DIV_ROUND_CLOSEST(qgv_peak_bw, 100);
+
+out:
+	if (new_bw_state->qgv_point_peakbw != old_bw_state->qgv_point_peakbw)  {
+		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int icl_find_qgv_points(struct drm_i915_private *i915,
 			       unsigned int data_rate,
 			       unsigned int num_active_planes,
@@ -928,8 +1007,12 @@ static int intel_bw_check_qgv_points(struct drm_i915_private *i915,
 
 	data_rate = DIV_ROUND_UP(data_rate, 1000);
 
-	return icl_find_qgv_points(i915, data_rate, num_active_planes,
-				   old_bw_state, new_bw_state);
+	if (DISPLAY_VER(i915) >= 14)
+		return mtl_find_qgv_points(i915, data_rate, num_active_planes,
+					   old_bw_state, new_bw_state);
+	else
+		return icl_find_qgv_points(i915, data_rate, num_active_planes,
+					   old_bw_state, new_bw_state);
 }
 
 static bool intel_bw_state_changed(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index f20292143745..67ae66a3fcdd 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -34,6 +34,12 @@ struct intel_bw_state {
 	/* bitmask of active pipes */
 	u8 active_pipes;
 
+	/*
+	 * From MTL onwards, to lock a QGV point, punit expects the peak BW of
+	 * the selected QGV point as the parameter in multiples of 100MB/s
+	 */
+	unsigned int qgv_point_peakbw;
+
 	/*
 	 * Current QGV points mask, which restricts
 	 * some particular SAGV states, not to confuse
-- 
2.34.1

[Intel-gfx] [PATCH v6 7/7] drm/i915/mtl: Add support for PM DEMAND

[Vinod Govindapillai]

From: Mika Kahola <mika.kahola@intel.com>

MTL introduces a new way to instruct the PUnit with
power and bandwidth requirements of DE. Add the functionality
to program the registers and handle waits using interrupts.
The current wait time for timeouts is programmed for 10 msecs to
factor in the worst case scenarios. Changes made to use REG_BIT
for a register that we touched(GEN8_DE_MISC_IER _MMIO).

Wa_14016740474 is added which applies to Xe_LPD+ display

v2: checkpatch warning fixes, simplify program pmdemand part

v3: update to dbufs and pipes values to pmdemand register(stan)
    Removed the macro usage in update_pmdemand_values()

v4: move the pmdemand_pre_plane_update before cdclk update
    pmdemand_needs_update included cdclk params comparisons
    pmdemand_state NULL check (Gustavo)
    pmdemand.o in sorted order in the makefile (Jani)
    update pmdemand misc irq handler loop (Gustavo)
    active phys bitmask and programming correction (Gustavo)

v5: simplify pmdemand_state structure
    simplify methods to find active phys and max port clock
    Timeout in case of previou pmdemand task pending (Gustavo)

Bspec: 66451, 64636, 64602, 64603
Cc: Matt Atwood <matthew.s.atwood@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Gustavo Sousa <gustavo.sousa@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_display.c  |  14 +
 .../gpu/drm/i915/display/intel_display_core.h |   9 +
 .../drm/i915/display/intel_display_driver.c   |   7 +
 .../drm/i915/display/intel_display_power.c    |   8 +
 drivers/gpu/drm/i915/display/intel_pmdemand.c | 503 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_pmdemand.h |  24 +
 drivers/gpu/drm/i915/i915_irq.c               |  23 +-
 drivers/gpu/drm/i915/i915_reg.h               |  36 +-
 9 files changed, 621 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_pmdemand.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_pmdemand.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7587fe856e67..2698ecf74844 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -270,6 +270,7 @@ i915-y += \
 	display/intel_pch_display.o \
 	display/intel_pch_refclk.o \
 	display/intel_plane_initial.o \
+	display/intel_pmdemand.o \
 	display/intel_psr.o \
 	display/intel_quirks.o \
 	display/intel_sprite.o \
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1d5d42a40803..d9daafe3362c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -99,6 +99,7 @@
 #include "intel_pcode.h"
 #include "intel_pipe_crc.h"
 #include "intel_plane_initial.h"
+#include "intel_pmdemand.h"
 #include "intel_pps.h"
 #include "intel_psr.h"
 #include "intel_sdvo.h"
@@ -6315,6 +6316,10 @@ int intel_atomic_check(struct drm_device *dev,
 			return ret;
 	}
 
+	ret = intel_pmdemand_atomic_check(state);
+	if (ret)
+		goto fail;
+
 	ret = intel_atomic_check_crtcs(state);
 	if (ret)
 		goto fail;
@@ -6960,6 +6965,14 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
 		crtc->config = new_crtc_state;
 
+	/*
+	 * In XE_LPD+ Pmdemand combines many parameters such as voltage index,
+	 * plls, cdclk frequency, QGV point selection parameter etc. Voltage
+	 * index, cdclk/ddiclk frequencies are supposed to be configured before
+	 * the cdclk config is set.
+	 */
+	intel_pmdemand_pre_plane_update(state);
+
 	if (state->modeset) {
 		drm_atomic_helper_update_legacy_modeset_state(dev, &state->base);
 
@@ -7079,6 +7092,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		intel_verify_planes(state);
 
 	intel_sagv_post_plane_update(state);
+	intel_pmdemand_post_plane_update(state);
 
 	drm_atomic_helper_commit_hw_done(&state->base);
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index 9f66d734edf6..ae45b2c42eb1 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -345,6 +345,15 @@ struct intel_display {
 		struct intel_global_obj obj;
 	} dbuf;
 
+	struct {
+		wait_queue_head_t waitqueue;
+
+		/* mutex to protect pmdemand programming sequence */
+		struct mutex lock;
+
+		struct intel_global_obj obj;
+	} pmdemand;
+
 	struct {
 		/*
 		 * dkl.phy_lock protects against concurrent access of the
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 60ce10fc7205..dc8de861339d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -47,6 +47,7 @@
 #include "intel_opregion.h"
 #include "intel_overlay.h"
 #include "intel_plane_initial.h"
+#include "intel_pmdemand.h"
 #include "intel_pps.h"
 #include "intel_quirks.h"
 #include "intel_vga.h"
@@ -211,6 +212,8 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
 	if (ret < 0)
 		goto cleanup_vga;
 
+	intel_pmdemand_init_early(i915);
+
 	intel_power_domains_init_hw(i915, false);
 
 	if (!HAS_DISPLAY(i915))
@@ -240,6 +243,10 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
 	if (ret)
 		goto cleanup_vga_client_pw_domain_dmc;
 
+	ret = intel_pmdemand_init(i915);
+	if (ret)
+		goto cleanup_vga_client_pw_domain_dmc;
+
 	init_llist_head(&i915->display.atomic_helper.free_list);
 	INIT_WORK(&i915->display.atomic_helper.free_work,
 		  intel_atomic_helper_free_state_worker);
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 5150069f3f82..f5c5a486efbc 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -20,6 +20,7 @@
 #include "intel_mchbar_regs.h"
 #include "intel_pch_refclk.h"
 #include "intel_pcode.h"
+#include "intel_pmdemand.h"
 #include "intel_pps_regs.h"
 #include "intel_snps_phy.h"
 #include "skl_watermark.h"
@@ -1085,6 +1086,10 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
 	dev_priv->display.dbuf.enabled_slices =
 		intel_enabled_dbuf_slices_mask(dev_priv);
 
+	if (DISPLAY_VER(dev_priv) >= 14)
+		intel_program_dbuf_pmdemand(dev_priv, BIT(DBUF_S1) |
+					    dev_priv->display.dbuf.enabled_slices);
+
 	/*
 	 * Just power up at least 1 slice, we will
 	 * figure out later which slices we have and what we need.
@@ -1096,6 +1101,9 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
 static void gen9_dbuf_disable(struct drm_i915_private *dev_priv)
 {
 	gen9_dbuf_slices_update(dev_priv, 0);
+
+	if (DISPLAY_VER(dev_priv) >= 14)
+		intel_program_dbuf_pmdemand(dev_priv, 0);
 }
 
 static void gen12_dbuf_slices_config(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.c b/drivers/gpu/drm/i915/display/intel_pmdemand.c
new file mode 100644
index 000000000000..843440b9430b
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.c
@@ -0,0 +1,503 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include <linux/bitops.h>
+
+#include "i915_drv.h"
+#include "i915_reg.h"
+#include "intel_bw.h"
+#include "intel_cdclk.h"
+#include "intel_cx0_phy.h"
+#include "intel_de.h"
+#include "intel_display.h"
+#include "intel_display_trace.h"
+#include "intel_pmdemand.h"
+#include "skl_watermark.h"
+
+struct pmdemand_params {
+	u16 qclk_gv_bw;
+	u8 voltage_index;
+	u8 qclk_gv_index;
+	u8 active_pipes;
+	u8 dbufs;
+	u8 active_phys;
+	u16 cdclk_freq_mhz;
+	u16 ddiclk_max;
+	u8 scalers;
+};
+struct intel_pmdemand_state {
+	struct intel_global_state base;
+
+	/* Parameters to be configured in the pmdemand registers */
+	struct pmdemand_params params;
+};
+
+#define to_intel_pmdemand_state(x) container_of((x) + BUILD_BUG_ON_ZERO(offsetof(struct intel_pmdemand_state, base)), \
+						struct intel_pmdemand_state, base)
+
+static struct intel_global_state *
+intel_pmdemand_duplicate_state(struct intel_global_obj *obj)
+{
+	struct intel_pmdemand_state *pmdmnd_state;
+
+	pmdmnd_state = kmemdup(obj->state, sizeof(*pmdmnd_state), GFP_KERNEL);
+	if (!pmdmnd_state)
+		return NULL;
+
+	return &pmdmnd_state->base;
+}
+
+static void intel_pmdemand_destroy_state(struct intel_global_obj *obj,
+					 struct intel_global_state *state)
+{
+	kfree(state);
+}
+
+static const struct intel_global_state_funcs intel_pmdemand_funcs = {
+	.atomic_duplicate_state = intel_pmdemand_duplicate_state,
+	.atomic_destroy_state = intel_pmdemand_destroy_state,
+};
+
+static struct intel_pmdemand_state *
+intel_atomic_get_pmdemand_state(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct intel_global_state *pmdemand_state =
+		intel_atomic_get_global_obj_state(state,
+						  &i915->display.pmdemand.obj);
+
+	if (IS_ERR(pmdemand_state))
+		return ERR_CAST(pmdemand_state);
+
+	return to_intel_pmdemand_state(pmdemand_state);
+}
+
+static struct intel_pmdemand_state *
+intel_atomic_get_old_pmdemand_state(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct intel_global_state *pmdemand_state =
+		intel_atomic_get_old_global_obj_state(state,
+						      &i915->display.pmdemand.obj);
+
+	if (!pmdemand_state)
+		return NULL;
+
+	return to_intel_pmdemand_state(pmdemand_state);
+}
+
+static struct intel_pmdemand_state *
+intel_atomic_get_new_pmdemand_state(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct intel_global_state *pmdemand_state =
+		intel_atomic_get_new_global_obj_state(state,
+						      &i915->display.pmdemand.obj);
+
+	if (!pmdemand_state)
+		return NULL;
+
+	return to_intel_pmdemand_state(pmdemand_state);
+}
+
+int intel_pmdemand_init(struct drm_i915_private *i915)
+{
+	struct intel_pmdemand_state *pmdemand_state;
+
+	pmdemand_state = kzalloc(sizeof(*pmdemand_state), GFP_KERNEL);
+	if (!pmdemand_state)
+		return -ENOMEM;
+
+	intel_atomic_global_obj_init(i915, &i915->display.pmdemand.obj,
+				     &pmdemand_state->base,
+				     &intel_pmdemand_funcs);
+
+	if (IS_MTL_DISPLAY_STEP(i915, STEP_A0, STEP_C0))
+		/* Wa_14016740474 */
+		intel_de_rmw(i915, XELPD_CHICKEN_DCPR_3, 0, DMD_RSP_TIMEOUT_DISABLE);
+
+	return 0;
+}
+
+void intel_pmdemand_init_early(struct drm_i915_private *i915)
+{
+	mutex_init(&i915->display.pmdemand.lock);
+	init_waitqueue_head(&i915->display.pmdemand.waitqueue);
+}
+
+static u16 pmdemand_max_ddiclk(struct intel_atomic_state *state,
+			       struct intel_pmdemand_state *pmdemand_state)
+{
+	int max = 0;
+	struct intel_crtc *crtc;
+	int i;
+	const struct intel_crtc_state *new_crtc_state, *old_crtc_state;
+
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i)
+		max = max(new_crtc_state->port_clock, max);
+
+	return DIV_ROUND_UP(max, 1000);
+}
+
+static u8
+pmdemand_active_non_tc_phys(struct drm_i915_private *i915,
+			    struct intel_atomic_state *state,
+			    struct intel_pmdemand_state *pmdemand_state)
+{
+	struct drm_connector_list_iter conn_iter;
+	struct intel_connector *connector;
+	u8 phys = 0;
+
+	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
+		enum phy phy = intel_port_to_phy(i915, connector->encoder->port);
+
+		if (intel_phy_is_tc(i915, phy))
+			continue;
+
+		if (connector->get_hw_state(connector))
+			phys++;
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return phys;
+}
+
+
+static bool pmdemand_needs_update(struct intel_atomic_state *state)
+{
+	bool states_checked = false;
+	struct intel_crtc *crtc;
+	int i;
+	const struct intel_crtc_state *new_crtc_state, *old_crtc_state;
+
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		const struct intel_bw_state *new_bw_state, *old_bw_state;
+		const struct intel_cdclk_state *new_cdclk_state;
+		const struct intel_cdclk_state *old_cdclk_state;
+		const struct intel_dbuf_state *new_dbuf_state, *old_dbuf_state;
+
+		if (old_crtc_state->port_clock != new_crtc_state->port_clock)
+			return true;
+
+		/*
+		 * For the below settings once through the loop is enough.
+		 * Some pmdemand_atomic_check calls might trigger read lock not
+		 * taken assert if these following checks are kept outside this
+		 * loop.
+		 */
+		if (states_checked)
+			continue;
+
+		new_bw_state = intel_atomic_get_new_bw_state(state);
+		old_bw_state = intel_atomic_get_old_bw_state(state);
+		if (new_bw_state && new_bw_state->qgv_point_peakbw !=
+		    old_bw_state->qgv_point_peakbw)
+			return true;
+
+		new_dbuf_state = intel_atomic_get_new_dbuf_state(state);
+		old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
+		if (new_dbuf_state && new_dbuf_state->active_pipes !=
+		    old_dbuf_state->active_pipes)
+			return true;
+
+		new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
+		old_cdclk_state = intel_atomic_get_old_cdclk_state(state);
+		if (new_cdclk_state &&
+		    (new_cdclk_state->logical.cdclk !=
+		     old_cdclk_state->logical.cdclk ||
+		     new_cdclk_state->logical.voltage_level !=
+		     old_cdclk_state->logical.voltage_level))
+			return true;
+
+		states_checked = true;
+	}
+
+	return false;
+}
+
+int intel_pmdemand_atomic_check(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_bw_state *new_bw_state;
+	const struct intel_cdclk_state *new_cdclk_state;
+	const struct intel_dbuf_state *new_dbuf_state;
+	struct intel_pmdemand_state *new_pmdemand_state;
+	int ret;
+
+	if (DISPLAY_VER(i915) < 14)
+		return 0;
+
+	if (!pmdemand_needs_update(state))
+		return 0;
+
+	new_pmdemand_state = intel_atomic_get_pmdemand_state(state);
+	if (IS_ERR(new_pmdemand_state))
+		return PTR_ERR(new_pmdemand_state);
+
+	ret = intel_atomic_lock_global_state(&new_pmdemand_state->base);
+	if (ret)
+		return ret;
+
+	new_bw_state = intel_atomic_get_bw_state(state);
+	if (IS_ERR(new_bw_state))
+		return PTR_ERR(new_bw_state);
+
+	/* firmware will calculate the qclck_gc_index, requirement is set to 0 */
+	new_pmdemand_state->params.qclk_gv_index = 0;
+	new_pmdemand_state->params.qclk_gv_bw =
+		min_t(u16, new_bw_state->qgv_point_peakbw, 0xffff);
+
+	new_dbuf_state = intel_atomic_get_dbuf_state(state);
+	if (IS_ERR(new_dbuf_state))
+		return PTR_ERR(new_dbuf_state);
+
+	new_pmdemand_state->params.active_pipes =
+		min_t(u8, hweight8(new_dbuf_state->active_pipes), 3);
+
+	new_cdclk_state = intel_atomic_get_cdclk_state(state);
+	if (IS_ERR(new_cdclk_state))
+		return PTR_ERR(new_cdclk_state);
+
+	new_pmdemand_state->params.voltage_index =
+		new_cdclk_state->logical.voltage_level;
+	new_pmdemand_state->params.cdclk_freq_mhz =
+		DIV_ROUND_UP(new_cdclk_state->logical.cdclk, 1000);
+
+	new_pmdemand_state->params.ddiclk_max =
+		pmdemand_max_ddiclk(state, new_pmdemand_state);
+
+	new_pmdemand_state->params.active_phys =
+		pmdemand_active_non_tc_phys(i915, state, new_pmdemand_state);
+
+	/*
+	 * Setting scalers to max as it can not be calculated during flips and
+	 * fastsets without taking global states locks.
+	 */
+	new_pmdemand_state->params.scalers = 7;
+
+	ret = intel_atomic_serialize_global_state(&new_pmdemand_state->base);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static bool intel_pmdemand_check_prev_transaction(struct drm_i915_private *i915)
+{
+	return !(intel_de_wait_for_clear(i915,
+					 XELPDP_INITIATE_PMDEMAND_REQUEST(1),
+					 XELPDP_PMDEMAND_REQ_ENABLE, 10) ||
+		 intel_de_wait_for_clear(i915,
+					 GEN12_DCPR_STATUS_1,
+					 XELPDP_PMDEMAND_INFLIGHT_STATUS, 10));
+}
+
+static bool intel_pmdemand_req_complete(struct drm_i915_private *i915)
+{
+	return !(intel_de_read(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1)) &
+		 XELPDP_PMDEMAND_REQ_ENABLE);
+}
+
+static int intel_pmdemand_wait(struct drm_i915_private *i915)
+{
+	DEFINE_WAIT(wait);
+	int ret;
+	const unsigned int timeout_ms = 10;
+
+	ret = wait_event_timeout(i915->display.pmdemand.waitqueue,
+				 intel_pmdemand_req_complete(i915),
+				 msecs_to_jiffies_timeout(timeout_ms));
+	if (ret == 0)
+		drm_err(&i915->drm,
+			"timed out waiting for Punit PM Demand Response\n");
+
+	return ret;
+}
+
+/* Required to be programmed during Display Init Sequences. */
+void intel_program_dbuf_pmdemand(struct drm_i915_private *i915,
+				 u8 dbuf_slices)
+{
+	u32 dbufs = min_t(u32, hweight8(dbuf_slices), 3);
+
+	mutex_lock(&i915->display.pmdemand.lock);
+	if (drm_WARN_ON(&i915->drm,
+			!intel_pmdemand_check_prev_transaction(i915)))
+		goto unlock;
+
+	intel_de_rmw(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(0),
+		     XELPDP_PMDEMAND_DBUFS_MASK, XELPDP_PMDEMAND_DBUFS(dbufs));
+	intel_de_rmw(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1), 0,
+		     XELPDP_PMDEMAND_REQ_ENABLE);
+
+	intel_pmdemand_wait(i915);
+
+unlock:
+	mutex_unlock(&i915->display.pmdemand.lock);
+}
+
+static void update_pmdemand_values(const struct intel_pmdemand_state *new,
+				   const struct intel_pmdemand_state *old,
+				   u32 *reg1, u32 *reg2)
+{
+	u32 plls, tmp;
+
+	/*
+	 * The pmdemand parameter updates happens in two steps. Pre plane and
+	 * post plane updates. During the pre plane, as DE might still be
+	 * handling with some old operations, to avoid unwanted performance
+	 * issues, program the pmdemand parameters with higher of old and new
+	 * values. And then after once settled, use the new parameter values
+	 * as part of the post plane update.
+	 */
+
+	/* Set 1*/
+	*reg1 &= ~XELPDP_PMDEMAND_QCLK_GV_BW_MASK;
+	tmp = old ? max(old->params.qclk_gv_bw, new->params.qclk_gv_bw) :
+		    new->params.qclk_gv_bw;
+	*reg1 |= XELPDP_PMDEMAND_QCLK_GV_BW(tmp);
+
+	*reg1 &= ~XELPDP_PMDEMAND_VOLTAGE_INDEX_MASK;
+	tmp = old ? max(old->params.voltage_index, new->params.voltage_index) :
+		    new->params.voltage_index;
+	*reg1 |= XELPDP_PMDEMAND_VOLTAGE_INDEX(tmp);
+
+	*reg1 &= ~XELPDP_PMDEMAND_QCLK_GV_INDEX_MASK;
+	tmp = old ? max(old->params.qclk_gv_index, new->params.qclk_gv_index) :
+		    new->params.qclk_gv_index;
+	*reg1 |= XELPDP_PMDEMAND_QCLK_GV_INDEX(tmp);
+
+	*reg1 &= ~XELPDP_PMDEMAND_PIPES_MASK;
+	tmp = old ? max(old->params.active_pipes, new->params.active_pipes) :
+		    new->params.active_pipes;
+	*reg1 |= XELPDP_PMDEMAND_PIPES(tmp);
+
+	*reg1 &= ~XELPDP_PMDEMAND_PHYS_MASK;
+	plls = old ? max(old->params.active_phys, new->params.active_phys) :
+		     new->params.active_phys;
+	plls = min_t(u32, plls, 7);
+	*reg1 |= XELPDP_PMDEMAND_PHYS(plls);
+
+	/* Set 2*/
+	*reg2 &= ~XELPDP_PMDEMAND_CDCLK_FREQ_MASK;
+	tmp = old ? max(old->params.cdclk_freq_mhz,
+			new->params.cdclk_freq_mhz) :
+		    new->params.cdclk_freq_mhz;
+	*reg2 |= XELPDP_PMDEMAND_CDCLK_FREQ(tmp);
+
+	*reg2 &= ~XELPDP_PMDEMAND_DDICLK_FREQ_MASK;
+	tmp = old ? max(old->params.ddiclk_max, new->params.ddiclk_max) :
+		    new->params.ddiclk_max;
+	*reg2 |= XELPDP_PMDEMAND_DDICLK_FREQ(tmp);
+
+	*reg2 &= ~XELPDP_PMDEMAND_SCALERS_MASK;
+	tmp = old ? max(old->params.scalers, new->params.scalers) :
+		    new->params.scalers;
+	*reg2 |= XELPDP_PMDEMAND_SCALERS(tmp);
+
+	/*
+	 * Active_PLLs starts with 1 because of CDCLK PLL.
+	 * TODO: Missing to account genlock filter when it gets used.
+	 */
+	plls = min_t(u32, plls + 1, 7);
+	*reg2 &= ~XELPDP_PMDEMAND_PLLS_MASK;
+	*reg2 |= XELPDP_PMDEMAND_PLLS(plls);
+}
+
+static void intel_program_pmdemand(struct drm_i915_private *i915,
+				   const struct intel_pmdemand_state *new,
+				   const struct intel_pmdemand_state *old)
+{
+	bool changed = false;
+	u32 reg1, mod_reg1;
+	u32 reg2, mod_reg2;
+
+	mutex_lock(&i915->display.pmdemand.lock);
+	if (drm_WARN_ON(&i915->drm,
+			!intel_pmdemand_check_prev_transaction(i915)))
+		goto unlock;
+
+	reg1 = intel_de_read(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(0));
+	mod_reg1 = reg1;
+
+	reg2 = intel_de_read(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1));
+	mod_reg2 = reg2;
+
+	update_pmdemand_values(new, old, &mod_reg1, &mod_reg2);
+
+	if (reg1 != mod_reg1) {
+		intel_de_write(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(0),
+			       mod_reg1);
+		changed = true;
+	}
+
+	if (reg2 != mod_reg2) {
+		intel_de_write(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1),
+			       mod_reg2);
+		changed = true;
+	}
+
+	/* Initiate pm demand request only if register values are changed */
+	if (!changed)
+		goto unlock;
+
+	drm_dbg_kms(&i915->drm,
+		    "initate pmdemand request values: (0x%x 0x%x)\n",
+		    mod_reg1, mod_reg2);
+
+	intel_de_rmw(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1), 0,
+			XELPDP_PMDEMAND_REQ_ENABLE);
+
+	intel_pmdemand_wait(i915);
+
+unlock:
+	mutex_unlock(&i915->display.pmdemand.lock);
+}
+
+static bool
+intel_pmdemand_state_changed(const struct intel_pmdemand_state *new,
+			     const struct intel_pmdemand_state *old)
+{
+	return memcmp(&new->params, &old->params, sizeof(new->params)) != 0;
+}
+
+void intel_pmdemand_pre_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_pmdemand_state *new_pmdmnd_state =
+		intel_atomic_get_new_pmdemand_state(state);
+	const struct intel_pmdemand_state *old_pmdmnd_state =
+		intel_atomic_get_old_pmdemand_state(state);
+
+	if (DISPLAY_VER(i915) < 14)
+		return;
+
+	if (!new_pmdmnd_state ||
+	    !intel_pmdemand_state_changed(new_pmdmnd_state, old_pmdmnd_state))
+		return;
+
+	intel_program_pmdemand(i915, new_pmdmnd_state, old_pmdmnd_state);
+}
+
+void intel_pmdemand_post_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_pmdemand_state *new_pmdmnd_state =
+		intel_atomic_get_new_pmdemand_state(state);
+	const struct intel_pmdemand_state *old_pmdmnd_state =
+		intel_atomic_get_old_pmdemand_state(state);
+
+	if (DISPLAY_VER(i915) < 14)
+		return;
+
+	if (!new_pmdmnd_state ||
+	    !intel_pmdemand_state_changed(new_pmdmnd_state, old_pmdmnd_state))
+		return;
+
+	intel_program_pmdemand(i915, new_pmdmnd_state, NULL);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.h b/drivers/gpu/drm/i915/display/intel_pmdemand.h
new file mode 100644
index 000000000000..2883b5d97a44
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef __INTEL_PMDEMAND_H__
+#define __INTEL_PMDEMAND_H__
+
+#include <linux/types.h>
+
+struct drm_i915_private;
+struct intel_atomic_state;
+struct intel_crtc_state;
+struct intel_plane_state;
+
+void intel_pmdemand_init_early(struct drm_i915_private *i915);
+int intel_pmdemand_init(struct drm_i915_private *i915);
+void intel_program_dbuf_pmdemand(struct drm_i915_private *i915,
+				 u8 dbuf_slices);
+void intel_pmdemand_pre_plane_update(struct intel_atomic_state *state);
+void intel_pmdemand_post_plane_update(struct intel_atomic_state *state);
+int intel_pmdemand_atomic_check(struct intel_atomic_state *state);
+
+#endif /* __INTEL_PMDEMAND_H__ */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 02b6cbb832e9..49459ab70dd1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -43,6 +43,7 @@
 #include "display/intel_gmbus.h"
 #include "display/intel_hotplug.h"
 #include "display/intel_lpe_audio.h"
+#include "display/intel_pmdemand.h"
 #include "display/intel_psr.h"
 #include "display/intel_psr_regs.h"
 
@@ -1981,12 +1982,27 @@ static u32 gen8_de_pipe_fault_mask(struct drm_i915_private *dev_priv)
 		return GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
 }
 
+static void intel_pmdemand_irq_handler(struct drm_i915_private *dev_priv)
+{
+	wake_up_all(&dev_priv->display.pmdemand.waitqueue);
+}
+
 static void
 gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
 {
 	bool found = false;
 
-	if (iir & GEN8_DE_MISC_GSE) {
+	if (DISPLAY_VER(dev_priv) >= 14) {
+		if (iir & (XELPDP_PMDEMAND_RSP |
+			   XELPDP_PMDEMAND_RSPTOUT_ERR)) {
+			if (iir & XELPDP_PMDEMAND_RSPTOUT_ERR)
+				drm_dbg(&dev_priv->drm,
+					"Error waiting for Punit PM Demand Response\n");
+
+			intel_pmdemand_irq_handler(dev_priv);
+			found = true;
+		}
+	} else if (iir & GEN8_DE_MISC_GSE) {
 		intel_opregion_asle_intr(dev_priv);
 		found = true;
 	}
@@ -3737,7 +3753,10 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
 		de_port_masked |= BXT_DE_PORT_GMBUS;
 
-	if (DISPLAY_VER(dev_priv) >= 11) {
+	if (DISPLAY_VER(dev_priv) >= 14)
+		de_misc_masked |= XELPDP_PMDEMAND_RSPTOUT_ERR |
+				  XELPDP_PMDEMAND_RSP;
+	else if (DISPLAY_VER(dev_priv) >= 11) {
 		enum port port;
 
 		if (intel_bios_is_dsi_present(dev_priv, &port))
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2a9ab8de8421..91fb12b65c92 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4450,8 +4450,10 @@
 #define GEN8_DE_MISC_IMR _MMIO(0x44464)
 #define GEN8_DE_MISC_IIR _MMIO(0x44468)
 #define GEN8_DE_MISC_IER _MMIO(0x4446c)
-#define  GEN8_DE_MISC_GSE		(1 << 27)
-#define  GEN8_DE_EDP_PSR		(1 << 19)
+#define  XELPDP_PMDEMAND_RSPTOUT_ERR	REG_BIT(27)
+#define  GEN8_DE_MISC_GSE		REG_BIT(27)
+#define  GEN8_DE_EDP_PSR		REG_BIT(19)
+#define  XELPDP_PMDEMAND_RSP		REG_BIT(3)
 
 #define GEN8_PCU_ISR _MMIO(0x444e0)
 #define GEN8_PCU_IMR _MMIO(0x444e4)
@@ -4536,6 +4538,33 @@
 #define  XELPDP_DP_ALT_HPD_LONG_DETECT		REG_BIT(1)
 #define  XELPDP_DP_ALT_HPD_SHORT_DETECT		REG_BIT(0)
 
+#define XELPDP_INITIATE_PMDEMAND_REQUEST(dword)		_MMIO(0x45230 + 4 * (dword))
+#define  XELPDP_PMDEMAND_QCLK_GV_BW_MASK		REG_GENMASK(31, 16)
+#define  XELPDP_PMDEMAND_QCLK_GV_BW(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_QCLK_GV_BW_MASK, x)
+#define  XELPDP_PMDEMAND_VOLTAGE_INDEX_MASK		REG_GENMASK(14, 12)
+#define  XELPDP_PMDEMAND_VOLTAGE_INDEX(x)		REG_FIELD_PREP(XELPDP_PMDEMAND_VOLTAGE_INDEX_MASK, x)
+#define  XELPDP_PMDEMAND_QCLK_GV_INDEX_MASK		REG_GENMASK(11, 8)
+#define  XELPDP_PMDEMAND_QCLK_GV_INDEX(x)		REG_FIELD_PREP(XELPDP_PMDEMAND_QCLK_GV_INDEX_MASK, x)
+#define  XELPDP_PMDEMAND_PIPES_MASK			REG_GENMASK(7, 6)
+#define  XELPDP_PMDEMAND_PIPES(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_PIPES_MASK, x)
+#define  XELPDP_PMDEMAND_DBUFS_MASK			REG_GENMASK(5, 4)
+#define  XELPDP_PMDEMAND_DBUFS(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_DBUFS_MASK, x)
+#define  XELPDP_PMDEMAND_PHYS_MASK			REG_GENMASK(2, 0)
+#define  XELPDP_PMDEMAND_PHYS(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_PHYS_MASK, x)
+
+#define  XELPDP_PMDEMAND_REQ_ENABLE			REG_BIT(31)
+#define  XELPDP_PMDEMAND_CDCLK_FREQ_MASK		REG_GENMASK(30, 20)
+#define  XELPDP_PMDEMAND_CDCLK_FREQ(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_CDCLK_FREQ_MASK, x)
+#define  XELPDP_PMDEMAND_DDICLK_FREQ_MASK		REG_GENMASK(18, 8)
+#define  XELPDP_PMDEMAND_DDICLK_FREQ(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_DDICLK_FREQ_MASK, x)
+#define  XELPDP_PMDEMAND_SCALERS_MASK			REG_GENMASK(6, 4)
+#define  XELPDP_PMDEMAND_SCALERS(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_SCALERS_MASK, x)
+#define  XELPDP_PMDEMAND_PLLS_MASK			REG_GENMASK(2, 0)
+#define  XELPDP_PMDEMAND_PLLS(x)			REG_FIELD_PREP(XELPDP_PMDEMAND_PLLS_MASK, x)
+
+#define GEN12_DCPR_STATUS_1				_MMIO(0x46440)
+#define  XELPDP_PMDEMAND_INFLIGHT_STATUS		REG_BIT(26)
+
 #define ILK_DISPLAY_CHICKEN2	_MMIO(0x42004)
 /* Required on all Ironlake and Sandybridge according to the B-Spec. */
 #define   ILK_ELPIN_409_SELECT	REG_BIT(25)
@@ -4695,6 +4724,9 @@
 #define   DCPR_SEND_RESP_IMM			REG_BIT(25)
 #define   DCPR_CLEAR_MEMSTAT_DIS		REG_BIT(24)
 
+#define XELPD_CHICKEN_DCPR_3			_MMIO(0x46438)
+#define   DMD_RSP_TIMEOUT_DISABLE		REG_BIT(19)
+
 #define SKL_DFSM			_MMIO(0x51000)
 #define   SKL_DFSM_DISPLAY_PM_DISABLE	(1 << 27)
 #define   SKL_DFSM_DISPLAY_HDCP_DISABLE	(1 << 25)
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.BUILD: failure for mtl: add support for pmdemand (rev6)

[Patchwork]

== Series Details ==

Series: mtl: add support for pmdemand (rev6)
URL   : https://patchwork.freedesktop.org/series/116949/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/116949/revisions/6/mbox/ not applied
Applying: drm/i915: fix the derating percentage for MTL
Applying: drm/i915: update the QGV point frequency calculations
Applying: drm/i915: store the peak bw per QGV point
Applying: drm/i915: extract intel_bw_check_qgv_points()
Applying: drm/i915: modify max_bw to return index to intel_bw_info
Applying: drm/i915/mtl: find the best QGV point for the SAGV configuration
Applying: drm/i915/mtl: Add support for PM DEMAND
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/Makefile
M	drivers/gpu/drm/i915/display/intel_display.c
M	drivers/gpu/drm/i915/display/intel_display_power.c
M	drivers/gpu/drm/i915/i915_irq.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_irq.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_irq.c
Auto-merging drivers/gpu/drm/i915/display/intel_display_power.c
Auto-merging drivers/gpu/drm/i915/display/intel_display.c
Auto-merging drivers/gpu/drm/i915/Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0007 drm/i915/mtl: Add support for PM DEMAND
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced

Re: [Intel-gfx] [PATCH v6 2/7] drm/i915: update the QGV point frequency calculations

[Jani Nikula]

On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> From MTL onwwards, pcode locks the QGV point based on peak BW of
> the intended QGV point passed by the driver. So the peak BW
> calculation must match the value expected by the pcode. Update
> the calculations as per the Bspec.
>
> v2: use DIV_ROUND_* macro for the calculations (Ville)
>
> Bspec: 64636
>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index ab405c48ca3a..c8075a37c3ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -182,7 +182,7 @@ static int mtl_read_qgv_point_info(struct drm_i915_private *dev_priv,
>  	val2 = intel_uncore_read(&dev_priv->uncore,
>  				 MTL_MEM_SS_INFO_QGV_POINT_HIGH(point));
>  	dclk = REG_FIELD_GET(MTL_DCLK_MASK, val);
> -	sp->dclk = DIV_ROUND_UP((16667 * dclk), 1000);
> +	sp->dclk = DIV_ROUND_CLOSEST(16667 * dclk + 500, 1000);

What's with the +500 there?

BR,
Jani.


>  	sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val);
>  	sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v6 4/7] drm/i915: extract intel_bw_check_qgv_points()

[Jani Nikula]

On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> Extract intel_bw_check_qgv_points() from intel_bw_atomic_check
> to facilitate future platform variations in handling SAGV
> configurations.
>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 235 +++++++++++++-----------
>  1 file changed, 130 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index db117638d23b..d83aafd0cc2b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -803,6 +803,128 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
>  	return to_intel_bw_state(bw_state);
>  }
>  
> +static int icl_find_qgv_points(struct drm_i915_private *i915,
> +			       unsigned int data_rate,
> +			       unsigned int num_active_planes,
> +			       const struct intel_bw_state *old_bw_state,
> +			       struct intel_bw_state *new_bw_state)
> +{
> +	unsigned int max_bw_point = 0;
> +	unsigned int max_bw = 0;
> +	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;

Please just use signed ints whenever you don't have a specific reason
not to. Ditto for parameters being passed. Mixing signed and unsigned
will lead to trouble.

> +	u16 psf_points = 0;
> +	u16 qgv_points = 0;
> +	int i;
> +	int ret;
> +
> +	ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_qgv_points; i++) {

This is signed and unsigned comparison.

> +		unsigned int max_data_rate;
> +
> +		if (DISPLAY_VER(i915) > 11)
> +			max_data_rate = tgl_max_bw(i915, num_active_planes, i);
> +		else
> +			max_data_rate = icl_max_bw(i915, num_active_planes, i);
> +		/*
> +		 * We need to know which qgv point gives us
> +		 * maximum bandwidth in order to disable SAGV
> +		 * if we find that we exceed SAGV block time
> +		 * with watermarks. By that moment we already
> +		 * have those, as it is calculated earlier in
> +		 * intel_atomic_check,
> +		 */
> +		if (max_data_rate > max_bw) {
> +			max_bw_point = i;
> +			max_bw = max_data_rate;
> +		}
> +		if (max_data_rate >= data_rate)
> +			qgv_points |= BIT(i);
> +
> +		drm_dbg_kms(&i915->drm, "QGV point %d: max bw %d required %d\n",
> +			    i, max_data_rate, data_rate);
> +	}
> +
> +	for (i = 0; i < num_psf_gv_points; i++) {
> +		unsigned int max_data_rate = adl_psf_bw(i915, i);
> +
> +		if (max_data_rate >= data_rate)
> +			psf_points |= BIT(i);
> +
> +		drm_dbg_kms(&i915->drm, "PSF GV point %d: max bw %d"
> +			    " required %d\n",
> +			    i, max_data_rate, data_rate);
> +	}
> +
> +	/*
> +	 * BSpec states that we always should have at least one allowed point
> +	 * left, so if we couldn't - simply reject the configuration for obvious
> +	 * reasons.
> +	 */
> +	if (qgv_points == 0) {
> +		drm_dbg_kms(&i915->drm, "No QGV points provide sufficient memory"
> +			    " bandwidth %d for display configuration(%d active planes).\n",
> +			    data_rate, num_active_planes);
> +		return -EINVAL;
> +	}
> +
> +	if (num_psf_gv_points > 0 && psf_points == 0) {
> +		drm_dbg_kms(&i915->drm, "No PSF GV points provide sufficient memory"
> +			    " bandwidth %d for display configuration(%d active planes).\n",
> +			    data_rate, num_active_planes);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Leave only single point with highest bandwidth, if
> +	 * we can't enable SAGV due to the increased memory latency it may
> +	 * cause.
> +	 */
> +	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> +		qgv_points = BIT(max_bw_point);
> +		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> +			    max_bw_point);
> +	}
> +
> +	/*
> +	 * We store the ones which need to be masked as that is what PCode
> +	 * actually accepts as a parameter.
> +	 */
> +	new_bw_state->qgv_points_mask =
> +		~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> +		  ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> +		icl_qgv_points_mask(i915);
> +
> +	/*
> +	 * If the actual mask had changed we need to make sure that
> +	 * the commits are serialized(in case this is a nomodeset, nonblocking)
> +	 */
> +	if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
> +		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_bw_check_qgv_points(struct drm_i915_private *i915,
> +				     const struct intel_bw_state *old_bw_state,
> +				     struct intel_bw_state *new_bw_state)
> +{
> +	unsigned int data_rate = intel_bw_data_rate(i915, new_bw_state);
> +	unsigned int num_active_planes =
> +			intel_bw_num_active_planes(i915, new_bw_state);
> +
> +	data_rate = DIV_ROUND_UP(data_rate, 1000);
> +
> +	return icl_find_qgv_points(i915, data_rate, num_active_planes,
> +				   old_bw_state, new_bw_state);
> +}
> +
>  static bool intel_bw_state_changed(struct drm_i915_private *i915,
>  				   const struct intel_bw_state *old_bw_state,
>  				   const struct intel_bw_state *new_bw_state)
> @@ -1049,20 +1171,14 @@ static int intel_bw_check_data_rate(struct intel_atomic_state *state, bool *chan
>  
>  int intel_bw_atomic_check(struct intel_atomic_state *state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_bw_state *old_bw_state;
> -	struct intel_bw_state *new_bw_state;
> -	unsigned int data_rate;
> -	unsigned int num_active_planes;
> -	int i, ret;
> -	u16 qgv_points = 0, psf_points = 0;
> -	unsigned int max_bw_point = 0, max_bw = 0;
> -	unsigned int num_qgv_points = dev_priv->display.bw.max[0].num_qgv_points;
> -	unsigned int num_psf_gv_points = dev_priv->display.bw.max[0].num_psf_gv_points;
>  	bool changed = false;
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct intel_bw_state *new_bw_state;
> +	const struct intel_bw_state *old_bw_state;
> +	int ret;
>  
>  	/* FIXME earlier gens need some checks too */
> -	if (DISPLAY_VER(dev_priv) < 11)
> +	if (DISPLAY_VER(i915) < 11)
>  		return 0;
>  
>  	ret = intel_bw_check_data_rate(state, &changed);
> @@ -1073,8 +1189,8 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	new_bw_state = intel_atomic_get_new_bw_state(state);
>  
>  	if (new_bw_state &&
> -	    intel_can_enable_sagv(dev_priv, old_bw_state) !=
> -	    intel_can_enable_sagv(dev_priv, new_bw_state))
> +	    intel_can_enable_sagv(i915, old_bw_state) !=
> +	    intel_can_enable_sagv(i915, new_bw_state))
>  		changed = true;
>  
>  	/*
> @@ -1084,101 +1200,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	if (!changed)
>  		return 0;
>  
> -	ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +	ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state);
>  	if (ret)
>  		return ret;
>  
> -	data_rate = intel_bw_data_rate(dev_priv, new_bw_state);
> -	data_rate = DIV_ROUND_UP(data_rate, 1000);
> -
> -	num_active_planes = intel_bw_num_active_planes(dev_priv, new_bw_state);
> -
> -	for (i = 0; i < num_qgv_points; i++) {
> -		unsigned int max_data_rate;
> -
> -		if (DISPLAY_VER(dev_priv) > 11)
> -			max_data_rate = tgl_max_bw(dev_priv, num_active_planes, i);
> -		else
> -			max_data_rate = icl_max_bw(dev_priv, num_active_planes, i);
> -		/*
> -		 * We need to know which qgv point gives us
> -		 * maximum bandwidth in order to disable SAGV
> -		 * if we find that we exceed SAGV block time
> -		 * with watermarks. By that moment we already
> -		 * have those, as it is calculated earlier in
> -		 * intel_atomic_check,
> -		 */
> -		if (max_data_rate > max_bw) {
> -			max_bw_point = i;
> -			max_bw = max_data_rate;
> -		}
> -		if (max_data_rate >= data_rate)
> -			qgv_points |= BIT(i);
> -
> -		drm_dbg_kms(&dev_priv->drm, "QGV point %d: max bw %d required %d\n",
> -			    i, max_data_rate, data_rate);
> -	}
> -
> -	for (i = 0; i < num_psf_gv_points; i++) {
> -		unsigned int max_data_rate = adl_psf_bw(dev_priv, i);
> -
> -		if (max_data_rate >= data_rate)
> -			psf_points |= BIT(i);
> -
> -		drm_dbg_kms(&dev_priv->drm, "PSF GV point %d: max bw %d"
> -			    " required %d\n",
> -			    i, max_data_rate, data_rate);
> -	}
> -
> -	/*
> -	 * BSpec states that we always should have at least one allowed point
> -	 * left, so if we couldn't - simply reject the configuration for obvious
> -	 * reasons.
> -	 */
> -	if (qgv_points == 0) {
> -		drm_dbg_kms(&dev_priv->drm, "No QGV points provide sufficient memory"
> -			    " bandwidth %d for display configuration(%d active planes).\n",
> -			    data_rate, num_active_planes);
> -		return -EINVAL;
> -	}
> -
> -	if (num_psf_gv_points > 0 && psf_points == 0) {
> -		drm_dbg_kms(&dev_priv->drm, "No PSF GV points provide sufficient memory"
> -			    " bandwidth %d for display configuration(%d active planes).\n",
> -			    data_rate, num_active_planes);
> -		return -EINVAL;
> -	}
> -
> -	/*
> -	 * Leave only single point with highest bandwidth, if
> -	 * we can't enable SAGV due to the increased memory latency it may
> -	 * cause.
> -	 */
> -	if (!intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		qgv_points = BIT(max_bw_point);
> -		drm_dbg_kms(&dev_priv->drm, "No SAGV, using single QGV point %d\n",
> -			    max_bw_point);
> -	}
> -
> -	/*
> -	 * We store the ones which need to be masked as that is what PCode
> -	 * actually accepts as a parameter.
> -	 */
> -	new_bw_state->qgv_points_mask =
> -		~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> -		  ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> -		icl_qgv_points_mask(dev_priv);
> -
> -	/*
> -	 * If the actual mask had changed we need to make sure that
> -	 * the commits are serialized(in case this is a nomodeset, nonblocking)
> -	 */
> -	if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
> -		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v6 2/7] drm/i915: update the QGV point frequency calculations

[Govindapillai, Vinod]

On Tue, 2023-05-23 at 12:01 +0300, Jani Nikula wrote:
> On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> > From MTL onwwards, pcode locks the QGV point based on peak BW of
> > the intended QGV point passed by the driver. So the peak BW
> > calculation must match the value expected by the pcode. Update
> > the calculations as per the Bspec.
> > 
> > v2: use DIV_ROUND_* macro for the calculations (Ville)
> > 
> > Bspec: 64636
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index ab405c48ca3a..c8075a37c3ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -182,7 +182,7 @@ static int mtl_read_qgv_point_info(struct drm_i915_private *dev_priv,
> >         val2 = intel_uncore_read(&dev_priv->uncore,
> >                                  MTL_MEM_SS_INFO_QGV_POINT_HIGH(point));
> >         dclk = REG_FIELD_GET(MTL_DCLK_MASK, val);
> > -       sp->dclk = DIV_ROUND_UP((16667 * dclk), 1000);
> > +       sp->dclk = DIV_ROUND_CLOSEST(16667 * dclk + 500, 1000);
> 
> What's with the +500 there?

This is what pcode expects. Somehow pcode use this formula and we have to exactly match this. Got
this confirmed from Art.

BR
Vinod

> 
> BR,
> Jani.
> 
> 
> >         sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val);
> >         sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);
> 

Re: [Intel-gfx] [PATCH v6 4/7] drm/i915: extract intel_bw_check_qgv_points()

[Govindapillai, Vinod]

On Tue, 2023-05-23 at 12:05 +0300, Jani Nikula wrote:
> On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> > Extract intel_bw_check_qgv_points() from intel_bw_atomic_check
> > to facilitate future platform variations in handling SAGV
> > configurations.
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 235 +++++++++++++-----------
> >  1 file changed, 130 insertions(+), 105 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index db117638d23b..d83aafd0cc2b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -803,6 +803,128 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
> >         return to_intel_bw_state(bw_state);
> >  }
> >  
> > +static int icl_find_qgv_points(struct drm_i915_private *i915,
> > +                              unsigned int data_rate,
> > +                              unsigned int num_active_planes,
> > +                              const struct intel_bw_state *old_bw_state,
> > +                              struct intel_bw_state *new_bw_state)
> > +{
> > +       unsigned int max_bw_point = 0;
> > +       unsigned int max_bw = 0;
> > +       unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> > +       unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> 
> Please just use signed ints whenever you don't have a specific reason
> not to. Ditto for parameters being passed. Mixing signed and unsigned
> will lead to trouble.

Okay. usually I try to follow that. Here all these unsigned ints are needed because of the return
types
> 
> > +       u16 psf_points = 0;
> > +       u16 qgv_points = 0;
> > +       int i;
> > +       int ret;
> > +
> > +       ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > +       if (ret)
> > +               return ret;
> > +
> > +       for (i = 0; i < num_qgv_points; i++) {
> 
> This is signed and unsigned comparison.

I can update that.

Thanks
Vinod
> 
> > +               unsigned int max_data_rate;
> > +
> > +               if (DISPLAY_VER(i915) > 11)
> > +                       max_data_rate = tgl_max_bw(i915, num_active_planes, i);
> > +               else
> > +                       max_data_rate = icl_max_bw(i915, num_active_planes, i);
> > +               /*
> > +                * We need to know which qgv point gives us
> > +                * maximum bandwidth in order to disable SAGV
> > +                * if we find that we exceed SAGV block time
> > +                * with watermarks. By that moment we already
> > +                * have those, as it is calculated earlier in
> > +                * intel_atomic_check,
> > +                */
> > +               if (max_data_rate > max_bw) {
> > +                       max_bw_point = i;
> > +                       max_bw = max_data_rate;
> > +               }
> > +               if (max_data_rate >= data_rate)
> > +                       qgv_points |= BIT(i);
> > +
> > +               drm_dbg_kms(&i915->drm, "QGV point %d: max bw %d required %d\n",
> > +                           i, max_data_rate, data_rate);
> > +       }
> > +
> > +       for (i = 0; i < num_psf_gv_points; i++) {
> > +               unsigned int max_data_rate = adl_psf_bw(i915, i);
> > +
> > +               if (max_data_rate >= data_rate)
> > +                       psf_points |= BIT(i);
> > +
> > +               drm_dbg_kms(&i915->drm, "PSF GV point %d: max bw %d"
> > +                           " required %d\n",
> > +                           i, max_data_rate, data_rate);
> > +       }
> > +
> > +       /*
> > +        * BSpec states that we always should have at least one allowed point
> > +        * left, so if we couldn't - simply reject the configuration for obvious
> > +        * reasons.
> > +        */
> > +       if (qgv_points == 0) {
> > +               drm_dbg_kms(&i915->drm, "No QGV points provide sufficient memory"
> > +                           " bandwidth %d for display configuration(%d active planes).\n",
> > +                           data_rate, num_active_planes);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (num_psf_gv_points > 0 && psf_points == 0) {
> > +               drm_dbg_kms(&i915->drm, "No PSF GV points provide sufficient memory"
> > +                           " bandwidth %d for display configuration(%d active planes).\n",
> > +                           data_rate, num_active_planes);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /*
> > +        * Leave only single point with highest bandwidth, if
> > +        * we can't enable SAGV due to the increased memory latency it may
> > +        * cause.
> > +        */
> > +       if (!intel_can_enable_sagv(i915, new_bw_state)) {
> > +               qgv_points = BIT(max_bw_point);
> > +               drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> > +                           max_bw_point);
> > +       }
> > +
> > +       /*
> > +        * We store the ones which need to be masked as that is what PCode
> > +        * actually accepts as a parameter.
> > +        */
> > +       new_bw_state->qgv_points_mask =
> > +               ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> > +                 ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> > +               icl_qgv_points_mask(i915);
> > +
> > +       /*
> > +        * If the actual mask had changed we need to make sure that
> > +        * the commits are serialized(in case this is a nomodeset, nonblocking)
> > +        */
> > +       if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
> > +               ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int intel_bw_check_qgv_points(struct drm_i915_private *i915,
> > +                                    const struct intel_bw_state *old_bw_state,
> > +                                    struct intel_bw_state *new_bw_state)
> > +{
> > +       unsigned int data_rate = intel_bw_data_rate(i915, new_bw_state);
> > +       unsigned int num_active_planes =
> > +                       intel_bw_num_active_planes(i915, new_bw_state);
> > +
> > +       data_rate = DIV_ROUND_UP(data_rate, 1000);
> > +
> > +       return icl_find_qgv_points(i915, data_rate, num_active_planes,
> > +                                  old_bw_state, new_bw_state);
> > +}
> > +
> >  static bool intel_bw_state_changed(struct drm_i915_private *i915,
> >                                    const struct intel_bw_state *old_bw_state,
> >                                    const struct intel_bw_state *new_bw_state)
> > @@ -1049,20 +1171,14 @@ static int intel_bw_check_data_rate(struct intel_atomic_state *state,
> > bool *chan
> >  
> >  int intel_bw_atomic_check(struct intel_atomic_state *state)
> >  {
> > -       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > -       const struct intel_bw_state *old_bw_state;
> > -       struct intel_bw_state *new_bw_state;
> > -       unsigned int data_rate;
> > -       unsigned int num_active_planes;
> > -       int i, ret;
> > -       u16 qgv_points = 0, psf_points = 0;
> > -       unsigned int max_bw_point = 0, max_bw = 0;
> > -       unsigned int num_qgv_points = dev_priv->display.bw.max[0].num_qgv_points;
> > -       unsigned int num_psf_gv_points = dev_priv->display.bw.max[0].num_psf_gv_points;
> >         bool changed = false;
> > +       struct drm_i915_private *i915 = to_i915(state->base.dev);
> > +       struct intel_bw_state *new_bw_state;
> > +       const struct intel_bw_state *old_bw_state;
> > +       int ret;
> >  
> >         /* FIXME earlier gens need some checks too */
> > -       if (DISPLAY_VER(dev_priv) < 11)
> > +       if (DISPLAY_VER(i915) < 11)
> >                 return 0;
> >  
> >         ret = intel_bw_check_data_rate(state, &changed);
> > @@ -1073,8 +1189,8 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> >         new_bw_state = intel_atomic_get_new_bw_state(state);
> >  
> >         if (new_bw_state &&
> > -           intel_can_enable_sagv(dev_priv, old_bw_state) !=
> > -           intel_can_enable_sagv(dev_priv, new_bw_state))
> > +           intel_can_enable_sagv(i915, old_bw_state) !=
> > +           intel_can_enable_sagv(i915, new_bw_state))
> >                 changed = true;
> >  
> >         /*
> > @@ -1084,101 +1200,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> >         if (!changed)
> >                 return 0;
> >  
> > -       ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > +       ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state);
> >         if (ret)
> >                 return ret;
> >  
> > -       data_rate = intel_bw_data_rate(dev_priv, new_bw_state);
> > -       data_rate = DIV_ROUND_UP(data_rate, 1000);
> > -
> > -       num_active_planes = intel_bw_num_active_planes(dev_priv, new_bw_state);
> > -
> > -       for (i = 0; i < num_qgv_points; i++) {
> > -               unsigned int max_data_rate;
> > -
> > -               if (DISPLAY_VER(dev_priv) > 11)
> > -                       max_data_rate = tgl_max_bw(dev_priv, num_active_planes, i);
> > -               else
> > -                       max_data_rate = icl_max_bw(dev_priv, num_active_planes, i);
> > -               /*
> > -                * We need to know which qgv point gives us
> > -                * maximum bandwidth in order to disable SAGV
> > -                * if we find that we exceed SAGV block time
> > -                * with watermarks. By that moment we already
> > -                * have those, as it is calculated earlier in
> > -                * intel_atomic_check,
> > -                */
> > -               if (max_data_rate > max_bw) {
> > -                       max_bw_point = i;
> > -                       max_bw = max_data_rate;
> > -               }
> > -               if (max_data_rate >= data_rate)
> > -                       qgv_points |= BIT(i);
> > -
> > -               drm_dbg_kms(&dev_priv->drm, "QGV point %d: max bw %d required %d\n",
> > -                           i, max_data_rate, data_rate);
> > -       }
> > -
> > -       for (i = 0; i < num_psf_gv_points; i++) {
> > -               unsigned int max_data_rate = adl_psf_bw(dev_priv, i);
> > -
> > -               if (max_data_rate >= data_rate)
> > -                       psf_points |= BIT(i);
> > -
> > -               drm_dbg_kms(&dev_priv->drm, "PSF GV point %d: max bw %d"
> > -                           " required %d\n",
> > -                           i, max_data_rate, data_rate);
> > -       }
> > -
> > -       /*
> > -        * BSpec states that we always should have at least one allowed point
> > -        * left, so if we couldn't - simply reject the configuration for obvious
> > -        * reasons.
> > -        */
> > -       if (qgv_points == 0) {
> > -               drm_dbg_kms(&dev_priv->drm, "No QGV points provide sufficient memory"
> > -                           " bandwidth %d for display configuration(%d active planes).\n",
> > -                           data_rate, num_active_planes);
> > -               return -EINVAL;
> > -       }
> > -
> > -       if (num_psf_gv_points > 0 && psf_points == 0) {
> > -               drm_dbg_kms(&dev_priv->drm, "No PSF GV points provide sufficient memory"
> > -                           " bandwidth %d for display configuration(%d active planes).\n",
> > -                           data_rate, num_active_planes);
> > -               return -EINVAL;
> > -       }
> > -
> > -       /*
> > -        * Leave only single point with highest bandwidth, if
> > -        * we can't enable SAGV due to the increased memory latency it may
> > -        * cause.
> > -        */
> > -       if (!intel_can_enable_sagv(dev_priv, new_bw_state)) {
> > -               qgv_points = BIT(max_bw_point);
> > -               drm_dbg_kms(&dev_priv->drm, "No SAGV, using single QGV point %d\n",
> > -                           max_bw_point);
> > -       }
> > -
> > -       /*
> > -        * We store the ones which need to be masked as that is what PCode
> > -        * actually accepts as a parameter.
> > -        */
> > -       new_bw_state->qgv_points_mask =
> > -               ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> > -                 ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> > -               icl_qgv_points_mask(dev_priv);
> > -
> > -       /*
> > -        * If the actual mask had changed we need to make sure that
> > -        * the commits are serialized(in case this is a nomodeset, nonblocking)
> > -        */
> > -       if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
> > -               ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> > -               if (ret)
> > -                       return ret;
> > -       }
> > -
> >         return 0;
> >  }
> 

Re: [Intel-gfx] [PATCH v6 2/7] drm/i915: update the QGV point frequency calculations

[Jani Nikula]

On Tue, 23 May 2023, "Govindapillai, Vinod" <vinod.govindapillai@intel.com> wrote:
> On Tue, 2023-05-23 at 12:01 +0300, Jani Nikula wrote:
>> On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
>> > From MTL onwwards, pcode locks the QGV point based on peak BW of
>> > the intended QGV point passed by the driver. So the peak BW
>> > calculation must match the value expected by the pcode. Update
>> > the calculations as per the Bspec.
>> > 
>> > v2: use DIV_ROUND_* macro for the calculations (Ville)
>> > 
>> > Bspec: 64636
>> > 
>> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
>> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>> > index ab405c48ca3a..c8075a37c3ab 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
>> > @@ -182,7 +182,7 @@ static int mtl_read_qgv_point_info(struct drm_i915_private *dev_priv,
>> >         val2 = intel_uncore_read(&dev_priv->uncore,
>> >                                  MTL_MEM_SS_INFO_QGV_POINT_HIGH(point));
>> >         dclk = REG_FIELD_GET(MTL_DCLK_MASK, val);
>> > -       sp->dclk = DIV_ROUND_UP((16667 * dclk), 1000);
>> > +       sp->dclk = DIV_ROUND_CLOSEST(16667 * dclk + 500, 1000);
>> 
>> What's with the +500 there?
>
> This is what pcode expects. Somehow pcode use this formula and we have to exactly match this. Got
> this confirmed from Art.

I'm guessing all it really means is to round to closest, and they
specified it like that instead of saying "round to closest".

Essentially (x + (div/2)) / div is what DIV_ROUND_CLOSEST() does.

It's odd to do it *twice*, and surely not what they expect.

BR,
Jani.

>
> BR
> Vinod
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >         sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val);
>> >         sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v6 7/7] drm/i915/mtl: Add support for PM DEMAND

[Gustavo Sousa]

Quoting Vinod Govindapillai (2023-05-22 20:07:59-03:00)
>From: Mika Kahola <mika.kahola@intel.com>
>
>MTL introduces a new way to instruct the PUnit with
>power and bandwidth requirements of DE. Add the functionality
>to program the registers and handle waits using interrupts.
>The current wait time for timeouts is programmed for 10 msecs to
>factor in the worst case scenarios. Changes made to use REG_BIT
>for a register that we touched(GEN8_DE_MISC_IER _MMIO).
>
>Wa_14016740474 is added which applies to Xe_LPD+ display
>
>v2: checkpatch warning fixes, simplify program pmdemand part
>
>v3: update to dbufs and pipes values to pmdemand register(stan)
>    Removed the macro usage in update_pmdemand_values()
>
>v4: move the pmdemand_pre_plane_update before cdclk update
>    pmdemand_needs_update included cdclk params comparisons
>    pmdemand_state NULL check (Gustavo)
>    pmdemand.o in sorted order in the makefile (Jani)
>    update pmdemand misc irq handler loop (Gustavo)
>    active phys bitmask and programming correction (Gustavo)
>
>v5: simplify pmdemand_state structure
>    simplify methods to find active phys and max port clock
>    Timeout in case of previou pmdemand task pending (Gustavo)
>
>Bspec: 66451, 64636, 64602, 64603
>Cc: Matt Atwood <matthew.s.atwood@intel.com>
>Cc: Matt Roper <matthew.d.roper@intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Cc: Gustavo Sousa <gustavo.sousa@intel.com>
>Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
>Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>---
> drivers/gpu/drm/i915/Makefile                 |   1 +
> drivers/gpu/drm/i915/display/intel_display.c  |  14 +
> .../gpu/drm/i915/display/intel_display_core.h |   9 +
> .../drm/i915/display/intel_display_driver.c   |   7 +
> .../drm/i915/display/intel_display_power.c    |   8 +
> drivers/gpu/drm/i915/display/intel_pmdemand.c | 503 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_pmdemand.h |  24 +
> drivers/gpu/drm/i915/i915_irq.c               |  23 +-
> drivers/gpu/drm/i915/i915_reg.h               |  36 +-
> 9 files changed, 621 insertions(+), 4 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/display/intel_pmdemand.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_pmdemand.h
>
>diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>index 7587fe856e67..2698ecf74844 100644
>--- a/drivers/gpu/drm/i915/Makefile
>+++ b/drivers/gpu/drm/i915/Makefile
>@@ -270,6 +270,7 @@ i915-y += \
>         display/intel_pch_display.o \
>         display/intel_pch_refclk.o \
>         display/intel_plane_initial.o \
>+        display/intel_pmdemand.o \
>         display/intel_psr.o \
>         display/intel_quirks.o \
>         display/intel_sprite.o \
>diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>index 1d5d42a40803..d9daafe3362c 100644
>--- a/drivers/gpu/drm/i915/display/intel_display.c
>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>@@ -99,6 +99,7 @@
> #include "intel_pcode.h"
> #include "intel_pipe_crc.h"
> #include "intel_plane_initial.h"
>+#include "intel_pmdemand.h"
> #include "intel_pps.h"
> #include "intel_psr.h"
> #include "intel_sdvo.h"
>@@ -6315,6 +6316,10 @@ int intel_atomic_check(struct drm_device *dev,
>                         return ret;
>         }
> 
>+        ret = intel_pmdemand_atomic_check(state);
>+        if (ret)
>+                goto fail;
>+
>         ret = intel_atomic_check_crtcs(state);
>         if (ret)
>                 goto fail;
>@@ -6960,6 +6965,14 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>         for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
>                 crtc->config = new_crtc_state;
> 
>+        /*
>+         * In XE_LPD+ Pmdemand combines many parameters such as voltage index,
>+         * plls, cdclk frequency, QGV point selection parameter etc. Voltage
>+         * index, cdclk/ddiclk frequencies are supposed to be configured before
>+         * the cdclk config is set.
>+         */
>+        intel_pmdemand_pre_plane_update(state);
>+
>         if (state->modeset) {
>                 drm_atomic_helper_update_legacy_modeset_state(dev, &state->base);
> 
>@@ -7079,6 +7092,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>                 intel_verify_planes(state);
> 
>         intel_sagv_post_plane_update(state);
>+        intel_pmdemand_post_plane_update(state);
> 
>         drm_atomic_helper_commit_hw_done(&state->base);
> 
>diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>index 9f66d734edf6..ae45b2c42eb1 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_core.h
>+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>@@ -345,6 +345,15 @@ struct intel_display {
>                 struct intel_global_obj obj;
>         } dbuf;
> 
>+        struct {
>+                wait_queue_head_t waitqueue;
>+
>+                /* mutex to protect pmdemand programming sequence */
>+                struct mutex lock;
>+
>+                struct intel_global_obj obj;
>+        } pmdemand;
>+
>         struct {
>                 /*
>                  * dkl.phy_lock protects against concurrent access of the
>diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>index 60ce10fc7205..dc8de861339d 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>@@ -47,6 +47,7 @@
> #include "intel_opregion.h"
> #include "intel_overlay.h"
> #include "intel_plane_initial.h"
>+#include "intel_pmdemand.h"
> #include "intel_pps.h"
> #include "intel_quirks.h"
> #include "intel_vga.h"
>@@ -211,6 +212,8 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>         if (ret < 0)
>                 goto cleanup_vga;
> 
>+        intel_pmdemand_init_early(i915);
>+
>         intel_power_domains_init_hw(i915, false);
> 
>         if (!HAS_DISPLAY(i915))
>@@ -240,6 +243,10 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>         if (ret)
>                 goto cleanup_vga_client_pw_domain_dmc;
> 
>+        ret = intel_pmdemand_init(i915);
>+        if (ret)
>+                goto cleanup_vga_client_pw_domain_dmc;
>+
>         init_llist_head(&i915->display.atomic_helper.free_list);
>         INIT_WORK(&i915->display.atomic_helper.free_work,
>                   intel_atomic_helper_free_state_worker);
>diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>index 5150069f3f82..f5c5a486efbc 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_power.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>@@ -20,6 +20,7 @@
> #include "intel_mchbar_regs.h"
> #include "intel_pch_refclk.h"
> #include "intel_pcode.h"
>+#include "intel_pmdemand.h"
> #include "intel_pps_regs.h"
> #include "intel_snps_phy.h"
> #include "skl_watermark.h"
>@@ -1085,6 +1086,10 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
>         dev_priv->display.dbuf.enabled_slices =
>                 intel_enabled_dbuf_slices_mask(dev_priv);
> 
>+        if (DISPLAY_VER(dev_priv) >= 14)
>+                intel_program_dbuf_pmdemand(dev_priv, BIT(DBUF_S1) |
>+                                            dev_priv->display.dbuf.enabled_slices);
>+
>         /*
>          * Just power up at least 1 slice, we will
>          * figure out later which slices we have and what we need.
>@@ -1096,6 +1101,9 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
> static void gen9_dbuf_disable(struct drm_i915_private *dev_priv)
> {
>         gen9_dbuf_slices_update(dev_priv, 0);
>+
>+        if (DISPLAY_VER(dev_priv) >= 14)
>+                intel_program_dbuf_pmdemand(dev_priv, 0);
> }
> 
> static void gen12_dbuf_slices_config(struct drm_i915_private *dev_priv)
>diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.c b/drivers/gpu/drm/i915/display/intel_pmdemand.c
>new file mode 100644
>index 000000000000..843440b9430b
>--- /dev/null
>+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.c
>@@ -0,0 +1,503 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+#include <linux/bitops.h>
>+
>+#include "i915_drv.h"
>+#include "i915_reg.h"
>+#include "intel_bw.h"
>+#include "intel_cdclk.h"
>+#include "intel_cx0_phy.h"
>+#include "intel_de.h"
>+#include "intel_display.h"
>+#include "intel_display_trace.h"
>+#include "intel_pmdemand.h"
>+#include "skl_watermark.h"
>+
>+struct pmdemand_params {
>+        u16 qclk_gv_bw;
>+        u8 voltage_index;
>+        u8 qclk_gv_index;
>+        u8 active_pipes;
>+        u8 dbufs;
>+        u8 active_phys;
>+        u16 cdclk_freq_mhz;
>+        u16 ddiclk_max;
>+        u8 scalers;
>+};
>+struct intel_pmdemand_state {
>+        struct intel_global_state base;
>+
>+        /* Parameters to be configured in the pmdemand registers */
>+        struct pmdemand_params params;
>+};
>+
>+#define to_intel_pmdemand_state(x) container_of((x) + BUILD_BUG_ON_ZERO(offsetof(struct intel_pmdemand_state, base)), \
>+                                                struct intel_pmdemand_state, base)
>+
>+static struct intel_global_state *
>+intel_pmdemand_duplicate_state(struct intel_global_obj *obj)
>+{
>+        struct intel_pmdemand_state *pmdmnd_state;
>+
>+        pmdmnd_state = kmemdup(obj->state, sizeof(*pmdmnd_state), GFP_KERNEL);
>+        if (!pmdmnd_state)
>+                return NULL;
>+
>+        return &pmdmnd_state->base;
>+}
>+
>+static void intel_pmdemand_destroy_state(struct intel_global_obj *obj,
>+                                         struct intel_global_state *state)
>+{
>+        kfree(state);
>+}
>+
>+static const struct intel_global_state_funcs intel_pmdemand_funcs = {
>+        .atomic_duplicate_state = intel_pmdemand_duplicate_state,
>+        .atomic_destroy_state = intel_pmdemand_destroy_state,
>+};
>+
>+static struct intel_pmdemand_state *
>+intel_atomic_get_pmdemand_state(struct intel_atomic_state *state)
>+{
>+        struct drm_i915_private *i915 = to_i915(state->base.dev);
>+        struct intel_global_state *pmdemand_state =
>+                intel_atomic_get_global_obj_state(state,
>+                                                  &i915->display.pmdemand.obj);
>+
>+        if (IS_ERR(pmdemand_state))
>+                return ERR_CAST(pmdemand_state);
>+
>+        return to_intel_pmdemand_state(pmdemand_state);
>+}
>+
>+static struct intel_pmdemand_state *
>+intel_atomic_get_old_pmdemand_state(struct intel_atomic_state *state)
>+{
>+        struct drm_i915_private *i915 = to_i915(state->base.dev);
>+        struct intel_global_state *pmdemand_state =
>+                intel_atomic_get_old_global_obj_state(state,
>+                                                      &i915->display.pmdemand.obj);
>+
>+        if (!pmdemand_state)
>+                return NULL;
>+
>+        return to_intel_pmdemand_state(pmdemand_state);
>+}
>+
>+static struct intel_pmdemand_state *
>+intel_atomic_get_new_pmdemand_state(struct intel_atomic_state *state)
>+{
>+        struct drm_i915_private *i915 = to_i915(state->base.dev);
>+        struct intel_global_state *pmdemand_state =
>+                intel_atomic_get_new_global_obj_state(state,
>+                                                      &i915->display.pmdemand.obj);
>+
>+        if (!pmdemand_state)
>+                return NULL;
>+
>+        return to_intel_pmdemand_state(pmdemand_state);
>+}
>+
>+int intel_pmdemand_init(struct drm_i915_private *i915)
>+{
>+        struct intel_pmdemand_state *pmdemand_state;
>+
>+        pmdemand_state = kzalloc(sizeof(*pmdemand_state), GFP_KERNEL);
>+        if (!pmdemand_state)
>+                return -ENOMEM;
>+
>+        intel_atomic_global_obj_init(i915, &i915->display.pmdemand.obj,
>+                                     &pmdemand_state->base,
>+                                     &intel_pmdemand_funcs);
>+
>+        if (IS_MTL_DISPLAY_STEP(i915, STEP_A0, STEP_C0))
>+                /* Wa_14016740474 */
>+                intel_de_rmw(i915, XELPD_CHICKEN_DCPR_3, 0, DMD_RSP_TIMEOUT_DISABLE);
>+
>+        return 0;
>+}
>+
>+void intel_pmdemand_init_early(struct drm_i915_private *i915)
>+{
>+        mutex_init(&i915->display.pmdemand.lock);
>+        init_waitqueue_head(&i915->display.pmdemand.waitqueue);
>+}
>+
>+static u16 pmdemand_max_ddiclk(struct intel_atomic_state *state,
>+                               struct intel_pmdemand_state *pmdemand_state)
>+{
>+        int max = 0;
>+        struct intel_crtc *crtc;
>+        int i;
>+        const struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>+
>+        for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>+                                            new_crtc_state, i)

Wouldn't we need to check if this CRTC is active here and adjust the
actions according to the outcome?

>+                max = max(new_crtc_state->port_clock, max);
>+
>+        return DIV_ROUND_UP(max, 1000);

I still think this would not work on every scenario: my understanding is
that we are not iterating through all active crtcs here, but only those
in *this* atomic update. So, for example, if we are only updating CRTC A
here but already have active CRTCs B and C, and one of them have higher
port clock than A's, then we would wrongly select A's port clock as the
maximum one.

Now, we could think of the option of defining the initial value for the
max port clock to the one from the old intel_pmdemand_state and then do
the loop. In the example above, if B is the one with the maximum, then
things would work out for that specific example.

Unfortunately, that still would not be the correct solution. An example
of how this could give us the wrong value: let's say CRTC A is the one
with the maximum port clock among three currently active CRTCs A, B and
C. That means that in the next atomic update, we would initialize the
max variable with A's port clock. If such update is for *disabling* CRTC
A, we would be initializing that variable with an invalid value.

That is why I still think that we need to have a solution similar to
what is done in intel_bw_check_data_rate(): to use an array so that we
have an up-to-date list of active port clocks (we could have the
inactive ones as zeros).

>+}
>+
>+static u8
>+pmdemand_active_non_tc_phys(struct drm_i915_private *i915,
>+                            struct intel_atomic_state *state,
>+                            struct intel_pmdemand_state *pmdemand_state)
>+{
>+        struct drm_connector_list_iter conn_iter;
>+        struct intel_connector *connector;
>+        u8 phys = 0;
>+
>+        drm_connector_list_iter_begin(&i915->drm, &conn_iter);
>+        for_each_intel_connector_iter(connector, &conn_iter) {
>+                enum phy phy = intel_port_to_phy(i915, connector->encoder->port);
>+
>+                if (intel_phy_is_tc(i915, phy))
>+                        continue;
>+
>+                if (connector->get_hw_state(connector))
>+                        phys++;
>+        }
>+        drm_connector_list_iter_end(&conn_iter);
>+
>+        return phys;

Hm... I do not think we should really look at the hardware at this point of the
atomic transaction. The transaction will potentially change the hardware state
when committed. If we are disabling or enabling phys, then we would program PM
Demand an outdated value.

Regarding the previous implementation (from patch v3 or v5) I also do not think
it would cover all scenarios for similar reasons stated above for the the max
port clock calculation. I still think a proper implementation would be doing
something similar to what is done in intel_calc_active_pipes(): to use a mask so
that we have a view of all active phys.

Note that using a mask is similar to using an array like in the proposed
solution for the max port clock. Using a mask here is just because we are
tracking boolean states, which could be efficiently done with a mask,
considering that we have enough bits.

Well, all that said (about this and pmdemand_max_ddiclk), I quite new on atomic
modesetting and I might be making wrong assumptions here. Ville, could you
please provide some input on this matter?

>+}
>+
>+
>+static bool pmdemand_needs_update(struct intel_atomic_state *state)
>+{
>+        bool states_checked = false;
>+        struct intel_crtc *crtc;
>+        int i;
>+        const struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>+
>+        for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>+                                            new_crtc_state, i) {
>+                const struct intel_bw_state *new_bw_state, *old_bw_state;
>+                const struct intel_cdclk_state *new_cdclk_state;
>+                const struct intel_cdclk_state *old_cdclk_state;
>+                const struct intel_dbuf_state *new_dbuf_state, *old_dbuf_state;
>+
>+                if (old_crtc_state->port_clock != new_crtc_state->port_clock)
>+                        return true;
>+
>+                /*
>+                 * For the below settings once through the loop is enough.
>+                 * Some pmdemand_atomic_check calls might trigger read lock not
>+                 * taken assert if these following checks are kept outside this
>+                 * loop.
>+                 */
>+                if (states_checked)
>+                        continue;
>+
>+                new_bw_state = intel_atomic_get_new_bw_state(state);
>+                old_bw_state = intel_atomic_get_old_bw_state(state);
>+                if (new_bw_state && new_bw_state->qgv_point_peakbw !=
>+                    old_bw_state->qgv_point_peakbw)
>+                        return true;
>+
>+                new_dbuf_state = intel_atomic_get_new_dbuf_state(state);
>+                old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
>+                if (new_dbuf_state && new_dbuf_state->active_pipes !=
>+                    old_dbuf_state->active_pipes)
>+                        return true;
>+
>+                new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
>+                old_cdclk_state = intel_atomic_get_old_cdclk_state(state);
>+                if (new_cdclk_state &&
>+                    (new_cdclk_state->logical.cdclk !=
>+                     old_cdclk_state->logical.cdclk ||
>+                     new_cdclk_state->logical.voltage_level !=
>+                     old_cdclk_state->logical.voltage_level))
>+                        return true;
>+
>+                states_checked = true;
>+        }
>+
>+        return false;
>+}
>+
>+int intel_pmdemand_atomic_check(struct intel_atomic_state *state)
>+{
>+        struct drm_i915_private *i915 = to_i915(state->base.dev);
>+        const struct intel_bw_state *new_bw_state;
>+        const struct intel_cdclk_state *new_cdclk_state;
>+        const struct intel_dbuf_state *new_dbuf_state;
>+        struct intel_pmdemand_state *new_pmdemand_state;
>+        int ret;
>+
>+        if (DISPLAY_VER(i915) < 14)
>+                return 0;
>+
>+        if (!pmdemand_needs_update(state))
>+                return 0;
>+
>+        new_pmdemand_state = intel_atomic_get_pmdemand_state(state);
>+        if (IS_ERR(new_pmdemand_state))
>+                return PTR_ERR(new_pmdemand_state);
>+
>+        ret = intel_atomic_lock_global_state(&new_pmdemand_state->base);
>+        if (ret)
>+                return ret;
>+
>+        new_bw_state = intel_atomic_get_bw_state(state);
>+        if (IS_ERR(new_bw_state))
>+                return PTR_ERR(new_bw_state);
>+
>+        /* firmware will calculate the qclck_gc_index, requirement is set to 0 */
>+        new_pmdemand_state->params.qclk_gv_index = 0;
>+        new_pmdemand_state->params.qclk_gv_bw =
>+                min_t(u16, new_bw_state->qgv_point_peakbw, 0xffff);
>+
>+        new_dbuf_state = intel_atomic_get_dbuf_state(state);
>+        if (IS_ERR(new_dbuf_state))
>+                return PTR_ERR(new_dbuf_state);
>+
>+        new_pmdemand_state->params.active_pipes =
>+                min_t(u8, hweight8(new_dbuf_state->active_pipes), 3);
>+
>+        new_cdclk_state = intel_atomic_get_cdclk_state(state);
>+        if (IS_ERR(new_cdclk_state))
>+                return PTR_ERR(new_cdclk_state);
>+
>+        new_pmdemand_state->params.voltage_index =
>+                new_cdclk_state->logical.voltage_level;
>+        new_pmdemand_state->params.cdclk_freq_mhz =
>+                DIV_ROUND_UP(new_cdclk_state->logical.cdclk, 1000);
>+
>+        new_pmdemand_state->params.ddiclk_max =
>+                pmdemand_max_ddiclk(state, new_pmdemand_state);
>+
>+        new_pmdemand_state->params.active_phys =
>+                pmdemand_active_non_tc_phys(i915, state, new_pmdemand_state);
>+
>+        /*
>+         * Setting scalers to max as it can not be calculated during flips and
>+         * fastsets without taking global states locks.
>+         */
>+        new_pmdemand_state->params.scalers = 7;
>+
>+        ret = intel_atomic_serialize_global_state(&new_pmdemand_state->base);
>+        if (ret)
>+                return ret;
>+
>+        return 0;
>+}
>+
>+static bool intel_pmdemand_check_prev_transaction(struct drm_i915_private *i915)
>+{
>+        return !(intel_de_wait_for_clear(i915,
>+                                         XELPDP_INITIATE_PMDEMAND_REQUEST(1),
>+                                         XELPDP_PMDEMAND_REQ_ENABLE, 10) ||
>+                 intel_de_wait_for_clear(i915,
>+                                         GEN12_DCPR_STATUS_1,
>+                                         XELPDP_PMDEMAND_INFLIGHT_STATUS, 10));
>+}
>+
>+static bool intel_pmdemand_req_complete(struct drm_i915_private *i915)
>+{
>+        return !(intel_de_read(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1)) &
>+                 XELPDP_PMDEMAND_REQ_ENABLE);
>+}
>+
>+static int intel_pmdemand_wait(struct drm_i915_private *i915)
>+{
>+        DEFINE_WAIT(wait);
>+        int ret;
>+        const unsigned int timeout_ms = 10;
>+
>+        ret = wait_event_timeout(i915->display.pmdemand.waitqueue,
>+                                 intel_pmdemand_req_complete(i915),
>+                                 msecs_to_jiffies_timeout(timeout_ms));
>+        if (ret == 0)
>+                drm_err(&i915->drm,
>+                        "timed out waiting for Punit PM Demand Response\n");
>+
>+        return ret;
>+}
>+
>+/* Required to be programmed during Display Init Sequences. */
>+void intel_program_dbuf_pmdemand(struct drm_i915_private *i915,
>+                                 u8 dbuf_slices)
>+{
>+        u32 dbufs = min_t(u32, hweight8(dbuf_slices), 3);
>+
>+        mutex_lock(&i915->display.pmdemand.lock);
>+        if (drm_WARN_ON(&i915->drm,
>+                        !intel_pmdemand_check_prev_transaction(i915)))
>+                goto unlock;
>+
>+        intel_de_rmw(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(0),
>+                     XELPDP_PMDEMAND_DBUFS_MASK, XELPDP_PMDEMAND_DBUFS(dbufs));
>+        intel_de_rmw(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1), 0,
>+                     XELPDP_PMDEMAND_REQ_ENABLE);
>+
>+        intel_pmdemand_wait(i915);
>+
>+unlock:
>+        mutex_unlock(&i915->display.pmdemand.lock);
>+}
>+
>+static void update_pmdemand_values(const struct intel_pmdemand_state *new,
>+                                   const struct intel_pmdemand_state *old,
>+                                   u32 *reg1, u32 *reg2)
>+{
>+        u32 plls, tmp;
>+
>+        /*
>+         * The pmdemand parameter updates happens in two steps. Pre plane and
>+         * post plane updates. During the pre plane, as DE might still be
>+         * handling with some old operations, to avoid unwanted performance
>+         * issues, program the pmdemand parameters with higher of old and new
>+         * values. And then after once settled, use the new parameter values
>+         * as part of the post plane update.
>+         */
>+
>+        /* Set 1*/
>+        *reg1 &= ~XELPDP_PMDEMAND_QCLK_GV_BW_MASK;
>+        tmp = old ? max(old->params.qclk_gv_bw, new->params.qclk_gv_bw) :
>+                    new->params.qclk_gv_bw;
>+        *reg1 |= XELPDP_PMDEMAND_QCLK_GV_BW(tmp);
>+
>+        *reg1 &= ~XELPDP_PMDEMAND_VOLTAGE_INDEX_MASK;
>+        tmp = old ? max(old->params.voltage_index, new->params.voltage_index) :
>+                    new->params.voltage_index;
>+        *reg1 |= XELPDP_PMDEMAND_VOLTAGE_INDEX(tmp);
>+
>+        *reg1 &= ~XELPDP_PMDEMAND_QCLK_GV_INDEX_MASK;
>+        tmp = old ? max(old->params.qclk_gv_index, new->params.qclk_gv_index) :
>+                    new->params.qclk_gv_index;
>+        *reg1 |= XELPDP_PMDEMAND_QCLK_GV_INDEX(tmp);
>+
>+        *reg1 &= ~XELPDP_PMDEMAND_PIPES_MASK;
>+        tmp = old ? max(old->params.active_pipes, new->params.active_pipes) :
>+                    new->params.active_pipes;
>+        *reg1 |= XELPDP_PMDEMAND_PIPES(tmp);
>+
>+        *reg1 &= ~XELPDP_PMDEMAND_PHYS_MASK;
>+        plls = old ? max(old->params.active_phys, new->params.active_phys) :
>+                     new->params.active_phys;
>+        plls = min_t(u32, plls, 7);
>+        *reg1 |= XELPDP_PMDEMAND_PHYS(plls);
>+
>+        /* Set 2*/
>+        *reg2 &= ~XELPDP_PMDEMAND_CDCLK_FREQ_MASK;
>+        tmp = old ? max(old->params.cdclk_freq_mhz,
>+                        new->params.cdclk_freq_mhz) :
>+                    new->params.cdclk_freq_mhz;
>+        *reg2 |= XELPDP_PMDEMAND_CDCLK_FREQ(tmp);
>+
>+        *reg2 &= ~XELPDP_PMDEMAND_DDICLK_FREQ_MASK;
>+        tmp = old ? max(old->params.ddiclk_max, new->params.ddiclk_max) :
>+                    new->params.ddiclk_max;
>+        *reg2 |= XELPDP_PMDEMAND_DDICLK_FREQ(tmp);
>+
>+        *reg2 &= ~XELPDP_PMDEMAND_SCALERS_MASK;
>+        tmp = old ? max(old->params.scalers, new->params.scalers) :
>+                    new->params.scalers;
>+        *reg2 |= XELPDP_PMDEMAND_SCALERS(tmp);
>+
>+        /*
>+         * Active_PLLs starts with 1 because of CDCLK PLL.
>+         * TODO: Missing to account genlock filter when it gets used.
>+         */
>+        plls = min_t(u32, plls + 1, 7);
>+        *reg2 &= ~XELPDP_PMDEMAND_PLLS_MASK;
>+        *reg2 |= XELPDP_PMDEMAND_PLLS(plls);
>+}
>+
>+static void intel_program_pmdemand(struct drm_i915_private *i915,
>+                                   const struct intel_pmdemand_state *new,
>+                                   const struct intel_pmdemand_state *old)
>+{
>+        bool changed = false;
>+        u32 reg1, mod_reg1;
>+        u32 reg2, mod_reg2;
>+
>+        mutex_lock(&i915->display.pmdemand.lock);
>+        if (drm_WARN_ON(&i915->drm,
>+                        !intel_pmdemand_check_prev_transaction(i915)))
>+                goto unlock;
>+
>+        reg1 = intel_de_read(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(0));
>+        mod_reg1 = reg1;
>+
>+        reg2 = intel_de_read(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1));
>+        mod_reg2 = reg2;
>+
>+        update_pmdemand_values(new, old, &mod_reg1, &mod_reg2);
>+
>+        if (reg1 != mod_reg1) {
>+                intel_de_write(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(0),
>+                               mod_reg1);
>+                changed = true;
>+        }
>+
>+        if (reg2 != mod_reg2) {
>+                intel_de_write(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1),
>+                               mod_reg2);
>+                changed = true;
>+        }
>+
>+        /* Initiate pm demand request only if register values are changed */
>+        if (!changed)
>+                goto unlock;
>+
>+        drm_dbg_kms(&i915->drm,
>+                    "initate pmdemand request values: (0x%x 0x%x)\n",
>+                    mod_reg1, mod_reg2);
>+
>+        intel_de_rmw(i915, XELPDP_INITIATE_PMDEMAND_REQUEST(1), 0,
>+                        XELPDP_PMDEMAND_REQ_ENABLE);
>+
>+        intel_pmdemand_wait(i915);
>+
>+unlock:
>+        mutex_unlock(&i915->display.pmdemand.lock);
>+}
>+
>+static bool
>+intel_pmdemand_state_changed(const struct intel_pmdemand_state *new,
>+                             const struct intel_pmdemand_state *old)
>+{
>+        return memcmp(&new->params, &old->params, sizeof(new->params)) != 0;

This is much nicer to read now! Good call grouping the values in a
"params" member :-)

--
Gustavo Sousa

>+}
>+
>+void intel_pmdemand_pre_plane_update(struct intel_atomic_state *state)
>+{
>+        struct drm_i915_private *i915 = to_i915(state->base.dev);
>+        const struct intel_pmdemand_state *new_pmdmnd_state =
>+                intel_atomic_get_new_pmdemand_state(state);
>+        const struct intel_pmdemand_state *old_pmdmnd_state =
>+                intel_atomic_get_old_pmdemand_state(state);
>+
>+        if (DISPLAY_VER(i915) < 14)
>+                return;
>+
>+        if (!new_pmdmnd_state ||
>+            !intel_pmdemand_state_changed(new_pmdmnd_state, old_pmdmnd_state))
>+                return;
>+
>+        intel_program_pmdemand(i915, new_pmdmnd_state, old_pmdmnd_state);
>+}
>+
>+void intel_pmdemand_post_plane_update(struct intel_atomic_state *state)
>+{
>+        struct drm_i915_private *i915 = to_i915(state->base.dev);
>+        const struct intel_pmdemand_state *new_pmdmnd_state =
>+                intel_atomic_get_new_pmdemand_state(state);
>+        const struct intel_pmdemand_state *old_pmdmnd_state =
>+                intel_atomic_get_old_pmdemand_state(state);
>+
>+        if (DISPLAY_VER(i915) < 14)
>+                return;
>+
>+        if (!new_pmdmnd_state ||
>+            !intel_pmdemand_state_changed(new_pmdmnd_state, old_pmdmnd_state))
>+                return;
>+
>+        intel_program_pmdemand(i915, new_pmdmnd_state, NULL);
>+}
>diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.h b/drivers/gpu/drm/i915/display/intel_pmdemand.h
>new file mode 100644
>index 000000000000..2883b5d97a44
>--- /dev/null
>+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.h
>@@ -0,0 +1,24 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2023 Intel Corporation
>+ */
>+
>+#ifndef __INTEL_PMDEMAND_H__
>+#define __INTEL_PMDEMAND_H__
>+
>+#include <linux/types.h>
>+
>+struct drm_i915_private;
>+struct intel_atomic_state;
>+struct intel_crtc_state;
>+struct intel_plane_state;
>+
>+void intel_pmdemand_init_early(struct drm_i915_private *i915);
>+int intel_pmdemand_init(struct drm_i915_private *i915);
>+void intel_program_dbuf_pmdemand(struct drm_i915_private *i915,
>+                                 u8 dbuf_slices);
>+void intel_pmdemand_pre_plane_update(struct intel_atomic_state *state);
>+void intel_pmdemand_post_plane_update(struct intel_atomic_state *state);
>+int intel_pmdemand_atomic_check(struct intel_atomic_state *state);
>+
>+#endif /* __INTEL_PMDEMAND_H__ */
>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>index 02b6cbb832e9..49459ab70dd1 100644
>--- a/drivers/gpu/drm/i915/i915_irq.c
>+++ b/drivers/gpu/drm/i915/i915_irq.c
>@@ -43,6 +43,7 @@
> #include "display/intel_gmbus.h"
> #include "display/intel_hotplug.h"
> #include "display/intel_lpe_audio.h"
>+#include "display/intel_pmdemand.h"
> #include "display/intel_psr.h"
> #include "display/intel_psr_regs.h"
> 
>@@ -1981,12 +1982,27 @@ static u32 gen8_de_pipe_fault_mask(struct drm_i915_private *dev_priv)
>                 return GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
> }
> 
>+static void intel_pmdemand_irq_handler(struct drm_i915_private *dev_priv)
>+{
>+        wake_up_all(&dev_priv->display.pmdemand.waitqueue);
>+}
>+
> static void
> gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
> {
>         bool found = false;
> 
>-        if (iir & GEN8_DE_MISC_GSE) {
>+        if (DISPLAY_VER(dev_priv) >= 14) {
>+                if (iir & (XELPDP_PMDEMAND_RSP |
>+                           XELPDP_PMDEMAND_RSPTOUT_ERR)) {
>+                        if (iir & XELPDP_PMDEMAND_RSPTOUT_ERR)
>+                                drm_dbg(&dev_priv->drm,
>+                                        "Error waiting for Punit PM Demand Response\n");
>+
>+                        intel_pmdemand_irq_handler(dev_priv);
>+                        found = true;
>+                }
>+        } else if (iir & GEN8_DE_MISC_GSE) {
>                 intel_opregion_asle_intr(dev_priv);
>                 found = true;
>         }
>@@ -3737,7 +3753,10 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>         if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
>                 de_port_masked |= BXT_DE_PORT_GMBUS;
> 
>-        if (DISPLAY_VER(dev_priv) >= 11) {
>+        if (DISPLAY_VER(dev_priv) >= 14)
>+                de_misc_masked |= XELPDP_PMDEMAND_RSPTOUT_ERR |
>+                                  XELPDP_PMDEMAND_RSP;
>+        else if (DISPLAY_VER(dev_priv) >= 11) {
>                 enum port port;
> 
>                 if (intel_bios_is_dsi_present(dev_priv, &port))
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 2a9ab8de8421..91fb12b65c92 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -4450,8 +4450,10 @@
> #define GEN8_DE_MISC_IMR _MMIO(0x44464)
> #define GEN8_DE_MISC_IIR _MMIO(0x44468)
> #define GEN8_DE_MISC_IER _MMIO(0x4446c)
>-#define  GEN8_DE_MISC_GSE                (1 << 27)
>-#define  GEN8_DE_EDP_PSR                (1 << 19)
>+#define  XELPDP_PMDEMAND_RSPTOUT_ERR        REG_BIT(27)
>+#define  GEN8_DE_MISC_GSE                REG_BIT(27)
>+#define  GEN8_DE_EDP_PSR                REG_BIT(19)
>+#define  XELPDP_PMDEMAND_RSP                REG_BIT(3)
> 
> #define GEN8_PCU_ISR _MMIO(0x444e0)
> #define GEN8_PCU_IMR _MMIO(0x444e4)
>@@ -4536,6 +4538,33 @@
> #define  XELPDP_DP_ALT_HPD_LONG_DETECT                REG_BIT(1)
> #define  XELPDP_DP_ALT_HPD_SHORT_DETECT                REG_BIT(0)
> 
>+#define XELPDP_INITIATE_PMDEMAND_REQUEST(dword)                _MMIO(0x45230 + 4 * (dword))
>+#define  XELPDP_PMDEMAND_QCLK_GV_BW_MASK                REG_GENMASK(31, 16)
>+#define  XELPDP_PMDEMAND_QCLK_GV_BW(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_QCLK_GV_BW_MASK, x)
>+#define  XELPDP_PMDEMAND_VOLTAGE_INDEX_MASK                REG_GENMASK(14, 12)
>+#define  XELPDP_PMDEMAND_VOLTAGE_INDEX(x)                REG_FIELD_PREP(XELPDP_PMDEMAND_VOLTAGE_INDEX_MASK, x)
>+#define  XELPDP_PMDEMAND_QCLK_GV_INDEX_MASK                REG_GENMASK(11, 8)
>+#define  XELPDP_PMDEMAND_QCLK_GV_INDEX(x)                REG_FIELD_PREP(XELPDP_PMDEMAND_QCLK_GV_INDEX_MASK, x)
>+#define  XELPDP_PMDEMAND_PIPES_MASK                        REG_GENMASK(7, 6)
>+#define  XELPDP_PMDEMAND_PIPES(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_PIPES_MASK, x)
>+#define  XELPDP_PMDEMAND_DBUFS_MASK                        REG_GENMASK(5, 4)
>+#define  XELPDP_PMDEMAND_DBUFS(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_DBUFS_MASK, x)
>+#define  XELPDP_PMDEMAND_PHYS_MASK                        REG_GENMASK(2, 0)
>+#define  XELPDP_PMDEMAND_PHYS(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_PHYS_MASK, x)
>+
>+#define  XELPDP_PMDEMAND_REQ_ENABLE                        REG_BIT(31)
>+#define  XELPDP_PMDEMAND_CDCLK_FREQ_MASK                REG_GENMASK(30, 20)
>+#define  XELPDP_PMDEMAND_CDCLK_FREQ(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_CDCLK_FREQ_MASK, x)
>+#define  XELPDP_PMDEMAND_DDICLK_FREQ_MASK                REG_GENMASK(18, 8)
>+#define  XELPDP_PMDEMAND_DDICLK_FREQ(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_DDICLK_FREQ_MASK, x)
>+#define  XELPDP_PMDEMAND_SCALERS_MASK                        REG_GENMASK(6, 4)
>+#define  XELPDP_PMDEMAND_SCALERS(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_SCALERS_MASK, x)
>+#define  XELPDP_PMDEMAND_PLLS_MASK                        REG_GENMASK(2, 0)
>+#define  XELPDP_PMDEMAND_PLLS(x)                        REG_FIELD_PREP(XELPDP_PMDEMAND_PLLS_MASK, x)
>+
>+#define GEN12_DCPR_STATUS_1                                _MMIO(0x46440)
>+#define  XELPDP_PMDEMAND_INFLIGHT_STATUS                REG_BIT(26)
>+
> #define ILK_DISPLAY_CHICKEN2        _MMIO(0x42004)
> /* Required on all Ironlake and Sandybridge according to the B-Spec. */
> #define   ILK_ELPIN_409_SELECT        REG_BIT(25)
>@@ -4695,6 +4724,9 @@
> #define   DCPR_SEND_RESP_IMM                        REG_BIT(25)
> #define   DCPR_CLEAR_MEMSTAT_DIS                REG_BIT(24)
> 
>+#define XELPD_CHICKEN_DCPR_3                        _MMIO(0x46438)
>+#define   DMD_RSP_TIMEOUT_DISABLE                REG_BIT(19)
>+
> #define SKL_DFSM                        _MMIO(0x51000)
> #define   SKL_DFSM_DISPLAY_PM_DISABLE        (1 << 27)
> #define   SKL_DFSM_DISPLAY_HDCP_DISABLE        (1 << 25)
>-- 
>2.34.1
>

Re: [Intel-gfx] [PATCH v6 2/7] drm/i915: update the QGV point frequency calculations

[Govindapillai, Vinod]

On Tue, 2023-05-23 at 16:14 +0300, Jani Nikula wrote:
> On Tue, 23 May 2023, "Govindapillai, Vinod" <vinod.govindapillai@intel.com> wrote:
> > On Tue, 2023-05-23 at 12:01 +0300, Jani Nikula wrote:
> > > On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> > > > From MTL onwwards, pcode locks the QGV point based on peak BW of
> > > > the intended QGV point passed by the driver. So the peak BW
> > > > calculation must match the value expected by the pcode. Update
> > > > the calculations as per the Bspec.
> > > > 
> > > > v2: use DIV_ROUND_* macro for the calculations (Ville)
> > > > 
> > > > Bspec: 64636
> > > > 
> > > > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > > > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > index ab405c48ca3a..c8075a37c3ab 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > @@ -182,7 +182,7 @@ static int mtl_read_qgv_point_info(struct drm_i915_private *dev_priv,
> > > >         val2 = intel_uncore_read(&dev_priv->uncore,
> > > >                                  MTL_MEM_SS_INFO_QGV_POINT_HIGH(point));
> > > >         dclk = REG_FIELD_GET(MTL_DCLK_MASK, val);
> > > > -       sp->dclk = DIV_ROUND_UP((16667 * dclk), 1000);
> > > > +       sp->dclk = DIV_ROUND_CLOSEST(16667 * dclk + 500, 1000);
> > > 
> > > What's with the +500 there?
> > 
> > This is what pcode expects. Somehow pcode use this formula and we have to exactly match this.
> > Got
> > this confirmed from Art.
> 
> I'm guessing all it really means is to round to closest, and they
> specified it like that instead of saying "round to closest".
> 
> Essentially (x + (div/2)) / div is what DIV_ROUND_CLOSEST() does.
> 
> It's odd to do it *twice*, and surely not what they expect.
> 
> BR,
> Jani.

Ah.. ok! Thanks! will update this!

BR
Vinod

> 
> > 
> > BR
> > Vinod
> > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > >         sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val);
> > > >         sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);
> > > 
> > 
>