[PATCH v4 0/9] drm/msm/dpu: rework debugfs interface of dpu_core_perf

[PATCH v4 0/9] drm/msm/dpu: rework debugfs interface of dpu_core_perf

[Dmitry Baryshkov]

Bring back a set of patches extracted from [1] per Abhinav's suggestion.

Rework debugging overrides for the bandwidth and clock settings. Instead
of specifying the 'mode' and some values, allow one to set the affected
value directly.

[1] https://patchwork.freedesktop.org/series/119552/#rev2

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v4:
- Dropped core_perf: from patch subject (Abhinav)
- Fixed indentation of _dpu_core_perf_crtc_update_bus() (Abhinav)
- Expanded commit messages to reflect different questions (Abhinav)
- Reworked existing files to specify max allowed average bandwidth
  (Abhinav)
- Fixed u32 vs u64 and KBps vs Bps values in debugfs interface
- Link to v3: https://lore.kernel.org/r/20240314-dpu-perf-rework-v3-0-79fa4e065574@linaro.org

---
Dmitry Baryshkov (9):
      drm/msm/dpu: extract bandwidth aggregation function
      drm/msm/dpu: remove duplicate code calculating sum of bandwidths
      drm/msm/dpu: change ib values to u32
      drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote
      drm/msm/dpu: also use KBps for bw_ctl output
      drm/msm/dpu: rename average bandwidth-related debugfs files
      drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()
      drm/msm/dpu: rework core_perf debugfs overrides
      drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 207 ++++++++------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  16 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |   5 +-
 3 files changed, 62 insertions(+), 166 deletions(-)
---
base-commit: 23ec7472e8aaa96838a61819a97882b5a7e17e42
change-id: 20240314-dpu-perf-rework-97fca999eb46

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v4 1/9] drm/msm/dpu: extract bandwidth aggregation function

[Dmitry Baryshkov]

In preparation to refactoring the dpu_core_perf debugfs interface,
extract the bandwidth aggregation function from
_dpu_core_perf_crtc_update_bus().

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 45 +++++++++++++++------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 6f0a37f954fe8797a4e3a34e7876a93d5e477642..c7ac1140e79dbf48566a89fa4d70f6bec69d1d81 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -210,36 +210,41 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 	return 0;
 }
 
-static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
-		struct drm_crtc *crtc)
+static void dpu_core_perf_aggregate(struct drm_device *ddev,
+				    enum dpu_crtc_client_type curr_client_type,
+				    struct dpu_core_perf_params *perf)
 {
-	struct dpu_core_perf_params perf = { 0 };
-	enum dpu_crtc_client_type curr_client_type
-					= dpu_crtc_get_client_type(crtc);
-	struct drm_crtc *tmp_crtc;
 	struct dpu_crtc_state *dpu_cstate;
-	int i, ret = 0;
-	u64 avg_bw;
-
-	if (!kms->num_paths)
-		return 0;
+	struct drm_crtc *tmp_crtc;
 
-	drm_for_each_crtc(tmp_crtc, crtc->dev) {
+	drm_for_each_crtc(tmp_crtc, ddev) {
 		if (tmp_crtc->enabled &&
-			curr_client_type ==
-				dpu_crtc_get_client_type(tmp_crtc)) {
+		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
 			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
 
-			perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
-					dpu_cstate->new_perf.max_per_pipe_ib);
+			perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
+						    dpu_cstate->new_perf.max_per_pipe_ib);
 
-			perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
+			perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
 
-			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
-				  tmp_crtc->base.id,
-				  dpu_cstate->new_perf.bw_ctl, kms->num_paths);
+			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
+					 tmp_crtc->base.id,
+					 dpu_cstate->new_perf.bw_ctl);
 		}
 	}
+}
+
+static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
+					  struct drm_crtc *crtc)
+{
+	struct dpu_core_perf_params perf = { 0 };
+	int i, ret = 0;
+	u64 avg_bw;
+
+	if (!kms->num_paths)
+		return 0;
+
+	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
 
 	avg_bw = perf.bw_ctl;
 	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/

-- 
2.39.5

[PATCH v4 2/9] drm/msm/dpu: remove duplicate code calculating sum of bandwidths

[Dmitry Baryshkov]

The code in dpu_core_perf_crtc_check() mostly duplicates code in
dpu_core_perf_aggregate(). Remove the duplication by reusing the latter
function.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 94 +++++++++++----------------
 1 file changed, 38 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index c7ac1140e79dbf48566a89fa4d70f6bec69d1d81..f0d490afb53be2f4bc706af91da05bb893a5fe34 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -140,6 +140,30 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
 			perf->max_per_pipe_ib, perf->bw_ctl);
 }
 
+static void dpu_core_perf_aggregate(struct drm_device *ddev,
+				    enum dpu_crtc_client_type curr_client_type,
+				    struct dpu_core_perf_params *perf)
+{
+	struct dpu_crtc_state *dpu_cstate;
+	struct drm_crtc *tmp_crtc;
+
+	drm_for_each_crtc(tmp_crtc, ddev) {
+		if (tmp_crtc->enabled &&
+		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
+			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
+
+			perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
+						    dpu_cstate->new_perf.max_per_pipe_ib);
+
+			perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
+
+			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
+					 tmp_crtc->base.id,
+					 dpu_cstate->new_perf.bw_ctl);
+		}
+	}
+}
+
 /**
  * dpu_core_perf_crtc_check - validate performance of the given crtc state
  * @crtc: Pointer to crtc
@@ -150,11 +174,9 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 		struct drm_crtc_state *state)
 {
 	u32 bw, threshold;
-	u64 bw_sum_of_intfs = 0;
-	enum dpu_crtc_client_type curr_client_type;
 	struct dpu_crtc_state *dpu_cstate;
-	struct drm_crtc *tmp_crtc;
 	struct dpu_kms *kms;
+	struct dpu_core_perf_params perf;
 
 	if (!crtc || !state) {
 		DPU_ERROR("invalid crtc\n");
@@ -172,68 +194,28 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 	/* obtain new values */
 	_dpu_core_perf_calc_crtc(&kms->perf, crtc, state, &dpu_cstate->new_perf);
 
-	bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl;
-	curr_client_type = dpu_crtc_get_client_type(crtc);
-
-	drm_for_each_crtc(tmp_crtc, crtc->dev) {
-		if (tmp_crtc->enabled &&
-		    dpu_crtc_get_client_type(tmp_crtc) == curr_client_type &&
-		    tmp_crtc != crtc) {
-			struct dpu_crtc_state *tmp_cstate =
-				to_dpu_crtc_state(tmp_crtc->state);
-
-			DRM_DEBUG_ATOMIC("crtc:%d bw:%llu ctrl:%d\n",
-					 tmp_crtc->base.id, tmp_cstate->new_perf.bw_ctl,
-					 tmp_cstate->bw_control);
-
-			bw_sum_of_intfs += tmp_cstate->new_perf.bw_ctl;
-		}
+	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
 
-		/* convert bandwidth to kb */
-		bw = DIV_ROUND_UP_ULL(bw_sum_of_intfs, 1000);
-		DRM_DEBUG_ATOMIC("calculated bandwidth=%uk\n", bw);
+	/* convert bandwidth to kb */
+	bw = DIV_ROUND_UP_ULL(perf.bw_ctl, 1000);
+	DRM_DEBUG_ATOMIC("calculated bandwidth=%uk\n", bw);
 
-		threshold = kms->perf.perf_cfg->max_bw_high;
+	threshold = kms->perf.perf_cfg->max_bw_high;
 
-		DRM_DEBUG_ATOMIC("final threshold bw limit = %d\n", threshold);
+	DRM_DEBUG_ATOMIC("final threshold bw limit = %d\n", threshold);
 
-		if (!threshold) {
-			DPU_ERROR("no bandwidth limits specified\n");
-			return -E2BIG;
-		} else if (bw > threshold) {
-			DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
-					threshold);
-			return -E2BIG;
-		}
+	if (!threshold) {
+		DPU_ERROR("no bandwidth limits specified\n");
+		return -E2BIG;
+	} else if (bw > threshold) {
+		DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
+				threshold);
+		return -E2BIG;
 	}
 
 	return 0;
 }
 
-static void dpu_core_perf_aggregate(struct drm_device *ddev,
-				    enum dpu_crtc_client_type curr_client_type,
-				    struct dpu_core_perf_params *perf)
-{
-	struct dpu_crtc_state *dpu_cstate;
-	struct drm_crtc *tmp_crtc;
-
-	drm_for_each_crtc(tmp_crtc, ddev) {
-		if (tmp_crtc->enabled &&
-		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
-			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
-
-			perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
-						    dpu_cstate->new_perf.max_per_pipe_ib);
-
-			perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
-
-			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
-					 tmp_crtc->base.id,
-					 dpu_cstate->new_perf.bw_ctl);
-		}
-	}
-}
-
 static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 					  struct drm_crtc *crtc)
 {

-- 
2.39.5

Re: [PATCH v4 2/9] drm/msm/dpu: remove duplicate code calculating sum of bandwidths

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> The code in dpu_core_perf_crtc_check() mostly duplicates code in
> dpu_core_perf_aggregate(). Remove the duplication by reusing the latter
> function.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 94 +++++++++++----------------
>   1 file changed, 38 insertions(+), 56 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v4 3/9] drm/msm/dpu: change ib values to u32

[Dmitry Baryshkov]

The IB values in core_perf calculations (max_per_pipe_ib,
fix_core_ib_vote) are expressed in KBps and are passed to icc_set_bw
without additional division. Change type of those values to u32.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index f0d490afb53be2f4bc706af91da05bb893a5fe34..7263ab63a692554cd51a7fd91bd6250330179240 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -135,7 +135,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
 	}
 
 	DRM_DEBUG_ATOMIC(
-		"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
+		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%llu\n",
 			crtc->base.id, perf->core_clk_rate,
 			perf->max_per_pipe_ib, perf->bw_ctl);
 }
@@ -477,7 +477,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 			(u32 *)perf, &dpu_core_perf_mode_fops);
 	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
 			&perf->fix_core_clk_rate);
-	debugfs_create_u64("fix_core_ib_vote", 0600, entry,
+	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
 			&perf->fix_core_ib_vote);
 	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
 			&perf->fix_core_ab_vote);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 451bf8021114d9d4a2dfdbb81ed4150fc559c681..ca4595b4ec217697849af02446b23ed0857a0295 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -19,7 +19,7 @@
  * @core_clk_rate: core clock rate request
  */
 struct dpu_core_perf_params {
-	u64 max_per_pipe_ib;
+	u32 max_per_pipe_ib;
 	u64 bw_ctl;
 	u64 core_clk_rate;
 };
@@ -50,7 +50,7 @@ struct dpu_core_perf {
 	struct dpu_core_perf_tune perf_tune;
 	u32 enable_bw_release;
 	u64 fix_core_clk_rate;
-	u64 fix_core_ib_vote;
+	u32 fix_core_ib_vote;
 	u64 fix_core_ab_vote;
 };
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 7191b1a6d41b3a96f956d199398f12b2923e8c82..8a523eb308630943871c2e075d3d0d9094606d05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1487,7 +1487,7 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
 	seq_printf(s, "core_clk_rate: %llu\n",
 			dpu_crtc->cur_perf.core_clk_rate);
 	seq_printf(s, "bw_ctl: %llu\n", dpu_crtc->cur_perf.bw_ctl);
-	seq_printf(s, "max_per_pipe_ib: %llu\n",
+	seq_printf(s, "max_per_pipe_ib: %u\n",
 				dpu_crtc->cur_perf.max_per_pipe_ib);
 
 	return 0;

-- 
2.39.5

Re: [PATCH v4 3/9] drm/msm/dpu: change ib values to u32

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> The IB values in core_perf calculations (max_per_pipe_ib,
> fix_core_ib_vote) are expressed in KBps and are passed to icc_set_bw
> without additional division. Change type of those values to u32.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 ++--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 2 +-
>   3 files changed, 5 insertions(+), 5 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Dmitry Baryshkov]

The fix_core_ab_vote is an average bandwidth value, used for bandwidth
overrides in several cases. However there is an internal inconsistency:
fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
in Bps.

Fix that by changing the type of the variable to u32 and using * 1000ULL
multiplier when setting up the dpu_core_perf_params::bw_ctl value.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
 		perf->max_per_pipe_ib = 0;
 		perf->core_clk_rate = 0;
 	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
-		perf->bw_ctl = core_perf->fix_core_ab_vote;
+		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
 		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
 		perf->core_clk_rate = core_perf->fix_core_clk_rate;
 	} else {
@@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 			&perf->fix_core_clk_rate);
 	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
 			&perf->fix_core_ib_vote);
-	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
+	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
 			&perf->fix_core_ab_vote);
 
 	return 0;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -51,7 +51,7 @@ struct dpu_core_perf {
 	u32 enable_bw_release;
 	u64 fix_core_clk_rate;
 	u32 fix_core_ib_vote;
-	u64 fix_core_ab_vote;
+	u32 fix_core_ab_vote;
 };
 
 int dpu_core_perf_crtc_check(struct drm_crtc *crtc,

-- 
2.39.5

Re: [PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> The fix_core_ab_vote is an average bandwidth value, used for bandwidth
> overrides in several cases. However there is an internal inconsistency:
> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
> in Bps.
> 
> Fix that by changing the type of the variable to u32 and using * 1000ULL
> multiplier when setting up the dpu_core_perf_params::bw_ctl value.
> 

Actually after looking at this, I have another question.

How did you conclude that fix_core_ib_vote is in KBps?

min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?

It depends on the interpretation perhaps. If the debugfs was supposed to 
operate under the expectation that the provided value will be 
pre-multiplied by 1000 and given then that explains why it was not 
multiplied again.

And I cross-checked some of the internal usages of the debugfs, the 
values provided to it were in Bps and not KBps.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>   		perf->max_per_pipe_ib = 0;
>   		perf->core_clk_rate = 0;
>   	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> -		perf->bw_ctl = core_perf->fix_core_ab_vote;
> +		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>   		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>   		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>   	} else {
> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>   			&perf->fix_core_clk_rate);
>   	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>   			&perf->fix_core_ib_vote);
> -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
> +	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
>   			&perf->fix_core_ab_vote);
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -51,7 +51,7 @@ struct dpu_core_perf {
>   	u32 enable_bw_release;
>   	u64 fix_core_clk_rate;
>   	u32 fix_core_ib_vote;
> -	u64 fix_core_ab_vote;
> +	u32 fix_core_ab_vote;
>   };
>   
>   int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> 

Re: [PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Dmitry Baryshkov]

On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > The fix_core_ab_vote is an average bandwidth value, used for bandwidth
> > overrides in several cases. However there is an internal inconsistency:
> > fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
> > in Bps.
> > 
> > Fix that by changing the type of the variable to u32 and using * 1000ULL
> > multiplier when setting up the dpu_core_perf_params::bw_ctl value.
> > 
> 
> Actually after looking at this, I have another question.
> 
> How did you conclude that fix_core_ib_vote is in KBps?
> 
> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?
> 
> It depends on the interpretation perhaps. If the debugfs was supposed to
> operate under the expectation that the provided value will be pre-multiplied
> by 1000 and given then that explains why it was not multiplied again.
> 
> And I cross-checked some of the internal usages of the debugfs, the values
> provided to it were in Bps and not KBps.

Well... As you wrote min_dram_ib is in KBps. So, by comparing the next
two lines, fix_core_ib_vote should also be in kBps, as there is no
premultiplier:

                perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
[...]
                perf->max_per_pipe_ib = perf_cfg->min_dram_ib;

And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw()
without any modifications:

                icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);


> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> >   		perf->max_per_pipe_ib = 0;
> >   		perf->core_clk_rate = 0;
> >   	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > -		perf->bw_ctl = core_perf->fix_core_ab_vote;
> > +		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> >   		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> >   		perf->core_clk_rate = core_perf->fix_core_clk_rate;
> >   	} else {
> > @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> >   			&perf->fix_core_clk_rate);
> >   	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
> >   			&perf->fix_core_ib_vote);
> > -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
> > +	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
> >   			&perf->fix_core_ab_vote);
> >   	return 0;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > @@ -51,7 +51,7 @@ struct dpu_core_perf {
> >   	u32 enable_bw_release;
> >   	u64 fix_core_clk_rate;
> >   	u32 fix_core_ib_vote;
> > -	u64 fix_core_ab_vote;
> > +	u32 fix_core_ab_vote;
> >   };
> >   int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Abhinav Kumar]

On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth
>>> overrides in several cases. However there is an internal inconsistency:
>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
>>> in Bps.
>>>
>>> Fix that by changing the type of the variable to u32 and using * 1000ULL
>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value.
>>>
>>
>> Actually after looking at this, I have another question.
>>
>> How did you conclude that fix_core_ib_vote is in KBps?
>>
>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?
>>
>> It depends on the interpretation perhaps. If the debugfs was supposed to
>> operate under the expectation that the provided value will be pre-multiplied
>> by 1000 and given then that explains why it was not multiplied again.
>>
>> And I cross-checked some of the internal usages of the debugfs, the values
>> provided to it were in Bps and not KBps.
> 
> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next
> two lines, fix_core_ib_vote should also be in kBps, as there is no
> premultiplier:
> 
>                  perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> [...]
>                  perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> 
> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw()
> without any modifications:
> 
>                  icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> 

Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote 
is always in Bps and not in KBps because bw_ctl is in Bps.

Is it really a discrepancy that fix_core_ib_vote is defined in KBps, 
while fix_core_ab_vote is defined in Bps because they are just following 
the units in which bw_ctl and max_per_pipe_ib were defined in resp.

> 
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>    		perf->max_per_pipe_ib = 0;
>>>    		perf->core_clk_rate = 0;
>>>    	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote;
>>> +		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>>    		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>>    		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>>    	} else {
>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>    			&perf->fix_core_clk_rate);
>>>    	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>>    			&perf->fix_core_ib_vote);
>>> -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
>>> +	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
>>>    			&perf->fix_core_ab_vote);
>>>    	return 0;
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> @@ -51,7 +51,7 @@ struct dpu_core_perf {
>>>    	u32 enable_bw_release;
>>>    	u64 fix_core_clk_rate;
>>>    	u32 fix_core_ib_vote;
>>> -	u64 fix_core_ab_vote;
>>> +	u32 fix_core_ab_vote;
>>>    };
>>>    int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>
> 

