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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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
