On 1/11/2025 5:08 AM, Dmitry Baryshkov wrote:
On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxx>
---
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,