Re: [PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Dmitry Baryshkov]

On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote:
>> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote:
>>> 
>>> 
>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth
>>>> overrides in several cases. However there is an internal inconsistency:
>>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
>>>> in Bps.
>>>> 
>>>> Fix that by changing the type of the variable to u32 and using * 1000ULL
>>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value.
>>>> 
>>> 
>>> Actually after looking at this, I have another question.
>>> 
>>> How did you conclude that fix_core_ib_vote is in KBps?
>>> 
>>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?
>>> 
>>> It depends on the interpretation perhaps. If the debugfs was supposed to
>>> operate under the expectation that the provided value will be pre-multiplied
>>> by 1000 and given then that explains why it was not multiplied again.
>>> 
>>> And I cross-checked some of the internal usages of the debugfs, the values
>>> provided to it were in Bps and not KBps.
>> 
>> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next
>> two lines, fix_core_ib_vote should also be in kBps, as there is no
>> premultiplier:
>> 
>>                  perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>> [...]
>>                  perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>> 
>> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw()
>> without any modifications:
>> 
>>                  icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>> 
>
>Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps.
>
>Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp.

Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps.

>
>> 
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
>>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>> index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>>    		perf->max_per_pipe_ib = 0;
>>>>    		perf->core_clk_rate = 0;
>>>>    	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote;
>>>> +		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>>>    		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>>>    		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>>>    	} else {
>>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>>    			&perf->fix_core_clk_rate);
>>>>    	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>>>    			&perf->fix_core_ib_vote);
>>>> -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
>>>> +	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
>>>>    			&perf->fix_core_ab_vote);
>>>>    	return 0;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>> index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>> @@ -51,7 +51,7 @@ struct dpu_core_perf {
>>>>    	u32 enable_bw_release;
>>>>    	u64 fix_core_clk_rate;
>>>>    	u32 fix_core_ib_vote;
>>>> -	u64 fix_core_ab_vote;
>>>> +	u32 fix_core_ab_vote;
>>>>    };
>>>>    int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>> 
>> 

Re: [PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Abhinav Kumar]

On 1/11/2025 5:08 AM, Dmitry Baryshkov wrote:
> On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>> On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth
>>>>> overrides in several cases. However there is an internal inconsistency:
>>>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
>>>>> in Bps.
>>>>>
>>>>> Fix that by changing the type of the variable to u32 and using * 1000ULL
>>>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value.
>>>>>
>>>>
>>>> Actually after looking at this, I have another question.
>>>>
>>>> How did you conclude that fix_core_ib_vote is in KBps?
>>>>
>>>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?
>>>>
>>>> It depends on the interpretation perhaps. If the debugfs was supposed to
>>>> operate under the expectation that the provided value will be pre-multiplied
>>>> by 1000 and given then that explains why it was not multiplied again.
>>>>
>>>> And I cross-checked some of the internal usages of the debugfs, the values
>>>> provided to it were in Bps and not KBps.
>>>
>>> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next
>>> two lines, fix_core_ib_vote should also be in kBps, as there is no
>>> premultiplier:
>>>
>>>                   perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>> [...]
>>>                   perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>
>>> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw()
>>> without any modifications:
>>>
>>>                   icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>>>
>>
>> Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps.
>>
>> Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp.
> 
> Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps.
> 

In that case, the documentation for both needs to be updated as well as 
it still says both are in bps not kbps.

  * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
  * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
  */
struct dpu_core_perf {
         const struct dpu_perf_cfg *perf_cfg;
         u64 core_clk_rate;
         u64 max_core_clk_rate;
         struct dpu_core_perf_tune perf_tune;
         u32 enable_bw_release;
         u64 fix_core_clk_rate;
         u64 fix_core_ib_vote;
         u64 fix_core_ab_vote;
};

>>
>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
>>>>>     2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>>>     		perf->max_per_pipe_ib = 0;
>>>>>     		perf->core_clk_rate = 0;
>>>>>     	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote;
>>>>> +		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>>>>     		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>>>>     		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>>>>     	} else {
>>>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>>>     			&perf->fix_core_clk_rate);
>>>>>     	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>>>>     			&perf->fix_core_ib_vote);
>>>>> -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
>>>>> +	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
>>>>>     			&perf->fix_core_ab_vote);
>>>>>     	return 0;
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> @@ -51,7 +51,7 @@ struct dpu_core_perf {
>>>>>     	u32 enable_bw_release;
>>>>>     	u64 fix_core_clk_rate;
>>>>>     	u32 fix_core_ib_vote;
>>>>> -	u64 fix_core_ab_vote;
>>>>> +	u32 fix_core_ab_vote;
>>>>>     };
>>>>>     int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>>>
>>>
> 

Re: [PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Dmitry Baryshkov]

On Tue, 14 Jan 2025 at 03:31, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/11/2025 5:08 AM, Dmitry Baryshkov wrote:
> > On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >> On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote:
> >>> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> >>>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth
> >>>>> overrides in several cases. However there is an internal inconsistency:
> >>>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
> >>>>> in Bps.
> >>>>>
> >>>>> Fix that by changing the type of the variable to u32 and using * 1000ULL
> >>>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value.
> >>>>>
> >>>>
> >>>> Actually after looking at this, I have another question.
> >>>>
> >>>> How did you conclude that fix_core_ib_vote is in KBps?
> >>>>
> >>>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?
> >>>>
> >>>> It depends on the interpretation perhaps. If the debugfs was supposed to
> >>>> operate under the expectation that the provided value will be pre-multiplied
> >>>> by 1000 and given then that explains why it was not multiplied again.
> >>>>
> >>>> And I cross-checked some of the internal usages of the debugfs, the values
> >>>> provided to it were in Bps and not KBps.
> >>>
> >>> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next
> >>> two lines, fix_core_ib_vote should also be in kBps, as there is no
> >>> premultiplier:
> >>>
> >>>                   perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> >>> [...]
> >>>                   perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> >>>
> >>> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw()
> >>> without any modifications:
> >>>
> >>>                   icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> >>>
> >>
> >> Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps.
> >>
> >> Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp.
> >
> > Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps.
> >
>
> In that case, the documentation for both needs to be updated as well as
> it still says both are in bps not kbps.

Ack, I missed it.

>
>   * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
>   * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
>   */
> struct dpu_core_perf {
>          const struct dpu_perf_cfg *perf_cfg;
>          u64 core_clk_rate;
>          u64 max_core_clk_rate;
>          struct dpu_core_perf_tune perf_tune;
>          u32 enable_bw_release;
>          u64 fix_core_clk_rate;
>          u64 fix_core_ib_vote;
>          u64 fix_core_ab_vote;
> };
>
> >>
> >>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
> >>>>>     2 files changed, 3 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>>>> index 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> >>>>>                   perf->max_per_pipe_ib = 0;
> >>>>>                   perf->core_clk_rate = 0;
> >>>>>           } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> >>>>> -         perf->bw_ctl = core_perf->fix_core_ab_vote;
> >>>>> +         perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> >>>>>                   perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> >>>>>                   perf->core_clk_rate = core_perf->fix_core_clk_rate;
> >>>>>           } else {
> >>>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> >>>>>                           &perf->fix_core_clk_rate);
> >>>>>           debugfs_create_u32("fix_core_ib_vote", 0600, entry,
> >>>>>                           &perf->fix_core_ib_vote);
> >>>>> - debugfs_create_u64("fix_core_ab_vote", 0600, entry,
> >>>>> + debugfs_create_u32("fix_core_ab_vote", 0600, entry,
> >>>>>                           &perf->fix_core_ab_vote);
> >>>>>           return 0;
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>>>> index ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>>>> @@ -51,7 +51,7 @@ struct dpu_core_perf {
> >>>>>           u32 enable_bw_release;
> >>>>>           u64 fix_core_clk_rate;
> >>>>>           u32 fix_core_ib_vote;
> >>>>> - u64 fix_core_ab_vote;
> >>>>> + u32 fix_core_ab_vote;
> >>>>>     };
> >>>>>     int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> >>>>>
> >>>
> >



-- 
With best wishes
Dmitry

[PATCH v4 5/9] drm/msm/dpu: also use KBps for bw_ctl output

[Dmitry Baryshkov]

Change debugfs and log entries to use KBps / u32 for bw_ctl and similar
data.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 5 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 7cabc8f26908cfd2dbbffebd7c70fc37d9159733..b93f7556f187d46b325a689ad01ec177cecbc37a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -135,9 +135,10 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
 	}
 
 	DRM_DEBUG_ATOMIC(
-		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%llu\n",
+		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
 			crtc->base.id, perf->core_clk_rate,
-			perf->max_per_pipe_ib, perf->bw_ctl);
+			perf->max_per_pipe_ib,
+			(u32)DIV_ROUND_UP_ULL(perf->bw_ctl, 1000));
 }
 
 static void dpu_core_perf_aggregate(struct drm_device *ddev,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 8a523eb308630943871c2e075d3d0d9094606d05..ac3c6c5ad1cec3856f0eff2ed71153d3c2dc279e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1486,7 +1486,8 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
 	seq_printf(s, "intf_mode: %d\n", dpu_crtc_get_intf_mode(crtc));
 	seq_printf(s, "core_clk_rate: %llu\n",
 			dpu_crtc->cur_perf.core_clk_rate);
-	seq_printf(s, "bw_ctl: %llu\n", dpu_crtc->cur_perf.bw_ctl);
+	seq_printf(s, "bw_ctl: %uk\n",
+		   (u32)DIV_ROUND_UP_ULL(dpu_crtc->cur_perf.bw_ctl, 1000));
 	seq_printf(s, "max_per_pipe_ib: %u\n",
 				dpu_crtc->cur_perf.max_per_pipe_ib);
 

-- 
2.39.5

Re: [PATCH v4 5/9] drm/msm/dpu: also use KBps for bw_ctl output

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> Change debugfs and log entries to use KBps / u32 for bw_ctl and similar
> data.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 5 +++--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 3 ++-
>   2 files changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v4 6/9] drm/msm/dpu: rename average bandwidth-related debugfs files

[Dmitry Baryshkov]

Rename the debugfs files to match their purpose and the patter provided
by other bandwidth and clock-related files:

threshold_high -> max_core_ab
threshold_low -> low_core_ab

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index b93f7556f187d46b325a689ad01ec177cecbc37a..70f43e8359caee2082f2ca9944a17a6a20aa3d49 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -464,9 +464,9 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 			&perf->core_clk_rate);
 	debugfs_create_u32("enable_bw_release", 0600, entry,
 			(u32 *)&perf->enable_bw_release);
-	debugfs_create_u32("threshold_low", 0400, entry,
+	debugfs_create_u32("low_core_ab", 0400, entry,
 			(u32 *)&perf->perf_cfg->max_bw_low);
-	debugfs_create_u32("threshold_high", 0400, entry,
+	debugfs_create_u32("max_core_ab", 0400, entry,
 			(u32 *)&perf->perf_cfg->max_bw_high);
 	debugfs_create_u32("min_core_ib", 0400, entry,
 			(u32 *)&perf->perf_cfg->min_core_ib);

-- 
2.39.5

Re: [PATCH v4 6/9] drm/msm/dpu: rename average bandwidth-related debugfs files

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> Rename the debugfs files to match their purpose and the patter provided
> by other bandwidth and clock-related files:
> 
> threshold_high -> max_core_ab
> threshold_low -> low_core_ab
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Dmitry Baryshkov]

Move perf mode handling for the bandwidth to
_dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
and then aggregating known values.

Note, this changes the fix_core_ab_vote. Previously it would be
multiplied per the CRTC number, now it will be used directly for
interconnect voting. This better reflects user requirements in the case
of different resolutions being set on different CRTCs: instead of using
the same bandwidth for each CRTC (which is incorrect) user can now
calculate overall bandwidth required by all outputs and use that value.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
 		return;
 	}
 
-	memset(perf, 0, sizeof(struct dpu_core_perf_params));
-
-	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
-		perf->bw_ctl = 0;
-		perf->max_per_pipe_ib = 0;
-		perf->core_clk_rate = 0;
-	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
-		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
-		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
-		perf->core_clk_rate = core_perf->fix_core_clk_rate;
-	} else {
-		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
-		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
-		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
-	}
-
+	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
+	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
 	DRM_DEBUG_ATOMIC(
 		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
 			crtc->base.id, perf->core_clk_rate,
@@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 {
 	struct dpu_core_perf_params perf = { 0 };
 	int i, ret = 0;
-	u64 avg_bw;
+	u32 avg_bw;
+	u32 peak_bw;
 
 	if (!kms->num_paths)
 		return 0;
 
-	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
+	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
+		avg_bw = 0;
+		peak_bw = 0;
+	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
+		avg_bw = kms->perf.fix_core_ab_vote;
+		peak_bw = kms->perf.fix_core_ib_vote;
+	} else {
+		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
+
+		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
+		peak_bw = perf.max_per_pipe_ib;
+	}
 
-	avg_bw = perf.bw_ctl;
-	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
+	avg_bw /= kms->num_paths;
 
 	for (i = 0; i < kms->num_paths; i++)
-		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
+		icc_set_bw(kms->path[i], avg_bw, peak_bw);
 
 	return ret;
 }

-- 
2.39.5

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> Move perf mode handling for the bandwidth to
> _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
> and then aggregating known values.
> 
> Note, this changes the fix_core_ab_vote. Previously it would be
> multiplied per the CRTC number, now it will be used directly for
> interconnect voting. This better reflects user requirements in the case
> of different resolutions being set on different CRTCs: instead of using
> the same bandwidth for each CRTC (which is incorrect) user can now
> calculate overall bandwidth required by all outputs and use that value.
> 

There are two things this change is doing:

1) Dropping the core_clk_rate setting because its already handled inside 
_dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update() 
will still work.

and

2) Then this part of moving the ab/ib setting to 
_dpu_core_perf_crtc_update_bus().

Can we split this into two changes so that its clear that dropping 
core_clk_rate setting in this change will not cause an issue.


> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
>   1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>   		return;
>   	}
>   
> -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
> -
> -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> -		perf->bw_ctl = 0;
> -		perf->max_per_pipe_ib = 0;
> -		perf->core_clk_rate = 0;
> -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
> -	} else {
> -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> -	}
> -
> +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>   	DRM_DEBUG_ATOMIC(
>   		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
>   			crtc->base.id, perf->core_clk_rate,
> @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>   {
>   	struct dpu_core_perf_params perf = { 0 };
>   	int i, ret = 0;
> -	u64 avg_bw;
> +	u32 avg_bw;
> +	u32 peak_bw;
>   
>   	if (!kms->num_paths)
>   		return 0;
>   
> -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> +		avg_bw = 0;
> +		peak_bw = 0;
> +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> +		avg_bw = kms->perf.fix_core_ab_vote;
> +		peak_bw = kms->perf.fix_core_ib_vote;
> +	} else {
> +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> +
> +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> +		peak_bw = perf.max_per_pipe_ib;
> +	}
>   
> -	avg_bw = perf.bw_ctl;
> -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
> +	avg_bw /= kms->num_paths;
>   
>   	for (i = 0; i < kms->num_paths; i++)
> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
>   
>   	return ret;
>   }
> 

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Dmitry Baryshkov]

On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > Move perf mode handling for the bandwidth to
> > _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
> > and then aggregating known values.
> > 
> > Note, this changes the fix_core_ab_vote. Previously it would be
> > multiplied per the CRTC number, now it will be used directly for
> > interconnect voting. This better reflects user requirements in the case
> > of different resolutions being set on different CRTCs: instead of using
> > the same bandwidth for each CRTC (which is incorrect) user can now
> > calculate overall bandwidth required by all outputs and use that value.
> > 
> 
> There are two things this change is doing:
> 
> 1) Dropping the core_clk_rate setting because its already handled inside
> _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
> will still work.
> 
> and
> 
> 2) Then this part of moving the ab/ib setting to
> _dpu_core_perf_crtc_update_bus().
> 
> Can we split this into two changes so that its clear that dropping
> core_clk_rate setting in this change will not cause an issue.

Ack

> 
> 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
> >   1 file changed, 19 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> >   		return;
> >   	}
> > -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
> > -
> > -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > -		perf->bw_ctl = 0;
> > -		perf->max_per_pipe_ib = 0;
> > -		perf->core_clk_rate = 0;
> > -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> > -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> > -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
> > -	} else {
> > -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > -	}
> > -
> > +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> >   	DRM_DEBUG_ATOMIC(
> >   		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
> >   			crtc->base.id, perf->core_clk_rate,
> > @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> >   {
> >   	struct dpu_core_perf_params perf = { 0 };
> >   	int i, ret = 0;
> > -	u64 avg_bw;
> > +	u32 avg_bw;
> > +	u32 peak_bw;
> >   	if (!kms->num_paths)
> >   		return 0;
> > -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > +		avg_bw = 0;
> > +		peak_bw = 0;
> > +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > +		avg_bw = kms->perf.fix_core_ab_vote;
> > +		peak_bw = kms->perf.fix_core_ib_vote;
> > +	} else {
> > +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > +
> > +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > +		peak_bw = perf.max_per_pipe_ib;
> > +	}
> > -	avg_bw = perf.bw_ctl;
> > -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
> > +	avg_bw /= kms->num_paths;
> >   	for (i = 0; i < kms->num_paths; i++)
> > -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> > +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
> >   	return ret;
> >   }
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Abhinav Kumar]

On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
> On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>> Move perf mode handling for the bandwidth to
>>> _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
>>> and then aggregating known values.
>>>
>>> Note, this changes the fix_core_ab_vote. Previously it would be
>>> multiplied per the CRTC number, now it will be used directly for
>>> interconnect voting. This better reflects user requirements in the case
>>> of different resolutions being set on different CRTCs: instead of using
>>> the same bandwidth for each CRTC (which is incorrect) user can now
>>> calculate overall bandwidth required by all outputs and use that value.
>>>
>>
>> There are two things this change is doing:
>>
>> 1) Dropping the core_clk_rate setting because its already handled inside
>> _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
>> will still work.
>>
>> and
>>
>> 2) Then this part of moving the ab/ib setting to
>> _dpu_core_perf_crtc_update_bus().
>>
>> Can we split this into two changes so that its clear that dropping
>> core_clk_rate setting in this change will not cause an issue.
> 
> Ack
> 

