Messages in this thread | | | Subject | Re: [PATCH 3/3] drm/msm/dpu: add support for clk and bw scaling for display | From | Dmitry Baryshkov <> | Date | Tue, 27 Oct 2020 11:32:36 +0300 |
| |
Hello,
On 04/08/2020 18:40, Rob Clark wrote: > On Thu, Jul 16, 2020 at 4:36 AM Kalyan Thota <kalyan_t@codeaurora.org> wrote: >> >> This change adds support to scale src clk and bandwidth as >> per composition requirements. >> >> Interconnect registration for bw has been moved to mdp >> device node from mdss to facilitate the scaling. >> >> Changes in v1: >> - Address armv7 compilation issues with the patch (Rob) >> >> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
Kalyan, back in July you promised to provide a followup patchset, removing code duplication. It's close to November now. Are there any plans for the followup or is a forgotten topic?
> > Reviewed-by: Rob Clark <robdclark@chromium.org> > >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 109 +++++++++++++++++++++---- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 + >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 37 ++++++++- >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + >> drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 9 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 84 +++++++++++++++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 + >> 8 files changed, 233 insertions(+), 23 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 7c230f7..e52bc44 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >> @@ -29,6 +29,74 @@ enum dpu_perf_mode { >> DPU_PERF_MODE_MAX >> }; >> >> +/** >> + * @_dpu_core_perf_calc_bw() - to calculate BW per crtc >> + * @kms - pointer to the dpu_kms >> + * @crtc - pointer to a crtc >> + * Return: returns aggregated BW for all planes in crtc. >> + */ >> +static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms, >> + struct drm_crtc *crtc) >> +{ >> + struct drm_plane *plane; >> + struct dpu_plane_state *pstate; >> + u64 crtc_plane_bw = 0; >> + u32 bw_factor; >> + >> + drm_atomic_crtc_for_each_plane(plane, crtc) { >> + pstate = to_dpu_plane_state(plane->state); >> + if (!pstate) >> + continue; >> + >> + crtc_plane_bw += pstate->plane_fetch_bw; >> + } >> + >> + bw_factor = kms->catalog->perf.bw_inefficiency_factor; >> + if (bw_factor) { >> + crtc_plane_bw *= bw_factor; >> + do_div(crtc_plane_bw, 100); >> + } >> + >> + return crtc_plane_bw; >> +} >> + >> +/** >> + * _dpu_core_perf_calc_clk() - to calculate clock per crtc >> + * @kms - pointer to the dpu_kms >> + * @crtc - pointer to a crtc >> + * @state - pointer to a crtc state >> + * Return: returns max clk for all planes in crtc. >> + */ >> +static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms, >> + struct drm_crtc *crtc, struct drm_crtc_state *state) >> +{ >> + struct drm_plane *plane; >> + struct dpu_plane_state *pstate; >> + struct drm_display_mode *mode; >> + u64 crtc_clk; >> + u32 clk_factor; >> + >> + mode = &state->adjusted_mode; >> + >> + crtc_clk = mode->vtotal * mode->hdisplay * drm_mode_vrefresh(mode); >> + >> + drm_atomic_crtc_for_each_plane(plane, crtc) { >> + pstate = to_dpu_plane_state(plane->state); >> + if (!pstate) >> + continue; >> + >> + crtc_clk = max(pstate->plane_clk, crtc_clk); >> + } >> + >> + clk_factor = kms->catalog->perf.clk_inefficiency_factor; >> + if (clk_factor) { >> + crtc_clk *= clk_factor; >> + do_div(crtc_clk, 100); >> + } >> + >> + return crtc_clk; >> +} >> + >> static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) >> { >> struct msm_drm_private *priv; >> @@ -51,12 +119,7 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms, >> dpu_cstate = to_dpu_crtc_state(state); >> memset(perf, 0, sizeof(struct dpu_core_perf_params)); >> >> - if (!dpu_cstate->bw_control) { >> - perf->bw_ctl = kms->catalog->perf.max_bw_high * >> - 1000ULL; >> - perf->max_per_pipe_ib = perf->bw_ctl; >> - perf->core_clk_rate = kms->perf.max_core_clk_rate; >> - } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) { >> + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) { >> perf->bw_ctl = 0; >> perf->max_per_pipe_ib = 0; >> perf->core_clk_rate = 0;
Now bw_control is unused and can be removed alltogether.
>> @@ -64,6 +127,10 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms, >> perf->bw_ctl = kms->perf.fix_core_ab_vote; >> perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote; >> perf->core_clk_rate = kms->perf.fix_core_clk_rate; >> + } else { >> + perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc); >> + perf->max_per_pipe_ib = kms->catalog->perf.min_dram_ib; >> + perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state); >> } >> >> DPU_DEBUG( >> @@ -115,11 +182,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >> DPU_DEBUG("crtc:%d bw:%llu ctrl:%d\n", >> tmp_crtc->base.id, tmp_cstate->new_perf.bw_ctl, >> tmp_cstate->bw_control); >> - /* >> - * For bw check only use the bw if the >> - * atomic property has been already set >> - */ >> - if (tmp_cstate->bw_control) >> + >> bw_sum_of_intfs += tmp_cstate->new_perf.bw_ctl;
Just a nitpick: indent is wrong.
>> } >> >> @@ -131,9 +194,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >> >> DPU_DEBUG("final threshold bw limit = %d\n", threshold); >> >> - if (!dpu_cstate->bw_control) { >> - DPU_DEBUG("bypass bandwidth check\n"); >> - } else if (!threshold) { >> + if (!threshold) { >> DPU_ERROR("no bandwidth limits specified\n"); >> return -E2BIG; >> } else if (bw > threshold) { >> @@ -154,7 +215,11 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms, >> = dpu_crtc_get_client_type(crtc); >> struct drm_crtc *tmp_crtc; >> struct dpu_crtc_state *dpu_cstate; >> - int ret = 0; >> + int i, ret = 0; >> + u64 avg_bw; >> + >> + if (!kms->num_paths) >> + return -EINVAL;
This broke bandwidth setting for everybody except sc7180, since _dpu_core_perf_crtc_update_bus will be still called for them, and returning -EINVAL here prevents dpu_core_perf_crtc_update() from setting clock rate. Returning 0 here fixes the issue.
>> >> drm_for_each_crtc(tmp_crtc, crtc->dev) { >> if (tmp_crtc->enabled && >> @@ -165,10 +230,20 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
-- With best wishes Dmitry
| |