On 3/19/2024 3:25 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 23:35, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 3/19/2024 1:43 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 22:34, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 3/13/2024 6:10 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.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 39 +++++++++++++--------------
1 file changed, 19 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 87b892069526..ff2942a6a678 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -118,21 +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;
- 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=%llu core_ab=%llu\n",
@@ -233,18 +221,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;
Why were avg_bw and peak_bw values brought down to u32?
I think we might go higher so u64 was better.
First of all, icc_set_bw takes u32, not u64. The unit is 1000 bps, not
1 bps, so sensible values fit into u32.
True and agreed.
Would have been better to send this update as a separate patch so that
its clear why you are actually doing this downgrade instead of this
being hidden in this cleanup perhaps even with a Fixes tag then.
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;
Instead of changing the value of avg_bw like mentioned in commit text,
why cant we do avg_bw = fix_core_ab * (drm_mode_config::num_crtc);
Any reason you want to change it from "per CRTC fixed" to just "fixed"?
Now, the user who wants to hard-code this also needs to first account
for number of CRTCs from the dri state and then program the fixed value
using debugfs. Thats not convenient.
Different CRTCs have different bandwidth values, so programming as
value-per-CRTC is not efficient. In the end we care for the overall
bandwidth, so one has to calculate the expected value then divide it
per num_crtc.
Yes, different CRTCs will have different bandwidth values as each CRTC
might be driving a different resolution.
So you are expecting the user to program the total bandwidth they are
expecting (sum_of_crtcs).
Then why would they have to divide it per num_crtc?
After this change, I think you are expecting the overall bandwidth right?
I think some --help option for this debugfs should be written now or
later to explain what to do with node,
+ } else {
+ dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), &perf);
Where is this function dpu_core_perf_aggregate() defined? I dont see it
in msm-next
In the previous patch.
Sorry, my bad. I thought it had a different name in the prev patch :/
No problems.
+
+ 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;
}