Actually I think this is incorrect.

If the user puts in an incorrect value beyond the bounds, earlier the 
code will reject that by failing the in _dpu_core_perf_calc_crtc().

Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond 
the check phase so incorrect values cannot be rejected.

So we will still need to preserve overriding the values in 
_dpu_core_perf_calc_crtc().

>>
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
>>>    1 file changed, 19 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>    		return;
>>>    	}
>>> -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>>> -
>>> -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>> -		perf->bw_ctl = 0;
>>> -		perf->max_per_pipe_ib = 0;
>>> -		perf->core_clk_rate = 0;
>>> -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>> -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>> -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>> -	} else {
>>> -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>> -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>> -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>> -	}
>>> -
>>> +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>> +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>    	DRM_DEBUG_ATOMIC(
>>>    		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
>>>    			crtc->base.id, perf->core_clk_rate,
>>> @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>    {
>>>    	struct dpu_core_perf_params perf = { 0 };
>>>    	int i, ret = 0;
>>> -	u64 avg_bw;
>>> +	u32 avg_bw;
>>> +	u32 peak_bw;
>>>    	if (!kms->num_paths)
>>>    		return 0;
>>> -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>> +		avg_bw = 0;
>>> +		peak_bw = 0;
>>> +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>> +		avg_bw = kms->perf.fix_core_ab_vote;
>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>> +	} else {
>>> +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>> +
>>> +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>> +		peak_bw = perf.max_per_pipe_ib;
>>> +	}
>>> -	avg_bw = perf.bw_ctl;
>>> -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>>> +	avg_bw /= kms->num_paths;
>>>    	for (i = 0; i < kms->num_paths; i++)
>>> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>>> +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
>>>    	return ret;
>>>    }
>>>
> 

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Dmitry Baryshkov]

On Tue, Jan 14, 2025 at 01:18:26PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
> > On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > > > Move perf mode handling for the bandwidth to
> > > > _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
> > > > and then aggregating known values.
> > > > 
> > > > Note, this changes the fix_core_ab_vote. Previously it would be
> > > > multiplied per the CRTC number, now it will be used directly for
> > > > interconnect voting. This better reflects user requirements in the case
> > > > of different resolutions being set on different CRTCs: instead of using
> > > > the same bandwidth for each CRTC (which is incorrect) user can now
> > > > calculate overall bandwidth required by all outputs and use that value.
> > > > 
> > > 
> > > There are two things this change is doing:
> > > 
> > > 1) Dropping the core_clk_rate setting because its already handled inside
> > > _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
> > > will still work.
> > > 
> > > and
> > > 
> > > 2) Then this part of moving the ab/ib setting to
> > > _dpu_core_perf_crtc_update_bus().
> > > 
> > > Can we split this into two changes so that its clear that dropping
> > > core_clk_rate setting in this change will not cause an issue.
> > 
> > Ack
> > 
> 
> Actually I think this is incorrect.
> 
> If the user puts in an incorrect value beyond the bounds, earlier the code
> will reject that by failing the in _dpu_core_perf_calc_crtc().

This function doesn't perform any validation nor returns an error code.
Probably you've meant some other function.

> 
> Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond the
> check phase so incorrect values cannot be rejected.
> 
> So we will still need to preserve overriding the values in
> _dpu_core_perf_calc_crtc().
> 
> > > 
> > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
> > > >    1 file changed, 19 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> > > >    		return;
> > > >    	}
> > > > -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
> > > > -
> > > > -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > -		perf->bw_ctl = 0;
> > > > -		perf->max_per_pipe_ib = 0;
> > > > -		perf->core_clk_rate = 0;
> > > > -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> > > > -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> > > > -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
> > > > -	} else {
> > > > -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > > > -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > > > -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > > > -	}
> > > > -
> > > > +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > > > +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > > > +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > > >    	DRM_DEBUG_ATOMIC(
> > > >    		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
> > > >    			crtc->base.id, perf->core_clk_rate,
> > > > @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> > > >    {
> > > >    	struct dpu_core_perf_params perf = { 0 };
> > > >    	int i, ret = 0;
> > > > -	u64 avg_bw;
> > > > +	u32 avg_bw;
> > > > +	u32 peak_bw;
> > > >    	if (!kms->num_paths)
> > > >    		return 0;
> > > > -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > +		avg_bw = 0;
> > > > +		peak_bw = 0;
> > > > +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > +		avg_bw = kms->perf.fix_core_ab_vote;
> > > > +		peak_bw = kms->perf.fix_core_ib_vote;
> > > > +	} else {
> > > > +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > +
> > > > +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > +		peak_bw = perf.max_per_pipe_ib;
> > > > +	}
> > > > -	avg_bw = perf.bw_ctl;
> > > > -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
> > > > +	avg_bw /= kms->num_paths;
> > > >    	for (i = 0; i < kms->num_paths; i++)
> > > > -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> > > > +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
> > > >    	return ret;
> > > >    }
> > > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Abhinav Kumar]

On 1/15/2025 12:27 AM, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 01:18:26PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
>>> On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>> Move perf mode handling for the bandwidth to
>>>>> _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
>>>>> and then aggregating known values.
>>>>>
>>>>> Note, this changes the fix_core_ab_vote. Previously it would be
>>>>> multiplied per the CRTC number, now it will be used directly for
>>>>> interconnect voting. This better reflects user requirements in the case
>>>>> of different resolutions being set on different CRTCs: instead of using
>>>>> the same bandwidth for each CRTC (which is incorrect) user can now
>>>>> calculate overall bandwidth required by all outputs and use that value.
>>>>>
>>>>
>>>> There are two things this change is doing:
>>>>
>>>> 1) Dropping the core_clk_rate setting because its already handled inside
>>>> _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
>>>> will still work.
>>>>
>>>> and
>>>>
>>>> 2) Then this part of moving the ab/ib setting to
>>>> _dpu_core_perf_crtc_update_bus().
>>>>
>>>> Can we split this into two changes so that its clear that dropping
>>>> core_clk_rate setting in this change will not cause an issue.
>>>
>>> Ack
>>>
>>
>> Actually I think this is incorrect.
>>
>> If the user puts in an incorrect value beyond the bounds, earlier the code
>> will reject that by failing the in _dpu_core_perf_calc_crtc().
> 
> This function doesn't perform any validation nor returns an error code.
> Probably you've meant some other function.
> 

Sorry, let me explain a little more to complete the flow I am seeing.

_dpu_core_perf_calc_crtc() gets called by dpu_core_perf_crtc_check().

That one checks against erroneous values.

                 if (!threshold) {
                         DPU_ERROR("no bandwidth limits specified\n");
                         return -E2BIG;
                 } else if (bw > threshold) {
                         DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
                                         threshold);
                         return -E2BIG;
                 }

>>
>> Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond the
>> check phase so incorrect values cannot be rejected.
>>
>> So we will still need to preserve overriding the values in
>> _dpu_core_perf_calc_crtc().
>>
>>>>
>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
>>>>>     1 file changed, 19 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>>>     		return;
>>>>>     	}
>>>>> -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>>>>> -
>>>>> -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>> -		perf->bw_ctl = 0;
>>>>> -		perf->max_per_pipe_ib = 0;
>>>>> -		perf->core_clk_rate = 0;
>>>>> -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>>>> -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>>>> -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>>>> -	} else {
>>>>> -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>>>> -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>>> -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>>> -	}
>>>>> -
>>>>> +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>>>> +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>>> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>>>     	DRM_DEBUG_ATOMIC(
>>>>>     		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
>>>>>     			crtc->base.id, perf->core_clk_rate,
>>>>> @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>>>     {
>>>>>     	struct dpu_core_perf_params perf = { 0 };
>>>>>     	int i, ret = 0;
>>>>> -	u64 avg_bw;
>>>>> +	u32 avg_bw;
>>>>> +	u32 peak_bw;
>>>>>     	if (!kms->num_paths)
>>>>>     		return 0;
>>>>> -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>> +		avg_bw = 0;
>>>>> +		peak_bw = 0;
>>>>> +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>> +		avg_bw = kms->perf.fix_core_ab_vote;
>>>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>>>> +	} else {
>>>>> +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>> +
>>>>> +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>> +		peak_bw = perf.max_per_pipe_ib;
>>>>> +	}
>>>>> -	avg_bw = perf.bw_ctl;
>>>>> -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>>>>> +	avg_bw /= kms->num_paths;
>>>>>     	for (i = 0; i < kms->num_paths; i++)
>>>>> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>>>>> +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
>>>>>     	return ret;
>>>>>     }
>>>>>
>>>
> 

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 11:41:27AM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/15/2025 12:27 AM, Dmitry Baryshkov wrote:
> > On Tue, Jan 14, 2025 at 01:18:26PM -0800, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
> > > > On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
> > > > > 
> > > > > 
> > > > > On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > > > > > Move perf mode handling for the bandwidth to
> > > > > > _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
> > > > > > and then aggregating known values.
> > > > > > 
> > > > > > Note, this changes the fix_core_ab_vote. Previously it would be
> > > > > > multiplied per the CRTC number, now it will be used directly for
> > > > > > interconnect voting. This better reflects user requirements in the case
> > > > > > of different resolutions being set on different CRTCs: instead of using
> > > > > > the same bandwidth for each CRTC (which is incorrect) user can now
> > > > > > calculate overall bandwidth required by all outputs and use that value.
> > > > > > 
> > > > > 
> > > > > There are two things this change is doing:
> > > > > 
> > > > > 1) Dropping the core_clk_rate setting because its already handled inside
> > > > > _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
> > > > > will still work.
> > > > > 
> > > > > and
> > > > > 
> > > > > 2) Then this part of moving the ab/ib setting to
> > > > > _dpu_core_perf_crtc_update_bus().
> > > > > 
> > > > > Can we split this into two changes so that its clear that dropping
> > > > > core_clk_rate setting in this change will not cause an issue.
> > > > 
> > > > Ack
> > > > 
> > > 
> > > Actually I think this is incorrect.
> > > 
> > > If the user puts in an incorrect value beyond the bounds, earlier the code
> > > will reject that by failing the in _dpu_core_perf_calc_crtc().
> > 
> > This function doesn't perform any validation nor returns an error code.
> > Probably you've meant some other function.
> > 
> 
> Sorry, let me explain a little more to complete the flow I am seeing.
> 
> _dpu_core_perf_calc_crtc() gets called by dpu_core_perf_crtc_check().
> 
> That one checks against erroneous values.
> 
>                 if (!threshold) {
>                         DPU_ERROR("no bandwidth limits specified\n");
>                         return -E2BIG;
>                 } else if (bw > threshold) {
>                         DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
>                                         threshold);
>                         return -E2BIG;
>                 }

Here we are checking that the selected set of modes doesn't overload
defined platform requirements. However I think that it should be
possible for the user to attempt to overcome predefined bandwidth
limitations in attempt to debug the issue. ICC framework handles that
perfectly (and if you check, until the sync_state is reached all BW's
are assumed to be UINT_MAX). Maybe I should document it in the commit
message that after this commit forced BWs are not a subject to the
catalog limitations.

> 
> > > 
> > > Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond the
> > > check phase so incorrect values cannot be rejected.
> > > 
> > > So we will still need to preserve overriding the values in
> > > _dpu_core_perf_calc_crtc().
> > > 
> > > > > 
> > > > > 
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
> > > > > >     1 file changed, 19 insertions(+), 21 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> > > > > >     		return;
> > > > > >     	}
> > > > > > -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
> > > > > > -
> > > > > > -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > > > -		perf->bw_ctl = 0;
> > > > > > -		perf->max_per_pipe_ib = 0;
> > > > > > -		perf->core_clk_rate = 0;
> > > > > > -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > > > -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> > > > > > -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> > > > > > -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
> > > > > > -	} else {
> > > > > > -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > > > > > -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > > > > > -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > > > > > -	}
> > > > > > -
> > > > > > +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > > > > > +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > > > > > +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > > > > >     	DRM_DEBUG_ATOMIC(
> > > > > >     		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
> > > > > >     			crtc->base.id, perf->core_clk_rate,
> > > > > > @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> > > > > >     {
> > > > > >     	struct dpu_core_perf_params perf = { 0 };
> > > > > >     	int i, ret = 0;
> > > > > > -	u64 avg_bw;
> > > > > > +	u32 avg_bw;
> > > > > > +	u32 peak_bw;
> > > > > >     	if (!kms->num_paths)
> > > > > >     		return 0;
> > > > > > -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > > > +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > > > +		avg_bw = 0;
> > > > > > +		peak_bw = 0;
> > > > > > +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > > > +		avg_bw = kms->perf.fix_core_ab_vote;
> > > > > > +		peak_bw = kms->perf.fix_core_ib_vote;
> > > > > > +	} else {
> > > > > > +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > > > +
> > > > > > +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > > > +		peak_bw = perf.max_per_pipe_ib;
> > > > > > +	}
> > > > > > -	avg_bw = perf.bw_ctl;
> > > > > > -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
> > > > > > +	avg_bw /= kms->num_paths;
> > > > > >     	for (i = 0; i < kms->num_paths; i++)
> > > > > > -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> > > > > > +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
> > > > > >     	return ret;
> > > > > >     }
> > > > > > 
> > > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Abhinav Kumar]

On 1/15/2025 4:32 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 11:41:27AM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/15/2025 12:27 AM, Dmitry Baryshkov wrote:
>>> On Tue, Jan 14, 2025 at 01:18:26PM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
>>>>> On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>>>> Move perf mode handling for the bandwidth to
>>>>>>> _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
>>>>>>> and then aggregating known values.
>>>>>>>
>>>>>>> Note, this changes the fix_core_ab_vote. Previously it would be
>>>>>>> multiplied per the CRTC number, now it will be used directly for
>>>>>>> interconnect voting. This better reflects user requirements in the case
>>>>>>> of different resolutions being set on different CRTCs: instead of using
>>>>>>> the same bandwidth for each CRTC (which is incorrect) user can now
>>>>>>> calculate overall bandwidth required by all outputs and use that value.
>>>>>>>
>>>>>>
>>>>>> There are two things this change is doing:
>>>>>>
>>>>>> 1) Dropping the core_clk_rate setting because its already handled inside
>>>>>> _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
>>>>>> will still work.
>>>>>>
>>>>>> and
>>>>>>
>>>>>> 2) Then this part of moving the ab/ib setting to
>>>>>> _dpu_core_perf_crtc_update_bus().
>>>>>>
>>>>>> Can we split this into two changes so that its clear that dropping
>>>>>> core_clk_rate setting in this change will not cause an issue.
>>>>>
>>>>> Ack
>>>>>
>>>>
>>>> Actually I think this is incorrect.
>>>>
>>>> If the user puts in an incorrect value beyond the bounds, earlier the code
>>>> will reject that by failing the in _dpu_core_perf_calc_crtc().
>>>
>>> This function doesn't perform any validation nor returns an error code.
>>> Probably you've meant some other function.
>>>
>>
>> Sorry, let me explain a little more to complete the flow I am seeing.
>>
>> _dpu_core_perf_calc_crtc() gets called by dpu_core_perf_crtc_check().
>>
>> That one checks against erroneous values.
>>
>>                  if (!threshold) {
>>                          DPU_ERROR("no bandwidth limits specified\n");
>>                          return -E2BIG;
>>                  } else if (bw > threshold) {
>>                          DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
>>                                          threshold);
>>                          return -E2BIG;
>>                  }
> 
> Here we are checking that the selected set of modes doesn't overload
> defined platform requirements. However I think that it should be
> possible for the user to attempt to overcome predefined bandwidth
> limitations in attempt to debug the issue. ICC framework handles that
> perfectly (and if you check, until the sync_state is reached all BW's
> are assumed to be UINT_MAX). Maybe I should document it in the commit
> message that after this commit forced BWs are not a subject to the
> catalog limitations.
> 

hmmm, yes this was the validation I was referring to.

I didnt get why a user should be allowed to go beyond the platform 
limits, what purpose does that serve , its not leading to any conclusion 
or towards the resolution of the issue. With the platform validation not 
only we are enforcing the limits but also making sure that random values 
given by the user dont cause more harm than good.

