Re: [PATCH] interconnect: Skip call into provider if initial bw is zero

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 19/01/2023 22:18, Vivek Aknurwar wrote:
> Hi Bryan,
> Thanks for taking time to review the patch.
> 
> On 1/13/2023 5:40 PM, Bryan O'Donoghue wrote:
>> On 14/01/2023 01:24, Bryan O'Donoghue wrote:
>>> On 13/01/2023 22:07, Vivek Aknurwar wrote:
>>>> Currently framework sets bw even when init bw requirements are zero 
>>>> during
>>>> provider registration, thus resulting bulk of set bw to hw.
>>>> Avoid this behaviour by skipping provider set bw calls if init bw is 
>>>> zero.
>>>>
>>>> Signed-off-by: Vivek Aknurwar <quic_viveka@quicinc.com>
>>>> ---
>>>>   drivers/interconnect/core.c | 17 ++++++++++-------
>>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>>>> index 25debde..43ed595 100644
>>>> --- a/drivers/interconnect/core.c
>>>> +++ b/drivers/interconnect/core.c
>>>> @@ -977,14 +977,17 @@ void icc_node_add(struct icc_node *node, 
>>>> struct icc_provider *provider)
>>>>       node->avg_bw = node->init_avg;
>>>>       node->peak_bw = node->init_peak;
>>>> -    if (provider->pre_aggregate)
>>>> -        provider->pre_aggregate(node);
>>>> -
>>>> -    if (provider->aggregate)
>>>> -        provider->aggregate(node, 0, node->init_avg, node->init_peak,
>>>> -                    &node->avg_bw, &node->peak_bw);
>>>> +    if (node->avg_bw || node->peak_bw) {
>>>> +        if (provider->pre_aggregate)
>>>> +            provider->pre_aggregate(node);
>>>> +
>>>> +        if (provider->aggregate)
>>>> +            provider->aggregate(node, 0, node->init_avg, 
>>>> node->init_peak,
>>>> +                        &node->avg_bw, &node->peak_bw);
>>>> +        if (provider->set)
>>>> +            provider->set(node, node);
>>>> +    }
>>>> -    provider->set(node, node);
>>>>       node->avg_bw = 0;
>>>>       node->peak_bw = 0;
>>>
>>> I have the same comment/question for this patch that I had for the 
>>> qcom arch specific version of it. This patch seems to be doing at a 
>>> higher level what the patch below was doing at a lower level.
>>>
>>> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m0c90588d0d1e2ab88c39be8f5f3a8f0b61396349
>>>
>>> what happens to earlier silicon - qcom silicon which previously made 
>>> explicit zero requests ?
> 
> This patch is to optimize and avoid all those bw 0 requests on each node 
> addition during probe (which results in rpmh remote calls) for upcoming 
> targets.

So why not change it just for rpmh ?

You are changing it for rpm here, as well as for Samsung and NXP 
interconnects.

Taking rpm as an example, for certain generations of silicon we make an 
explicit zero call.

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_bimc.c#L1367

Here's the original RPM commit that sets a zero

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d91d108656a7a44a6dfcfb318a25d39c5418e54b

>>> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m589e8280de470e038249bb362634221771d845dd
>>>
>>> https://lkml.org/lkml/2023/1/3/1232
>>>
>>> Isn't it a better idea to let lower layer drivers differentiate what 
>>> they do ?
> 
> AFAIU lower layer driver can/should not differentiate between normal 
> flow calls vs made as a result from probe/initialization of driver. 
> Hence even bw 0 request is honored as like client in general wish to 
> vote 0 as in an normal use case.

But surely if I vote zero, then I mean to vote zero ?

Do we know that for every architecture and for every different supported 
that ignoring a zero vote is the right thing to do ?

I don't think we do know that.

https://lore.kernel.org/linux-arm-msm/20230116132152.405535-1-konrad.dybcio@linaro.org/

I think for older rpm this is a departure from long existing logic.

Maybe its entirely benign but, IMO you should be proposing this change 
at the rpmh level only, not at the top level across multiple different 
interconnect arches.

---
bod