[Freedreno] [PATCH v3 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides

[Abhinav Kumar]

On 7/10/2023 7:34 PM, Dmitry Baryshkov wrote:
> On 11/07/2023 05:31, Abhinav Kumar wrote:
>>
>>
>> On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:
>>> The values in struct dpu_core_perf_tune are fixed per the core perf
>>> mode. Drop the 'tune' values and substitute them with known values when
>>> performing perf management.
>>>
>>> Note: min_bus_vote was not used at all, so it is just silently dropped.
>>>
>>
>> Interesting ..... should bring this back properly. Will take it up.
> 
> Ack, thanks.
> 
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ++++++++-----------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  4 ---
>>>   2 files changed, 12 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 05d340aa18c5..348550ac7e51 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -235,7 +235,7 @@ 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;
>>
>> avg_bw seems unused in this patch, so unrelated change?
>>
>>>       if (!kms->num_paths)
>>>           return 0;
>>> @@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct 
>>> drm_crtc *crtc)
>>>   static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>   {
>>> -    u64 clk_rate = kms->perf.perf_tune.min_core_clk;
>>> +    u64 clk_rate;
>>>       struct drm_crtc *crtc;
>>>       struct dpu_crtc_state *dpu_cstate;
>>> +    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
>>> +        return kms->perf.fix_core_clk_rate;
>>> +
>>> +    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
>>> +        return kms->perf.max_core_clk_rate;
>>> +
>>>       drm_for_each_crtc(crtc, kms->dev) {
>>>           if (crtc->enabled) {
>>>               dpu_cstate = to_dpu_crtc_state(crtc->state);
>>> @@ -305,11 +311,6 @@ static u64 
>>> _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>           }
>>>       }
>>> -    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
>>> -        clk_rate = kms->perf.fix_core_clk_rate;
>>> -
>>> -    DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
>>> -
>>
>> Why dont you move both FIXED and MINIMUM handling below instead of above.
>>
>> So that they will just override the clk_rate and you can keep this 
>> useful log here and it matches where the function is.
> 
> I can keep the log in the next version. The logic was quite simple: 
> there is no need to loop over CRTCs if we know that we are overriding 
> the value.
> 

Yes I understood that part. I wanted to keep the log and the function 
together that way we are logging whats the value its going to return.

This patch is logging it at the caller. Thats the only difference.

I am fine either way. Once the avg_bw change is removed, I can ack this.

>>
>> This chunk looks better that way.
>>
>> <skipping the rest as it LGTM>
>