>>
>>>>
>>>> Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond the
>>>> check phase so incorrect values cannot be rejected.
>>>>
>>>> So we will still need to preserve overriding the values in
>>>> _dpu_core_perf_calc_crtc().
>>>>
>>>>>>
>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
>>>>>>>      1 file changed, 19 insertions(+), 21 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>>>>>      		return;
>>>>>>>      	}
>>>>>>> -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>>>>>>> -
>>>>>>> -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>>>> -		perf->bw_ctl = 0;
>>>>>>> -		perf->max_per_pipe_ib = 0;
>>>>>>> -		perf->core_clk_rate = 0;
>>>>>>> -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>>>>>> -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>>>>>> -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>>>>>> -	} else {
>>>>>>> -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>>>>>> -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>>>>> -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>>>>> -	}
>>>>>>> -
>>>>>>> +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>>>>>> +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>>>>> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>>>>>      	DRM_DEBUG_ATOMIC(
>>>>>>>      		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
>>>>>>>      			crtc->base.id, perf->core_clk_rate,
>>>>>>> @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>>>>>      {
>>>>>>>      	struct dpu_core_perf_params perf = { 0 };
>>>>>>>      	int i, ret = 0;
>>>>>>> -	u64 avg_bw;
>>>>>>> +	u32 avg_bw;
>>>>>>> +	u32 peak_bw;
>>>>>>>      	if (!kms->num_paths)
>>>>>>>      		return 0;
>>>>>>> -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>>>> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>>>> +		avg_bw = 0;
>>>>>>> +		peak_bw = 0;
>>>>>>> +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>>>> +		avg_bw = kms->perf.fix_core_ab_vote;
>>>>>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>>>>>> +	} else {
>>>>>>> +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>>>> +
>>>>>>> +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>>>> +		peak_bw = perf.max_per_pipe_ib;
>>>>>>> +	}
>>>>>>> -	avg_bw = perf.bw_ctl;
>>>>>>> -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>>>>>>> +	avg_bw /= kms->num_paths;
>>>>>>>      	for (i = 0; i < kms->num_paths; i++)
>>>>>>> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>>>>>>> +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
>>>>>>>      	return ret;
>>>>>>>      }
>>>>>>>
>>>>>
>>>
> 

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 04:40:39PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/15/2025 4:32 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 15, 2025 at 11:41:27AM -0800, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 1/15/2025 12:27 AM, Dmitry Baryshkov wrote:
> > > > On Tue, Jan 14, 2025 at 01:18:26PM -0800, Abhinav Kumar wrote:
> > > > > 
> > > > > 
> > > > > On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
> > > > > > On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > > > > > > > Move perf mode handling for the bandwidth to
> > > > > > > > _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
> > > > > > > > and then aggregating known values.
> > > > > > > > 
> > > > > > > > Note, this changes the fix_core_ab_vote. Previously it would be
> > > > > > > > multiplied per the CRTC number, now it will be used directly for
> > > > > > > > interconnect voting. This better reflects user requirements in the case
> > > > > > > > of different resolutions being set on different CRTCs: instead of using
> > > > > > > > the same bandwidth for each CRTC (which is incorrect) user can now
> > > > > > > > calculate overall bandwidth required by all outputs and use that value.
> > > > > > > > 
> > > > > > > 
> > > > > > > There are two things this change is doing:
> > > > > > > 
> > > > > > > 1) Dropping the core_clk_rate setting because its already handled inside
> > > > > > > _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
> > > > > > > will still work.
> > > > > > > 
> > > > > > > and
> > > > > > > 
> > > > > > > 2) Then this part of moving the ab/ib setting to
> > > > > > > _dpu_core_perf_crtc_update_bus().
> > > > > > > 
> > > > > > > Can we split this into two changes so that its clear that dropping
> > > > > > > core_clk_rate setting in this change will not cause an issue.
> > > > > > 
> > > > > > Ack
> > > > > > 
> > > > > 
> > > > > Actually I think this is incorrect.
> > > > > 
> > > > > If the user puts in an incorrect value beyond the bounds, earlier the code
> > > > > will reject that by failing the in _dpu_core_perf_calc_crtc().
> > > > 
> > > > This function doesn't perform any validation nor returns an error code.
> > > > Probably you've meant some other function.
> > > > 
> > > 
> > > Sorry, let me explain a little more to complete the flow I am seeing.
> > > 
> > > _dpu_core_perf_calc_crtc() gets called by dpu_core_perf_crtc_check().
> > > 
> > > That one checks against erroneous values.
> > > 
> > >                  if (!threshold) {
> > >                          DPU_ERROR("no bandwidth limits specified\n");
> > >                          return -E2BIG;
> > >                  } else if (bw > threshold) {
> > >                          DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
> > >                                          threshold);
> > >                          return -E2BIG;
> > >                  }
> > 
> > Here we are checking that the selected set of modes doesn't overload
> > defined platform requirements. However I think that it should be
> > possible for the user to attempt to overcome predefined bandwidth
> > limitations in attempt to debug the issue. ICC framework handles that
> > perfectly (and if you check, until the sync_state is reached all BW's
> > are assumed to be UINT_MAX). Maybe I should document it in the commit
> > message that after this commit forced BWs are not a subject to the
> > catalog limitations.
> > 
> 
> hmmm, yes this was the validation I was referring to.
> 
> I didnt get why a user should be allowed to go beyond the platform limits,
> what purpose does that serve , its not leading to any conclusion or towards
> the resolution of the issue. With the platform validation not only we are
> enforcing the limits but also making sure that random values given by the
> user dont cause more harm than good.

If debugfs files are being used to overwrite the data, then the user is
an advanced user. Possible usage cases might include explicitly
overclocking the platform, performing validation checks or just
attempting to understand the underfill issues. Thus I belive the
advanced user should be given a power to shoot their leg by specifying
hugher values than specified in the catalog. As I wrote, ICC driver
already uses UINT_MAX for bandwidth values during the system bootup.
RPM(h) will enforce bandwidth limitations on those votes.

> 
> > > 
> > > > > 
> > > > > Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond the
> > > > > check phase so incorrect values cannot be rejected.
> > > > > 
> > > > > So we will still need to preserve overriding the values in
> > > > > _dpu_core_perf_calc_crtc().
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > ---
> > > > > > > >      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
> > > > > > > >      1 file changed, 19 insertions(+), 21 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > > > index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
> > > > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > > > @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> > > > > > > >      		return;
> > > > > > > >      	}
> > > > > > > > -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
> > > > > > > > -
> > > > > > > > -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > > > > > -		perf->bw_ctl = 0;
> > > > > > > > -		perf->max_per_pipe_ib = 0;
> > > > > > > > -		perf->core_clk_rate = 0;
> > > > > > > > -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > > > > > -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
> > > > > > > > -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
> > > > > > > > -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
> > > > > > > > -	} else {
> > > > > > > > -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > > > > > > > -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > > > > > > > -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > > > > > > > -	}
> > > > > > > > -
> > > > > > > > +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > > > > > > > +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> > > > > > > > +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> > > > > > > >      	DRM_DEBUG_ATOMIC(
> > > > > > > >      		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
> > > > > > > >      			crtc->base.id, perf->core_clk_rate,
> > > > > > > > @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> > > > > > > >      {
> > > > > > > >      	struct dpu_core_perf_params perf = { 0 };
> > > > > > > >      	int i, ret = 0;
> > > > > > > > -	u64 avg_bw;
> > > > > > > > +	u32 avg_bw;
> > > > > > > > +	u32 peak_bw;
> > > > > > > >      	if (!kms->num_paths)
> > > > > > > >      		return 0;
> > > > > > > > -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > > > > > +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > > > > > +		avg_bw = 0;
> > > > > > > > +		peak_bw = 0;
> > > > > > > > +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > > > > > +		avg_bw = kms->perf.fix_core_ab_vote;
> > > > > > > > +		peak_bw = kms->perf.fix_core_ib_vote;
> > > > > > > > +	} else {
> > > > > > > > +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > > > > > +
> > > > > > > > +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > > > > > +		peak_bw = perf.max_per_pipe_ib;
> > > > > > > > +	}
> > > > > > > > -	avg_bw = perf.bw_ctl;
> > > > > > > > -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
> > > > > > > > +	avg_bw /= kms->num_paths;
> > > > > > > >      	for (i = 0; i < kms->num_paths; i++)
> > > > > > > > -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> > > > > > > > +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
> > > > > > > >      	return ret;
> > > > > > > >      }
> > > > > > > > 
> > > > > > 
> > > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 7/9] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()

[Abhinav Kumar]

On 1/15/2025 5:14 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 04:40:39PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/15/2025 4:32 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jan 15, 2025 at 11:41:27AM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/15/2025 12:27 AM, Dmitry Baryshkov wrote:
>>>>> On Tue, Jan 14, 2025 at 01:18:26PM -0800, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 1/14/2025 3:10 AM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Jan 13, 2025 at 07:38:16PM -0800, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>>>>>> Move perf mode handling for the bandwidth to
>>>>>>>>> _dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
>>>>>>>>> and then aggregating known values.
>>>>>>>>>
>>>>>>>>> Note, this changes the fix_core_ab_vote. Previously it would be
>>>>>>>>> multiplied per the CRTC number, now it will be used directly for
>>>>>>>>> interconnect voting. This better reflects user requirements in the case
>>>>>>>>> of different resolutions being set on different CRTCs: instead of using
>>>>>>>>> the same bandwidth for each CRTC (which is incorrect) user can now
>>>>>>>>> calculate overall bandwidth required by all outputs and use that value.
>>>>>>>>>
>>>>>>>>
>>>>>>>> There are two things this change is doing:
>>>>>>>>
>>>>>>>> 1) Dropping the core_clk_rate setting because its already handled inside
>>>>>>>> _dpu_core_perf_get_core_clk_rate() and hence dpu_core_perf_crtc_update()
>>>>>>>> will still work.
>>>>>>>>
>>>>>>>> and
>>>>>>>>
>>>>>>>> 2) Then this part of moving the ab/ib setting to
>>>>>>>> _dpu_core_perf_crtc_update_bus().
>>>>>>>>
>>>>>>>> Can we split this into two changes so that its clear that dropping
>>>>>>>> core_clk_rate setting in this change will not cause an issue.
>>>>>>>
>>>>>>> Ack
>>>>>>>
>>>>>>
>>>>>> Actually I think this is incorrect.
>>>>>>
>>>>>> If the user puts in an incorrect value beyond the bounds, earlier the code
>>>>>> will reject that by failing the in _dpu_core_perf_calc_crtc().
>>>>>
>>>>> This function doesn't perform any validation nor returns an error code.
>>>>> Probably you've meant some other function.
>>>>>
>>>>
>>>> Sorry, let me explain a little more to complete the flow I am seeing.
>>>>
>>>> _dpu_core_perf_calc_crtc() gets called by dpu_core_perf_crtc_check().
>>>>
>>>> That one checks against erroneous values.
>>>>
>>>>                   if (!threshold) {
>>>>                           DPU_ERROR("no bandwidth limits specified\n");
>>>>                           return -E2BIG;
>>>>                   } else if (bw > threshold) {
>>>>                           DPU_ERROR("exceeds bandwidth: %ukb > %ukb\n", bw,
>>>>                                           threshold);
>>>>                           return -E2BIG;
>>>>                   }
>>>
>>> Here we are checking that the selected set of modes doesn't overload
>>> defined platform requirements. However I think that it should be
>>> possible for the user to attempt to overcome predefined bandwidth
>>> limitations in attempt to debug the issue. ICC framework handles that
>>> perfectly (and if you check, until the sync_state is reached all BW's
>>> are assumed to be UINT_MAX). Maybe I should document it in the commit
>>> message that after this commit forced BWs are not a subject to the
>>> catalog limitations.
>>>
>>
>> hmmm, yes this was the validation I was referring to.
>>
>> I didnt get why a user should be allowed to go beyond the platform limits,
>> what purpose does that serve , its not leading to any conclusion or towards
>> the resolution of the issue. With the platform validation not only we are
>> enforcing the limits but also making sure that random values given by the
>> user dont cause more harm than good.
> 
> If debugfs files are being used to overwrite the data, then the user is
> an advanced user. Possible usage cases might include explicitly
> overclocking the platform, performing validation checks or just
> attempting to understand the underfill issues. Thus I belive the
> advanced user should be given a power to shoot their leg by specifying
> hugher values than specified in the catalog. As I wrote, ICC driver
> already uses UINT_MAX for bandwidth values during the system bootup.
> RPM(h) will enforce bandwidth limitations on those votes.
> 

As per our discussion, there are two benefits of going beyond dpu 
advertised platform limits:

1) Making sure the catalog limits are indeed correct
2) If (1) is not an issue, then it allows a developer to go beyond the 
value and see whether this makes any difference to the issue

Although (2) makes it outside the scope of display debugging really, its 
still a datapoint.

So with the commit text adjusted to note this change and with the patch 
split discussed ealier in this thread, we can go ahead with this.

Thanks

