[PATCH v4 4/9] drm/msm/dpu: make fix_core_ab_vote consistent with fix_core_ib_vote

[Abhinav Kumar]

On 1/11/2025 5:08 AM, Dmitry Baryshkov wrote:
> On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>> On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote:
>>>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth
>>>>> overrides in several cases. However there is an internal inconsistency:
>>>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined
>>>>> in Bps.
>>>>>
>>>>> Fix that by changing the type of the variable to u32 and using * 1000ULL
>>>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value.
>>>>>
>>>>
>>>> Actually after looking at this, I have another question.
>>>>
>>>> How did you conclude that fix_core_ib_vote is in KBps?
>>>>
>>>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote?
>>>>
>>>> It depends on the interpretation perhaps. If the debugfs was supposed to
>>>> operate under the expectation that the provided value will be pre-multiplied
>>>> by 1000 and given then that explains why it was not multiplied again.
>>>>
>>>> And I cross-checked some of the internal usages of the debugfs, the values
>>>> provided to it were in Bps and not KBps.
>>>
>>> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next
>>> two lines, fix_core_ib_vote should also be in kBps, as there is no
>>> premultiplier:
>>>
>>>                   perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
>>> [...]
>>>                   perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
>>>
>>> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw()
>>> without any modifications:
>>>
>>>                   icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>>>
>>
>> Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps.
>>
>> Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp.
> 
> Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps.
> 

In that case, the documentation for both needs to be updated as well as 
it still says both are in bps not kbps.

  * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
  * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
  */
struct dpu_core_perf {
         const struct dpu_perf_cfg *perf_cfg;
         u64 core_clk_rate;
         u64 max_core_clk_rate;
         struct dpu_core_perf_tune perf_tune;
         u32 enable_bw_release;
         u64 fix_core_clk_rate;
         u64 fix_core_ib_vote;
         u64 fix_core_ab_vote;
};

>>
>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +-
>>>>>     2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
>>>>>     		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->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 {
>>>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>>>     			&perf->fix_core_clk_rate);
>>>>>     	debugfs_create_u32("fix_core_ib_vote", 0600, entry,
>>>>>     			&perf->fix_core_ib_vote);
>>>>> -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
>>>>> +	debugfs_create_u32("fix_core_ab_vote", 0600, entry,
>>>>>     			&perf->fix_core_ab_vote);
>>>>>     	return 0;
>>>>> 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> @@ -51,7 +51,7 @@ struct dpu_core_perf {
>>>>>     	u32 enable_bw_release;
>>>>>     	u64 fix_core_clk_rate;
>>>>>     	u32 fix_core_ib_vote;
>>>>> -	u64 fix_core_ab_vote;
>>>>> +	u32 fix_core_ab_vote;
>>>>>     };
>>>>>     int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>>>
>>>
>