Re: [PATCH 3/3] drm/msm/dpu: add support for clk and bw scaling for display

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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