>>
>>>>
>>>>>>
>>>>>> Now, if we move it to _dpu_core_perf_crtc_update_bus(), this is beyond the
>>>>>> check phase so incorrect values cannot be rejected.
>>>>>>
>>>>>> So we will still need to preserve overriding the values in
>>>>>> _dpu_core_perf_calc_crtc().
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>> ---
>>>>>>>>>       drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 40 +++++++++++++--------------
>>>>>>>>>       1 file changed, 19 insertions(+), 21 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>>>> index 70f43e8359caee2082f2ca9944a17a6a20aa3d49..7ff3405c6867556a8dc776783b91f1da6c86ef3f 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>>>> @@ -118,22 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>>>>>>>       		return;
>>>>>>>>>       	}
>>>>>>>>> -	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>>>>>>>>> -
>>>>>>>>> -	if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>>>>>> -		perf->bw_ctl = 0;
>>>>>>>>> -		perf->max_per_pipe_ib = 0;
>>>>>>>>> -		perf->core_clk_rate = 0;
>>>>>>>>> -	} else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>>>>>> -		perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL;
>>>>>>>>> -		perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>>>>>>>> -		perf->core_clk_rate = core_perf->fix_core_clk_rate;
>>>>>>>>> -	} else {
>>>>>>>>> -		perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>>>>>>>> -		perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>>>>>>> -		perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>>>>>>> -	}
>>>>>>>>> -
>>>>>>>>> +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
>>>>>>>>> +	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>>>>>>> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>>>>>>>>>       	DRM_DEBUG_ATOMIC(
>>>>>>>>>       		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
>>>>>>>>>       			crtc->base.id, perf->core_clk_rate,
>>>>>>>>> @@ -222,18 +209,29 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>>>>>>>       {
>>>>>>>>>       	struct dpu_core_perf_params perf = { 0 };
>>>>>>>>>       	int i, ret = 0;
>>>>>>>>> -	u64 avg_bw;
>>>>>>>>> +	u32 avg_bw;
>>>>>>>>> +	u32 peak_bw;
>>>>>>>>>       	if (!kms->num_paths)
>>>>>>>>>       		return 0;
>>>>>>>>> -	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>>>>>> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>>>>>> +		avg_bw = 0;
>>>>>>>>> +		peak_bw = 0;
>>>>>>>>> +	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>>>>>> +		avg_bw = kms->perf.fix_core_ab_vote;
>>>>>>>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>>>>>>>> +	} else {
>>>>>>>>> +		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>>>>>> +
>>>>>>>>> +		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>>>>>> +		peak_bw = perf.max_per_pipe_ib;
>>>>>>>>> +	}
>>>>>>>>> -	avg_bw = perf.bw_ctl;
>>>>>>>>> -	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>>>>>>>>> +	avg_bw /= kms->num_paths;
>>>>>>>>>       	for (i = 0; i < kms->num_paths; i++)
>>>>>>>>> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>>>>>>>>> +		icc_set_bw(kms->path[i], avg_bw, peak_bw);
>>>>>>>>>       	return ret;
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>
>>>>>
>>>
> 

[PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Dmitry Baryshkov]

Currently debugfs provides separate 'modes' to override calculated
MDP_CLK rate and interconnect bandwidth votes. Change that to allow
overriding individual values (e.g. one can override just clock or just
average bandwidth vote). The maximum values allowed for those entries by
the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
files in debugfs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
 2 files changed, 9 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -17,20 +17,6 @@
 #include "dpu_crtc.h"
 #include "dpu_core_perf.h"
 
-/**
- * enum dpu_perf_mode - performance tuning mode
- * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
- * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
- * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
- * @DPU_PERF_MODE_MAX: maximum value, used for error checking
- */
-enum dpu_perf_mode {
-	DPU_PERF_MODE_NORMAL,
-	DPU_PERF_MODE_MINIMUM,
-	DPU_PERF_MODE_FIXED,
-	DPU_PERF_MODE_MAX
-};
-
 /**
  * _dpu_core_perf_calc_bw() - to calculate BW per crtc
  * @perf_cfg: performance configuration
@@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 	if (!kms->num_paths)
 		return 0;
 
-	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
-		avg_bw = 0;
-		peak_bw = 0;
-	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
+	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
+
+	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
+	peak_bw = perf.max_per_pipe_ib;
+
+	if (kms->perf.fix_core_ab_vote)
 		avg_bw = kms->perf.fix_core_ab_vote;
-		peak_bw = kms->perf.fix_core_ib_vote;
-	} else {
-		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
 
-		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
-		peak_bw = perf.max_per_pipe_ib;
-	}
+	if (kms->perf.fix_core_ib_vote)
+		peak_bw = kms->perf.fix_core_ib_vote;
 
 	avg_bw /= kms->num_paths;
 
@@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 	struct drm_crtc *crtc;
 	struct dpu_crtc_state *dpu_cstate;
 
-	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
+	if (kms->perf.fix_core_clk_rate)
 		return kms->perf.fix_core_clk_rate;
 
-	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
-		return kms->perf.max_core_clk_rate;
-
 	clk_rate = 0;
 	drm_for_each_crtc(crtc, kms->dev) {
 		if (crtc->enabled) {
@@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 
 #ifdef CONFIG_DEBUG_FS
 
-static ssize_t _dpu_core_perf_mode_write(struct file *file,
-		    const char __user *user_buf, size_t count, loff_t *ppos)
-{
-	struct dpu_core_perf *perf = file->private_data;
-	u32 perf_mode = 0;
-	int ret;
-
-	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
-	if (ret)
-		return ret;
-
-	if (perf_mode >= DPU_PERF_MODE_MAX)
-		return -EINVAL;
-
-	if (perf_mode == DPU_PERF_MODE_FIXED) {
-		DRM_INFO("fix performance mode\n");
-	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
-		/* run the driver with max clk and BW vote */
-		DRM_INFO("minimum performance mode\n");
-	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
-		/* reset the perf tune params to 0 */
-		DRM_INFO("normal performance mode\n");
-	}
-	perf->perf_tune.mode = perf_mode;
-
-	return count;
-}
-
-static ssize_t _dpu_core_perf_mode_read(struct file *file,
-			char __user *buff, size_t count, loff_t *ppos)
-{
-	struct dpu_core_perf *perf = file->private_data;
-	int len;
-	char buf[128];
-
-	len = scnprintf(buf, sizeof(buf),
-			"mode %d\n",
-			perf->perf_tune.mode);
-
-	return simple_read_from_buffer(buff, count, ppos, buf, len);
-}
-
-static const struct file_operations dpu_core_perf_mode_fops = {
-	.open = simple_open,
-	.read = _dpu_core_perf_mode_read,
-	.write = _dpu_core_perf_mode_write,
-};
-
 /**
  * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
  * @dpu_kms: Pointer to the dpu_kms struct
@@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 			(u32 *)&perf->perf_cfg->min_llcc_ib);
 	debugfs_create_u32("min_dram_ib", 0400, entry,
 			(u32 *)&perf->perf_cfg->min_dram_ib);
-	debugfs_create_file("perf_mode", 0600, entry,
-			(u32 *)perf, &dpu_core_perf_mode_fops);
 	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
 			&perf->fix_core_clk_rate);
 	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -24,20 +24,11 @@ struct dpu_core_perf_params {
 	u64 core_clk_rate;
 };
 
-/**
- * struct dpu_core_perf_tune - definition of performance tuning control
- * @mode: performance mode
- */
-struct dpu_core_perf_tune {
-	u32 mode;
-};
-
 /**
  * struct dpu_core_perf - definition of core performance context
  * @perf_cfg: Platform-specific performance configuration
  * @core_clk_rate: current core clock rate
  * @max_core_clk_rate: maximum allowable core clock rate
- * @perf_tune: debug control for performance tuning
  * @enable_bw_release: debug control for bandwidth release
  * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
  * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
@@ -47,7 +38,6 @@ struct dpu_core_perf {
 	const struct dpu_perf_cfg *perf_cfg;
 	u64 core_clk_rate;
 	u64 max_core_clk_rate;
-	struct dpu_core_perf_tune perf_tune;
 	u32 enable_bw_release;
 	u64 fix_core_clk_rate;
 	u32 fix_core_ib_vote;

-- 
2.39.5

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> Currently debugfs provides separate 'modes' to override calculated
> MDP_CLK rate and interconnect bandwidth votes. Change that to allow
> overriding individual values (e.g. one can override just clock or just
> average bandwidth vote). The maximum values allowed for those entries by
> the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
> files in debugfs.
> 

Apart from the concern I highlighted in the previous patch, the only 
issue I have with this is that, this went from a one step process of 
using the "mode" this has become a two step one.

There were essentially two modes we are talking about - "fixed" and 
"minimum"

With respect to "fixed" this is totally fine because this is preserving 
that functionality because to be able to set the fixed mode the end user 
must know what values they want to try anyway.

With respect to "minimum" mode, is where this approach is not that 
great. The end users of this can be non-display developers too such as 
our QA teams who might want to perform a first level of triage on the 
issues and route it accordingly. This is especially true for underruns 
and some performance lags as well.

If you really dont like the term "modes", to preserve the "minimum" 
mode, how about just using a bool debugfs like "max_perf_params" which 
internally maxes out the max MDP clock and ab/ib params.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
>   2 files changed, 9 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -17,20 +17,6 @@
>   #include "dpu_crtc.h"
>   #include "dpu_core_perf.h"
>   
> -/**
> - * enum dpu_perf_mode - performance tuning mode
> - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
> - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
> - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
> - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
> - */
> -enum dpu_perf_mode {
> -	DPU_PERF_MODE_NORMAL,
> -	DPU_PERF_MODE_MINIMUM,
> -	DPU_PERF_MODE_FIXED,
> -	DPU_PERF_MODE_MAX
> -};
> -
>   /**
>    * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>    * @perf_cfg: performance configuration
> @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>   	if (!kms->num_paths)
>   		return 0;
>   
> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> -		avg_bw = 0;
> -		peak_bw = 0;
> -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> +
> +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> +	peak_bw = perf.max_per_pipe_ib;
> +
> +	if (kms->perf.fix_core_ab_vote)
>   		avg_bw = kms->perf.fix_core_ab_vote;
> -		peak_bw = kms->perf.fix_core_ib_vote;
> -	} else {
> -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>   
> -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> -		peak_bw = perf.max_per_pipe_ib;
> -	}
> +	if (kms->perf.fix_core_ib_vote)
> +		peak_bw = kms->perf.fix_core_ib_vote;
>   
>   	avg_bw /= kms->num_paths;
>   
> @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>   	struct drm_crtc *crtc;
>   	struct dpu_crtc_state *dpu_cstate;
>   
> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
> +	if (kms->perf.fix_core_clk_rate)
>   		return kms->perf.fix_core_clk_rate;
>   
> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
> -		return kms->perf.max_core_clk_rate;
> -
>   	clk_rate = 0;
>   	drm_for_each_crtc(crtc, kms->dev) {
>   		if (crtc->enabled) {
> @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   
>   #ifdef CONFIG_DEBUG_FS
>   
> -static ssize_t _dpu_core_perf_mode_write(struct file *file,
> -		    const char __user *user_buf, size_t count, loff_t *ppos)
> -{
> -	struct dpu_core_perf *perf = file->private_data;
> -	u32 perf_mode = 0;
> -	int ret;
> -
> -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
> -	if (ret)
> -		return ret;
> -
> -	if (perf_mode >= DPU_PERF_MODE_MAX)
> -		return -EINVAL;
> -
> -	if (perf_mode == DPU_PERF_MODE_FIXED) {
> -		DRM_INFO("fix performance mode\n");
> -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
> -		/* run the driver with max clk and BW vote */
> -		DRM_INFO("minimum performance mode\n");
> -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
> -		/* reset the perf tune params to 0 */
> -		DRM_INFO("normal performance mode\n");
> -	}
> -	perf->perf_tune.mode = perf_mode;
> -
> -	return count;
> -}
> -
> -static ssize_t _dpu_core_perf_mode_read(struct file *file,
> -			char __user *buff, size_t count, loff_t *ppos)
> -{
> -	struct dpu_core_perf *perf = file->private_data;
> -	int len;
> -	char buf[128];
> -
> -	len = scnprintf(buf, sizeof(buf),
> -			"mode %d\n",
> -			perf->perf_tune.mode);
> -
> -	return simple_read_from_buffer(buff, count, ppos, buf, len);
> -}
> -
> -static const struct file_operations dpu_core_perf_mode_fops = {
> -	.open = simple_open,
> -	.read = _dpu_core_perf_mode_read,
> -	.write = _dpu_core_perf_mode_write,
> -};
> -
>   /**
>    * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
>    * @dpu_kms: Pointer to the dpu_kms struct
> @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>   			(u32 *)&perf->perf_cfg->min_llcc_ib);
>   	debugfs_create_u32("min_dram_ib", 0400, entry,
>   			(u32 *)&perf->perf_cfg->min_dram_ib);
> -	debugfs_create_file("perf_mode", 0600, entry,
> -			(u32 *)perf, &dpu_core_perf_mode_fops);
>   	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
>   			&perf->fix_core_clk_rate);
>   	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
>   	u64 core_clk_rate;
>   };
>   
> -/**
> - * struct dpu_core_perf_tune - definition of performance tuning control
> - * @mode: performance mode
> - */
> -struct dpu_core_perf_tune {
> -	u32 mode;
> -};
> -
>   /**
>    * struct dpu_core_perf - definition of core performance context
>    * @perf_cfg: Platform-specific performance configuration
>    * @core_clk_rate: current core clock rate
>    * @max_core_clk_rate: maximum allowable core clock rate
> - * @perf_tune: debug control for performance tuning
>    * @enable_bw_release: debug control for bandwidth release
>    * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
>    * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
> @@ -47,7 +38,6 @@ struct dpu_core_perf {
>   	const struct dpu_perf_cfg *perf_cfg;
>   	u64 core_clk_rate;
>   	u64 max_core_clk_rate;
> -	struct dpu_core_perf_tune perf_tune;
>   	u32 enable_bw_release;
>   	u64 fix_core_clk_rate;
>   	u32 fix_core_ib_vote;
> 

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Dmitry Baryshkov]

On Tue, Jan 14, 2025 at 02:02:54PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > Currently debugfs provides separate 'modes' to override calculated
> > MDP_CLK rate and interconnect bandwidth votes. Change that to allow
> > overriding individual values (e.g. one can override just clock or just
> > average bandwidth vote). The maximum values allowed for those entries by
> > the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
> > files in debugfs.
> > 
> 
> Apart from the concern I highlighted in the previous patch, the only issue I
> have with this is that, this went from a one step process of using the
> "mode" this has become a two step one.
> 
> There were essentially two modes we are talking about - "fixed" and
> "minimum"
> 
> With respect to "fixed" this is totally fine because this is preserving that
> functionality because to be able to set the fixed mode the end user must
> know what values they want to try anyway.
> 
> With respect to "minimum" mode, is where this approach is not that great.
> The end users of this can be non-display developers too such as our QA teams
> who might want to perform a first level of triage on the issues and route it
> accordingly. This is especially true for underruns and some performance lags
> as well.
> 
> If you really dont like the term "modes", to preserve the "minimum" mode,
> how about just using a bool debugfs like "max_perf_params" which internally
> maxes out the max MDP clock and ab/ib params.

That's what I'm trying to avoid - having an extra debugfs file which
overrides other files. It is much easier to work if there is no need to
switch modes, it is easy to overlook it. I think it should be fine to
use `cat max_foo > fix_foo` to override each of the params. After
renaming the threshold_high to max_core_ab the names of the debugfs
files match.

> 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
> >   2 files changed, 9 insertions(+), 88 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > @@ -17,20 +17,6 @@
> >   #include "dpu_crtc.h"
> >   #include "dpu_core_perf.h"
> > -/**
> > - * enum dpu_perf_mode - performance tuning mode
> > - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
> > - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
> > - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
> > - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
> > - */
> > -enum dpu_perf_mode {
> > -	DPU_PERF_MODE_NORMAL,
> > -	DPU_PERF_MODE_MINIMUM,
> > -	DPU_PERF_MODE_FIXED,
> > -	DPU_PERF_MODE_MAX
> > -};
> > -
> >   /**
> >    * _dpu_core_perf_calc_bw() - to calculate BW per crtc
> >    * @perf_cfg: performance configuration
> > @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> >   	if (!kms->num_paths)
> >   		return 0;
> > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > -		avg_bw = 0;
> > -		peak_bw = 0;
> > -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > +
> > +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > +	peak_bw = perf.max_per_pipe_ib;
> > +
> > +	if (kms->perf.fix_core_ab_vote)
> >   		avg_bw = kms->perf.fix_core_ab_vote;
> > -		peak_bw = kms->perf.fix_core_ib_vote;
> > -	} else {
> > -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > -		peak_bw = perf.max_per_pipe_ib;
> > -	}
> > +	if (kms->perf.fix_core_ib_vote)
> > +		peak_bw = kms->perf.fix_core_ib_vote;
> >   	avg_bw /= kms->num_paths;
> > @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> >   	struct drm_crtc *crtc;
> >   	struct dpu_crtc_state *dpu_cstate;
> > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
> > +	if (kms->perf.fix_core_clk_rate)
> >   		return kms->perf.fix_core_clk_rate;
> > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
> > -		return kms->perf.max_core_clk_rate;
> > -
> >   	clk_rate = 0;
> >   	drm_for_each_crtc(crtc, kms->dev) {
> >   		if (crtc->enabled) {
> > @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >   #ifdef CONFIG_DEBUG_FS
> > -static ssize_t _dpu_core_perf_mode_write(struct file *file,
> > -		    const char __user *user_buf, size_t count, loff_t *ppos)
> > -{
> > -	struct dpu_core_perf *perf = file->private_data;
> > -	u32 perf_mode = 0;
> > -	int ret;
> > -
> > -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (perf_mode >= DPU_PERF_MODE_MAX)
> > -		return -EINVAL;
> > -
> > -	if (perf_mode == DPU_PERF_MODE_FIXED) {
> > -		DRM_INFO("fix performance mode\n");
> > -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
> > -		/* run the driver with max clk and BW vote */
> > -		DRM_INFO("minimum performance mode\n");
> > -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
> > -		/* reset the perf tune params to 0 */
> > -		DRM_INFO("normal performance mode\n");
> > -	}
> > -	perf->perf_tune.mode = perf_mode;
> > -
> > -	return count;
> > -}
> > -
> > -static ssize_t _dpu_core_perf_mode_read(struct file *file,
> > -			char __user *buff, size_t count, loff_t *ppos)
> > -{
> > -	struct dpu_core_perf *perf = file->private_data;
> > -	int len;
> > -	char buf[128];
> > -
> > -	len = scnprintf(buf, sizeof(buf),
> > -			"mode %d\n",
> > -			perf->perf_tune.mode);
> > -
> > -	return simple_read_from_buffer(buff, count, ppos, buf, len);
> > -}
> > -
> > -static const struct file_operations dpu_core_perf_mode_fops = {
> > -	.open = simple_open,
> > -	.read = _dpu_core_perf_mode_read,
> > -	.write = _dpu_core_perf_mode_write,
> > -};
> > -
> >   /**
> >    * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
> >    * @dpu_kms: Pointer to the dpu_kms struct
> > @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> >   			(u32 *)&perf->perf_cfg->min_llcc_ib);
> >   	debugfs_create_u32("min_dram_ib", 0400, entry,
> >   			(u32 *)&perf->perf_cfg->min_dram_ib);
> > -	debugfs_create_file("perf_mode", 0600, entry,
> > -			(u32 *)perf, &dpu_core_perf_mode_fops);
> >   	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
> >   			&perf->fix_core_clk_rate);
> >   	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
> >   	u64 core_clk_rate;
> >   };
> > -/**
> > - * struct dpu_core_perf_tune - definition of performance tuning control
> > - * @mode: performance mode
> > - */
> > -struct dpu_core_perf_tune {
> > -	u32 mode;
> > -};
> > -
> >   /**
> >    * struct dpu_core_perf - definition of core performance context
> >    * @perf_cfg: Platform-specific performance configuration
> >    * @core_clk_rate: current core clock rate
> >    * @max_core_clk_rate: maximum allowable core clock rate
> > - * @perf_tune: debug control for performance tuning
> >    * @enable_bw_release: debug control for bandwidth release
> >    * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
> >    * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
> > @@ -47,7 +38,6 @@ struct dpu_core_perf {
> >   	const struct dpu_perf_cfg *perf_cfg;
> >   	u64 core_clk_rate;
> >   	u64 max_core_clk_rate;
> > -	struct dpu_core_perf_tune perf_tune;
> >   	u32 enable_bw_release;
> >   	u64 fix_core_clk_rate;
> >   	u32 fix_core_ib_vote;
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Abhinav Kumar]

On 1/15/2025 12:41 AM, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 02:02:54PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>> Currently debugfs provides separate 'modes' to override calculated
>>> MDP_CLK rate and interconnect bandwidth votes. Change that to allow
>>> overriding individual values (e.g. one can override just clock or just
>>> average bandwidth vote). The maximum values allowed for those entries by
>>> the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
>>> files in debugfs.
>>>
>>
>> Apart from the concern I highlighted in the previous patch, the only issue I
>> have with this is that, this went from a one step process of using the
>> "mode" this has become a two step one.
>>
>> There were essentially two modes we are talking about - "fixed" and
>> "minimum"
>>
>> With respect to "fixed" this is totally fine because this is preserving that
>> functionality because to be able to set the fixed mode the end user must
>> know what values they want to try anyway.
>>
>> With respect to "minimum" mode, is where this approach is not that great.
>> The end users of this can be non-display developers too such as our QA teams
>> who might want to perform a first level of triage on the issues and route it
>> accordingly. This is especially true for underruns and some performance lags
>> as well.
>>
>> If you really dont like the term "modes", to preserve the "minimum" mode,
>> how about just using a bool debugfs like "max_perf_params" which internally
>> maxes out the max MDP clock and ab/ib params.
> 
> That's what I'm trying to avoid - having an extra debugfs file which
> overrides other files. It is much easier to work if there is no need to
> switch modes, it is easy to overlook it. I think it should be fine to
> use `cat max_foo > fix_foo` to override each of the params. After
> renaming the threshold_high to max_core_ab the names of the debugfs
> files match.
> 

Its just a difference in interpretation IMO.

the "fixed" mode is trying to given an option to incrementally try and 
see which value really works and also to see whether its the clock OR 
the bandwidth which is making the difference. So individual control of 
those.

The "max" mode is trying to see if even the max values of everything 
cannot fix the problem. BTW, the max was maxing out BOTH the DPU clocks 
and BW.

So this is not just 2 extra reads for the user but 3. (ab/ib/dpu_clk) if 
we drop "max" and use "fixed" for max as well and even for that the user 
has to refer the max DPU clock value.


>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
>>>    2 files changed, 9 insertions(+), 88 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -17,20 +17,6 @@
>>>    #include "dpu_crtc.h"
>>>    #include "dpu_core_perf.h"
>>> -/**
>>> - * enum dpu_perf_mode - performance tuning mode
>>> - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
>>> - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
>>> - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
>>> - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
>>> - */
>>> -enum dpu_perf_mode {
>>> -	DPU_PERF_MODE_NORMAL,
>>> -	DPU_PERF_MODE_MINIMUM,
>>> -	DPU_PERF_MODE_FIXED,
>>> -	DPU_PERF_MODE_MAX
>>> -};
>>> -
>>>    /**
>>>     * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>>>     * @perf_cfg: performance configuration
>>> @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>    	if (!kms->num_paths)
>>>    		return 0;
>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>> -		avg_bw = 0;
>>> -		peak_bw = 0;
>>> -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>> +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>> +
>>> +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>> +	peak_bw = perf.max_per_pipe_ib;
>>> +
>>> +	if (kms->perf.fix_core_ab_vote)
>>>    		avg_bw = kms->perf.fix_core_ab_vote;
>>> -		peak_bw = kms->perf.fix_core_ib_vote;
>>> -	} else {
>>> -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>> -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>> -		peak_bw = perf.max_per_pipe_ib;
>>> -	}
>>> +	if (kms->perf.fix_core_ib_vote)
>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>>    	avg_bw /= kms->num_paths;
>>> @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>    	struct drm_crtc *crtc;
>>>    	struct dpu_crtc_state *dpu_cstate;
>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
>>> +	if (kms->perf.fix_core_clk_rate)
>>>    		return kms->perf.fix_core_clk_rate;
>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
>>> -		return kms->perf.max_core_clk_rate;
>>> -
>>>    	clk_rate = 0;
>>>    	drm_for_each_crtc(crtc, kms->dev) {
>>>    		if (crtc->enabled) {
>>> @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>    #ifdef CONFIG_DEBUG_FS
>>> -static ssize_t _dpu_core_perf_mode_write(struct file *file,
>>> -		    const char __user *user_buf, size_t count, loff_t *ppos)
>>> -{
>>> -	struct dpu_core_perf *perf = file->private_data;
>>> -	u32 perf_mode = 0;
>>> -	int ret;
>>> -
>>> -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	if (perf_mode >= DPU_PERF_MODE_MAX)
>>> -		return -EINVAL;
>>> -
>>> -	if (perf_mode == DPU_PERF_MODE_FIXED) {
>>> -		DRM_INFO("fix performance mode\n");
>>> -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
>>> -		/* run the driver with max clk and BW vote */
>>> -		DRM_INFO("minimum performance mode\n");
>>> -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
>>> -		/* reset the perf tune params to 0 */
>>> -		DRM_INFO("normal performance mode\n");
>>> -	}
>>> -	perf->perf_tune.mode = perf_mode;
>>> -
>>> -	return count;
>>> -}
>>> -
>>> -static ssize_t _dpu_core_perf_mode_read(struct file *file,
>>> -			char __user *buff, size_t count, loff_t *ppos)
>>> -{
>>> -	struct dpu_core_perf *perf = file->private_data;
>>> -	int len;
>>> -	char buf[128];
>>> -
>>> -	len = scnprintf(buf, sizeof(buf),
>>> -			"mode %d\n",
>>> -			perf->perf_tune.mode);
>>> -
>>> -	return simple_read_from_buffer(buff, count, ppos, buf, len);
>>> -}
>>> -
>>> -static const struct file_operations dpu_core_perf_mode_fops = {
>>> -	.open = simple_open,
>>> -	.read = _dpu_core_perf_mode_read,
>>> -	.write = _dpu_core_perf_mode_write,
>>> -};
>>> -
>>>    /**
>>>     * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
>>>     * @dpu_kms: Pointer to the dpu_kms struct
>>> @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>    			(u32 *)&perf->perf_cfg->min_llcc_ib);
>>>    	debugfs_create_u32("min_dram_ib", 0400, entry,
>>>    			(u32 *)&perf->perf_cfg->min_dram_ib);
>>> -	debugfs_create_file("perf_mode", 0600, entry,
>>> -			(u32 *)perf, &dpu_core_perf_mode_fops);
>>>    	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
>>>    			&perf->fix_core_clk_rate);
>>>    	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
>>>    	u64 core_clk_rate;
>>>    };
>>> -/**
>>> - * struct dpu_core_perf_tune - definition of performance tuning control
>>> - * @mode: performance mode
>>> - */
>>> -struct dpu_core_perf_tune {
>>> -	u32 mode;
>>> -};
>>> -
>>>    /**
>>>     * struct dpu_core_perf - definition of core performance context
>>>     * @perf_cfg: Platform-specific performance configuration
>>>     * @core_clk_rate: current core clock rate
>>>     * @max_core_clk_rate: maximum allowable core clock rate
>>> - * @perf_tune: debug control for performance tuning
>>>     * @enable_bw_release: debug control for bandwidth release
>>>     * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
>>>     * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
>>> @@ -47,7 +38,6 @@ struct dpu_core_perf {
>>>    	const struct dpu_perf_cfg *perf_cfg;
>>>    	u64 core_clk_rate;
>>>    	u64 max_core_clk_rate;
>>> -	struct dpu_core_perf_tune perf_tune;
>>>    	u32 enable_bw_release;
>>>    	u64 fix_core_clk_rate;
>>>    	u32 fix_core_ib_vote;
>>>
> 

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 11:51:20AM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/15/2025 12:41 AM, Dmitry Baryshkov wrote:
> > On Tue, Jan 14, 2025 at 02:02:54PM -0800, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > > > Currently debugfs provides separate 'modes' to override calculated
> > > > MDP_CLK rate and interconnect bandwidth votes. Change that to allow
> > > > overriding individual values (e.g. one can override just clock or just
> > > > average bandwidth vote). The maximum values allowed for those entries by
> > > > the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
> > > > files in debugfs.
> > > > 
> > > 
> > > Apart from the concern I highlighted in the previous patch, the only issue I
> > > have with this is that, this went from a one step process of using the
> > > "mode" this has become a two step one.
> > > 
> > > There were essentially two modes we are talking about - "fixed" and
> > > "minimum"
> > > 
> > > With respect to "fixed" this is totally fine because this is preserving that
> > > functionality because to be able to set the fixed mode the end user must
> > > know what values they want to try anyway.
> > > 
> > > With respect to "minimum" mode, is where this approach is not that great.
> > > The end users of this can be non-display developers too such as our QA teams
> > > who might want to perform a first level of triage on the issues and route it
> > > accordingly. This is especially true for underruns and some performance lags
> > > as well.
> > > 
> > > If you really dont like the term "modes", to preserve the "minimum" mode,
> > > how about just using a bool debugfs like "max_perf_params" which internally
> > > maxes out the max MDP clock and ab/ib params.
> > 
> > That's what I'm trying to avoid - having an extra debugfs file which
> > overrides other files. It is much easier to work if there is no need to
> > switch modes, it is easy to overlook it. I think it should be fine to
> > use `cat max_foo > fix_foo` to override each of the params. After
> > renaming the threshold_high to max_core_ab the names of the debugfs
> > files match.
> > 
> 
> Its just a difference in interpretation IMO.
> 
> the "fixed" mode is trying to given an option to incrementally try and see
> which value really works and also to see whether its the clock OR the
> bandwidth which is making the difference. So individual control of those.
> 
> The "max" mode is trying to see if even the max values of everything cannot
> fix the problem. BTW, the max was maxing out BOTH the DPU clocks and BW.
> 
> So this is not just 2 extra reads for the user but 3. (ab/ib/dpu_clk) if we
> drop "max" and use "fixed" for max as well and even for that the user has to
> refer the max DPU clock value.

Yes, I understand that. However I still think that it's easier than
having a set of 'fix_foo' values which are silently ignored because of
the preselected mode.

I can probably see an option: use your max_perf_params idea, but in a
form of a write-only file which immediately selects max values for clock
rate and both bandwidths. WDYT?

> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
> > > >    2 files changed, 9 insertions(+), 88 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > @@ -17,20 +17,6 @@
> > > >    #include "dpu_crtc.h"
> > > >    #include "dpu_core_perf.h"
> > > > -/**
> > > > - * enum dpu_perf_mode - performance tuning mode
> > > > - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
> > > > - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
> > > > - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
> > > > - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
> > > > - */
> > > > -enum dpu_perf_mode {
> > > > -	DPU_PERF_MODE_NORMAL,
> > > > -	DPU_PERF_MODE_MINIMUM,
> > > > -	DPU_PERF_MODE_FIXED,
> > > > -	DPU_PERF_MODE_MAX
> > > > -};
> > > > -
> > > >    /**
> > > >     * _dpu_core_perf_calc_bw() - to calculate BW per crtc
> > > >     * @perf_cfg: performance configuration
> > > > @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> > > >    	if (!kms->num_paths)
> > > >    		return 0;
> > > > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > -		avg_bw = 0;
> > > > -		peak_bw = 0;
> > > > -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > +
> > > > +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > +	peak_bw = perf.max_per_pipe_ib;
> > > > +
> > > > +	if (kms->perf.fix_core_ab_vote)
> > > >    		avg_bw = kms->perf.fix_core_ab_vote;
> > > > -		peak_bw = kms->perf.fix_core_ib_vote;
> > > > -	} else {
> > > > -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > -		peak_bw = perf.max_per_pipe_ib;
> > > > -	}
> > > > +	if (kms->perf.fix_core_ib_vote)
> > > > +		peak_bw = kms->perf.fix_core_ib_vote;
> > > >    	avg_bw /= kms->num_paths;
> > > > @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> > > >    	struct drm_crtc *crtc;
> > > >    	struct dpu_crtc_state *dpu_cstate;
> > > > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
> > > > +	if (kms->perf.fix_core_clk_rate)
> > > >    		return kms->perf.fix_core_clk_rate;
> > > > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
> > > > -		return kms->perf.max_core_clk_rate;
> > > > -
> > > >    	clk_rate = 0;
> > > >    	drm_for_each_crtc(crtc, kms->dev) {
> > > >    		if (crtc->enabled) {
> > > > @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > >    #ifdef CONFIG_DEBUG_FS
> > > > -static ssize_t _dpu_core_perf_mode_write(struct file *file,
> > > > -		    const char __user *user_buf, size_t count, loff_t *ppos)
> > > > -{
> > > > -	struct dpu_core_perf *perf = file->private_data;
> > > > -	u32 perf_mode = 0;
> > > > -	int ret;
> > > > -
> > > > -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
> > > > -	if (ret)
> > > > -		return ret;
> > > > -
> > > > -	if (perf_mode >= DPU_PERF_MODE_MAX)
> > > > -		return -EINVAL;
> > > > -
> > > > -	if (perf_mode == DPU_PERF_MODE_FIXED) {
> > > > -		DRM_INFO("fix performance mode\n");
> > > > -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
> > > > -		/* run the driver with max clk and BW vote */
> > > > -		DRM_INFO("minimum performance mode\n");
> > > > -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
> > > > -		/* reset the perf tune params to 0 */
> > > > -		DRM_INFO("normal performance mode\n");
> > > > -	}
> > > > -	perf->perf_tune.mode = perf_mode;
> > > > -
> > > > -	return count;
> > > > -}
> > > > -
> > > > -static ssize_t _dpu_core_perf_mode_read(struct file *file,
> > > > -			char __user *buff, size_t count, loff_t *ppos)
> > > > -{
> > > > -	struct dpu_core_perf *perf = file->private_data;
> > > > -	int len;
> > > > -	char buf[128];
> > > > -
> > > > -	len = scnprintf(buf, sizeof(buf),
> > > > -			"mode %d\n",
> > > > -			perf->perf_tune.mode);
> > > > -
> > > > -	return simple_read_from_buffer(buff, count, ppos, buf, len);
> > > > -}
> > > > -
> > > > -static const struct file_operations dpu_core_perf_mode_fops = {
> > > > -	.open = simple_open,
> > > > -	.read = _dpu_core_perf_mode_read,
> > > > -	.write = _dpu_core_perf_mode_write,
> > > > -};
> > > > -
> > > >    /**
> > > >     * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
> > > >     * @dpu_kms: Pointer to the dpu_kms struct
> > > > @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> > > >    			(u32 *)&perf->perf_cfg->min_llcc_ib);
> > > >    	debugfs_create_u32("min_dram_ib", 0400, entry,
> > > >    			(u32 *)&perf->perf_cfg->min_dram_ib);
> > > > -	debugfs_create_file("perf_mode", 0600, entry,
> > > > -			(u32 *)perf, &dpu_core_perf_mode_fops);
> > > >    	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
> > > >    			&perf->fix_core_clk_rate);
> > > >    	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
> > > >    	u64 core_clk_rate;
> > > >    };
> > > > -/**
> > > > - * struct dpu_core_perf_tune - definition of performance tuning control
> > > > - * @mode: performance mode
> > > > - */
> > > > -struct dpu_core_perf_tune {
> > > > -	u32 mode;
> > > > -};
> > > > -
> > > >    /**
> > > >     * struct dpu_core_perf - definition of core performance context
> > > >     * @perf_cfg: Platform-specific performance configuration
> > > >     * @core_clk_rate: current core clock rate
> > > >     * @max_core_clk_rate: maximum allowable core clock rate
> > > > - * @perf_tune: debug control for performance tuning
> > > >     * @enable_bw_release: debug control for bandwidth release
> > > >     * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
> > > >     * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
> > > > @@ -47,7 +38,6 @@ struct dpu_core_perf {
> > > >    	const struct dpu_perf_cfg *perf_cfg;
> > > >    	u64 core_clk_rate;
> > > >    	u64 max_core_clk_rate;
> > > > -	struct dpu_core_perf_tune perf_tune;
> > > >    	u32 enable_bw_release;
> > > >    	u64 fix_core_clk_rate;
> > > >    	u32 fix_core_ib_vote;
> > > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Abhinav Kumar]

On 1/15/2025 4:35 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 11:51:20AM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/15/2025 12:41 AM, Dmitry Baryshkov wrote:
>>> On Tue, Jan 14, 2025 at 02:02:54PM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>> Currently debugfs provides separate 'modes' to override calculated
>>>>> MDP_CLK rate and interconnect bandwidth votes. Change that to allow
>>>>> overriding individual values (e.g. one can override just clock or just
>>>>> average bandwidth vote). The maximum values allowed for those entries by
>>>>> the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
>>>>> files in debugfs.
>>>>>
>>>>
>>>> Apart from the concern I highlighted in the previous patch, the only issue I
>>>> have with this is that, this went from a one step process of using the
>>>> "mode" this has become a two step one.
>>>>
>>>> There were essentially two modes we are talking about - "fixed" and
>>>> "minimum"
>>>>
>>>> With respect to "fixed" this is totally fine because this is preserving that
>>>> functionality because to be able to set the fixed mode the end user must
>>>> know what values they want to try anyway.
>>>>
>>>> With respect to "minimum" mode, is where this approach is not that great.
>>>> The end users of this can be non-display developers too such as our QA teams
>>>> who might want to perform a first level of triage on the issues and route it
>>>> accordingly. This is especially true for underruns and some performance lags
>>>> as well.
>>>>
>>>> If you really dont like the term "modes", to preserve the "minimum" mode,
>>>> how about just using a bool debugfs like "max_perf_params" which internally
>>>> maxes out the max MDP clock and ab/ib params.
>>>
>>> That's what I'm trying to avoid - having an extra debugfs file which
>>> overrides other files. It is much easier to work if there is no need to
>>> switch modes, it is easy to overlook it. I think it should be fine to
>>> use `cat max_foo > fix_foo` to override each of the params. After
>>> renaming the threshold_high to max_core_ab the names of the debugfs
>>> files match.
>>>
>>
>> Its just a difference in interpretation IMO.
>>
>> the "fixed" mode is trying to given an option to incrementally try and see
>> which value really works and also to see whether its the clock OR the
>> bandwidth which is making the difference. So individual control of those.
>>
>> The "max" mode is trying to see if even the max values of everything cannot
>> fix the problem. BTW, the max was maxing out BOTH the DPU clocks and BW.
>>
>> So this is not just 2 extra reads for the user but 3. (ab/ib/dpu_clk) if we
>> drop "max" and use "fixed" for max as well and even for that the user has to
>> refer the max DPU clock value.
> 
> Yes, I understand that. However I still think that it's easier than
> having a set of 'fix_foo' values which are silently ignored because of
> the preselected mode.
> 
> I can probably see an option: use your max_perf_params idea, but in a
> form of a write-only file which immediately selects max values for clock
> rate and both bandwidths. WDYT?
> 

Sorry I am missing something here. This is the same thing I had in mind 
to have it as a bool when someone does echo 1 > max_perf_params, it will 
immediately max the values for clock rate and bandwidth.

So to summarize, there are four nodes:

1) fix_core_ab_vote
2) fix_core_ib_vote
3) fix_core_clk_rate

These individually control their respective params

4) max_perf_params - which maxes out all of the above

Is this what you are referring to as well?

>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
>>>>>     2 files changed, 9 insertions(+), 88 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> @@ -17,20 +17,6 @@
>>>>>     #include "dpu_crtc.h"
>>>>>     #include "dpu_core_perf.h"
>>>>> -/**
>>>>> - * enum dpu_perf_mode - performance tuning mode
>>>>> - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
>>>>> - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
>>>>> - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
>>>>> - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
>>>>> - */
>>>>> -enum dpu_perf_mode {
>>>>> -	DPU_PERF_MODE_NORMAL,
>>>>> -	DPU_PERF_MODE_MINIMUM,
>>>>> -	DPU_PERF_MODE_FIXED,
>>>>> -	DPU_PERF_MODE_MAX
>>>>> -};
>>>>> -
>>>>>     /**
>>>>>      * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>>>>>      * @perf_cfg: performance configuration
>>>>> @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>>>     	if (!kms->num_paths)
>>>>>     		return 0;
>>>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>> -		avg_bw = 0;
>>>>> -		peak_bw = 0;
>>>>> -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>> +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>> +
>>>>> +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>> +	peak_bw = perf.max_per_pipe_ib;
>>>>> +
>>>>> +	if (kms->perf.fix_core_ab_vote)
>>>>>     		avg_bw = kms->perf.fix_core_ab_vote;
>>>>> -		peak_bw = kms->perf.fix_core_ib_vote;
>>>>> -	} else {
>>>>> -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>> -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>> -		peak_bw = perf.max_per_pipe_ib;
>>>>> -	}
>>>>> +	if (kms->perf.fix_core_ib_vote)
>>>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>>>>     	avg_bw /= kms->num_paths;
>>>>> @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>>>     	struct drm_crtc *crtc;
>>>>>     	struct dpu_crtc_state *dpu_cstate;
>>>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
>>>>> +	if (kms->perf.fix_core_clk_rate)
>>>>>     		return kms->perf.fix_core_clk_rate;
>>>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
>>>>> -		return kms->perf.max_core_clk_rate;
>>>>> -
>>>>>     	clk_rate = 0;
>>>>>     	drm_for_each_crtc(crtc, kms->dev) {
>>>>>     		if (crtc->enabled) {
>>>>> @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>     #ifdef CONFIG_DEBUG_FS
>>>>> -static ssize_t _dpu_core_perf_mode_write(struct file *file,
>>>>> -		    const char __user *user_buf, size_t count, loff_t *ppos)
>>>>> -{
>>>>> -	struct dpu_core_perf *perf = file->private_data;
>>>>> -	u32 perf_mode = 0;
>>>>> -	int ret;
>>>>> -
>>>>> -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
>>>>> -	if (ret)
>>>>> -		return ret;
>>>>> -
>>>>> -	if (perf_mode >= DPU_PERF_MODE_MAX)
>>>>> -		return -EINVAL;
>>>>> -
>>>>> -	if (perf_mode == DPU_PERF_MODE_FIXED) {
>>>>> -		DRM_INFO("fix performance mode\n");
>>>>> -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
>>>>> -		/* run the driver with max clk and BW vote */
>>>>> -		DRM_INFO("minimum performance mode\n");
>>>>> -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
>>>>> -		/* reset the perf tune params to 0 */
>>>>> -		DRM_INFO("normal performance mode\n");
>>>>> -	}
>>>>> -	perf->perf_tune.mode = perf_mode;
>>>>> -
>>>>> -	return count;
>>>>> -}
>>>>> -
>>>>> -static ssize_t _dpu_core_perf_mode_read(struct file *file,
>>>>> -			char __user *buff, size_t count, loff_t *ppos)
>>>>> -{
>>>>> -	struct dpu_core_perf *perf = file->private_data;
>>>>> -	int len;
>>>>> -	char buf[128];
>>>>> -
>>>>> -	len = scnprintf(buf, sizeof(buf),
>>>>> -			"mode %d\n",
>>>>> -			perf->perf_tune.mode);
>>>>> -
>>>>> -	return simple_read_from_buffer(buff, count, ppos, buf, len);
>>>>> -}
>>>>> -
>>>>> -static const struct file_operations dpu_core_perf_mode_fops = {
>>>>> -	.open = simple_open,
>>>>> -	.read = _dpu_core_perf_mode_read,
>>>>> -	.write = _dpu_core_perf_mode_write,
>>>>> -};
>>>>> -
>>>>>     /**
>>>>>      * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
>>>>>      * @dpu_kms: Pointer to the dpu_kms struct
>>>>> @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>>>     			(u32 *)&perf->perf_cfg->min_llcc_ib);
>>>>>     	debugfs_create_u32("min_dram_ib", 0400, entry,
>>>>>     			(u32 *)&perf->perf_cfg->min_dram_ib);
>>>>> -	debugfs_create_file("perf_mode", 0600, entry,
>>>>> -			(u32 *)perf, &dpu_core_perf_mode_fops);
>>>>>     	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
>>>>>     			&perf->fix_core_clk_rate);
>>>>>     	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
>>>>>     	u64 core_clk_rate;
>>>>>     };
>>>>> -/**
>>>>> - * struct dpu_core_perf_tune - definition of performance tuning control
>>>>> - * @mode: performance mode
>>>>> - */
>>>>> -struct dpu_core_perf_tune {
>>>>> -	u32 mode;
>>>>> -};
>>>>> -
>>>>>     /**
>>>>>      * struct dpu_core_perf - definition of core performance context
>>>>>      * @perf_cfg: Platform-specific performance configuration
>>>>>      * @core_clk_rate: current core clock rate
>>>>>      * @max_core_clk_rate: maximum allowable core clock rate
>>>>> - * @perf_tune: debug control for performance tuning
>>>>>      * @enable_bw_release: debug control for bandwidth release
>>>>>      * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
>>>>>      * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
>>>>> @@ -47,7 +38,6 @@ struct dpu_core_perf {
>>>>>     	const struct dpu_perf_cfg *perf_cfg;
>>>>>     	u64 core_clk_rate;
>>>>>     	u64 max_core_clk_rate;
>>>>> -	struct dpu_core_perf_tune perf_tune;
>>>>>     	u32 enable_bw_release;
>>>>>     	u64 fix_core_clk_rate;
>>>>>     	u32 fix_core_ib_vote;
>>>>>
>>>
> 

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 04:47:34PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/15/2025 4:35 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 15, 2025 at 11:51:20AM -0800, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 1/15/2025 12:41 AM, Dmitry Baryshkov wrote:
> > > > On Tue, Jan 14, 2025 at 02:02:54PM -0800, Abhinav Kumar wrote:
> > > > > 
> > > > > 
> > > > > On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > > > > > Currently debugfs provides separate 'modes' to override calculated
> > > > > > MDP_CLK rate and interconnect bandwidth votes. Change that to allow
> > > > > > overriding individual values (e.g. one can override just clock or just
> > > > > > average bandwidth vote). The maximum values allowed for those entries by
> > > > > > the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
> > > > > > files in debugfs.
> > > > > > 
> > > > > 
> > > > > Apart from the concern I highlighted in the previous patch, the only issue I
> > > > > have with this is that, this went from a one step process of using the
> > > > > "mode" this has become a two step one.
> > > > > 
> > > > > There were essentially two modes we are talking about - "fixed" and
> > > > > "minimum"
> > > > > 
> > > > > With respect to "fixed" this is totally fine because this is preserving that
> > > > > functionality because to be able to set the fixed mode the end user must
> > > > > know what values they want to try anyway.
> > > > > 
> > > > > With respect to "minimum" mode, is where this approach is not that great.
> > > > > The end users of this can be non-display developers too such as our QA teams
> > > > > who might want to perform a first level of triage on the issues and route it
> > > > > accordingly. This is especially true for underruns and some performance lags
> > > > > as well.
> > > > > 
> > > > > If you really dont like the term "modes", to preserve the "minimum" mode,
> > > > > how about just using a bool debugfs like "max_perf_params" which internally
> > > > > maxes out the max MDP clock and ab/ib params.
> > > > 
> > > > That's what I'm trying to avoid - having an extra debugfs file which
> > > > overrides other files. It is much easier to work if there is no need to
> > > > switch modes, it is easy to overlook it. I think it should be fine to
> > > > use `cat max_foo > fix_foo` to override each of the params. After
> > > > renaming the threshold_high to max_core_ab the names of the debugfs
> > > > files match.
> > > > 
> > > 
> > > Its just a difference in interpretation IMO.
> > > 
> > > the "fixed" mode is trying to given an option to incrementally try and see
> > > which value really works and also to see whether its the clock OR the
> > > bandwidth which is making the difference. So individual control of those.
> > > 
> > > The "max" mode is trying to see if even the max values of everything cannot
> > > fix the problem. BTW, the max was maxing out BOTH the DPU clocks and BW.
> > > 
> > > So this is not just 2 extra reads for the user but 3. (ab/ib/dpu_clk) if we
> > > drop "max" and use "fixed" for max as well and even for that the user has to
> > > refer the max DPU clock value.
> > 
> > Yes, I understand that. However I still think that it's easier than
> > having a set of 'fix_foo' values which are silently ignored because of
> > the preselected mode.
> > 
> > I can probably see an option: use your max_perf_params idea, but in a
> > form of a write-only file which immediately selects max values for clock
> > rate and both bandwidths. WDYT?
> > 
> 
> Sorry I am missing something here. This is the same thing I had in mind to
> have it as a bool when someone does echo 1 > max_perf_params, it will
> immediately max the values for clock rate and bandwidth.
> 
> So to summarize, there are four nodes:
> 
> 1) fix_core_ab_vote
> 2) fix_core_ib_vote
> 3) fix_core_clk_rate
> 
> These individually control their respective params
> 
> 4) max_perf_params - which maxes out all of the above
> 
> Is this what you are referring to as well?

4) ... which updates those to the max values.

Yes. But the file is not modal, you don't have to echo 0 to it to stop
using the max values.

> 
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
> > > > > >     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
> > > > > >     2 files changed, 9 insertions(+), 88 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > @@ -17,20 +17,6 @@
> > > > > >     #include "dpu_crtc.h"
> > > > > >     #include "dpu_core_perf.h"
> > > > > > -/**
> > > > > > - * enum dpu_perf_mode - performance tuning mode
> > > > > > - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
> > > > > > - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
> > > > > > - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
> > > > > > - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
> > > > > > - */
> > > > > > -enum dpu_perf_mode {
> > > > > > -	DPU_PERF_MODE_NORMAL,
> > > > > > -	DPU_PERF_MODE_MINIMUM,
> > > > > > -	DPU_PERF_MODE_FIXED,
> > > > > > -	DPU_PERF_MODE_MAX
> > > > > > -};
> > > > > > -
> > > > > >     /**
> > > > > >      * _dpu_core_perf_calc_bw() - to calculate BW per crtc
> > > > > >      * @perf_cfg: performance configuration
> > > > > > @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> > > > > >     	if (!kms->num_paths)
> > > > > >     		return 0;
> > > > > > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> > > > > > -		avg_bw = 0;
> > > > > > -		peak_bw = 0;
> > > > > > -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> > > > > > +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > > > +
> > > > > > +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > > > +	peak_bw = perf.max_per_pipe_ib;
> > > > > > +
> > > > > > +	if (kms->perf.fix_core_ab_vote)
> > > > > >     		avg_bw = kms->perf.fix_core_ab_vote;
> > > > > > -		peak_bw = kms->perf.fix_core_ib_vote;
> > > > > > -	} else {
> > > > > > -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> > > > > > -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > > > > > -		peak_bw = perf.max_per_pipe_ib;
> > > > > > -	}
> > > > > > +	if (kms->perf.fix_core_ib_vote)
> > > > > > +		peak_bw = kms->perf.fix_core_ib_vote;
> > > > > >     	avg_bw /= kms->num_paths;
> > > > > > @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> > > > > >     	struct drm_crtc *crtc;
> > > > > >     	struct dpu_crtc_state *dpu_cstate;
> > > > > > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
> > > > > > +	if (kms->perf.fix_core_clk_rate)
> > > > > >     		return kms->perf.fix_core_clk_rate;
> > > > > > -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
> > > > > > -		return kms->perf.max_core_clk_rate;
> > > > > > -
> > > > > >     	clk_rate = 0;
> > > > > >     	drm_for_each_crtc(crtc, kms->dev) {
> > > > > >     		if (crtc->enabled) {
> > > > > > @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > > > >     #ifdef CONFIG_DEBUG_FS
> > > > > > -static ssize_t _dpu_core_perf_mode_write(struct file *file,
> > > > > > -		    const char __user *user_buf, size_t count, loff_t *ppos)
> > > > > > -{
> > > > > > -	struct dpu_core_perf *perf = file->private_data;
> > > > > > -	u32 perf_mode = 0;
> > > > > > -	int ret;
> > > > > > -
> > > > > > -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
> > > > > > -	if (ret)
> > > > > > -		return ret;
> > > > > > -
> > > > > > -	if (perf_mode >= DPU_PERF_MODE_MAX)
> > > > > > -		return -EINVAL;
> > > > > > -
> > > > > > -	if (perf_mode == DPU_PERF_MODE_FIXED) {
> > > > > > -		DRM_INFO("fix performance mode\n");
> > > > > > -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
> > > > > > -		/* run the driver with max clk and BW vote */
> > > > > > -		DRM_INFO("minimum performance mode\n");
> > > > > > -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
> > > > > > -		/* reset the perf tune params to 0 */
> > > > > > -		DRM_INFO("normal performance mode\n");
> > > > > > -	}
> > > > > > -	perf->perf_tune.mode = perf_mode;
> > > > > > -
> > > > > > -	return count;
> > > > > > -}
> > > > > > -
> > > > > > -static ssize_t _dpu_core_perf_mode_read(struct file *file,
> > > > > > -			char __user *buff, size_t count, loff_t *ppos)
> > > > > > -{
> > > > > > -	struct dpu_core_perf *perf = file->private_data;
> > > > > > -	int len;
> > > > > > -	char buf[128];
> > > > > > -
> > > > > > -	len = scnprintf(buf, sizeof(buf),
> > > > > > -			"mode %d\n",
> > > > > > -			perf->perf_tune.mode);
> > > > > > -
> > > > > > -	return simple_read_from_buffer(buff, count, ppos, buf, len);
> > > > > > -}
> > > > > > -
> > > > > > -static const struct file_operations dpu_core_perf_mode_fops = {
> > > > > > -	.open = simple_open,
> > > > > > -	.read = _dpu_core_perf_mode_read,
> > > > > > -	.write = _dpu_core_perf_mode_write,
> > > > > > -};
> > > > > > -
> > > > > >     /**
> > > > > >      * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
> > > > > >      * @dpu_kms: Pointer to the dpu_kms struct
> > > > > > @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> > > > > >     			(u32 *)&perf->perf_cfg->min_llcc_ib);
> > > > > >     	debugfs_create_u32("min_dram_ib", 0400, entry,
> > > > > >     			(u32 *)&perf->perf_cfg->min_dram_ib);
> > > > > > -	debugfs_create_file("perf_mode", 0600, entry,
> > > > > > -			(u32 *)perf, &dpu_core_perf_mode_fops);
> > > > > >     	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
> > > > > >     			&perf->fix_core_clk_rate);
> > > > > >     	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > > > index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > > > @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
> > > > > >     	u64 core_clk_rate;
> > > > > >     };
> > > > > > -/**
> > > > > > - * struct dpu_core_perf_tune - definition of performance tuning control
> > > > > > - * @mode: performance mode
> > > > > > - */
> > > > > > -struct dpu_core_perf_tune {
> > > > > > -	u32 mode;
> > > > > > -};
> > > > > > -
> > > > > >     /**
> > > > > >      * struct dpu_core_perf - definition of core performance context
> > > > > >      * @perf_cfg: Platform-specific performance configuration
> > > > > >      * @core_clk_rate: current core clock rate
> > > > > >      * @max_core_clk_rate: maximum allowable core clock rate
> > > > > > - * @perf_tune: debug control for performance tuning
> > > > > >      * @enable_bw_release: debug control for bandwidth release
> > > > > >      * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
> > > > > >      * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
> > > > > > @@ -47,7 +38,6 @@ struct dpu_core_perf {
> > > > > >     	const struct dpu_perf_cfg *perf_cfg;
> > > > > >     	u64 core_clk_rate;
> > > > > >     	u64 max_core_clk_rate;
> > > > > > -	struct dpu_core_perf_tune perf_tune;
> > > > > >     	u32 enable_bw_release;
> > > > > >     	u64 fix_core_clk_rate;
> > > > > >     	u32 fix_core_ib_vote;
> > > > > > 
> > > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 8/9] drm/msm/dpu: rework core_perf debugfs overrides

[Abhinav Kumar]

On 1/15/2025 5:15 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 04:47:34PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/15/2025 4:35 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jan 15, 2025 at 11:51:20AM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/15/2025 12:41 AM, Dmitry Baryshkov wrote:
>>>>> On Tue, Jan 14, 2025 at 02:02:54PM -0800, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>>>> Currently debugfs provides separate 'modes' to override calculated
>>>>>>> MDP_CLK rate and interconnect bandwidth votes. Change that to allow
>>>>>>> overriding individual values (e.g. one can override just clock or just
>>>>>>> average bandwidth vote). The maximum values allowed for those entries by
>>>>>>> the platform can be read from the 'max_core_ab' and 'max_core_clk_rate'
>>>>>>> files in debugfs.
>>>>>>>
>>>>>>
>>>>>> Apart from the concern I highlighted in the previous patch, the only issue I
>>>>>> have with this is that, this went from a one step process of using the
>>>>>> "mode" this has become a two step one.
>>>>>>
>>>>>> There were essentially two modes we are talking about - "fixed" and
>>>>>> "minimum"
>>>>>>
>>>>>> With respect to "fixed" this is totally fine because this is preserving that
>>>>>> functionality because to be able to set the fixed mode the end user must
>>>>>> know what values they want to try anyway.
>>>>>>
>>>>>> With respect to "minimum" mode, is where this approach is not that great.
>>>>>> The end users of this can be non-display developers too such as our QA teams
>>>>>> who might want to perform a first level of triage on the issues and route it
>>>>>> accordingly. This is especially true for underruns and some performance lags
>>>>>> as well.
>>>>>>
>>>>>> If you really dont like the term "modes", to preserve the "minimum" mode,
>>>>>> how about just using a bool debugfs like "max_perf_params" which internally
>>>>>> maxes out the max MDP clock and ab/ib params.
>>>>>
>>>>> That's what I'm trying to avoid - having an extra debugfs file which
>>>>> overrides other files. It is much easier to work if there is no need to
>>>>> switch modes, it is easy to overlook it. I think it should be fine to
>>>>> use `cat max_foo > fix_foo` to override each of the params. After
>>>>> renaming the threshold_high to max_core_ab the names of the debugfs
>>>>> files match.
>>>>>
>>>>
>>>> Its just a difference in interpretation IMO.
>>>>
>>>> the "fixed" mode is trying to given an option to incrementally try and see
>>>> which value really works and also to see whether its the clock OR the
>>>> bandwidth which is making the difference. So individual control of those.
>>>>
>>>> The "max" mode is trying to see if even the max values of everything cannot
>>>> fix the problem. BTW, the max was maxing out BOTH the DPU clocks and BW.
>>>>
>>>> So this is not just 2 extra reads for the user but 3. (ab/ib/dpu_clk) if we
>>>> drop "max" and use "fixed" for max as well and even for that the user has to
>>>> refer the max DPU clock value.
>>>
>>> Yes, I understand that. However I still think that it's easier than
>>> having a set of 'fix_foo' values which are silently ignored because of
>>> the preselected mode.
>>>
>>> I can probably see an option: use your max_perf_params idea, but in a
>>> form of a write-only file which immediately selects max values for clock
>>> rate and both bandwidths. WDYT?
>>>
>>
>> Sorry I am missing something here. This is the same thing I had in mind to
>> have it as a bool when someone does echo 1 > max_perf_params, it will
>> immediately max the values for clock rate and bandwidth.
>>
>> So to summarize, there are four nodes:
>>
>> 1) fix_core_ab_vote
>> 2) fix_core_ib_vote
>> 3) fix_core_clk_rate
>>
>> These individually control their respective params
>>
>> 4) max_perf_params - which maxes out all of the above
>>
>> Is this what you are referring to as well?
> 
> 4) ... which updates those to the max values.
> 
> Yes. But the file is not modal, you don't have to echo 0 to it to stop
> using the max values.
> 

Yes this is fine. Please go ahead.

>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++------------------------
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 ---
>>>>>>>      2 files changed, 9 insertions(+), 88 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> index 7ff3405c6867556a8dc776783b91f1da6c86ef3f..913eb4c01abe10c1ed84215fbbee50abd69e9317 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> @@ -17,20 +17,6 @@
>>>>>>>      #include "dpu_crtc.h"
>>>>>>>      #include "dpu_core_perf.h"
>>>>>>> -/**
>>>>>>> - * enum dpu_perf_mode - performance tuning mode
>>>>>>> - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
>>>>>>> - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
>>>>>>> - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
>>>>>>> - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
>>>>>>> - */
>>>>>>> -enum dpu_perf_mode {
>>>>>>> -	DPU_PERF_MODE_NORMAL,
>>>>>>> -	DPU_PERF_MODE_MINIMUM,
>>>>>>> -	DPU_PERF_MODE_FIXED,
>>>>>>> -	DPU_PERF_MODE_MAX
>>>>>>> -};
>>>>>>> -
>>>>>>>      /**
>>>>>>>       * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>>>>>>>       * @perf_cfg: performance configuration
>>>>>>> @@ -215,18 +201,16 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>>>>>>      	if (!kms->num_paths)
>>>>>>>      		return 0;
>>>>>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>>>>>>> -		avg_bw = 0;
>>>>>>> -		peak_bw = 0;
>>>>>>> -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
>>>>>>> +	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>>>> +
>>>>>>> +	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>>>> +	peak_bw = perf.max_per_pipe_ib;
>>>>>>> +
>>>>>>> +	if (kms->perf.fix_core_ab_vote)
>>>>>>>      		avg_bw = kms->perf.fix_core_ab_vote;
>>>>>>> -		peak_bw = kms->perf.fix_core_ib_vote;
>>>>>>> -	} else {
>>>>>>> -		dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>>>>>>> -		avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
>>>>>>> -		peak_bw = perf.max_per_pipe_ib;
>>>>>>> -	}
>>>>>>> +	if (kms->perf.fix_core_ib_vote)
>>>>>>> +		peak_bw = kms->perf.fix_core_ib_vote;
>>>>>>>      	avg_bw /= kms->num_paths;
>>>>>>> @@ -275,12 +259,9 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>>>>>      	struct drm_crtc *crtc;
>>>>>>>      	struct dpu_crtc_state *dpu_cstate;
>>>>>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
>>>>>>> +	if (kms->perf.fix_core_clk_rate)
>>>>>>>      		return kms->perf.fix_core_clk_rate;
>>>>>>> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
>>>>>>> -		return kms->perf.max_core_clk_rate;
>>>>>>> -
>>>>>>>      	clk_rate = 0;
>>>>>>>      	drm_for_each_crtc(crtc, kms->dev) {
>>>>>>>      		if (crtc->enabled) {
>>>>>>> @@ -396,54 +377,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>>>      #ifdef CONFIG_DEBUG_FS
>>>>>>> -static ssize_t _dpu_core_perf_mode_write(struct file *file,
>>>>>>> -		    const char __user *user_buf, size_t count, loff_t *ppos)
>>>>>>> -{
>>>>>>> -	struct dpu_core_perf *perf = file->private_data;
>>>>>>> -	u32 perf_mode = 0;
>>>>>>> -	int ret;
>>>>>>> -
>>>>>>> -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
>>>>>>> -	if (ret)
>>>>>>> -		return ret;
>>>>>>> -
>>>>>>> -	if (perf_mode >= DPU_PERF_MODE_MAX)
>>>>>>> -		return -EINVAL;
>>>>>>> -
>>>>>>> -	if (perf_mode == DPU_PERF_MODE_FIXED) {
>>>>>>> -		DRM_INFO("fix performance mode\n");
>>>>>>> -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
>>>>>>> -		/* run the driver with max clk and BW vote */
>>>>>>> -		DRM_INFO("minimum performance mode\n");
>>>>>>> -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
>>>>>>> -		/* reset the perf tune params to 0 */
>>>>>>> -		DRM_INFO("normal performance mode\n");
>>>>>>> -	}
>>>>>>> -	perf->perf_tune.mode = perf_mode;
>>>>>>> -
>>>>>>> -	return count;
>>>>>>> -}
>>>>>>> -
>>>>>>> -static ssize_t _dpu_core_perf_mode_read(struct file *file,
>>>>>>> -			char __user *buff, size_t count, loff_t *ppos)
>>>>>>> -{
>>>>>>> -	struct dpu_core_perf *perf = file->private_data;
>>>>>>> -	int len;
>>>>>>> -	char buf[128];
>>>>>>> -
>>>>>>> -	len = scnprintf(buf, sizeof(buf),
>>>>>>> -			"mode %d\n",
>>>>>>> -			perf->perf_tune.mode);
>>>>>>> -
>>>>>>> -	return simple_read_from_buffer(buff, count, ppos, buf, len);
>>>>>>> -}
>>>>>>> -
>>>>>>> -static const struct file_operations dpu_core_perf_mode_fops = {
>>>>>>> -	.open = simple_open,
>>>>>>> -	.read = _dpu_core_perf_mode_read,
>>>>>>> -	.write = _dpu_core_perf_mode_write,
>>>>>>> -};
>>>>>>> -
>>>>>>>      /**
>>>>>>>       * dpu_core_perf_debugfs_init - initialize debugfs for core performance context
>>>>>>>       * @dpu_kms: Pointer to the dpu_kms struct
>>>>>>> @@ -472,8 +405,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>>>>>      			(u32 *)&perf->perf_cfg->min_llcc_ib);
>>>>>>>      	debugfs_create_u32("min_dram_ib", 0400, entry,
>>>>>>>      			(u32 *)&perf->perf_cfg->min_dram_ib);
>>>>>>> -	debugfs_create_file("perf_mode", 0600, entry,
>>>>>>> -			(u32 *)perf, &dpu_core_perf_mode_fops);
>>>>>>>      	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
>>>>>>>      			&perf->fix_core_clk_rate);
>>>>>>>      	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>>>> index 5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d..9d8516ca32d162b1e277ec88067e5c21abeb2017 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>>>> @@ -24,20 +24,11 @@ struct dpu_core_perf_params {
>>>>>>>      	u64 core_clk_rate;
>>>>>>>      };
>>>>>>> -/**
>>>>>>> - * struct dpu_core_perf_tune - definition of performance tuning control
>>>>>>> - * @mode: performance mode
>>>>>>> - */
>>>>>>> -struct dpu_core_perf_tune {
>>>>>>> -	u32 mode;
>>>>>>> -};
>>>>>>> -
>>>>>>>      /**
>>>>>>>       * struct dpu_core_perf - definition of core performance context
>>>>>>>       * @perf_cfg: Platform-specific performance configuration
>>>>>>>       * @core_clk_rate: current core clock rate
>>>>>>>       * @max_core_clk_rate: maximum allowable core clock rate
>>>>>>> - * @perf_tune: debug control for performance tuning
>>>>>>>       * @enable_bw_release: debug control for bandwidth release
>>>>>>>       * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
>>>>>>>       * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
>>>>>>> @@ -47,7 +38,6 @@ struct dpu_core_perf {
>>>>>>>      	const struct dpu_perf_cfg *perf_cfg;
>>>>>>>      	u64 core_clk_rate;
>>>>>>>      	u64 max_core_clk_rate;
>>>>>>> -	struct dpu_core_perf_tune perf_tune;
>>>>>>>      	u32 enable_bw_release;
>>>>>>>      	u64 fix_core_clk_rate;
>>>>>>>      	u32 fix_core_ib_vote;
>>>>>>>
>>>>>
>>>
> 

[PATCH v4 9/9] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Dmitry Baryshkov]

The max_per_pipe_ib is a constant across all CRTCs and is read from the
catalog. The override value is also applied at the
_dpu_core_perf_crtc_update_bus() time. Drop corresponding calculations
and read the value directly at icc_set_bw() time.

Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 16 ++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  2 --
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 913eb4c01abe10c1ed84215fbbee50abd69e9317..62dab5883513dc570076da5a64e32e502dd4320b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -105,12 +105,10 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
 	}
 
 	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
-	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
 	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
 	DRM_DEBUG_ATOMIC(
-		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
+		"crtc=%d clk_rate=%llu core_ab=%u\n",
 			crtc->base.id, perf->core_clk_rate,
-			perf->max_per_pipe_ib,
 			(u32)DIV_ROUND_UP_ULL(perf->bw_ctl, 1000));
 }
 
@@ -126,9 +124,6 @@ static void dpu_core_perf_aggregate(struct drm_device *ddev,
 		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
 			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
 
-			perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
-						    dpu_cstate->new_perf.max_per_pipe_ib);
-
 			perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
 
 			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
@@ -204,7 +199,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
 
 	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
-	peak_bw = perf.max_per_pipe_ib;
+	peak_bw = kms->catalog->perf->min_dram_ib;
 
 	if (kms->perf.fix_core_ab_vote)
 		avg_bw = kms->perf.fix_core_ab_vote;
@@ -315,15 +310,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 		 * 2. new bandwidth vote - "ab or ib vote" is lower
 		 *    than current vote at end of commit or stop.
 		 */
