| Message ID | 20240314-dpu-perf-rework-v3-0-79fa4e065574@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: rework debugfs interface of dpu_core_perf | expand |
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@linaro.org> > --- > 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; > > 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); Where is this function dpu_core_perf_aggregate() defined? I dont see it in msm-next > + > + 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; > } >
On 3/19/2024 1:43 PM, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 22:34, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@linaro.org> >>> --- >>> 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. >>> >>> 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. >>> + } 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 :/ >> >>> + >>> + 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; >>> } >>> > > >
On Tue, 19 Mar 2024 at 23:35, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 3/19/2024 1:43 PM, Dmitry Baryshkov wrote: > > On Tue, 19 Mar 2024 at 22:34, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@linaro.org> > >>> --- > >>> 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. > > >>> > >>> 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. > > >>> + } 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; > >>> } > >>> > > > > > >
On 3/19/2024 3:25 PM, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 23:35, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 3/19/2024 1:43 PM, Dmitry Baryshkov wrote: >>> On Tue, 19 Mar 2024 at 22:34, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@linaro.org> >>>>> --- >>>>> 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; >>>>> } >>>>> >>> >>> >>> > > >
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote: > The max_per_pipe_ib is a constant across all CRTCs and is read from the > catalog. 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 | 17 +++++------------ > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 -- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 -- > 3 files changed, 5 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 2e78e57665fc..2fc05665dc7a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > @@ -105,13 +105,12 @@ 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=%llu core_ab=%llu\n", > + "crtc=%d clk_rate=%llu core_ab=%llu\n", > crtc->base.id, perf->core_clk_rate, > - perf->max_per_pipe_ib, perf->bw_ctl); > + perf->bw_ctl); > } > > int dpu_core_perf_crtc_check(struct drm_crtc *crtc, > @@ -190,9 +189,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 override case perf->max_per_pipe_ib = core_perf->fix_core_ib_vote So this is one case where max_per_pipe_ib can actually be changed right? Now, fix_core_ib_vote will not be used then? > - > perf->bw_ctl += dpu_cstate->new_perf.bw_ctl; > > DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n", > @@ -216,7 +212,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; > @@ -321,15 +317,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 5a3d18ca9555..a5a9c3389718 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 { > - u64 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 88c2e51ab166..771c04c1a5ea 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1389,8 +1389,6 @@ 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", > - dpu_crtc->cur_perf.max_per_pipe_ib); > > return 0; > } >
On Thu, 14 Mar 2024 03:10:40 +0200, Dmitry Baryshkov wrote: > 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 > > [...] Applied, thanks! [1/5] drm/msm/dpu: don't allow overriding data from catalog https://gitlab.freedesktop.org/drm/msm/-/commit/4f3b77ae5ff5 Best regards,
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> --- Dmitry Baryshkov (5): drm/msm/dpu: don't allow overriding data from catalog drm/msm/dpu: core_perf: extract bandwidth aggregation function 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 | 160 +++++++------------------- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 12 -- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 - 3 files changed, 44 insertions(+), 130 deletions(-) --- base-commit: 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd change-id: 20240314-dpu-perf-rework-97fca999eb46 Best regards,