-		if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
-			(new->max_per_pipe_ib > old->max_per_pipe_ib)))	||
-			(!params_changed && ((new->bw_ctl < old->bw_ctl) ||
-			(new->max_per_pipe_ib < old->max_per_pipe_ib)))) {
+		if ((params_changed && new->bw_ctl > old->bw_ctl) ||
+		    (!params_changed && new->bw_ctl < old->bw_ctl)) {
 			DRM_DEBUG_ATOMIC("crtc=%d p=%d new_bw=%llu,old_bw=%llu\n",
 				crtc->base.id, params_changed,
 				new->bw_ctl, old->bw_ctl);
 			old->bw_ctl = new->bw_ctl;
-			old->max_per_pipe_ib = new->max_per_pipe_ib;
 			update_bus = true;
 		}
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 9d8516ca32d162b1e277ec88067e5c21abeb2017..863a6fc1f30c21cf2030a30be5fe62b024b3b820 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -14,12 +14,10 @@
 
 /**
  * struct dpu_core_perf_params - definition of performance parameters
- * @max_per_pipe_ib: maximum instantaneous bandwidth request
  * @bw_ctl: arbitrated bandwidth request
  * @core_clk_rate: core clock rate request
  */
 struct dpu_core_perf_params {
-	u32 max_per_pipe_ib;
 	u64 bw_ctl;
 	u64 core_clk_rate;
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ac3c6c5ad1cec3856f0eff2ed71153d3c2dc279e..cc240d3c7faa89254a575237634d0d0fa8f04f73 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1488,8 +1488,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
 			dpu_crtc->cur_perf.core_clk_rate);
 	seq_printf(s, "bw_ctl: %uk\n",
 		   (u32)DIV_ROUND_UP_ULL(dpu_crtc->cur_perf.bw_ctl, 1000));
-	seq_printf(s, "max_per_pipe_ib: %u\n",
-				dpu_crtc->cur_perf.max_per_pipe_ib);
 
 	return 0;
 }

-- 
2.39.5

Re: [PATCH v4 9/9] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Abhinav Kumar]

On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> The max_per_pipe_ib is a constant across all CRTCs and is read from the
> catalog. The override value is also applied at the
> _dpu_core_perf_crtc_update_bus() time. Drop corresponding calculations
> and read the value directly at icc_set_bw() time.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 16 ++++------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  2 --
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  2 --
>   3 files changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 913eb4c01abe10c1ed84215fbbee50abd69e9317..62dab5883513dc570076da5a64e32e502dd4320b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -105,12 +105,10 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>   	}
>   
>   	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> -	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>   	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>   	DRM_DEBUG_ATOMIC(
> -		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
> +		"crtc=%d clk_rate=%llu core_ab=%u\n",
>   			crtc->base.id, perf->core_clk_rate,
> -			perf->max_per_pipe_ib,
>   			(u32)DIV_ROUND_UP_ULL(perf->bw_ctl, 1000));
>   }
>   
> @@ -126,9 +124,6 @@ static void dpu_core_perf_aggregate(struct drm_device *ddev,
>   		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
>   			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
>   
> -			perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
> -						    dpu_cstate->new_perf.max_per_pipe_ib);

During the enabled cases, this is fine since even if one crtc is 
enabled, its going to use the same value.

During disable to enable and enable to disable transitions, we do need 
to make it 0 right?

OR if its already being made 0, we need to make sure it gets updated by 
forcing update_bus to true?

Is this part being handled by this block dpu_core_perf_crtc_update()?

         } else {
                 DRM_DEBUG_ATOMIC("crtc=%d disable\n", crtc->base.id);
                 memset(old, 0, sizeof(*old));
                 update_bus = true;
                 update_clk = true;
         }

Please confirm this, I am fine with this change otherwise.

> -
>   			perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
>   
>   			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
> @@ -204,7 +199,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>   	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
>   
>   	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> -	peak_bw = perf.max_per_pipe_ib;
> +	peak_bw = kms->catalog->perf->min_dram_ib;
>   
>   	if (kms->perf.fix_core_ab_vote)
>   		avg_bw = kms->perf.fix_core_ab_vote;
> @@ -315,15 +310,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   		 * 2. new bandwidth vote - "ab or ib vote" is lower
>   		 *    than current vote at end of commit or stop.
>   		 */
> -		if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
> -			(new->max_per_pipe_ib > old->max_per_pipe_ib)))	||
> -			(!params_changed && ((new->bw_ctl < old->bw_ctl) ||
> -			(new->max_per_pipe_ib < old->max_per_pipe_ib)))) {
> +		if ((params_changed && new->bw_ctl > old->bw_ctl) ||
> +		    (!params_changed && new->bw_ctl < old->bw_ctl)) {
>   			DRM_DEBUG_ATOMIC("crtc=%d p=%d new_bw=%llu,old_bw=%llu\n",
>   				crtc->base.id, params_changed,
>   				new->bw_ctl, old->bw_ctl);
>   			old->bw_ctl = new->bw_ctl;
> -			old->max_per_pipe_ib = new->max_per_pipe_ib;
>   			update_bus = true;
>   		}
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index 9d8516ca32d162b1e277ec88067e5c21abeb2017..863a6fc1f30c21cf2030a30be5fe62b024b3b820 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -14,12 +14,10 @@
>   
>   /**
>    * struct dpu_core_perf_params - definition of performance parameters
> - * @max_per_pipe_ib: maximum instantaneous bandwidth request
>    * @bw_ctl: arbitrated bandwidth request
>    * @core_clk_rate: core clock rate request
>    */
>   struct dpu_core_perf_params {
> -	u32 max_per_pipe_ib;
>   	u64 bw_ctl;
>   	u64 core_clk_rate;
>   };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index ac3c6c5ad1cec3856f0eff2ed71153d3c2dc279e..cc240d3c7faa89254a575237634d0d0fa8f04f73 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1488,8 +1488,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
>   			dpu_crtc->cur_perf.core_clk_rate);
>   	seq_printf(s, "bw_ctl: %uk\n",
>   		   (u32)DIV_ROUND_UP_ULL(dpu_crtc->cur_perf.bw_ctl, 1000));
> -	seq_printf(s, "max_per_pipe_ib: %u\n",
> -				dpu_crtc->cur_perf.max_per_pipe_ib);
>   
>   	return 0;
>   }
> 

Re: [PATCH v4 9/9] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Dmitry Baryshkov]

On Tue, Jan 14, 2025 at 04:53:10PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
> > The max_per_pipe_ib is a constant across all CRTCs and is read from the
> > catalog. The override value is also applied at the
> > _dpu_core_perf_crtc_update_bus() time. Drop corresponding calculations
> > and read the value directly at icc_set_bw() time.
> > 
> > Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 16 ++++------------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  2 --
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  2 --
> >   3 files changed, 4 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > index 913eb4c01abe10c1ed84215fbbee50abd69e9317..62dab5883513dc570076da5a64e32e502dd4320b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > @@ -105,12 +105,10 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
> >   	}
> >   	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> > -	perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
> >   	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
> >   	DRM_DEBUG_ATOMIC(
> > -		"crtc=%d clk_rate=%llu core_ib=%u core_ab=%u\n",
> > +		"crtc=%d clk_rate=%llu core_ab=%u\n",
> >   			crtc->base.id, perf->core_clk_rate,
> > -			perf->max_per_pipe_ib,
> >   			(u32)DIV_ROUND_UP_ULL(perf->bw_ctl, 1000));
> >   }
> > @@ -126,9 +124,6 @@ static void dpu_core_perf_aggregate(struct drm_device *ddev,
> >   		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
> >   			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
> > -			perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
> > -						    dpu_cstate->new_perf.max_per_pipe_ib);
> 
> During the enabled cases, this is fine since even if one crtc is enabled,
> its going to use the same value.
> 
> During disable to enable and enable to disable transitions, we do need to
> make it 0 right?

Good point, I will check that later.

> 
> OR if its already being made 0, we need to make sure it gets updated by
> forcing update_bus to true?
> 
> Is this part being handled by this block dpu_core_perf_crtc_update()?
> 
>         } else {
>                 DRM_DEBUG_ATOMIC("crtc=%d disable\n", crtc->base.id);
>                 memset(old, 0, sizeof(*old));
>                 update_bus = true;
>                 update_clk = true;
>         }
> 
> Please confirm this, I am fine with this change otherwise.
> 
> > -
> >   			perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
> >   			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
> > @@ -204,7 +199,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
> >   	dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
> >   	avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
> > -	peak_bw = perf.max_per_pipe_ib;
> > +	peak_bw = kms->catalog->perf->min_dram_ib;
> >   	if (kms->perf.fix_core_ab_vote)
> >   		avg_bw = kms->perf.fix_core_ab_vote;
> > @@ -315,15 +310,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >   		 * 2. new bandwidth vote - "ab or ib vote" is lower
> >   		 *    than current vote at end of commit or stop.
> >   		 */
> > -		if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
> > -			(new->max_per_pipe_ib > old->max_per_pipe_ib)))	||
> > -			(!params_changed && ((new->bw_ctl < old->bw_ctl) ||
> > -			(new->max_per_pipe_ib < old->max_per_pipe_ib)))) {
> > +		if ((params_changed && new->bw_ctl > old->bw_ctl) ||
> > +		    (!params_changed && new->bw_ctl < old->bw_ctl)) {
> >   			DRM_DEBUG_ATOMIC("crtc=%d p=%d new_bw=%llu,old_bw=%llu\n",
> >   				crtc->base.id, params_changed,
> >   				new->bw_ctl, old->bw_ctl);
> >   			old->bw_ctl = new->bw_ctl;
> > -			old->max_per_pipe_ib = new->max_per_pipe_ib;
> >   			update_bus = true;
> >   		}
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > index 9d8516ca32d162b1e277ec88067e5c21abeb2017..863a6fc1f30c21cf2030a30be5fe62b024b3b820 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > @@ -14,12 +14,10 @@
> >   /**
> >    * struct dpu_core_perf_params - definition of performance parameters
> > - * @max_per_pipe_ib: maximum instantaneous bandwidth request
> >    * @bw_ctl: arbitrated bandwidth request
> >    * @core_clk_rate: core clock rate request
> >    */
> >   struct dpu_core_perf_params {
> > -	u32 max_per_pipe_ib;
> >   	u64 bw_ctl;
> >   	u64 core_clk_rate;
> >   };
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index ac3c6c5ad1cec3856f0eff2ed71153d3c2dc279e..cc240d3c7faa89254a575237634d0d0fa8f04f73 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1488,8 +1488,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
> >   			dpu_crtc->cur_perf.core_clk_rate);
> >   	seq_printf(s, "bw_ctl: %uk\n",
> >   		   (u32)DIV_ROUND_UP_ULL(dpu_crtc->cur_perf.bw_ctl, 1000));
> > -	seq_printf(s, "max_per_pipe_ib: %u\n",
> > -				dpu_crtc->cur_perf.max_per_pipe_ib);
> >   	return 0;
> >   }
> > 

-- 
With best wishes
Dmitry